Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement editing of questions #5614

Merged
merged 46 commits into from
Dec 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
d9f2824
Save.
tjiang11 Aug 30, 2018
f0cb957
Save.
tjiang11 Aug 30, 2018
9c8e702
Save. UrlInterpolationError topic null.
tjiang11 Aug 30, 2018
c93f186
Save.
tjiang11 Aug 30, 2018
f695dd6
Modify backend to return associated skills when fetching a question.
tjiang11 Aug 30, 2018
066faec
save.
tjiang11 Aug 30, 2018
07a11fb
Saving content works.
tjiang11 Aug 30, 2018
2386f69
Copy interaction.
tjiang11 Aug 30, 2018
4360648
Apply QuestionUpdateService for all changes.
tjiang11 Aug 30, 2018
a68adef
Circumvent race condition on initialization.
tjiang11 Aug 30, 2018
dd9d25f
Lint.
tjiang11 Aug 30, 2018
8a2c2bd
Clone UndoRedoService for questions.
tjiang11 Aug 30, 2018
4782099
Fetch question summaries after edit question.
tjiang11 Aug 30, 2018
a20b0ab
Create reusable function in QuestionEditorDirective.
tjiang11 Aug 30, 2018
4668f51
Lint.
tjiang11 Aug 30, 2018
fd91189
Add test.
tjiang11 Sep 1, 2018
2f4f1cc
Fix test.
tjiang11 Sep 1, 2018
3050886
Lint.
tjiang11 Sep 1, 2018
6d52d8c
Disable flag.
tjiang11 Sep 1, 2018
43807b1
Review changes.
tjiang11 Sep 3, 2018
7a4ff6f
Review changes.
tjiang11 Sep 9, 2018
41798c6
Review comments.
tjiang11 Sep 13, 2018
c496793
Lint.
tjiang11 Sep 13, 2018
fbdaa83
Review changes.
tjiang11 Sep 23, 2018
732be38
Deduplicate UndoRedoService.
tjiang11 Sep 23, 2018
7756f6d
Move updateFunction into QuestionUpdateService.
tjiang11 Sep 23, 2018
17ca89f
Fix test.
tjiang11 Oct 14, 2018
5785784
Lint.
tjiang11 Oct 14, 2018
13254ea
Update documentation.
tjiang11 Oct 15, 2018
fc4981d
Add tests for get_models_by_question_id
tjiang11 Oct 15, 2018
9ef2940
Update setQuestionStateData.
tjiang11 Oct 15, 2018
5861a5a
Fix test.
tjiang11 Oct 15, 2018
4ccea9c
Fix test.
tjiang11 Oct 15, 2018
0c589eb
Merge develop.
tjiang11 Oct 15, 2018
c0ad966
Lint.
tjiang11 Oct 15, 2018
9896d99
Add test.
tjiang11 Oct 25, 2018
4d3c329
Merge develop.
tjiang11 Oct 28, 2018
d37ab38
Lint.
tjiang11 Oct 28, 2018
f6f7e09
Edit questions in modal.
tjiang11 Oct 28, 2018
5722ca1
Add blank commit message.
tjiang11 Nov 4, 2018
3b5983f
Remove duplicate function; Fix issue with not being able to edit a qu…
tjiang11 Dec 1, 2018
c4c9b93
Merge develop.
tjiang11 Dec 1, 2018
98227d0
Lint.
tjiang11 Dec 1, 2018
5abc2c8
Lint.
tjiang11 Dec 1, 2018
387a9cb
Fix.
tjiang11 Dec 1, 2018
7a10a7b
Flip flag.
tjiang11 Dec 7, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions core/controllers/question_editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,17 @@ def get(self, question_id):
raise self.PageNotFoundException(
'The question with the given id doesn\'t exist.')

associated_skill_ids = [link.skill_id for link in (
question_services.get_question_skill_links_of_question(
question_id))]
associated_skills = skill_services.get_multi_skills(
associated_skill_ids)
associated_skill_dicts = [
skill.to_dict() for skill in associated_skills]

self.values.update({
'question_dict': question.to_dict()
'question_dict': question.to_dict(),
'associated_skill_dicts': associated_skill_dicts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

})
self.render_json(self.values)

Expand Down Expand Up @@ -154,7 +163,7 @@ def put(self, question_id):

