Skip to content

Commit

Permalink
Add linked_skill_ids to the Question model (oppia#6929)
Browse files Browse the repository at this point in the history 
* Test commit

* Backend changes for linked skill ids

* Push for now

* Added updation logic to linked_skill_ids

* Fixed linter issues

* Disable default variable issue in test_utils

* Revert package-lock.json

* Add test to check exception in test_multi_question_skill_links

* Linter fix

* Linter fix

* Fixed suggestion_registry

* Fixed suggestion_registry_test

* Fixed reader and topic editor tests

* Fixed backend tests

* Fix linter error

* fix e2e test and review comments

* Remove print statement

* Fix linter errors

* Fix linter errors

* Increase test coverage

* Fix linter errors

* Fix typo

* Fixes based on review comments

* Fix linter errors

* Fix linter errors

* Review comment fixes

* Removed required=True for linked_skill_ids

* Linter fix

* Review changes

* Fix tests

* Fix linter check

* Linter fix

* Remove trailing space

* Check if skill id exists in create_new_question_skill_link

* Review changes

* Fix backend tests

* Lint fix

* Fix suggestions_test

* Fix suggestion_registry_test

* Lint fix

* Review fixes

* Added a note for skillids in editableQuestionBackendApiService

* Fix topic editor test

* Fix sugesstion test and remove suggestion changes

* Remove space in suggestion

* Add linked_skill_ids in the suggestions_test

* Improve test coverage

* Fix suggestion test
  • Loading branch information
@vinitamurthi
vinitamurthi authored Jun 19, 2019
1 parent f13a410 commit 38b5373
Show file tree
Hide file tree
Showing 21 changed files with 460 additions and 134 deletions.
4 changes: 2 additions & 2 deletions core/controllers/acl_decorators_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2138,9 +2138,9 @@ def setUp(self):
self.question_id = question_services.get_new_question_id()
self.question = self.save_new_question(
self.question_id, self.admin_id,
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_services.create_new_question_skill_link(
self.question_id, self.skill_id, 0.5)
self.admin_id, self.question_id, self.skill_id, 0.5)

def test_admin_can_manage_question_skill_status(self):
self.login(self.ADMIN_EMAIL)
Expand Down
39 changes: 25 additions & 14 deletions core/controllers/question_editor.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,19 @@ class QuestionCreationHandler(base.BaseHandler):
"""A handler that creates the question model given a question dict."""

@acl_decorators.can_manage_question_skill_status
def post(self, skill_id):
def post(self, comma_separated_skill_ids):
"""Handles POST requests."""
skill_domain.Skill.require_valid_skill_id(skill_id)
skill = skill_services.get_skill_by_id(skill_id, strict=False)
if skill is None:
raise self.PageNotFoundException(
'The skill with the given id doesn\'t exist.')
skill_ids = comma_separated_skill_ids.split(',')
try:
for skill_id in skill_ids:
skill_domain.Skill.require_valid_skill_id(skill_id)
except Exception:
raise self.InvalidInputException

try:
skill_services.get_multi_skills(skill_ids)
except Exception, e:
raise self.PageNotFoundException(e)

question_dict = self.payload.get('question_dict')
if (
Expand All @@ -49,15 +55,20 @@ def post(self, skill_id):
question_dict['question_state_data_schema_version'] = (
feconf.CURRENT_STATE_SCHEMA_VERSION)
question_dict['id'] = question_services.get_new_question_id()
question_dict['linked_skill_ids'] = skill_ids
try:
question = question_domain.Question.from_dict(question_dict)
except:
except Exception:
raise self.InvalidInputException
question_services.add_question(self.user_id, question)
# TODO(vinitamurthi): Replace DEFAULT_SKILL_DIFFICULTY
# with a value passed from the frontend.
question_services.create_new_question_skill_link(
question.id, skill_id, constants.DEFAULT_SKILL_DIFFICULTY)
question_services.link_multiple_skills_for_question(
self.user_id,
question.id,
skill_ids,
[constants.DEFAULT_SKILL_DIFFICULTY] * len(skill_ids))

self.values.update({
'question_id': question.id
})
Expand All @@ -79,14 +90,15 @@ def post(self, question_id, skill_id):
# TODO(vinitamurthi): Replace DEFAULT_SKILL_DIFFICULTY
# with a value passed from the frontend.
question_services.create_new_question_skill_link(
question_id, skill_id, constants.DEFAULT_SKILL_DIFFICULTY)
self.user_id, question_id, skill_id,
constants.DEFAULT_SKILL_DIFFICULTY)
self.render_json(self.values)

@acl_decorators.can_manage_question_skill_status
def delete(self, question_id, skill_id):
"""Unlinks a question from a skill."""
question_services.delete_question_skill_link(
question_id, skill_id)
self.user_id, question_id, skill_id)
self.render_json(self.values)


