Skip to content

Commit

Permalink
Collection skills migration (oppia#3376)
Browse files Browse the repository at this point in the history 
* skills migration

* fix bugs

* lint

* Respond to review comments

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

* Rename prerequisite_skills to prerequisite_skill_ids and acquired_skills to acquired_skill_ids

* fix test

* Respond to review

* Add more validation
  • Loading branch information
@wxyxinyu
wxyxinyu authored Jul 24, 2017
1 parent 3bd7c3d commit 1612c32
Show file tree
Hide file tree
Showing 28 changed files with 1,633 additions and 360 deletions.
1 change: 0 additions & 1 deletion core/controllers/collection_viewer.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ def get(self, collection_id):
'is_logged_in': bool(self.user_id),
'collection_id': collection_id,
'collection_title': collection.title,
'collection_skills': collection.skills,
'is_private': rights_manager.is_collection_private(collection_id),
'meta_name': collection.title,
'meta_description': utils.capitalize_string(collection.objective)
Expand Down
428 changes: 334 additions & 94 deletions core/domain/collection_domain.py
View file

Large diffs are not rendered by default.

395 changes: 327 additions & 68 deletions core/domain/collection_domain_test.py
View file

Large diffs are not rendered by default.

12 changes: 10 additions & 2 deletions core/domain/collection_jobs_one_off_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,11 @@ def test_migration_job_migrates_collection_nodes(self):
objective='An objective',
tags=[],
schema_version=2,
nodes=[node.to_dict()],
nodes=[{
'exploration_id': self.EXP_ID,
'prerequisite_skills': [],
'acquired_skills': []
}],
)
model.commit(self.albert_id, 'Made a new collection!', [{
'cmd': collection_services.CMD_CREATE_NEW,
Expand All @@ -185,4 +189,8 @@ def test_migration_job_migrates_collection_nodes(self):

new_model = collection_models.CollectionModel.get(self.COLLECTION_ID)
self.assertEqual(
new_model.collection_contents, {'nodes': [node.to_dict()]})
new_model.collection_contents, {
'nodes': [node.to_dict()],
'skills': {},
'next_skill_id': 0
})
38 changes: 30 additions & 8 deletions core/domain/collection_services.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,14 @@ def get_collection_from_model(collection_model, run_conversion=True):
collection_domain.CollectionNode.from_dict(collection_node_dict)
for collection_node_dict in
versioned_collection_contents['collection_contents']['nodes']
],
], {
skill_id: collection_domain.CollectionSkill.from_dict(
skill_id, skill_dict)
for skill_id, skill_dict in
versioned_collection_contents[
'collection_contents']['skills'].iteritems()
},
versioned_collection_contents['collection_contents']['next_skill_id'],
collection_model.version, collection_model.created_on,
collection_model.last_updated)

Expand Down Expand Up @@ -593,12 +600,12 @@ def apply_change_list(collection_id, change_list):
collection_domain.CMD_EDIT_COLLECTION_NODE_PROPERTY):
collection_node = collection.get_node(change.exploration_id)
if (change.property_name ==
collection_domain.COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILLS): # pylint: disable=line-too-long
collection_node.update_prerequisite_skills(
collection_domain.COLLECTION_NODE_PROPERTY_PREREQUISITE_SKILL_IDS): # pylint: disable=line-too-long
collection_node.update_prerequisite_skill_ids(
change.new_value)
elif (change.property_name ==
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS): # pylint: disable=line-too-long
collection_node.update_acquired_skills(change.new_value)
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILL_IDS): # pylint: disable=line-too-long
collection_node.update_acquired_skill_ids(change.new_value)
elif change.cmd == collection_domain.CMD_EDIT_COLLECTION_PROPERTY:
if (change.property_name ==
collection_domain.COLLECTION_PROPERTY_TITLE):
Expand All @@ -623,6 +630,10 @@ def apply_change_list(collection_id, change_list):
# latest schema version. As a result, simply resaving the
# collection is sufficient to apply the schema migration.
continue
elif change.cmd == collection_domain.CMD_ADD_COLLECTION_SKILL:
collection.add_skill(change.name)
elif change.cmd == collection_domain.CMD_DELETE_COLLECTION_SKILL:
collection.delete_skill(change.skill_id)
return collection

except Exception as e:
Expand Down Expand Up @@ -727,7 +738,12 @@ def _save_collection(committer_id, collection, commit_message, change_list):
collection_model.collection_contents = {
'nodes': [
collection_node.to_dict() for collection_node in collection.nodes
]
],
'skills': {
skill_id: skill.to_dict()
for skill_id, skill in collection.skills.iteritems()
},
'next_skill_id': collection.next_skill_id
}
collection_model.node_count = len(collection_model.nodes)
collection_model.commit(committer_id, commit_message, change_list)
Expand Down Expand Up @@ -765,8 +781,13 @@ def _create_collection(committer_id, collection, commit_message, commit_cmds):
'nodes': [
collection_node.to_dict()
for collection_node in collection.nodes
]
}
],
'skills': {
skill_id: skill.to_dict()
for skill_id, skill in collection.skills.iteritems()
},
'next_skill_id': collection.next_skill_id
},
)
model.commit(committer_id, commit_message, commit_cmds)
collection.version += 1
Expand Down Expand Up @@ -895,6 +916,7 @@ def update_collection(
'received none.')