question_dict = question_services.get_question_by_id(
question_id).to_dict()
return self.render_json({
self.render_json({
'question_dict': question_dict
})

Expand Down
23 changes: 23 additions & 0 deletions core/controllers/question_editor_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,14 @@ def test_delete(self):

class EditableQuestionDataHandlerTest(BaseQuestionEditorControllerTest):
"""Tests get, put and delete methods of editable questions data handler."""
def setUp(self):
"""Completes the setup for QuestionSkillLinkHandlerTest."""
super(EditableQuestionDataHandlerTest, self).setUp()
self.skill_id = skill_services.get_new_skill_id()
self.save_new_skill(
self.skill_id, self.admin_id, 'Skill Description')
question_services.create_new_question_skill_link(
self.question_id, self.skill_id)

def test_get(self):
with self.swap(constants, 'ENABLE_NEW_STRUCTURES', True):
Expand All @@ -239,6 +247,11 @@ def test_get(self):
self.assertEqual(
response_dict['question_dict']['question_state_data'],
self.question.question_state_data.to_dict())
self.assertEqual(
len(response_dict['associated_skill_dicts']), 1)
self.assertEqual(
response_dict['associated_skill_dicts'][0]['id'],
self.skill_id)
self.logout()

self.login(self.TOPIC_MANAGER_EMAIL)
Expand All @@ -251,6 +264,11 @@ def test_get(self):
self.assertEqual(
response_dict['question_dict']['question_state_data'],
self.question.question_state_data.to_dict())
self.assertEqual(
len(response_dict['associated_skill_dicts']), 1)
self.assertEqual(
response_dict['associated_skill_dicts'][0]['id'],
self.skill_id)
self.logout()

# Check that question creator can view the data.
Expand All @@ -264,6 +282,11 @@ def test_get(self):
self.assertEqual(
response_dict['question_dict']['question_state_data'],
self.question.question_state_data.to_dict())
self.assertEqual(
len(response_dict['associated_skill_dicts']), 1)
self.assertEqual(
response_dict['associated_skill_dicts'][0]['id'],
self.skill_id)
self.logout()

def test_delete(self):
Expand Down
68 changes: 54 additions & 14 deletions core/domain/question_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,23 @@ def get_question_from_model(question_model):
question_model.created_on, question_model.last_updated)


def get_question_skill_link_from_model(question_skill_link_model):
aks681 marked this conversation as resolved.
Show resolved Hide resolved
"""Returns domain object representing the given question skill link model.

Args:
question_skill_link_model: QuestionSkillLinkModel. The question skill
link model loaded from the datastore.

Returns:
QuestionSkillLink. The domain object representing the question skill
link model.
"""

return question_domain.QuestionSkillLink(
question_skill_link_model.question_id,
question_skill_link_model.skill_id)


def get_question_by_id(question_id, strict=True):
"""Returns a domain object representing a question.

Expand All @@ -226,23 +243,43 @@ def get_question_by_id(question_id, strict=True):


def get_question_skill_links_of_skill(skill_id):
"""Returns a list of QuestionSkillLinkModels of
"""Returns a list of QuestionSkillLinks of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring doesn't match what the implementation is doing, I think?

But you're right, this ought to be returning domain objects, not datastore models. Could you please update the implementation to do this (and ensure the unit tests reflect the new contract)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

a particular skill ID.

Args:
skill_id: str. ID of the skill.

Returns:
list(QuestionSkillLinkModel). The list of question skill link
list(QuestionSkillLink). The list of question skill link
domain objects that are linked to the skill ID or an empty list
if the skill does not exist.
if the skill does not exist.
"""

question_skill_link_models = (
question_skill_links = [
get_question_skill_link_from_model(model) for model in
question_models.QuestionSkillLinkModel.get_models_by_skill_id(
skill_id)
)
return question_skill_link_models
skill_id)]
return question_skill_links


def get_question_skill_links_of_question(question_id):
"""Returns a list of QuestionSkillLinks of
a particular question ID.

Args:
question_id: str. ID of the question.

Returns:
list(QuestionSkillLink). The list of question skill link
domain objects that are linked to the question ID or an empty list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the domain objects for QuestionSkillLink are being returned currently. Can you change the returned object to be a list of QuestionSkillLink domain objects instead of datastore objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I think the docstring should also be changed to QuestionSkillLink, instead of QuestionSkillLinkModel?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

if the question does not exist.
"""

question_skill_links = [
get_question_skill_link_from_model(model) for model in
question_models.QuestionSkillLinkModel.get_models_by_question_id(
question_id)]
return question_skill_links


