Fix #6210: Added an exploration whitelist #6255

aks681 · 2019-02-12T18:10:03Z

Fix #6210. Restricted addition of prerequisite skill ids to states to just a set of whitelisted exploration, which are listed in config_domain.WHITELISTED_EXPLORATION_IDS_FOR_PLAYTHROUGHS.
Also fixed the issue description.

codecov-io · 2019-02-12T19:28:27Z

Codecov Report

Merging #6255 into develop will decrease coverage by 0.01%.
The diff coverage is 25%.

@@             Coverage Diff             @@
##           develop    #6255      +/-   ##
===========================================
- Coverage    75.47%   75.46%   -0.02%     
===========================================
  Files          958      958              
  Lines        77879    77891      +12     
  Branches      4634     4640       +6     
===========================================
+ Hits         58777    58778       +1     
- Misses       19102    19113      +11

Impacted Files	Coverage Δ
...es/dev/head/services/ExplorationFeaturesService.js	`14.28% <ø> (ø)`	⬆️
...head/pages/state_editor/StateResponsesDirective.js	`0.36% <ø> (ø)`	⬆️
core/controllers/features.py	`100% <ø> (ø)`	⬆️
...ates/dev/head/components/OutcomeEditorDirective.js	`1.07% <0%> (-0.04%)`	⬇️
...head/pages/exploration_editor/ExplorationEditor.js	`5.12% <0%> (-0.04%)`	⬇️
...ad/components/OutcomeDestinationEditorDirective.js	`1.69% <0%> (-0.1%)`	⬇️
core/controllers/features_test.py	`100% <100%> (ø)`	⬆️
.../dev/head/pages/state_editor/StateEditorService.js	`57.33% <20%> (-2.67%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1554216...d1a5a93. Read the comment docs.

seanlip

Thanks @aks681! Took a pass and left some comments (but I think those comments have an effect on the rest of the PR).

seanlip · 2019-02-12T21:08:42Z

core/controllers/features.py

@@ -39,4 +39,6 @@ def get(self, exploration_id):
                config_domain.IS_IMPROVEMENTS_TAB_ENABLED.value,
            'is_playthrough_recording_enabled':
                exploration_id in whitelisted_exploration_ids_for_playthroughs,
+            'can_add_prerequisite_skill_to_state':


Normally we use this phrasing ("can do XXX") to convey something about the user's permissions, so perhaps you want to go with something like are_prerequisite_skills_editable instead.

Having said that, I think it would actually be a better idea to combine this and the above property into a single "is_exploration_whitelisted" variable, and use that instead throughout. /cc @brianrodri

I actually think it's a good idea to expose it as a separate FE variable, it'll make changing things much simpler in the future and helps distinguish the fact that the two are separate but related.

But they're really based on the same constant, right? I don't see them ever giving different results, so in that case shouldn't the branching happen in the frontend (and only when that branching is actually needed)?

Hi @aks681, I chatted with @brianrodri about this, and we suggest:

(a) Send a single value from the backend
(b) Use this to populate a service in the frontend that's dedicated to answering the question "is exploration curated" (or "is exploration part of story" or something). Not sure if this should be part of ExplorationContextService or something else.
(c) Any other frontend services that need this should use the service in (b) as a source of truth.

Does that sound good?

Since all editor directives already import StateEditorService and it already stores whether the editor is a question or exploration editor, shall I return is_exploration_whitelisted from the backend and set a corresponding value in it?
Also, should is_playthrough_recording_enabled be removed? Since that is an exploration feature and does not tell the context of the exploration.

I think that sounds OK to me (on both counts).

seanlip · 2019-02-12T21:09:45Z

core/templates/dev/head/components/OutcomeDestinationEditorDirective.js

@@ -37,7 +37,7 @@ oppia.directive('outcomeDestinationEditor', [
            EXPLORATION_AND_SKILL_ID_PATTERN, PLACEHOLDER_OUTCOME_DEST) {
          var currentStateName = null;
          $scope.canAddPrerequisiteSkill =
-            constants.ENABLE_NEW_STRUCTURE_EDITORS;
+            StateEditorService.getCanAddPrerequisiteSkill();


Should it be: "constants.ENABLE_NEW_STRUCTURE_EDITORS && isExplorationWhitelisted()"? I.e. if we are not removing the "enable" flag yet we should keep it everywhere, and remove it "all at once" later.

Ditto elsewhere.

Btw, just to check, refresher exps are also behind a whitelist flag, right?

Ok.
For refresher explorations, I think the restriction is that only admins/moderators can add them. Other than that, they can be added to any exploration.

Hm, I think that's OK (though please check that the restriction you cited is actually the case). We plan to remove that feature in the long run anyway so I guess it's up to you whether to migrate it or not to the new flag.

Yes, that is the case, as seen here, where permission to add refresher exploration is restricted to only admins and moderators.

Done initial comment. Added new structures flag check as well.

seanlip · 2019-02-12T21:12:01Z

core/templates/dev/head/pages/state_editor/StateResponsesDirective.js

@@ -323,6 +323,9 @@ oppia.directive('stateResponses', [

                    EditorFirstTimeEventsService.registerFirstSaveRuleEvent();

+                    if (!$scope.tmpOutcome.missingPrerequisiteSkillId) {


What is its value before being null?

Ideally we would set this value to null at the very outset, rather than having a possible case where it's "kinda null but not really".

This is null when initialised, but I think it becomes undefined when an invalid sequence is entered, after validation by ng-pattern, which was causing the error in the issue.
So, here I am just checking if it's undefined and changing value to null.

Hm, but I think it should never become undefined in the first place. Can we find a way to make that not happen?

Done, used ng-model-options as explained here.

seanlip · 2019-02-13T21:45:08Z

@aks681 -- just to double-check, does this actually need to go in the current release? I thought we were only turning on the new structures flag in March; does this have ill-effects on the Feb release if not merged?

nithusha21 · 2019-02-14T19:18:39Z

Confirmed with @aks681, this PR need not go out in Feb as it would only affect stuff when the new structures editor flag is true.

seanlip

Thanks @aks681! Some minor changes but otherwise it's good to go I think.

core/templates/dev/head/components/OutcomeDestinationEditorDirective.js

seanlip · 2019-02-17T19:46:41Z

core/templates/dev/head/components/outcome_destination_editor_directive.html

@@ -22,7 +22,7 @@
  </div>
  <div style="margin-top: 12px;" ng-if="isSelfLoop() && canAddPrerequisiteSkill">
    <b>Prerequisite skill ID (optional):&nbsp;</b>
-    <input type="text" name="missingPrerequisiteSkillId" ng-model="outcome.missingPrerequisiteSkillId" class="form-control" tabindex="0" aria-invalid="false" ng-pattern="explorationAndSkillIdPattern">
+    <input type="text" name="missingPrerequisiteSkillId" ng-model="outcome.missingPrerequisiteSkillId" class="form-control" tabindex="0" aria-invalid="false" ng-model-options="{allowInvalid: true}" ng-pattern="explorationAndSkillIdPattern">


Does this mean that a non-empty invalid string can still be saved? If so, that might be a problem.

No, it doesn't allow it to be saved. ng-model-options just makes sure the scope variable is never undefined, i.e even if it's invalid, the value will be stored in the scope variable, but the Save button will still be disabled and there is still a red outline around the text box (tested it locally). So, I don't think it will be an issue.

seanlip · 2019-02-17T19:47:37Z

core/templates/dev/head/components/outcome_editor_directive.html

@@ -95,7 +95,7 @@
            (try again, with refresher exploration "<[outcome.refresherExplorationId]>")
          </span>
        </span>
-        <div style="margin-top: 1em;" ng-if="outcome.missingPrerequisiteSkillId && canAddPrerequisiteSkill">
+        <div style="margin-top: 1em;" ng-if="outcome.missingPrerequisiteSkillId && isSelfLoop(outcome) && canAddPrerequisiteSkill">


Just to check, why must the outcome be a self loop? Sometimes we might redirect to a helper card if there's a missing prerequisite, but knowing that there's a missing prerequisite might give useful information for skill mastery updates.

Currently, prerequisite skill ID input text box is only visible if it is a self loop, just like refresher explorations.
If it is not a self loop, then what would be the flow for the learner? Would the learner be shown the skill card and then the next card?
Since, usually self loops correspond to wrong answers and an answer that leads to another card is correct. If the next card is related to clarifying a wrong answer also, we can remove that option altogether and link that answer group with a skill anyway, so I think a skill need only be linked to a self looped answer group, is that correct?

I think the flow for the learner and whether the answer group is annotated with a prereq skill are two separate things. We can still keep that info there but not display the prereq skill card (e.g. it may be useful to record that the learner hasn't mastered the prereq yet). So probably still worth letting it be edited, right?

Now, for the learner flow when it's not a self loop -- I don't know the answer yet. But I think we can figure out when we implement the skill cards. We could always use logic that's something like: if self-loop, show skill card, otherwise ignore skill card (for purposes of flow) and move to destination card.

What do you think? If you agree, maybe delete the added clause.

Actually, in addition to the above, let's add a new flag for enabling this feature separately and turn it off for now (for all explorations). It blocks on the following:

Validation of the skill ID that's being entered

Making it possible for creators to select the skill ID from a dropdown.

Please file an issue for this (with checkboxes), with the last step being to turn the flag on.

Done. Created issue #6341
Also, deleted the self loop clause.

core/templates/dev/head/pages/exploration_editor/ExplorationEditor.js

seanlip

@aks681 Just one more comment, but after that please feel free to merge. Thanks!

seanlip · 2019-02-24T18:31:04Z

core/templates/dev/head/components/outcome_editor_directive.html

@@ -95,7 +95,7 @@
            (try again, with refresher exploration "<[outcome.refresherExplorationId]>")
          </span>
        </span>
-        <div style="margin-top: 1em;" ng-if="outcome.missingPrerequisiteSkillId && canAddPrerequisiteSkill">
+        <div style="margin-top: 1em;" ng-if="outcome.missingPrerequisiteSkillId && isSelfLoop(outcome) && canAddPrerequisiteSkill">


Actually, in addition to the above, let's add a new flag for enabling this feature separately and turn it off for now (for all explorations). It blocks on the following:

Validation of the skill ID that's being entered

Making it possible for creators to select the skill ID from a dropdown.

Please file an issue for this (with checkboxes), with the last step being to turn the flag on.

seanlip · 2019-02-27T20:05:09Z

Thanks @aks681!

aks681 added 3 commits February 12, 2019 22:58

fix oppia#6210

ec15ded

fixed issue

1ea1691

fix lint

4a1046a

aks681 requested review from seanlip and bansalnitish February 12, 2019 18:10

seanlip requested changes Feb 12, 2019

View reviewed changes

bansalnitish added the PR: for current release label Feb 13, 2019

nithusha21 removed the PR: for current release label Feb 14, 2019

aks681 added 2 commits February 16, 2019 02:11

fixed merge conflicts

473da12

minor change

304dd07

nithusha21 added the CHANGELOG: Infrastructural changes label Feb 17, 2019

made review changes

caf7df2

seanlip requested changes Feb 17, 2019

View reviewed changes

made review changes

e080d60

seanlip approved these changes Feb 24, 2019

View reviewed changes

seanlip requested changes Feb 24, 2019

View reviewed changes

aks681 added 2 commits February 27, 2019 19:12

Merge remote-tracking branch 'upstream/develop' into fix-issue

b950400

made review change

d1a5a93

seanlip approved these changes Feb 27, 2019

View reviewed changes

seanlip merged commit 76f5d20 into oppia:develop Feb 27, 2019

aks681 deleted the fix-issue branch March 24, 2019 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6210: Added an exploration whitelist #6255

Fix #6210: Added an exploration whitelist #6255

aks681 commented Feb 12, 2019 •

edited by nithusha21

Loading

codecov-io commented Feb 12, 2019 •

edited

Loading

seanlip left a comment •

edited

Loading

seanlip Feb 12, 2019

brianrodri Feb 12, 2019

seanlip Feb 12, 2019 •

edited

Loading

seanlip Feb 15, 2019 •

edited

Loading

aks681 Feb 17, 2019 •

edited

Loading

seanlip Feb 17, 2019

aks681 Feb 17, 2019

seanlip Feb 12, 2019

aks681 Feb 13, 2019

seanlip Feb 13, 2019

aks681 Feb 15, 2019

aks681 Feb 15, 2019

seanlip Feb 12, 2019

aks681 Feb 13, 2019

seanlip Feb 13, 2019

aks681 Feb 15, 2019 •

edited

Loading

seanlip commented Feb 13, 2019

nithusha21 commented Feb 14, 2019

seanlip left a comment

seanlip Feb 17, 2019

aks681 Feb 23, 2019

seanlip Feb 17, 2019

aks681 Feb 23, 2019

seanlip Feb 24, 2019

seanlip Feb 24, 2019

aks681 Feb 27, 2019 •

edited

Loading

seanlip left a comment

seanlip Feb 24, 2019

seanlip commented Feb 27, 2019

		@@ -323,6 +323,9 @@ oppia.directive('stateResponses', [

		EditorFirstTimeEventsService.registerFirstSaveRuleEvent();

		if (!$scope.tmpOutcome.missingPrerequisiteSkillId) {

Fix #6210: Added an exploration whitelist #6255

Fix #6210: Added an exploration whitelist #6255

Conversation

aks681 commented Feb 12, 2019 • edited by nithusha21 Loading

codecov-io commented Feb 12, 2019 • edited Loading

Codecov Report

seanlip left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

seanlip Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

aks681 Feb 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

seanlip commented Feb 13, 2019

nithusha21 commented Feb 14, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aks681 Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Feb 27, 2019

aks681 commented Feb 12, 2019 •

edited by nithusha21

Loading

codecov-io commented Feb 12, 2019 •

edited

Loading

seanlip left a comment •

edited

Loading

seanlip Feb 12, 2019 •

edited

Loading

seanlip Feb 15, 2019 •

edited

Loading

aks681 Feb 17, 2019 •

edited

Loading

aks681 Feb 15, 2019 •

edited

Loading

aks681 Feb 27, 2019 •

edited

Loading