collection = apply_change_list(collection_id, change_list)

_save_collection(committer_id, collection, commit_message, change_list)
update_collection_summary(collection.id, committer_id)

Expand Down
157 changes: 118 additions & 39 deletions core/domain/collection_services_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,15 +190,18 @@ def test_get_next_exploration_ids_to_complete_by_user(self):
collection_services.update_collection(
self.owner_id, self.COL_ID_0,
[{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'skill0'
}, {
'cmd': collection_domain.CMD_EDIT_COLLECTION_NODE_PROPERTY,
'exploration_id': self.EXP_ID_0,
'property_name': 'acquired_skills',
'new_value': ['0_exp_skill']
'property_name': 'acquired_skill_ids',
'new_value': ['skill0']
}, {
'cmd': collection_domain.CMD_EDIT_COLLECTION_NODE_PROPERTY,
'exploration_id': self.EXP_ID_2,
'property_name': 'prerequisite_skills',
'new_value': ['0_exp_skill']
'property_name': 'prerequisite_skill_ids',
'new_value': ['skill0']
}],
'Updated exp 2 to require exp 0 before being playable')

Expand Down Expand Up @@ -913,50 +916,131 @@ def test_update_collection_tags(self):
'new_value': ['duplicate', 'duplicate']
}], 'Add a new tag')

def test_update_collection_node_prerequisite_skills(self):
# Verify initial prerequisite skills are empty.
def test_add_collection_skill(self):
# Verify skills table is initially empty.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
self.assertEqual(collection.skills, {})

# Add a skill.
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'test'
}], 'Add a new skill')

# Verify that the skill is added.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
self.assertEqual(collection.skills.keys(), ['skill0'])

def test_adding_duplicate_collection_skill_raises_error(self):
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'test'
}], 'Add a new skill')

# Verify that error will be thrown when duplicate skill is added.
with self.assertRaisesRegexp(
ValueError,
'Skill with name "test" already exists.'):
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'test'
}], 'Add a new skill, again')

def test_delete_collection_skill(self):
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'test'
}], 'Add a new skill')

# Delete a skill.
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_DELETE_COLLECTION_SKILL,
'skill_id': 'skill0'
}], 'Delete skill')

# Verify that the skill is deleted.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
self.assertEqual(collection.skills.keys(), [])


def test_update_collection_node_prerequisite_skill_ids(self):
# Verify initial prerequisite skill ids are empty.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
collection_node = collection.get_node(self.EXPLORATION_ID)
self.assertEqual(collection_node.prerequisite_skills, [])
self.assertEqual(collection_node.prerequisite_skill_ids, [])

# Add skills
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'new skill 1'
}], 'Add a new skill')
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'new skill 2'
}], 'Add a new skill')

# Update the prerequisite skills.
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_EDIT_COLLECTION_NODE_PROPERTY,
'exploration_id': self.EXPLORATION_ID,
'property_name': 'prerequisite_skills',
'new_value': ['first', 'second']
}], 'Changed the prerequisite skills of a collection node')
'property_name': 'prerequisite_skill_ids',
'new_value': ['skill0', 'skill1']
}], 'Changed the prerequisite skill ids of a collection node')

# Verify the prerequisites are different.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
collection_node = collection.get_node(self.EXPLORATION_ID)
self.assertEqual(
collection_node.prerequisite_skills, ['first', 'second'])
collection_node.prerequisite_skill_ids, ['skill0', 'skill1'])

def test_update_collection_node_acquired_skills(self):
# Verify initial acquired skills are empty.
def test_update_collection_node_acquired_skill_ids(self):
# Verify initial acquired skill ids are empty.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
collection_node = collection.get_node(self.EXPLORATION_ID)
self.assertEqual(collection_node.acquired_skills, [])
self.assertEqual(collection_node.acquired_skill_ids, [])

# Add skills
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'new skill 1'
}], 'Add a new skill')
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_ADD_COLLECTION_SKILL,
'name': 'new skill 2'
}], 'Add a new skill')

# Update the acquired skills.
# Update the acquired skill ids.
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, [{
'cmd': collection_domain.CMD_EDIT_COLLECTION_NODE_PROPERTY,
'exploration_id': self.EXPLORATION_ID,
'property_name': 'acquired_skills',
'new_value': ['third', 'fourth']
}], 'Changed the acquired skills of a collection node')
'property_name': 'acquired_skill_ids',
'new_value': ['skill0', 'skill1']
}], 'Changed the acquired skill ids of a collection node')