def update_skill_ids_of_questions(curr_skill_id, new_skill_id):
Expand All @@ -253,17 +290,20 @@ def update_skill_ids_of_questions(curr_skill_id, new_skill_id):
curr_skill_id: str. ID of the current skill.
new_skill_id: str. ID of the superseding skill.
"""
old_question_skill_link_models = (
question_models.QuestionSkillLinkModel.get_models_by_skill_id(
curr_skill_id))
old_question_skill_links = get_question_skill_links_of_skill(curr_skill_id)
new_question_skill_links = []
new_question_skill_link_models = []
for question_skill_link in old_question_skill_links:
new_question_skill_links.append(
new_question_skill_link_models.append(
question_models.QuestionSkillLinkModel.create(
question_skill_link.question_id, new_skill_id)
)
question_models.QuestionSkillLinkModel.delete_multi_question_skill_links(
old_question_skill_links)
old_question_skill_link_models)
question_models.QuestionSkillLinkModel.put_multi_question_skill_links(
new_question_skill_links)
new_question_skill_link_models)


def get_question_summaries_linked_to_skills(
Expand Down Expand Up @@ -365,9 +405,9 @@ def apply_change_list(question_id, change_list):
if (change.property_name ==
question_domain.QUESTION_PROPERTY_LANGUAGE_CODE):
question.update_language_code(change.new_value)
elif (change.cmd ==
elif (change.property_name ==
question_domain.QUESTION_PROPERTY_QUESTION_STATE_DATA):
question.update_question_data(change.new_value)
question.update_question_state_data(change.new_value)

return question

Expand Down Expand Up @@ -490,7 +530,7 @@ def save_question_summary(question_summary):

def get_question_summary_from_model(question_summary_model):
"""Returns a domain object for an Oppia question summary given a
questioin summary model.
question summary model.

Args:
question_summary_model: QuestionSummaryModel.
Expand Down
39 changes: 39 additions & 0 deletions core/domain/question_services_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ def test_get_question_skill_links_of_skill(self):
'skill_1'))

self.assertEqual(len(question_skill_links), 2)
self.assertTrue(isinstance(question_skill_links[0],
question_domain.QuestionSkillLink))
question_ids = [question_skill.question_id for question_skill
in question_skill_links]
self.assertItemsEqual(
Expand Down Expand Up @@ -294,3 +296,40 @@ def test_created_question_rights(self):
Exception, 'Entity for class QuestionRightsModel with id '
'question_id not found'):
question_services.get_question_rights('question_id')

def test_get_question_skill_links_of_question(self):
# If the question id doesnt exist at all, it returns an empty list.
question_skill_links = (
question_services.get_question_skill_links_of_question(
'non_existent_question_id'))
self.assertEqual(len(question_skill_links), 0)

question_id_2 = question_services.get_new_question_id()
self.save_new_question(
question_id_2, self.editor_id,
self._create_valid_question_data('ABC'))

question_id_3 = question_services.get_new_question_id()
self.save_new_question(
question_id_3, self.editor_id,
self._create_valid_question_data('ABC'))
question_services.create_new_question_skill_link(
self.question_id, 'skill_1')
question_services.create_new_question_skill_link(
question_id_2, 'skill_1')
question_services.create_new_question_skill_link(
question_id_2, 'skill_2')
question_services.create_new_question_skill_link(
question_id_3, 'skill_2')

question_skill_links = (
question_services.get_question_skill_links_of_question(
question_id_2))

self.assertTrue(isinstance(question_skill_links[0],
question_domain.QuestionSkillLink))
self.assertEqual(len(question_skill_links), 2)
skill_ids = [question_skill.skill_id for question_skill
in question_skill_links]
self.assertItemsEqual(
skill_ids, ['skill_1', 'skill_2'])
20 changes: 20 additions & 0 deletions core/domain/skill_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,26 @@ def get_multi_skill_summaries(skill_ids):
return skill_summaries


def get_multi_skills(skill_ids):
"""Returns a list of skills matching the skill IDs provided.

Args:
skill_ids: list(str). List of skill IDs to get skills for.

Returns:
list(Skill). The list of skills matching the provided IDs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to reflect the implementation just excluding skills that are None.

Either throw an exception if a skill is requested that does not exist, or return None in the appropriate position; probably the former is better if you don't expect this to occur in practice. But just silently removing stuff altogether seems like it's not the right thing to do -- you'll end up with the length of the input list not matching the output list which is something the caller won't expect given the function contract.

There should also be tests for this error case, it's quite important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by raising Exception

"""
local_skill_models = skill_models.SkillModel.get_multi(skill_ids)
for skill_id, skill_model in zip(skill_ids, local_skill_models):
if skill_model is None:
raise Exception('No skill exists for ID %s' % skill_id)
skills = [
get_skill_from_model(skill_model)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if any of the skill_id doesn't exist? In that case, won't a None be returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

for skill_model in local_skill_models
if skill_model is not None]
return skills


