Collection skills migration #3376

wxyxinyu · 2017-04-29T19:11:39Z

This supercedes #3252, and introduces skills to both the frontend and backend.

codecov-io · 2017-04-29T20:05:32Z

Codecov Report

Merging #3376 into develop will increase coverage by 0.28%.
The diff coverage is 96.99%.

@@             Coverage Diff             @@
##           develop    #3376      +/-   ##
===========================================
+ Coverage    44.78%   45.07%   +0.28%     
===========================================
  Files          259      260       +1     
  Lines        20001    20113     +112     
  Branches      3146     3157      +11     
===========================================
+ Hits          8958     9066     +108     
- Misses       11043    11047       +4

Impacted Files	Coverage Δ
...d/domain/collection/CollectionNodeObjectFactory.js	`91.42% <ø> (ø)`	⬆️
...v/head/domain/collection/SkillListObjectFactory.js	`100% <100%> (ø)`	⬆️
.../head/domain/collection/CollectionUpdateService.js	`96.64% <100%> (+0.81%)`	⬆️
.../domain/collection/CollectionSkillObjectFactory.js	`100% <100%> (ø)`
...n_editor/editor_tab/CollectionLinearizerService.js	`100% <100%> (ø)`	⬆️
...d/domain/collection/CollectionValidationService.js	`100% <100%> (ø)`	⬆️
.../head/domain/collection/CollectionObjectFactory.js	`97.38% <92.72%> (-2.62%)`	⬇️

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 c50b196...3c13331. Read the comment docs.

wxyxinyu · 2017-05-01T03:56:23Z

A couple of points about the PR:

Is it ok that IDs are being generated both in the frontend and backend? If not, how can this be made better?
Should there be more validation happening?

BenHenning

Thanks @wxyxinyu! Couple more things:

I saw you added new validate functionality in the backend (both in Skill and in Collection); can you make sure there are corresponding tests for the new validate() calls?
Can you make sure CollectionValidatorService and CollectionValidatorServiceSpec is appropriately updated with the new validations that both the backend and frontend collection objects need to pass?

BenHenning · 2017-05-06T19:42:41Z

core/domain/collection_domain.py

@@ -199,9 +207,19 @@ def skills(self):
        return set(self.prerequisite_skills) | set(self.acquired_skills)

    def update_prerequisite_skills(self, prerequisite_skills):
+        """Updates prerequisite skills of collection node.
+
+        Note: Please use the add_collection_node_prerequisite_skill


If we don't want people using this or update_acquired_skills, why don't we rmeove them?

the update functions in collection_services still call these functions to update the actual skills, but those check for e.g. existence of skill ids before adding them.

note to self: this docstring is slightly wrong, it's now collections_services.update_collection_node_prerequisite_skills

Discussed during TL meeting: we want validation to be in the lowest common place, in this case in the domain object since there's no requirement for the services function to need to do any additional validation.

BenHenning · 2017-05-06T19:43:07Z

core/domain/collection_domain.py

+
+        Note: Please use the add_collection_node_prerequisite_skill
+        and delete_collection_node_prerequisite_skill functions to update
+        skills, as they check for consistency with the skills dict."""