# Verify the acquired are different.
collection = collection_services.get_collection_by_id(
self.COLLECTION_ID)
collection_node = collection.get_node(self.EXPLORATION_ID)
self.assertEqual(collection_node.acquired_skills, ['third', 'fourth'])
self.assertEqual(
collection_node.acquired_skill_ids, ['skill0', 'skill1'])


def _get_node_change_list(exploration_id, property_name, new_value):
"""Generates a change list for a single collection node change."""
Expand Down Expand Up @@ -1009,10 +1093,9 @@ def test_record_commit_message(self):
rights_manager.publish_exploration(self.owner_id, self.EXP_ID)

collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, _get_node_change_list(
self.EXP_ID,
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS,
['Skill']), 'A message')
self.owner_id, self.COLLECTION_ID, _get_collection_change_list(
collection_domain.COLLECTION_PROPERTY_TITLE,
'New Title'), 'A message')

self.assertEqual(
collection_services.get_collection_snapshots_metadata(
Expand All @@ -1029,31 +1112,27 @@ def test_demand_commit_message(self):
'received none.'
):
collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, _get_node_change_list(
self.EXP_ID,
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS,
['Skill']), '')
self.owner_id, self.COLLECTION_ID, _get_collection_change_list(
collection_domain.COLLECTION_PROPERTY_TITLE,
'New Title'), '')

def test_unpublished_collections_can_accept_commit_message(self):
"""Test unpublished collections can accept optional commit messages."""

collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, _get_node_change_list(
self.EXP_ID,
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS,
['Skill']), 'A message')
self.owner_id, self.COLLECTION_ID, _get_collection_change_list(
collection_domain.COLLECTION_PROPERTY_TITLE,
'New Title'), 'A message')

collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, _get_node_change_list(
self.EXP_ID,
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS,
['Skill']), '')
self.owner_id, self.COLLECTION_ID, _get_collection_change_list(
collection_domain.COLLECTION_PROPERTY_TITLE,
'New Title'), '')

collection_services.update_collection(
self.owner_id, self.COLLECTION_ID, _get_node_change_list(
self.EXP_ID,
collection_domain.COLLECTION_NODE_PROPERTY_ACQUIRED_SKILLS,
['Skill']), None)
self.owner_id, self.COLLECTION_ID, _get_collection_change_list(
collection_domain.COLLECTION_PROPERTY_TITLE,
'New Title'), None)


class CollectionSnapshotUnitTests(CollectionServicesUnitTests):
Expand Down
1 change: 0 additions & 1 deletion core/domain/summary_services.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ def get_learner_collection_dict_by_id(
completed_exp_ids = []

collection_dict = collection.to_dict()
collection_dict['skills'] = collection.skills
collection_dict['playthrough_dict'] = {
'next_exploration_ids': next_exploration_ids,
'completed_exploration_ids': completed_exp_ids
Expand Down
16 changes: 8 additions & 8 deletions core/templates/dev/head/domain/collection/CollectionNodeObjectFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ oppia.factory('CollectionNodeObjectFactory', [
var CollectionNode = function(collectionNodeBackendObject) {
this._explorationId = collectionNodeBackendObject.exploration_id;
this._prerequisiteSkillList = SkillListObjectFactory.create(
collectionNodeBackendObject.prerequisite_skills);
collectionNodeBackendObject.prerequisite_skill_ids);
this._acquiredSkillList = SkillListObjectFactory.create(
collectionNodeBackendObject.acquired_skills);
collectionNodeBackendObject.acquired_skill_ids);
this._explorationSummaryObject = angular.copy(
collectionNodeBackendObject.exploration_summary);
};
Expand Down Expand Up @@ -72,14 +72,14 @@ oppia.factory('CollectionNodeObjectFactory', [
}
};

// Returns a SkillsList object of the prerequisite skills of this collection
// node. Changes to the return SkillList object will be reflected in this
// collection node object.
// Returns a SkillsList object of the prerequisite skill ids of this
// collection node. Changes to the return SkillList object will be reflected
// in this collection node object.
CollectionNode.prototype.getPrerequisiteSkillList = function() {
return this._prerequisiteSkillList;
};

// Returns a SkillsList object of the acquired skills of this collection
// Returns a SkillsList object of the acquired skill ids of this collection
// node. Changes to the return SkillList object will be reflected in this
// collection node object.
CollectionNode.prototype.getAcquiredSkillList = function() {
Expand Down Expand Up @@ -120,8 +120,8 @@ oppia.factory('CollectionNodeObjectFactory', [
CollectionNode.createFromExplorationId = function(explorationId) {
return CollectionNode.create({
exploration_id: explorationId,
acquired_skills: [],
prerequisite_skills: [],
acquired_skill_ids: [],
prerequisite_skill_ids: [],
exploration_summary: null
});
};
Expand Down
Loading

0 comments on commit 1612c32

Please sign in to comment.