def get_skill_summary_from_model(skill_summary_model):
"""Returns a domain object for an Oppia skill summary given a
skill summary model.
Expand Down
22 changes: 22 additions & 0 deletions core/domain/skill_services_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,28 @@ def test_get_unpublished_skill_rights_by_creator(self):
skill_ids = [skill_rights_obj.id for skill_rights_obj in skill_rights]
self.assertListEqual(skill_ids, ['skill_a', 'skill_b'])

def test_get_multi_skills(self):
self.save_new_skill(
'skill_a', self.user_id_admin, 'Description A', misconceptions=[],
skill_contents=skill_domain.SkillContents(
state_domain.SubtitledHtml(
'1', 'Explanation'), [
state_domain.SubtitledHtml('2', 'Example 1')], {}))
self.save_new_skill(
'skill_b', self.user_id_admin, 'Description B', misconceptions=[],
skill_contents=skill_domain.SkillContents(
state_domain.SubtitledHtml(
'1', 'Explanation'), [
state_domain.SubtitledHtml('2', 'Example 1')], {}))
try:
skill_services.get_multi_skills(['skill_a', 'skill_b'])
except Exception:
self.fail(msg='Unexpected exception raised.')

with self.assertRaisesRegexp(
Exception, 'No skill exists for ID skill_c'):
skill_services.get_multi_skills(['skill_a', 'skill_c'])


class SkillMasteryServicesUnitTests(test_utils.GenericTestBase):
"""Test the skill mastery services module."""
Expand Down
6 changes: 4 additions & 2 deletions core/domain/state_domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,8 @@ def from_dict(cls, interaction_dict):
solution_dict = (
Solution.from_dict(
interaction_dict['id'], interaction_dict['solution'])
if interaction_dict['solution'] else None)
if (interaction_dict['solution'] and interaction_dict['id'])
else None)

return cls(
interaction_dict['id'],
Expand Down Expand Up @@ -1114,7 +1115,8 @@ def validate(self, exp_param_specs_dict, allow_null_interaction):
if not set(content_id_list) <= set(available_content_ids):
raise utils.ValidationError(
'Expected state content_ids_to_audio_translations to have all '
'of the listed content ids %s' % content_id_list)
'of the listed content ids %s, found %s' %
(content_id_list, available_content_ids))
if not isinstance(self.content_ids_to_audio_translations, dict):
raise utils.ValidationError(
'Expected state content_ids_to_audio_translations to be a dict,'
Expand Down
19 changes: 18 additions & 1 deletion core/storage/question/gae_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,26 @@ def get_models_by_skill_id(cls, skill_id):
return QuestionSkillLinkModel.query().filter(
cls.skill_id == skill_id).fetch()

@classmethod
def get_models_by_question_id(cls, question_id):
"""Returns a list of QuestionSkillLinkModels of a particular
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this correctly differentiate between "question ID doesn't exist" and "there are no question skill link models corresponding to this question ID"? Looking at the implementation I'm not sure it does, we might need to update the docstring to be clearer here.

Following on from my previous comment above, this needs backend tests:

  • for the usual case
  • for the case where the question ID is invalid
  • for the case where the question ID is valid but there are no skill IDs that correspond to it

to verify that this behaves according to the specification in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, but I don't think there's an expected difference in output for the second and third point, as they should both return an empty list. Within the context of the QuestionSkillLinkModel the conditions for the second and third case are the same: no entity exists with that question ID.

question ID.

Args:
question_id: str. ID of the question.

Returns:
list(QuestionSkillLinkModel)|None. The list of question skill link
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto -- question skill link domains --> question skill link models.

Domain objects refer to stuff defined in core/domain ("plain old python objects").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

models that are linked to the question ID, or None if there are no
question skill link models associated with the question ID.
"""
return QuestionSkillLinkModel.query().filter(
cls.question_id == question_id,
cls.deleted == False).fetch() #pylint: disable=singleton-comparison

@classmethod
def put_multi_question_skill_links(cls, question_skill_links):
"""Puts multiple question skill links into the datastore.
"""Puts multiple question skill link models into the datastore.

Args:
question_skill_links: list(QuestionSkillLink). The list of
Expand Down
29 changes: 29 additions & 0 deletions core/storage/question/gae_models_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,32 @@ def test_delete_multi_question_skill_link(self):
)
self.assertEqual(len(question_skill_links), 1)
self.assertEqual(question_skill_links[0].question_id, 'question_id3')

def test_get_models_by_question_id(self):
questionskilllink_model1 = (
question_models.QuestionSkillLinkModel.create(
'question_id1', 'skill_id1')
)
questionskilllink_model2 = (
question_models.QuestionSkillLinkModel.create(
'question_id2', 'skill_id1')
)
questionskilllink_model3 = (
question_models.QuestionSkillLinkModel.create(
'question_id2', 'skill_id3')
)

question_models.QuestionSkillLinkModel.put_multi_question_skill_links(
[questionskilllink_model1, questionskilllink_model2,
questionskilllink_model3])

question_skill_links = (
question_models.QuestionSkillLinkModel.get_models_by_question_id(
'question_id2')
)
self.assertEqual(len(question_skill_links), 2)
question_skill_links = (
question_models.QuestionSkillLinkModel.get_models_by_question_id(
'question_id3')
)
self.assertEqual(len(question_skill_links), 0)
Loading