Nit (style): multi-line docstring should have the """ ending on its own line. Ditto elsewhere.

BenHenning · 2017-05-06T19:44:36Z

core/domain/collection_domain.py

+        )
+
+    def validate(self):
+        """Validates various properties of collection skill."""


Nit (style): Single line docstrings should have an empty line between them and the code. Ditto elsewhere.

BenHenning · 2017-05-06T19:47:02Z

core/domain/collection_domain.py

+                'The question_ids list has duplicate entries: %s' %
+                self.question_ids)
+
+        for question_id in self.question_ids:


I recommend putting this validation before the uniqueness validation for question_ids. Right now, if I constructed a new CollectionSkill object from a list a dicts, it would fail in the uniqueness check above because the set() constructor would be trying to uniqueify a list of non-hashable types (dicts), which would be a confusing error. It would fail more clearly in this validation since dicts are not strings.

BenHenning · 2017-05-06T19:47:22Z

core/domain/collection_domain.py

 class Collection(object):
    """Domain object for an Oppia collection."""

    def __init__(self, collection_id, title, category, objective,
-                 language_code, tags, schema_version, nodes, version,
+                 language_code, tags, schema_version, nodes, skills,
+                 skill_id_count, version,
                 created_on=None, last_updated=None):


Nit: can you move this to the line above so that it wraps nicely?

BenHenning · 2017-05-06T20:38:19Z

core/templates/dev/head/domain/collection/CollectionSkillObjectFactory.js

+      return this._id;
+    };
+
+    // Returns the name of the skill.


Is this immutable also?

could you explain what immutable-ness is?

Immutable means that the caller can't change the internal state of the value.

no, we should be able to rename skills (this is to be implemented in a separate PR)

BenHenning · 2017-05-06T20:41:13Z

core/templates/dev/head/domain/collection/CollectionSkillObjectFactory.js

+      return new CollectionSkill(skillId, collectionSkillBackendObject);
+    };
+
+    // TODO(wxy): Ensure this matches the backend dict elements for


Is this a TODO or a note to developers?

not sure, there is a similar TODO for nodes. Is that one meant to be a note for a future test, or a note for developers?

in any case I changed it to a note to developers, since that seems to make more sense

BenHenning · 2017-05-06T20:44:03Z

core/templates/dev/head/pages/collection_editor/CollectionEditorStateServiceSpec.js


      // Simply applying a change is adequate for updating the skill list.
      var collection = CollectionEditorStateService.getCollection();
      CollectionUpdateService.setAcquiredSkills(
-        collection, '0', ['skill2', 'skill3']);
+        collection, '0', ['s1', 'skill3']);


We seem to be mixing skill names and IDs. Is that intentional?

no, they're all supposed to be skill IDs now

Is this going to be addressed in a follow-up commit, then? :)

BenHenning · 2017-05-06T20:45:21Z

core/templates/dev/head/pages/collection_editor/editor_tab/CollectionLinearizerService.js

@@ -205,12 +208,15 @@ oppia.factory('CollectionLinearizerService', [
          return false;
        }

+        var linearNodeList = _getCollectionNodesInPlayableOrder(collection);


Why pull out & add these new variables?

it's so that the skill associated to the node can be deleted as well (originally only the node is deleted)

BenHenning · 2017-05-06T20:51:30Z

core/templates/dev/head/domain/collection/CollectionObjectFactory.js

@@ -203,6 +223,85 @@ oppia.factory('CollectionObjectFactory', [
      return skillList;
    };

+    // Gets a new ID for a skill. This should be of the same form as in the
+    // backend.
+    Collection.prototype.getNewSkillId = function() {


You may want to mention in the documentation that this method has side effects (the skillIdCount/nextSkillId field is going to increment)

wxyxinyu

Thanks for the review, I added some comments addressing your comments. The rest I intend to fix in a commit coming up soon.

wxyxinyu · 2017-05-09T01:19:04Z

core/domain/collection_domain.py

@@ -199,9 +207,19 @@ def skills(self):
        return set(self.prerequisite_skills) | set(self.acquired_skills)

    def update_prerequisite_skills(self, prerequisite_skills):
+        """Updates prerequisite skills of collection node.
+
+        Note: Please use the add_collection_node_prerequisite_skill


the update functions in collection_services still call these functions to update the actual skills, but those check for e.g. existence of skill ids before adding them.

note to self: this docstring is slightly wrong, it's now collections_services.update_collection_node_prerequisite_skills

wxyxinyu · 2017-05-09T01:21:24Z

core/domain/collection_domain.py

+                raise ValueError(
+                    'Skill with name "%s" already exists.' % skill_name)
+
+        skill_id = 's' + str(self.skill_id_count)


