-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
d9f2824
f0cb957
9c8e702
c93f186
f695dd6
066faec
07a11fb
2386f69
4360648
a68adef
dd9d25f
8a2c2bd
4782099
a20b0ab
4668f51
fd91189
2f4f1cc
3050886
6d52d8c
43807b1
7a4ff6f
41798c6
c496793
fbdaa83
732be38
7756f6d
17ca89f
5785784
13254ea
fc4981d
9ef2940
5861a5a
4ccea9c
0c589eb
c0ad966
9896d99
4d3c329
d37ab38
f6f7e09
5722ca1
3b5983f
c4c9b93
98227d0
5abc2c8
387a9cb
7a10a7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pending comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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( | ||
|
@@ -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 | ||
|
||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Following on from my previous comment above, this needs backend tests:
to verify that this behaves according to the specification in the docstring. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added