Expand All @@ -105,10 +117,9 @@ def get(self, question_id):
raise self.PageNotFoundException(
'The question with the given id doesn\'t exist.')

associated_skills = question_services.get_skills_linked_to_question(
question_id)
associated_skill_dicts = [
skill.to_dict() for skill in associated_skills]
skill.to_dict() for skill in skill_services.get_multi_skills(
question.linked_skill_ids)]

self.values.update({
'question_dict': question.to_dict(),
Expand Down
54 changes: 32 additions & 22 deletions core/controllers/question_editor_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,20 @@ def setUp(self):
self.new_user = user_services.UserActionsInfo(self.new_user_id)
self.editor = user_services.UserActionsInfo(self.editor_id)

self.skill_id = skill_services.get_new_skill_id()
self.save_new_skill(self.skill_id, self.admin_id, 'Skill Description')

self.question_id = question_services.get_new_question_id()
self.question = self.save_new_question(
self.question_id, self.editor_id,
self._create_valid_question_data('ABC'))
self.question_id,
self.editor_id,
self._create_valid_question_data('ABC'),
[self.skill_id])


class QuestionCreationHandlerTest(BaseQuestionEditorControllerTests):
"""Tests returning of new question ids and creating questions."""

def setUp(self):
"""Completes the setup for QuestionSkillLinkHandlerTest."""
super(QuestionCreationHandlerTest, self).setUp()
self.skill_id = skill_services.get_new_skill_id()
self.save_new_skill(self.skill_id, self.admin_id, 'Skill Description')

def test_post_with_non_admin_or_topic_manager_email_disallows_access(self):
self.login(self.NEW_USER_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
Expand Down Expand Up @@ -97,6 +96,26 @@ def test_post_with_incorrect_skill_id_returns_404(self):
{}, csrf_token=csrf_token, expected_status_int=404)
self.logout()

def test_post_with_incorrect_list_of_skill_ids_returns_400(self):
self.login(self.ADMIN_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
incorrect_skill_id = [1, 2]
self.post_json(
'%s/%s' % (feconf.NEW_QUESTION_URL, incorrect_skill_id),
{}, csrf_token=csrf_token, expected_status_int=400)
self.logout()

def test_post_with_incorrect_type_of_skill_ids_returns_400(self):
self.login(self.ADMIN_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
csrf_token = self.get_csrf_token_from_response(response)
incorrect_skill_id = 1
self.post_json(
'%s/%s' % (feconf.NEW_QUESTION_URL, incorrect_skill_id),
{}, csrf_token=csrf_token, expected_status_int=400)
self.logout()

def test_post_with_incorrect_question_id_returns_400(self):
self.login(self.ADMIN_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
Expand Down Expand Up @@ -182,7 +201,7 @@ def setUp(self):
self.question_id_2 = question_services.get_new_question_id()
self.save_new_question(
self.question_id_2, self.editor_id,
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])

def test_post_with_non_admin_or_topic_manager_email_disallows_access(self):
self.login(self.NEW_USER_EMAIL)
Expand Down Expand Up @@ -254,9 +273,9 @@ def test_delete_with_non_admin_or_topic_manager_disallows_access(self):

def test_delete_with_admin_email_allows_question_deletion(self):
question_services.create_new_question_skill_link(
self.question_id, self.skill_id, 0.3)
self.editor_id, self.question_id, self.skill_id, 0.3)
question_services.create_new_question_skill_link(
self.question_id_2, self.skill_id, 0.3)
self.editor_id, self.question_id_2, self.skill_id, 0.3)
self.login(self.ADMIN_EMAIL)
self.delete_json(
'%s/%s/%s' % (
Expand All @@ -275,9 +294,9 @@ def test_delete_with_admin_email_allows_question_deletion(self):

def test_delete_with_topic_manager_email_allows_question_deletion(self):
question_services.create_new_question_skill_link(
self.question_id, self.skill_id, 0.5)
self.editor_id, self.question_id, self.skill_id, 0.5)
question_services.create_new_question_skill_link(
self.question_id_2, self.skill_id, 0.5)
self.editor_id, self.question_id_2, self.skill_id, 0.5)
self.login(self.TOPIC_MANAGER_EMAIL)
self.delete_json(
'%s/%s/%s' % (
Expand All @@ -298,15 +317,6 @@ def test_delete_with_topic_manager_email_allows_question_deletion(self):
class EditableQuestionDataHandlerTest(BaseQuestionEditorControllerTests):
"""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, 0.7)

def test_get_can_not_access_handler_with_invalid_question_id(self):
self.login(self.ADMIN_EMAIL)
self.get_json(
Expand Down
2 changes: 0 additions & 2 deletions core/controllers/reader.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,8 @@ def get(self, exploration_id):
story = story_services.get_story_by_id(story_id, strict=False)
if story is None:
raise self.InvalidInputException

if not story.has_exploration(exploration_id):
raise self.InvalidInputException

pretest_questions, _, next_start_cursor = (
question_services.get_questions_and_skill_descriptions_by_skill_ids(
feconf.NUM_PRETEST_QUESTIONS,
Expand Down
37 changes: 23 additions & 14 deletions core/controllers/reader_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ class ExplorationPretestsUnitTest(test_utils.GenericTestBase):
"""Test the handler for initialising exploration with
state_classifier_mapping.
"""
def setUp(self):
"""Before each individual test, initialize data."""
super(ExplorationPretestsUnitTest, self).setUp()

self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME)
self.editor_id = self.get_user_id_from_email(self.EDITOR_EMAIL)
self.skill_id = skill_services.get_new_skill_id()
self.save_new_skill(
self.skill_id, 'user', 'Description')

def test_get_exploration_pretests(self):
super(ExplorationPretestsUnitTest, self).setUp()
Expand All @@ -219,9 +228,6 @@ def test_get_exploration_pretests(self):
})
]
story_services.update_story('user', story_id, changelist, 'Added node.')
skill_id = skill_services.get_new_skill_id()
self.save_new_skill(
skill_id, 'user', 'Description')

exp_id = '0'
exp_id_2 = '1'
Expand All @@ -234,7 +240,7 @@ def test_get_exploration_pretests(self):
'property_name': (
story_domain.STORY_NODE_PROPERTY_PREREQUISITE_SKILL_IDS),
'old_value': [],
'new_value': [skill_id],
'new_value': [self.skill_id],
'node_id': 'node_1'
}), story_domain.StoryChange({
'cmd': story_domain.CMD_UPDATE_STORY_NODE_PROPERTY,
Expand All @@ -249,15 +255,15 @@ def test_get_exploration_pretests(self):
question_id = question_services.get_new_question_id()
self.save_new_question(
question_id, 'user',
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_id_2 = question_services.get_new_question_id()
self.save_new_question(
question_id_2, 'user',
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_services.create_new_question_skill_link(
question_id, skill_id, 0.3)
self.editor_id, question_id, self.skill_id, 0.3)
question_services.create_new_question_skill_link(
question_id_2, skill_id, 0.5)
self.editor_id, question_id_2, self.skill_id, 0.5)
# Call the handler.
with self.swap(feconf, 'NUM_PRETEST_QUESTIONS', 1):
json_response_1 = self.get_json(
Expand Down Expand Up @@ -292,23 +298,25 @@ class QuestionsUnitTest(test_utils.GenericTestBase):
def setUp(self):
"""Before each individual test, initialize data."""
super(QuestionsUnitTest, self).setUp()
self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME)
self.editor_id = self.get_user_id_from_email(self.EDITOR_EMAIL)

self.skill_id = skill_services.get_new_skill_id()
self.save_new_skill(self.skill_id, 'user', 'Description')

self.question_id = question_services.get_new_question_id()
self.save_new_question(
self.question_id, 'user',
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_services.create_new_question_skill_link(
self.question_id, self.skill_id, 0.5)
self.editor_id, self.question_id, self.skill_id, 0.5)

self.question_id_2 = question_services.get_new_question_id()
self.save_new_question(
self.question_id_2, 'user',
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_services.create_new_question_skill_link(
self.question_id_2, self.skill_id, 0.5)
self.editor_id, self.question_id_2, self.skill_id, 0.5)

def test_questions_are_returned_successfully(self):
# Call the handler.
Expand Down Expand Up @@ -340,9 +348,9 @@ def test_multiple_skill_id_returns_questions(self):
question_id_3 = question_services.get_new_question_id()
self.save_new_question(
question_id_3, 'user',
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_services.create_new_question_skill_link(
question_id_3, skill_id_2, 0.5)
self.editor_id, question_id_3, skill_id_2, 0.5)
url = '%s?question_count=%s&skill_ids=%s,%s&start_cursor=' % (
feconf.QUESTIONS_URL_PREFIX, '3', self.skill_id, skill_id_2)
json_response = self.get_json(url)
Expand Down Expand Up @@ -551,6 +559,7 @@ def setUp(self):

# Register users.
self.signup(self.EDITOR_EMAIL, self.EDITOR_USERNAME)
self.editor_id = self.get_user_id_from_email(self.EDITOR_USERNAME)
self.signup(self.NEW_USER_EMAIL, self.NEW_USER_USERNAME)
self.new_user_id = self.get_user_id_from_email(self.NEW_USER_EMAIL)

Expand Down
6 changes: 4 additions & 2 deletions core/controllers/suggestion_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,8 @@ def setUp(self):
'default_state').to_dict(),
'language_code': 'en',
'question_state_data_schema_version': (
feconf.CURRENT_STATE_SCHEMA_VERSION)
feconf.CURRENT_STATE_SCHEMA_VERSION),
'linked_skill_ids': [self.SKILL_ID]
}
self.login(self.AUTHOR_EMAIL)
response = self.get_html_response(feconf.CREATOR_DASHBOARD_URL)
Expand Down Expand Up @@ -750,7 +751,8 @@ def setUp(self):
'default_state').to_dict(),
'language_code': 'en',
'question_state_data_schema_version': (
feconf.CURRENT_STATE_SCHEMA_VERSION)
feconf.CURRENT_STATE_SCHEMA_VERSION),
'linked_skill_ids': [self.skill_id]
}

self.login(self.AUTHOR_EMAIL)
Expand Down
4 changes: 2 additions & 2 deletions core/controllers/topic_editor_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ def test_get(self):
question_id = question_services.get_new_question_id()
self.save_new_question(
question_id, self.admin_id,
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.skill_id])
question_services.create_new_question_skill_link(
question_id, self.skill_id, 0.5)
self.admin_id, question_id, self.skill_id, 0.5)

self.login(self.ADMIN_EMAIL)
with self.swap(constants, 'NUM_QUESTIONS_PER_PAGE', 1):
Expand Down
2 changes: 1 addition & 1 deletion core/controllers/topics_and_skills_dashboard.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ def post(self):
if old_skill is None:
raise self.PageNotFoundException(
Exception('The old skill with the given id doesn\'t exist.'))
question_services.update_skill_ids_of_questions(
question_services.replace_skill_id_for_all_questions(
old_skill_id, old_skill.description, new_skill_id)
changelist = [
skill_domain.SkillChange({
Expand Down
4 changes: 2 additions & 2 deletions core/controllers/topics_and_skills_dashboard_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,9 @@ def setUp(self):
self.question_id = question_services.get_new_question_id()
self.question = self.save_new_question(
self.question_id, self.admin_id,
self._create_valid_question_data('ABC'))
self._create_valid_question_data('ABC'), [self.linked_skill_id])
question_services.create_new_question_skill_link(
self.question_id, self.linked_skill_id, 0.5)
self.admin_id, self.question_id, self.linked_skill_id, 0.5)

def test_merge_skill(self):
self.login(self.ADMIN_EMAIL)
Expand Down
Loading

0 comments on commit 38b5373

Please sign in to comment.