it's just to make extra clear that the skill id is a string, not an integer

wxyxinyu · 2017-05-09T01:23:42Z

core/domain/collection_services.py

@@ -868,7 +947,10 @@ def update_collection(
            'Collection is public so expected a commit message but '
            'received none.')

+    collection = get_collection_by_id(collection_id)


no clue. might be a merge error?

wxyxinyu · 2017-05-09T01:24:47Z

core/templates/dev/head/domain/collection/CollectionObjectFactory.js

@@ -192,6 +211,7 @@ oppia.factory('CollectionObjectFactory', [
    // skills from all collections nodes within this domain object. Please note
    // this operation returns a new skill list everytime this function is
    // called.
+    // TODO(wxy): migrate this to use the skills dict instead.


yes, SkillListObjectFactorys are used for lists of skill ids in prereq/acquired skills

wxyxinyu · 2017-05-09T01:25:33Z

core/templates/dev/head/domain/collection/CollectionSkillObjectFactory.js

+      return this._id;
+    };
+
+    // Returns the name of the skill.


could you explain what immutable-ness is?

wxyxinyu · 2017-05-09T01:26:26Z

core/templates/dev/head/pages/collection_editor/CollectionEditorStateServiceSpec.js


      // Simply applying a change is adequate for updating the skill list.
      var collection = CollectionEditorStateService.getCollection();
      CollectionUpdateService.setAcquiredSkills(
-        collection, '0', ['skill2', 'skill3']);
+        collection, '0', ['s1', 'skill3']);


no, they're all supposed to be skill IDs now

wxyxinyu · 2017-05-09T01:27:53Z

core/templates/dev/head/pages/collection_editor/editor_tab/CollectionLinearizerService.js

@@ -205,12 +208,15 @@ oppia.factory('CollectionLinearizerService', [
          return false;
        }

+        var linearNodeList = _getCollectionNodesInPlayableOrder(collection);


it's so that the skill associated to the node can be deleted as well (originally only the node is deleted)

…lls to acquired_skill_ids

BenHenning

Thanks @wxyxinyu, this is looking awesome! It's really coming together now. :D I had a few comments regarding consistency in naming, and making sure we have enough and well-written tests. That being said, I'm also slightly concerned over SkillListObjectFactory and whether we're properly keeping it updated now. Have you tested the collection editor flows manually to make sure everything is still displaying & working correctly?

BenHenning · 2017-06-02T21:41:16Z

core/domain/collection_domain.py

@@ -36,8 +36,8 @@
 COLLECTION_PROPERTY_OBJECTIVE = 'objective'
 COLLECTION_PROPERTY_LANGUAGE_CODE = 'language_code'
 COLLECTION_PROPERTY_TAGS = 'tags'
-COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILLS = 'prerequisite_skills'
-COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS = 'acquired_skills'
+COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILLS = 'prerequisite_skill_ids'


Should this be renamed to COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILL_IDS? It's not actually changing skills now. Plus, it's technically a new command.

Can you add a comment to the beginning of the section explaining that 'prerequisite_skills' and 'acquired_skills' are now reserved and shouldn't be reused in the future? There's an example of doing this in exp_domain.

BenHenning · 2017-06-02T21:42:55Z

core/controllers/collection_viewer.py

@@ -68,7 +68,7 @@ def get(self, collection_id):
            'is_logged_in': bool(self.user_id),
            'collection_id': collection_id,
            'collection_title': collection.title,
-            'collection_skills': collection.skills,
+            'collection_skills': collection.skills.keys(),


Might we instead introduce a property like skill_names or skill_ids? Can you also make sure we properly refer to skills, skill IDs, and skill names correctly in all places referring to skills? You're changing the meaning of what 'skill' is and we want to be careful not to use the wrong word, as it will just introduce confusion. In this case, I suspect this ID should actually be collection_skill_ids.

this isn't being used anywhere; I removed it

BenHenning · 2017-06-02T21:46:36Z

core/domain/collection_domain.py

+    def from_dict(cls, skill_id, skill_dict):
+        return cls(
+            skill_id,
+            copy.deepcopy(skill_dict['name']),


Is it worth deep copying strings? I thought strings are immutable in Python.

the deepcopy seems to be done in other locations though

BenHenning · 2017-06-02T21:47:44Z

core/domain/collection_domain.py

+            copy.deepcopy(skill_dict['question_ids'])
+        )
+
+    def validate(self):


Are there tests verifying each of these new potential validation failures?

BenHenning · 2017-06-02T21:51:20Z

core/domain/collection_domain.py

+        """
+
+        for index, node in enumerate(collection_contents['nodes']):
+            collection_contents['nodes'][index]['prerequisite_skill_ids'] = (


I'm slightly confused by this. Why are we assigning prerequisite_skills to prerequisite_skill_ids? The IDs aren't skill names, right?

yeah, I just did the transformation in a really convoluted way

BenHenning · 2017-06-02T22:35:23Z

core/templates/dev/head/domain/collection/CollectionUpdateService.js

 oppia.constant(
-  'COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILLS', 'prerequisite_skills');
-oppia.constant('COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS', 'acquired_skills');
+  'COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILLS', 'prerequisite_skill_ids');


Should these constants also be renamed, per the similar comment earlier in collection_domain?

BenHenning · 2017-06-02T22:36:48Z

core/templates/dev/head/domain/collection/CollectionUpdateService.js

+       * undo/redo service.
+       */
+      addCollectionSkill: function(collection, skillName) {
+        var oldnextSkillId = collection.getNextSkillId();


oldNextSkillId

BenHenning · 2017-06-02T22:41:54Z

core/templates/dev/head/domain/collection/CollectionValidationService.js

@@ -117,6 +117,17 @@ oppia.factory('CollectionValidationService', [
          'learner.');
      }

+      var collectionSkills = collection.getCollectionSkills();


You could also use map() here, e.g.

collection.getCollectionSkills().map(function(collectionSkill) { return collectionSkill.getName(); }).forEach(function(skillName, index) { ... });

I'm not sure how this would work, since skills is an object and not an array.

BenHenning · 2017-06-02T22:44:53Z

core/templates/dev/head/domain/collection/SkillListObjectFactory.js

-  SkillList.prototype.removeSkillByName = function(skillName) {
-    var index = this.indexOfSkill(skillName);
+  SkillList.prototype.removeSkillById = function(skillId) {
+    var index = this.indexOfSkill(skillId);


Should indexOfSkill be renamed to indexOfSkillId?

I'm not refactoring the SkillList-related naming for now, since it might get removed entirely.

BenHenning · 2017-06-02T22:49:19Z

core/templates/dev/head/pages/collection_editor/editor_tab/CollectionLinearizerService.js

+        var nodeIndex = findNodeIndex(linearNodeList, explorationId);
+        var collectionNode = linearNodeList[nodeIndex];
+        var acquiredSkill =
+          collectionNode.getAcquiredSkillList().getSkills()[0];


I'm a bit iffy about using index 0; is there a better way for us to find out the skill ID associated with the exploration ID? Maybe using the exploration title to look up the corresponding skill ID? I'm not sure whether we can make the guarantee that acquired skills are going to remain ordered, or if we should.

I think the only guarantee that we're using is the linearity of the structure (there's only 1 element in the array), and that each acquired skill is only given by one node (so we should delete the acquired skill). The only way these assumptions would seem to change is if the concept of a "linearlizer" changes.

seanlip

Hi @wxyxinyu -- @BenHenning had less time than he expected leaving for his trip, and couldn't look at it in detail, but he sent me this note regarding your PR: "I trust your opinion on Xinyu's PR. It looked fine to me after the last big round of comments, other than the one thing I pointed out -- i.e. the skill list object factory bit. Since it sounds like that was resolved, I defer judgement to you."

My opinion is that if you think it's good, I'm happy, and I'm going to give LGTM. (I did do a quick skim and I didn't notice anything obviously wrong.) That said, if there are any specific parts you feel iffy about or would like me to look at prior to merge, could you please flag them for me? I would be happy to give you a second opinion on those.

Finally, thank you very much for doing this! I'm glad we can get the collections stuff in good shape for introducing questions.

seanlip · 2017-07-22T01:57:36Z

Update: @wxyxinyu will do a pass and test everything again, prior to final merge.

Ben is away.

wxyxinyu · 2017-07-24T02:50:29Z

core/domain/collection_domain.py

@@ -592,6 +733,48 @@ def _convert_collection_contents_v2_dict_to_v3_dict(
        return collection_contents

    @classmethod
+    def _convert_collection_contents_v3_dict_to_v4_dict(


@seanlip can you look at this function?

wxyxinyu · 2017-07-24T02:52:16Z

core/templates/dev/head/pages/collection_editor/editor_tab/CollectionLinearizerService.js

@@ -184,8 +184,11 @@ oppia.factory('CollectionLinearizerService', [
        var linearNodeList = _getCollectionNodesInPlayableOrder(collection);
        CollectionUpdateService.addCollectionNode(
          collection, explorationId, summaryBackendObject);
+        var skillName = summaryBackendObject.title;


@seanlip and also the changes in this file

LGTM assuming we're using skills where we need to use skills, and skill IDs where we need to use skill IDs.

wxyxinyu added 3 commits April 29, 2017 13:24

skills migration

59aec70

fix bugs

2112444

lint

0de04e5

wxyxinyu assigned BenHenning Apr 29, 2017

wxyxinyu requested a review from BenHenning April 29, 2017 19:11

BenHenning mentioned this pull request May 2, 2017

Preliminary skills migration #3252

Closed

BenHenning requested changes May 6, 2017

View reviewed changes

wxyxinyu commented May 9, 2017

View reviewed changes

wxyxinyu added 5 commits May 29, 2017 21:36

Respond to review comments

632e94d

remove skill id from prereq and acquired skills when skill is deleted

7de7f80

Rename prerequisite_skills to prerequisite_skill_ids and acquired_ski…

924149e

…lls to acquired_skill_ids

fix test

71c0b68

Merge branch 'develop' into collection-skills-migration-v2

74c43b4

BenHenning previously requested changes Jun 2, 2017

View reviewed changes

wxyxinyu mentioned this pull request Jun 4, 2017

Create CollectionSkill object and modify CollectionModel to add skills #2997

Closed

2 tasks

BenHenning assigned wxyxinyu and unassigned BenHenning Jun 20, 2017

wxyxinyu added 2 commits July 15, 2017 00:48

Respond to review

efbc709

Add more validation

b934761

seanlip approved these changes Jul 20, 2017

View reviewed changes

Merge branch 'develop' into collection-skills-migration-v2

3c13331

seanlip added the PR: LGTM label Jul 24, 2017

wxyxinyu commented Jul 24, 2017

View reviewed changes

wxyxinyu merged commit 1612c32 into develop Jul 24, 2017

wxyxinyu deleted the collection-skills-migration-v2 branch July 24, 2017 03:06

seanlip mentioned this pull request Jul 24, 2017

Deprecate SkillListObjectFactory. Remove unneeded skill methods from the frontend. Use skill IDs throughout where possible. #3676

Merged

Collection skills migration #3376

Collection skills migration #3376

Conversation

wxyxinyu commented Apr 29, 2017

codecov-io commented Apr 29, 2017 • edited Loading

Codecov Report

wxyxinyu commented May 1, 2017

BenHenning 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

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

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

wxyxinyu 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

Choose a reason for hiding this comment

BenHenning 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

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

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

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Jul 22, 2017

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

codecov-io commented Apr 29, 2017 •

edited

Loading