From 6d73c3627568dabd0d1b776d914e5e13c9984017 Mon Sep 17 00:00:00 2001 From: Kevin Thomas Date: Wed, 18 Dec 2019 15:26:08 +0530 Subject: [PATCH] =?UTF-8?q?Fix=20#8019:=20Adds=20abbreviated=20topic=20nam?= =?UTF-8?q?e=20and=20thumbnail=20fields=20in=E2=80=A6=20(#8067)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * adds abbreviated topic name and topic thumbnail fields * add tests * fix errors * fix lint errors * fix html lint issues * fix html issue * fix html lint issue * fix failures * fit -> it * add tests for topic_domain andtopic_services changes * fix lint issue * address comments * fix tests * add eol * fix test * address review comments * add tests for abbrev name validation * fix backend and e2e failures * address comments * fixes tests; addresses comments * fix issue with editing thumbnail * address comments * fix tests * address comments * fix lint issue * fix frontend test failure * change topicAndStoryEditor to run in dev mode * address review comments; add audit job to check for empty abbreviated name fields and test * address comments * address remaining comments * address review comments --- .github/CODEOWNERS | 3 +- .travis.yml | 2 +- core/controllers/acl_decorators_test.py | 24 +-- core/controllers/admin.py | 4 +- core/controllers/admin_test.py | 5 +- core/controllers/classroom_test.py | 4 +- core/controllers/community_dashboard_test.py | 4 +- core/controllers/creator_dashboard_test.py | 6 +- core/controllers/editor.py | 8 +- core/controllers/feedback_test.py | 4 +- core/controllers/practice_sessions_test.py | 6 +- core/controllers/questions_list_test.py | 4 +- core/controllers/reader_test.py | 3 +- core/controllers/resources.py | 2 +- core/controllers/resources_test.py | 4 +- core/controllers/review_tests_test.py | 5 +- core/controllers/skill_editor_test.py | 4 +- core/controllers/story_editor_test.py | 12 +- core/controllers/story_viewer_test.py | 8 +- core/controllers/subtopic_viewer_test.py | 8 +- core/controllers/suggestion_test.py | 4 +- core/controllers/topic_editor_test.py | 13 +- core/controllers/topic_viewer_test.py | 4 +- .../topics_and_skills_dashboard.py | 6 +- .../topics_and_skills_dashboard_test.py | 11 +- core/domain/fs_services.py | 11 +- core/domain/fs_services_test.py | 6 +- core/domain/opportunity_jobs_one_off_test.py | 4 +- core/domain/opportunity_services_test.py | 4 +- core/domain/prod_validation_jobs_one_off.py | 15 +- .../prod_validation_jobs_one_off_test.py | 108 +++++++---- core/domain/story_fetchers_test.py | 4 +- core/domain/story_jobs_one_off_test.py | 7 +- core/domain/story_services_test.py | 20 +-- core/domain/topic_domain.py | 69 ++++++- core/domain/topic_domain_test.py | 38 +++- core/domain/topic_fetchers.py | 2 + core/domain/topic_fetchers_test.py | 9 +- core/domain/topic_jobs_one_off_test.py | 12 +- core/domain/topic_services.py | 10 ++ core/domain/topic_services_test.py | 28 ++- core/domain/voiceover_services_test.py | 2 +- core/storage/topic/gae_models.py | 4 + core/storage/topic/gae_models_test.py | 7 +- ...ervice.ts.ts => topic-creation.service.ts} | 49 +++-- .../head/domain/topic/TopicObjectFactory.ts | 47 ++++- .../domain/topic/TopicObjectFactorySpec.ts | 4 + .../topic/topic-domain.constants.ajs.ts | 9 + .../domain/topic/topic-domain.constants.ts | 2 + .../head/domain/topic/topic-update.service.ts | 75 ++++++-- .../topic-editor-tab.directive.html | 34 +++- .../editor-tab/topic-editor-tab.directive.ts | 170 +++++++++++++++++- .../edit-topic-thumbnail-modal.template.html | 56 ++++++ .../navbar/topic-editor-navbar.directive.html | 13 +- .../navbar/topic-editor-navbar.directive.ts | 10 ++ ...s-and-skills-dashboard-navbar.directive.ts | 2 +- .../new-topic-name-editor.template.html | 31 ++-- ...cs-and-skills-dashboard-page.controller.ts | 2 +- .../services/assets-backend-api.service.ts | 19 +- .../image-upload-helper.service.spec.ts | 59 ++++++ .../services/image-upload-helper.service.ts | 73 ++++++++ .../protractor_desktop/topicAndStoryEditor.js | 17 +- .../topicsAndSkillsDashboard.js | 2 +- .../tests/protractor_utils/TopicEditorPage.js | 45 +++++ .../TopicsAndSkillsDashboardPage.js | 6 +- core/tests/test_utils.py | 17 +- .../templates/filepath-editor.directive.ts | 75 ++------ feconf.py | 2 + main.py | 2 +- 69 files changed, 1050 insertions(+), 289 deletions(-) rename core/templates/dev/head/components/entity-creation-services/{topic-creation.service.ts.ts => topic-creation.service.ts} (61%) create mode 100644 core/templates/dev/head/pages/topic-editor-page/modal-templates/edit-topic-thumbnail-modal.template.html create mode 100644 core/templates/dev/head/services/image-upload-helper.service.spec.ts create mode 100644 core/templates/dev/head/services/image-upload-helper.service.ts diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index ff05fb4d79a8..472bbb6b9f93 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -181,6 +181,7 @@ /core/templates/dev/head/services/editability.service*.ts @aks681 /core/templates/dev/head/services/exploration-features*.ts @aks681 /core/templates/dev/head/services/exploration-html-formatter.service*.ts @aks681 +/core/templates/dev/head/services/image-upload-helper.service*.ts @aks681 /core/templates/dev/head/services/validators.service*.ts @aks681 @@ -361,7 +362,7 @@ /core/domain/subtopic_page_services*.py @aks681 /core/domain/topic*.py @aks681 /core/storage/topic/ @aks681 -/core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts.ts @aks681 +/core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts @aks681 /core/templates/dev/head/domain/classroom/ @aks681 /core/templates/dev/head/domain/subtopic_viewer/ @aks681 /core/templates/dev/head/domain/topic/ @aks681 diff --git a/.travis.yml b/.travis.yml index 166e0fc3a7b7..7d00ccdf8c8b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -124,7 +124,7 @@ script: - if [ "$RUN_E2E_TESTS_SKILL_EDITOR" == 'true' ]; then travis_retry bash scripts/run_e2e_tests.sh --suite="skillEditor" --prod_env; fi - if [ "$RUN_E2E_TESTS_SUBSCRIPTIONS" == 'true' ]; then travis_retry bash scripts/run_e2e_tests.sh --suite="subscriptions" --prod_env; fi - if [ "$RUN_E2E_TESTS_TOPICS_AND_SKILLS_DASHBOARD" == 'true' ]; then travis_retry bash scripts/run_e2e_tests.sh --suite="topicsAndSkillsDashboard" --prod_env; fi -- if [ "$RUN_E2E_TESTS_TOPIC_AND_STORY_EDITOR" == 'true' ]; then travis_retry bash scripts/run_e2e_tests.sh --suite="topicAndStoryEditor" --prod_env; fi +- if [ "$RUN_E2E_TESTS_TOPIC_AND_STORY_EDITOR" == 'true' ]; then travis_retry bash scripts/run_e2e_tests.sh --suite="topicAndStoryEditor"; fi - if [ "$RUN_E2E_TESTS_USERS" == 'true' ]; then travis_retry bash scripts/run_e2e_tests.sh --suite="users" --prod_env; fi # These lines are commented out because these checks are being run on CircleCI # here: https://circleci.com/gh/oppia/oppia diff --git a/core/controllers/acl_decorators_test.py b/core/controllers/acl_decorators_test.py index 5a2cac24d7c7..f7fc238f1f14 100644 --- a/core/controllers/acl_decorators_test.py +++ b/core/controllers/acl_decorators_test.py @@ -1859,8 +1859,8 @@ def setUp(self): debug=feconf.DEBUG, )) self.save_new_topic( - self.topic_id, self.viewer_id, 'Name', 'Description', [], [], - [], [], 1) + self.topic_id, self.viewer_id, 'Name', 'abbrev', None, + 'Description', [], [], [], [], 1) topic_services.create_new_topic_rights(self.topic_id, self.admin_id) topic_services.assign_role( self.admin, self.manager, topic_domain.ROLE_MANAGER, self.topic_id) @@ -1926,8 +1926,8 @@ def setUp(self): self.story_id, self.admin_id, 'Title', 'Description', 'Notes', self.topic_id) self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', - [self.story_id], [], [], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [self.story_id], [], [], [], 1) topic_services.create_new_topic_rights(self.topic_id, self.admin_id) def test_can_not_edit_story_with_invalid_story_id(self): @@ -2015,8 +2015,8 @@ def setUp(self): debug=feconf.DEBUG, )) self.save_new_topic( - self.topic_id, self.viewer_id, 'Name', 'Description', [], [], - [], [], 1) + self.topic_id, self.viewer_id, 'Name', 'abbrev', None, + 'Description', [], [], [], [], 1) topic_services.create_new_topic_rights(self.topic_id, self.admin_id) topic_services.assign_role( self.admin, self.manager, topic_domain.ROLE_MANAGER, self.topic_id) @@ -2099,8 +2099,8 @@ def setUp(self): self.story_id, self.admin_id, 'Title', 'Description', 'Notes', self.topic_id) self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', - [self.story_id], [], [], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [self.story_id], [], [], [], 1) def test_cannot_access_non_existent_story(self): with self.swap(self, 'testapp', self.mock_testapp): @@ -2823,8 +2823,8 @@ def test_can_edit_topic(self): self.login(self.ADMIN_EMAIL) topic_id = topic_services.get_new_topic_id() self.save_new_topic( - topic_id, self.admin_id, 'Name', 'Description', - [], [], [], [], 1) + topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [], [], [], [], 1) with self.swap(self, 'testapp', self.mock_testapp): response = self.get_json('/mock_edit_entity/%s/%s' % ( feconf.ENTITY_TYPE_TOPIC, topic_id)) @@ -2851,8 +2851,8 @@ def test_can_edit_story(self): story_id, self.admin_id, 'Title', 'Description', 'Notes', topic_id) self.save_new_topic( - topic_id, self.admin_id, 'Name', 'Description', - [story_id], [], [], [], 1) + topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [story_id], [], [], [], 1) with self.swap(self, 'testapp', self.mock_testapp): response = self.get_json('/mock_edit_entity/%s/%s' % ( feconf.ENTITY_TYPE_STORY, story_id)) diff --git a/core/controllers/admin.py b/core/controllers/admin.py index a192f35142ee..db02533640fe 100644 --- a/core/controllers/admin.py +++ b/core/controllers/admin.py @@ -387,9 +387,9 @@ def _load_dummy_new_structures_data(self): self.user_id, question_id_3, skill_id_3, 0.7) topic_1 = topic_domain.Topic.create_default_topic( - topic_id_1, 'Dummy Topic 1') + topic_id_1, 'Dummy Topic 1', 'abbrev') topic_2 = topic_domain.Topic.create_default_topic( - topic_id_2, 'Empty Topic') + topic_id_2, 'Empty Topic', 'abbrev') topic_1.add_canonical_story(story_id) topic_1.add_uncategorized_skill_id(skill_id_1) diff --git a/core/controllers/admin_test.py b/core/controllers/admin_test.py index 7606f1192490..48ae9a7a1114 100644 --- a/core/controllers/admin_test.py +++ b/core/controllers/admin_test.py @@ -290,7 +290,7 @@ def test_regenerate_topic_related_opportunities_action(self): exp_services.save_new_exploration(owner_id, exploration) topic = topic_domain.Topic.create_default_topic( - topic_id=topic_id, name='topic') + topic_id=topic_id, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(owner_id, topic) story = story_domain.Story.create_default_story( @@ -872,7 +872,8 @@ def test_changing_user_role_from_exploration_editor_to_topic_manager(self): topic_id = topic_services.get_new_topic_id() self.save_new_topic( - topic_id, user_id, 'Name', 'Description', [], [], [], [], 1) + topic_id, user_id, 'Name', 'abbrev', None, + 'Description', [], [], [], [], 1) self.login(self.ADMIN_EMAIL, is_super_admin=True) diff --git a/core/controllers/classroom_test.py b/core/controllers/classroom_test.py index 994a655bce51..4a1099bc0b60 100644 --- a/core/controllers/classroom_test.py +++ b/core/controllers/classroom_test.py @@ -66,10 +66,10 @@ def test_get(self): topic_id_1 = topic_services.get_new_topic_id() topic_id_2 = topic_services.get_new_topic_id() private_topic = topic_domain.Topic.create_default_topic( - topic_id_1, 'private_topic_name') + topic_id_1, 'private_topic_name', 'abbrev') topic_services.save_new_topic(admin_id, private_topic) public_topic = topic_domain.Topic.create_default_topic( - topic_id_2, 'public_topic_name') + topic_id_2, 'public_topic_name', 'abbrev') topic_services.save_new_topic(admin_id, public_topic) topic_services.publish_topic(topic_id_2, admin_id) diff --git a/core/controllers/community_dashboard_test.py b/core/controllers/community_dashboard_test.py index 1a627b2314e6..b8519eb25131 100644 --- a/core/controllers/community_dashboard_test.py +++ b/core/controllers/community_dashboard_test.py @@ -66,7 +66,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) self.skill_id_0 = 'skill_id_0' @@ -317,7 +317,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) stories = [story_domain.Story.create_default_story( diff --git a/core/controllers/creator_dashboard_test.py b/core/controllers/creator_dashboard_test.py index c9d5718c4869..0d4337f9a1a6 100644 --- a/core/controllers/creator_dashboard_test.py +++ b/core/controllers/creator_dashboard_test.py @@ -709,9 +709,9 @@ def test_get_topic_summary_dicts_with_new_structure_players_enabled(self): response = self.get_json(feconf.CREATOR_DASHBOARD_DATA_URL) self.assertEqual(len(response['topic_summary_dicts']), 0) self.save_new_topic( - 'topic_id', self.owner_id, 'Name', 'Description', - ['story_id_1', 'story_id_2'], ['story_id_3'], - ['skill_id_1', 'skill_id_2'], [], 1) + 'topic_id', self.owner_id, 'Name', 'abbrev', None, + 'Description', ['story_id_1', 'story_id_2'], + ['story_id_3'], ['skill_id_1', 'skill_id_2'], [], 1) response = self.get_json(feconf.CREATOR_DASHBOARD_DATA_URL) self.assertEqual(len(response['topic_summary_dicts']), 1) self.assertTrue(isinstance(response['topic_summary_dicts'], list)) diff --git a/core/controllers/editor.py b/core/controllers/editor.py index 3685e23ddd99..b155524cb04c 100644 --- a/core/controllers/editor.py +++ b/core/controllers/editor.py @@ -623,6 +623,9 @@ def post(self, entity_type, entity_id): raw = self.request.get('image') filename = self.payload.get('filename') + filename_prefix = self.payload.get('filename_prefix') + if filename_prefix is None: + filename_prefix = self._FILENAME_PREFIX if not raw: raise self.InvalidInputException('No image supplied') @@ -659,7 +662,7 @@ def post(self, entity_type, entity_id): file_system_class = fs_services.get_entity_file_system_class() fs = fs_domain.AbstractFileSystem(file_system_class( entity_type, entity_id)) - filepath = '%s/%s' % (self._FILENAME_PREFIX, filename) + filepath = '%s/%s' % (filename_prefix, filename) if fs.isfile(filepath): raise self.InvalidInputException( @@ -667,7 +670,8 @@ def post(self, entity_type, entity_id): 'different name.' % filename) fs_services.save_original_and_compressed_versions_of_image( - self.user_id, filename, entity_type, entity_id, raw) + self.user_id, filename, entity_type, entity_id, + raw, filename_prefix) self.render_json({'filename': filename}) diff --git a/core/controllers/feedback_test.py b/core/controllers/feedback_test.py index cb861b56ce7b..e63258b3fa94 100644 --- a/core/controllers/feedback_test.py +++ b/core/controllers/feedback_test.py @@ -629,8 +629,8 @@ def setUp(self): self.topic_id = topic_services.get_new_topic_id() self.save_new_topic( - self.topic_id, self.owner_id, 'Name', 'Description', - [], [], [], [], 1) + self.topic_id, self.owner_id, 'Name', 'abbrev', None, + 'Description', [], [], [], [], 1) def test_get_feedback_threads_linked_to_topics(self): self.login(self.OWNER_EMAIL) diff --git a/core/controllers/practice_sessions_test.py b/core/controllers/practice_sessions_test.py index d26263ba31b7..d8e6d05283db 100644 --- a/core/controllers/practice_sessions_test.py +++ b/core/controllers/practice_sessions_test.py @@ -43,7 +43,7 @@ def setUp(self): self.save_new_skill(self.skill_id2, self.admin_id, 'Skill 2') self.topic = topic_domain.Topic.create_default_topic( - self.topic_id, 'public_topic_name') + self.topic_id, 'public_topic_name', 'abbrev') self.topic.uncategorized_skill_ids.append(self.skill_id1) self.topic.subtopics.append(topic_domain.Subtopic( 1, 'subtopic_name', [self.skill_id2])) @@ -51,7 +51,7 @@ def setUp(self): topic_services.save_new_topic(self.admin_id, self.topic) self.topic = topic_domain.Topic.create_default_topic( - self.topic_id_1, 'private_topic_name') + self.topic_id_1, 'private_topic_name', 'abbrev') topic_services.save_new_topic(self.admin_id, self.topic) topic_services.publish_topic(self.topic_id, self.admin_id) @@ -104,7 +104,7 @@ def test_get_fails_when_new_structures_not_enabled(self): def test_get_fails_when_skill_ids_dont_exist(self): topic = topic_domain.Topic.create_default_topic( - 'topic_id_3', 'topic_without_skills') + 'topic_id_3', 'topic_without_skills', 'abbrev') topic.uncategorized_skill_ids.append('non_existent_skill') topic_services.save_new_topic(self.admin_id, topic) topic_services.publish_topic('topic_id_3', self.admin_id) diff --git a/core/controllers/questions_list_test.py b/core/controllers/questions_list_test.py index c566671b92ac..cb9538b40b56 100644 --- a/core/controllers/questions_list_test.py +++ b/core/controllers/questions_list_test.py @@ -45,8 +45,8 @@ def setUp(self): self.skill_id_2, self.admin_id, 'Skill Description 2') self.topic_id = topic_services.get_new_topic_id() self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', [], [], - [self.skill_id, self.skill_id_2], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [], [], [self.skill_id, self.skill_id_2], [], 1) self.skill_id_3 = skill_services.get_new_skill_id() changelist = [topic_domain.TopicChange({ 'cmd': topic_domain.CMD_ADD_SUBTOPIC, diff --git a/core/controllers/reader_test.py b/core/controllers/reader_test.py index 3fcf014326aa..6021c633d956 100644 --- a/core/controllers/reader_test.py +++ b/core/controllers/reader_test.py @@ -260,7 +260,8 @@ def test_get_exploration_pretests(self): story_id = story_services.get_new_story_id() topic_id = topic_services.get_new_topic_id() self.save_new_topic( - topic_id, 'user', 'Topic', 'A new topic', [], [], [], [], 0) + topic_id, 'user', 'Topic', 'abbrev', None, 'A new topic', + [], [], [], [], 0) self.save_new_story( story_id, 'user', 'Title', 'Description', 'Notes', topic_id ) diff --git a/core/controllers/resources.py b/core/controllers/resources.py index 153a04a2b5de..29ab3539eb34 100644 --- a/core/controllers/resources.py +++ b/core/controllers/resources.py @@ -52,7 +52,7 @@ class AssetDevHandler(base.BaseHandler): image and audio files are served from GCS). """ - _SUPPORTED_TYPES = ['image', 'audio'] + _SUPPORTED_TYPES = ['image', 'audio', 'thumbnail'] _SUPPORTED_PAGE_CONTEXTS = [ feconf.ENTITY_TYPE_EXPLORATION, feconf.ENTITY_TYPE_SKILL, feconf.ENTITY_TYPE_TOPIC, feconf.ENTITY_TYPE_STORY, diff --git a/core/controllers/resources_test.py b/core/controllers/resources_test.py index e8d2478a6a2d..c5496fbcf3d2 100644 --- a/core/controllers/resources_test.py +++ b/core/controllers/resources_test.py @@ -149,8 +149,8 @@ def test_image_upload_and_download(self): story_id, admin_id, 'Title', 'Description', 'Notes', topic_id) self.save_new_topic( - topic_id, admin_id, 'Name', 'Description', - [story_id], [], [], [subtopic], 2) + topic_id, admin_id, 'Name', 'abbrev', None, + 'Description', [story_id], [], [], [subtopic], 2) self.save_new_skill(skill_id, admin_id, 'Description') # Page context: Exploration. diff --git a/core/controllers/review_tests_test.py b/core/controllers/review_tests_test.py index 0ffafcd7bba6..0a41ae1ed178 100644 --- a/core/controllers/review_tests_test.py +++ b/core/controllers/review_tests_test.py @@ -83,8 +83,9 @@ def setUp(self): self.story_id_2, 'Private Story Title', self.topic_id) story_services.save_new_story(self.admin_id, self.story_2) self.save_new_topic( - self.topic_id, 'user', 'Topic', 'A new topic', - [self.story_id_1, self.story_id_3], [], [], [], 0) + self.topic_id, 'user', 'Topic', 'abbrev', None, + 'A new topic', [self.story_id_1, self.story_id_3], + [], [], [], 0) topic_services.publish_topic(self.topic_id, self.admin_id) topic_services.publish_story( self.topic_id, self.story_id_1, self.admin_id) diff --git a/core/controllers/skill_editor_test.py b/core/controllers/skill_editor_test.py index 3d25ce1bdded..cb978dfdcd1c 100644 --- a/core/controllers/skill_editor_test.py +++ b/core/controllers/skill_editor_test.py @@ -49,8 +49,8 @@ def setUp(self): self.save_new_skill(self.skill_id_2, self.admin_id, 'Description') self.topic_id = topic_services.get_new_topic_id() self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', - [], [], [self.skill_id], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [], [], [self.skill_id], [], 1) def delete_skill_model_and_memcache(self, user_id, skill_id): """Deletes skill model and memcache corresponding to the given skill diff --git a/core/controllers/story_editor_test.py b/core/controllers/story_editor_test.py index f6f51b29e143..d1a0aee6c360 100644 --- a/core/controllers/story_editor_test.py +++ b/core/controllers/story_editor_test.py @@ -44,8 +44,8 @@ def setUp(self): self.story_id, self.admin_id, 'Title', 'Description', 'Notes', self.topic_id) self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', - [self.story_id], [], [], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [self.story_id], [], [], [], 1) class StoryPublicationTests(BaseStoryEditorControllerTests): @@ -186,8 +186,8 @@ def test_can_not_get_access_story_handler_with_invalid_topic_id(self): expected_status_int=404) self.save_new_topic( - 'topic_id_new', self.admin_id, 'Name 2', 'Description', - [], [], [], [], 1) + 'topic_id_new', self.admin_id, 'Name 2', 'abbrev', None, + 'Description', [], [], [], [], 1) # An error would be raised here also as the story is not in the given # topic. @@ -260,8 +260,8 @@ def test_put_can_not_access_story_handler_with_invalid_topic_id(self): # Raises error 404 even when topic is saved as the story id is not # associated with the new topic. self.save_new_topic( - 'topic_id_new', self.admin_id, 'Name 2', 'Description', - [], [], [], [], 1) + 'topic_id_new', self.admin_id, 'Name 2', 'abbrev', None, + 'Description', [], [], [], [], 1) csrf_token = self.get_new_csrf_token() self.put_json( diff --git a/core/controllers/story_viewer_test.py b/core/controllers/story_viewer_test.py index abf8c2e60571..9263a81b8cac 100644 --- a/core/controllers/story_viewer_test.py +++ b/core/controllers/story_viewer_test.py @@ -106,8 +106,8 @@ def setUp(self): story.story_contents.next_node_id = 'node_4' story_services.save_new_story(self.admin_id, story) self.save_new_topic( - self.TOPIC_ID, 'user', 'Topic', 'A new topic', [story.id], - [], [], [], 0) + self.TOPIC_ID, 'user', 'Topic', 'abbrev', None, + 'A new topic', [story.id], [], [], [], 0) topic_services.publish_topic(self.TOPIC_ID, self.admin_id) topic_services.publish_story( self.TOPIC_ID, self.STORY_ID_1, self.admin_id) @@ -169,8 +169,8 @@ def test_can_not_access_story_viewer_page_with_unpublished_story(self): def test_can_not_access_story_viewer_page_with_unpublished_topic(self): new_story_id = 'new_story_id' self.save_new_topic( - 'topic_id_1', 'user', 'Topic 2', 'A new topic', [new_story_id], - [], [], [], 0) + 'topic_id_1', 'user', 'Topic 2', 'abbrev', None, + 'A new topic', [new_story_id], [], [], [], 0) story = story_domain.Story.create_default_story( new_story_id, 'Title', 'topic_id_1') story_services.save_new_story(self.admin_id, story) diff --git a/core/controllers/subtopic_viewer_test.py b/core/controllers/subtopic_viewer_test.py index 7432f963ea83..3b4e0eb71558 100644 --- a/core/controllers/subtopic_viewer_test.py +++ b/core/controllers/subtopic_viewer_test.py @@ -62,12 +62,12 @@ def setUp(self): subtopic = topic_domain.Subtopic.create_default_subtopic( 1, 'Subtopic Title') self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', [], [], - [], [subtopic], 2) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [], [], [], [subtopic], 2) topic_services.publish_topic(self.topic_id, self.admin_id) self.save_new_topic( - 'topic_id_2', self.admin_id, 'Private_Name', 'Description', [], [], - [], [subtopic], 2) + 'topic_id_2', self.admin_id, 'Private_Name', 'abbrev', None, + 'Description', [], [], [], [subtopic], 2) self.recorded_voiceovers_dict = { 'voiceovers_mapping': { 'content': { diff --git a/core/controllers/suggestion_test.py b/core/controllers/suggestion_test.py index 606b864b0f95..f865a8242339 100644 --- a/core/controllers/suggestion_test.py +++ b/core/controllers/suggestion_test.py @@ -973,7 +973,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exploration) topic = topic_domain.Topic.create_default_topic( - topic_id=self.TOPIC_ID, name='topic') + topic_id=self.TOPIC_ID, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) story = story_domain.Story.create_default_story( @@ -1145,7 +1145,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exploration) topic = topic_domain.Topic.create_default_topic( - topic_id=self.TOPIC_ID, name='topic') + topic_id=self.TOPIC_ID, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) story = story_domain.Story.create_default_story( diff --git a/core/controllers/topic_editor_test.py b/core/controllers/topic_editor_test.py index a16c5db360a1..f97d40f98a32 100644 --- a/core/controllers/topic_editor_test.py +++ b/core/controllers/topic_editor_test.py @@ -55,8 +55,8 @@ def setUp(self): self.skill_id_2, self.admin_id, 'Skill Description 2') self.topic_id = topic_services.get_new_topic_id() self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', [], [], - [self.skill_id, self.skill_id_2], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [], [], [self.skill_id, self.skill_id_2], [], 1) changelist = [topic_domain.TopicChange({ 'cmd': topic_domain.CMD_ADD_SUBTOPIC, 'title': 'Title', @@ -84,8 +84,9 @@ def test_handler_updates_story_summary_dicts(self): self.assertEqual(response['additional_story_summary_dicts'], []) self.save_new_topic( - topic_id, self.admin_id, 'New name', 'New description', - [canonical_story_id], [additional_story_id], [self.skill_id], + topic_id, self.admin_id, 'New name', 'abbrev', None, + 'New description', [canonical_story_id], + [additional_story_id], [self.skill_id], [], 1) self.save_new_story( @@ -519,8 +520,8 @@ def test_editable_topic_handler_put(self): topic_id_1 = topic_services.get_new_topic_id() self.save_new_topic( - topic_id_1, self.admin_id, 'Name 1', 'Description 1', [], [], - [self.skill_id], [], 1) + topic_id_1, self.admin_id, 'Name 1', 'abbrev', None, + 'Description 1', [], [], [self.skill_id], [], 1) json_response = self.put_json( '%s/%s' % ( diff --git a/core/controllers/topic_viewer_test.py b/core/controllers/topic_viewer_test.py index 4ec253dc7ccd..c3f5cf8e017a 100644 --- a/core/controllers/topic_viewer_test.py +++ b/core/controllers/topic_viewer_test.py @@ -50,7 +50,7 @@ def setUp(self): self.story.description = 'story_description' self.topic = topic_domain.Topic.create_default_topic( - self.topic_id, 'public_topic_name') + self.topic_id, 'public_topic_name', 'abbrev') self.topic.uncategorized_skill_ids.append(self.skill_id_1) self.topic.subtopics.append(topic_domain.Subtopic( 1, 'subtopic_name', [self.skill_id_2])) @@ -62,7 +62,7 @@ def setUp(self): story_services.save_new_story(self.admin_id, self.story) self.topic = topic_domain.Topic.create_default_topic( - self.topic_id_1, 'private_topic_name') + self.topic_id_1, 'private_topic_name', 'abbrev') topic_services.save_new_topic(self.admin_id, self.topic) topic_services.publish_topic(self.topic_id, self.admin_id) diff --git a/core/controllers/topics_and_skills_dashboard.py b/core/controllers/topics_and_skills_dashboard.py index 92c6216579fc..9321602e4ef5 100644 --- a/core/controllers/topics_and_skills_dashboard.py +++ b/core/controllers/topics_and_skills_dashboard.py @@ -132,10 +132,12 @@ class NewTopicHandler(base.BaseHandler): def post(self): """Handles POST requests.""" name = self.payload.get('name') - + abbreviated_name = self.payload.get('abbreviated_name') topic_domain.Topic.require_valid_name(name) + topic_domain.Topic.require_valid_abbreviated_name(abbreviated_name) new_topic_id = topic_services.get_new_topic_id() - topic = topic_domain.Topic.create_default_topic(new_topic_id, name) + topic = topic_domain.Topic.create_default_topic( + new_topic_id, name, abbreviated_name) topic_services.save_new_topic(self.user_id, topic) self.render_json({ diff --git a/core/controllers/topics_and_skills_dashboard_test.py b/core/controllers/topics_and_skills_dashboard_test.py index cfcd8cd31003..91c664001725 100644 --- a/core/controllers/topics_and_skills_dashboard_test.py +++ b/core/controllers/topics_and_skills_dashboard_test.py @@ -48,8 +48,8 @@ def setUp(self): self.linked_skill_id, self.admin_id, 'Description 3') skill_services.publish_skill(self.linked_skill_id, self.admin_id) self.save_new_topic( - self.topic_id, self.admin_id, 'Name', 'Description', [], [], - [self.linked_skill_id], [], 1) + self.topic_id, self.admin_id, 'Name', 'abbrev', None, + 'Description', [], [], [self.linked_skill_id], [], 1) class TopicsAndSkillsDashboardPageDataHandlerTests( @@ -160,9 +160,12 @@ def setUp(self): def test_topic_creation(self): self.login(self.ADMIN_EMAIL) csrf_token = self.get_new_csrf_token() - + payload = { + 'name': 'Topic name', + 'abbreviated_name': 'name' + } json_response = self.post_json( - self.url, {'name': 'Topic name'}, csrf_token=csrf_token) + self.url, payload, csrf_token=csrf_token) topic_id = json_response['topicId'] self.assertEqual(len(topic_id), 12) self.assertIsNotNone( diff --git a/core/domain/fs_services.py b/core/domain/fs_services.py index 90e623af19cd..7765e882df09 100644 --- a/core/domain/fs_services.py +++ b/core/domain/fs_services.py @@ -26,7 +26,8 @@ def save_original_and_compressed_versions_of_image( - user_id, filename, entity_type, entity_id, original_image_content): + user_id, filename, entity_type, entity_id, + original_image_content, filename_prefix): """Saves the three versions of the image file. Args: @@ -35,19 +36,21 @@ def save_original_and_compressed_versions_of_image( entity_type: str. The type of the entity. entity_id: str. The id of the entity. original_image_content: str. The content of the original image. + filename_prefix: str. The string to prefix to the filename. """ - filepath = 'image/%s' % filename + filepath = '%s/%s' % (filename_prefix, filename) filename_wo_filetype = filename[:filename.rfind('.')] filetype = filename[filename.rfind('.') + 1:] compressed_image_filename = '%s_compressed.%s' % ( filename_wo_filetype, filetype) - compressed_image_filepath = 'image/%s' % compressed_image_filename + compressed_image_filepath = '%s/%s' % ( + filename_prefix, compressed_image_filename) micro_image_filename = '%s_micro.%s' % ( filename_wo_filetype, filetype) - micro_image_filepath = 'image/%s' % micro_image_filename + micro_image_filepath = '%s/%s' % (filename_prefix, micro_image_filename) file_system_class = get_entity_file_system_class() fs = fs_domain.AbstractFileSystem(file_system_class( diff --git a/core/domain/fs_services_test.py b/core/domain/fs_services_test.py index 6541aa19e921..25b891e4423a 100644 --- a/core/domain/fs_services_test.py +++ b/core/domain/fs_services_test.py @@ -79,7 +79,7 @@ def test_save_original_and_compressed_versions_of_image(self): fs.isfile('image/%s' % self.MICRO_IMAGE_FILENAME), False) fs_services.save_original_and_compressed_versions_of_image( self.USER, self.FILENAME, 'exploration', self.EXPLORATION_ID, - original_image_content) + original_image_content, 'image') self.assertEqual(fs.isfile('image/%s' % self.FILENAME), True) self.assertEqual( fs.isfile('image/%s' % self.COMPRESSED_IMAGE_FILENAME), True) @@ -111,7 +111,7 @@ def test_compress_image_on_prod_mode_with_big_image_size(self): fs_services.save_original_and_compressed_versions_of_image( self.USER, self.FILENAME, 'exploration', self.EXPLORATION_ID, - original_image_content) + original_image_content, 'image') self.assertTrue(fs.isfile('image/%s' % self.FILENAME)) self.assertTrue( @@ -156,7 +156,7 @@ def test_compress_image_on_prod_mode_with_small_image_size(self): fs_services.save_original_and_compressed_versions_of_image( self.USER, self.FILENAME, 'exploration', self.EXPLORATION_ID, - original_image_content) + original_image_content, 'image') self.assertTrue(fs.isfile('image/%s' % self.FILENAME)) self.assertTrue( diff --git a/core/domain/opportunity_jobs_one_off_test.py b/core/domain/opportunity_jobs_one_off_test.py index 7aaebc4fa734..b6e98283c8d7 100644 --- a/core/domain/opportunity_jobs_one_off_test.py +++ b/core/domain/opportunity_jobs_one_off_test.py @@ -67,11 +67,11 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic_1 = topic_domain.Topic.create_default_topic( - topic_id=self.topic_id_1, name='topic1') + topic_id=self.topic_id_1, name='topic1', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic_1) topic_2 = topic_domain.Topic.create_default_topic( - topic_id=self.topic_id_2, name='topic2') + topic_id=self.topic_id_2, name='topic2', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic_2) story_1 = story_domain.Story.create_default_story( diff --git a/core/domain/opportunity_services_test.py b/core/domain/opportunity_services_test.py index 786fe21ae346..7bda7695b10a 100644 --- a/core/domain/opportunity_services_test.py +++ b/core/domain/opportunity_services_test.py @@ -66,7 +66,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id=self.TOPIC_ID, name='topic') + topic_id=self.TOPIC_ID, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) story = story_domain.Story.create_default_story( @@ -562,7 +562,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id=self.TOPIC_ID, name='topic') + topic_id=self.TOPIC_ID, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) story = story_domain.Story.create_default_story( diff --git a/core/domain/prod_validation_jobs_one_off.py b/core/domain/prod_validation_jobs_one_off.py index ce821fe8cb38..6354e035f2ac 100644 --- a/core/domain/prod_validation_jobs_one_off.py +++ b/core/domain/prod_validation_jobs_one_off.py @@ -3531,12 +3531,25 @@ def _validate_uncategorized_skill_ids_not_in_subtopic_skill_ids(cls, item): 'in subtopic for entity with id %s' % ( item.id, skill_id, subtopic['id'])) + @classmethod + def _validate_abbreviated_name_is_nonempty(cls, item): + """Validate that abbreviated name is nonempty. + + Args: + item: ndb.Model. TopicModel to validate. + """ + if not item.abbreviated_name: + cls.errors['abbreviated name check'].append( + 'Entity id %s: Expected nonempty abbreviated name ' + 'received %s.' % (item.id, item.abbreviated_name)) + @classmethod def _get_custom_validation_functions(cls): return [ cls._validate_canonical_name_is_unique, cls._validate_canonical_name_matches_name_in_lowercase, - cls._validate_uncategorized_skill_ids_not_in_subtopic_skill_ids] + cls._validate_uncategorized_skill_ids_not_in_subtopic_skill_ids, + cls._validate_abbreviated_name_is_nonempty] class TopicSnapshotMetadataModelValidator(BaseSnapshotMetadataModelValidator): diff --git a/core/domain/prod_validation_jobs_one_off_test.py b/core/domain/prod_validation_jobs_one_off_test.py index 4df65e6101ed..e63cb596164b 100644 --- a/core/domain/prod_validation_jobs_one_off_test.py +++ b/core/domain/prod_validation_jobs_one_off_test.py @@ -2232,7 +2232,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id=self.TOPIC_ID, name='topic') + topic_id=self.TOPIC_ID, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) story = story_domain.Story.create_default_story( @@ -9391,7 +9391,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) language_codes = ['ar', 'en', 'en'] @@ -9594,7 +9594,7 @@ def setUp(self): self.user_id = self.get_user_id_from_email(USER_EMAIL) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') stories = [story_domain.Story.create_default_story( '%s' % i, @@ -9760,7 +9760,7 @@ def setUp(self): self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') stories = [story_domain.Story.create_default_story( '%s' % i, @@ -9878,7 +9878,7 @@ def setUp(self): self.owner_id = self.get_user_id_from_email(self.OWNER_EMAIL) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') stories = [story_domain.Story.create_default_story( '%s' % i, @@ -10102,7 +10102,7 @@ def setUp(self): language_codes = ['ar', 'en', 'en'] topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') stories = [story_domain.Story.create_default_story( '%s' % i, @@ -10532,8 +10532,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='Topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='Topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -10809,6 +10810,40 @@ def test_model_with_uncategorized_skill_id_in_subtopic(self): ), u'[u\'fully-validated TopicModel\', 2]'] run_job_and_check_output(self, expected_output, sort=True) + def test_model_with_abbreviated_name_is_empty(self): + self.model_instance_0.abbreviated_name = None + self.model_instance_0.commit(self.owner_id, '', []) + expected_output = [ + ( + u'[u\'failed validation check for abbreviated name check ' + 'of TopicModel\', ' + '[u\'Entity id 0: Expected nonempty abbreviated name ' + 'received None.\']]' + ), ( + u'[u\'failed validation check for domain object check ' + 'of TopicModel\', [u\'Entity id 0: Entity fails domain ' + 'validation with the error Abbreviated name should be a ' + 'string.\']]' + ), u'[u\'fully-validated TopicModel\', 2]'] + run_job_and_check_output(self, expected_output, sort=True) + + def test_model_with_abbreviated_name_is_empty_string(self): + self.model_instance_0.abbreviated_name = '' + self.model_instance_0.commit(self.owner_id, '', []) + expected_output = [ + ( + u'[u\'failed validation check for abbreviated name check ' + 'of TopicModel\', ' + '[u\'Entity id 0: Expected nonempty abbreviated name ' + 'received .\']]' + ), ( + u'[u\'failed validation check for domain object check ' + 'of TopicModel\', [u\'Entity id 0: Entity fails domain ' + 'validation with the error Abbreviated name field should ' + 'not be empty.\']]' + ), u'[u\'fully-validated TopicModel\', 2]'] + run_job_and_check_output(self, expected_output, sort=True) + class TopicSnapshotMetadataModelValidatorTests( test_utils.GenericTestBase): @@ -10826,8 +10861,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -11018,8 +11054,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -11178,8 +11215,9 @@ def setUp(self): self.manager2 = user_services.UserActionsInfo(self.manager2_id) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -11353,8 +11391,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -11557,8 +11596,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -11715,8 +11755,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -11997,8 +12038,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -12220,8 +12262,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -12431,8 +12474,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -12637,8 +12681,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -12804,8 +12849,9 @@ def setUp(self): self.set_admins([self.ADMIN_USERNAME]) topics = [topic_domain.Topic.create_default_topic( - topic_id='%s' % i, name='topic%s' % i) for i in python_utils.RANGE( - 3)] + topic_id='%s' % i, + name='topic%s' % i, + abbreviated_name='abbrev%s' % i) for i in python_utils.RANGE(3)] rubrics = [ skill_domain.Rubric( constants.SKILL_DIFFICULTIES[0], 'Explanation 1'), @@ -15036,7 +15082,7 @@ def setUp(self): rights_manager.publish_exploration(self.owner, exp.id) topic = topic_domain.Topic.create_default_topic( - topic_id='0', name='topic') + topic_id='0', name='topic', abbreviated_name='abbrev') story = story_domain.Story.create_default_story( 'story', diff --git a/core/domain/story_fetchers_test.py b/core/domain/story_fetchers_test.py index 042d61addaf8..001bc4cc3f31 100644 --- a/core/domain/story_fetchers_test.py +++ b/core/domain/story_fetchers_test.py @@ -42,8 +42,8 @@ def setUp(self): self.STORY_ID = story_services.get_new_story_id() self.TOPIC_ID = topic_services.get_new_topic_id() self.save_new_topic( - self.TOPIC_ID, self.USER_ID, 'Topic', 'A new topic', [], [], [], [], - 0) + self.TOPIC_ID, self.USER_ID, 'Topic', 'abbrev', None, + 'A new topic', [], [], [], [], 0) self.save_new_story( self.STORY_ID, self.USER_ID, 'Title', 'Description', 'Notes', self.TOPIC_ID) diff --git a/core/domain/story_jobs_one_off_test.py b/core/domain/story_jobs_one_off_test.py index eaf074625824..1011ad24e060 100644 --- a/core/domain/story_jobs_one_off_test.py +++ b/core/domain/story_jobs_one_off_test.py @@ -52,9 +52,10 @@ def setUp(self): self.skill_id_1 = 'skill_id_1' self.skill_id_2 = 'skill_id_2' self.save_new_topic( - self.TOPIC_ID, self.albert_id, 'Name', 'Description', - [self.story_id_1, self.story_id_2], [self.story_id_3], - [self.skill_id_1, self.skill_id_2], [], 1 + self.TOPIC_ID, self.albert_id, 'Name', 'abbrev', None, + 'Description', [self.story_id_1, self.story_id_2], + [self.story_id_3], [self.skill_id_1, self.skill_id_2], + [], 1 ) self.process_and_flush_pending_tasks() diff --git a/core/domain/story_services_test.py b/core/domain/story_services_test.py index 90d06fc0745b..b2e4a7c1da34 100644 --- a/core/domain/story_services_test.py +++ b/core/domain/story_services_test.py @@ -46,8 +46,8 @@ def setUp(self): self.STORY_ID = story_services.get_new_story_id() self.TOPIC_ID = topic_services.get_new_topic_id() self.save_new_topic( - self.TOPIC_ID, self.USER_ID, 'Topic', 'A new topic', [], [], [], [], - 0) + self.TOPIC_ID, self.USER_ID, 'Topic', 'abbrev', None, + 'A new topic', [], [], [], [], 0) self.save_new_story( self.STORY_ID, self.USER_ID, 'Title', 'Description', 'Notes', self.TOPIC_ID) @@ -227,8 +227,8 @@ def test_update_story_with_invalid_corresponding_topic_id_value(self): def test_update_story_which_not_corresponding_topic_id(self): topic_id = topic_services.get_new_topic_id() self.save_new_topic( - topic_id, self.USER_ID, 'A New Topic', 'A new topic description.', - [], [], [], [], 0) + topic_id, self.USER_ID, 'A New Topic', 'abbrev', None, + 'A new topic description.', [], [], [], [], 0) story_id = story_services.get_new_story_id() self.save_new_story( story_id, self.USER_ID, 'Title', 'Description', 'Notes', topic_id) @@ -478,8 +478,8 @@ def test_get_story_by_version(self): topic_id = topic_services.get_new_topic_id() story_id = story_services.get_new_story_id() self.save_new_topic( - topic_id, self.USER_ID, 'A different topic', 'A new topic', [], [], - [], [], 0) + topic_id, self.USER_ID, 'A different topic', 'abbrev', None, + 'A new topic', [], [], [], [], 0) self.save_new_story( story_id, self.USER_ID, 'new title', 'Description', 'Notes', topic_id) @@ -748,8 +748,8 @@ def setUp(self): self.owner_id = 'owner' self.TOPIC_ID = topic_services.get_new_topic_id() self.save_new_topic( - self.TOPIC_ID, self.USER_ID, 'New Topic', 'A new topic', [], [], [], - [], 0) + self.TOPIC_ID, self.USER_ID, 'New Topic', 'abbrev', None, + 'A new topic', [], [], [], [], 0) story = story_domain.Story.create_default_story( self.STORY_1_ID, 'Title', self.TOPIC_ID) story.description = ('Description') @@ -978,8 +978,8 @@ def test_migrate_story_contents_to_latest_schema(self): topic_id = topic_services.get_new_topic_id() user_id = 'user_id' self.save_new_topic( - topic_id, user_id, 'Topic', 'A new topic', [], [], [], [], - 0) + topic_id, user_id, 'Topic', 'abbrev', None, + 'A new topic', [], [], [], [], 0) self.save_new_story( story_id, user_id, 'Title', 'Description', 'Notes', topic_id) diff --git a/core/domain/topic_domain.py b/core/domain/topic_domain.py index 372cbb86f5f7..c7fc525f5607 100644 --- a/core/domain/topic_domain.py +++ b/core/domain/topic_domain.py @@ -41,6 +41,8 @@ # Do not modify the values of these constants. This is to preserve backwards # compatibility with previous change dicts. TOPIC_PROPERTY_NAME = 'name' +TOPIC_PROPERTY_ABBREVIATED_NAME = 'abbreviated_name' +TOPIC_PROPERTY_THUMBNAIL_FILENAME = 'thumbnail_filename' TOPIC_PROPERTY_DESCRIPTION = 'description' TOPIC_PROPERTY_CANONICAL_STORY_REFERENCES = 'canonical_story_references' TOPIC_PROPERTY_ADDITIONAL_STORY_REFERENCES = 'additional_story_references' @@ -93,10 +95,12 @@ class TopicChange(change_domain.BaseChange): # The allowed list of topic properties which can be used in # update_topic_property command. TOPIC_PROPERTIES = ( - TOPIC_PROPERTY_NAME, TOPIC_PROPERTY_DESCRIPTION, + TOPIC_PROPERTY_NAME, TOPIC_PROPERTY_ABBREVIATED_NAME, + TOPIC_PROPERTY_DESCRIPTION, TOPIC_PROPERTY_CANONICAL_STORY_REFERENCES, TOPIC_PROPERTY_ADDITIONAL_STORY_REFERENCES, - TOPIC_PROPERTY_LANGUAGE_CODE) + TOPIC_PROPERTY_LANGUAGE_CODE, + TOPIC_PROPERTY_THUMBNAIL_FILENAME) # The allowed list of subtopic properties which can be used in # update_subtopic_property command. @@ -371,9 +375,11 @@ class Topic(python_utils.OBJECT): """Domain object for an Oppia Topic.""" def __init__( - self, topic_id, name, description, canonical_story_references, - additional_story_references, uncategorized_skill_ids, subtopics, - subtopic_schema_version, next_subtopic_id, language_code, version, + self, topic_id, name, abbreviated_name, thumbnail_filename, + description, canonical_story_references, + additional_story_references, uncategorized_skill_ids, + subtopics, subtopic_schema_version, + next_subtopic_id, language_code, version, story_reference_schema_version, created_on=None, last_updated=None): """Constructs a Topic domain object. @@ -381,6 +387,8 @@ def __init__( Args: topic_id: str. The unique ID of the topic. name: str. The name of the topic. + abbreviated_name: str. The abbreviated topic name. + thumbnail_filename: str|None. The thumbnail filename of the topic. description: str. The description of the topic. canonical_story_references: list(StoryReference). A set of story reference objects representing the canonical stories that are @@ -407,6 +415,8 @@ def __init__( """ self.id = topic_id self.name = name + self.abbreviated_name = abbreviated_name + self.thumbnail_filename = thumbnail_filename self.canonical_name = name.lower() self.description = description self.canonical_story_references = canonical_story_references @@ -430,6 +440,8 @@ def to_dict(self): return { 'id': self.id, 'name': self.name, + 'abbreviated_name': self.abbreviated_name, + 'thumbnail_filename': self.thumbnail_filename, 'description': self.description, 'canonical_story_references': [ reference.to_dict() @@ -478,6 +490,24 @@ def require_valid_name(cls, name): if name == '': raise utils.ValidationError('Name field should not be empty') + @classmethod + def require_valid_abbreviated_name(cls, name): + """Checks whether the abbreviated name of the topic is a valid one. + + Args: + name: str. The abbreviated name to validate. + """ + if not isinstance(name, python_utils.BASESTRING): + raise utils.ValidationError('Abbreviated name should be a string.') + + if name == '': + raise utils.ValidationError( + 'Abbreviated name field should not be empty.') + + if len(name) > 12: + raise utils.ValidationError( + 'Abbreviated name field should not exceed 12 characters.') + def get_all_skill_ids(self): """Returns all the ids of all the skills present in the topic. @@ -655,6 +685,12 @@ def validate(self): valid. """ self.require_valid_name(self.name) + self.require_valid_abbreviated_name(self.abbreviated_name) + if self.thumbnail_filename is not None and not ( + isinstance(self.thumbnail_filename, python_utils.BASESTRING)): + raise utils.ValidationError( + 'Expected thumbnail filename to be a string, received %s' + % self.thumbnail_filename) if not isinstance(self.description, python_utils.BASESTRING): raise utils.ValidationError( 'Expected description to be a string, received %s' @@ -744,7 +780,7 @@ def validate(self): % self.uncategorized_skill_ids) @classmethod - def create_default_topic(cls, topic_id, name): + def create_default_topic(cls, topic_id, name, abbreviated_name): """Returns a topic domain object with default values. This is for the frontend where a default blank topic would be shown to the user when the topic is created for the first time. @@ -752,12 +788,13 @@ def create_default_topic(cls, topic_id, name): Args: topic_id: str. The unique id of the topic. name: str. The initial name for the topic. + abbreviated_name: str. The abbreviated name for the topic. Returns: Topic. The Topic domain object with the default values. """ return cls( - topic_id, name, + topic_id, name, abbreviated_name, None, feconf.DEFAULT_TOPIC_DESCRIPTION, [], [], [], [], feconf.CURRENT_SUBTOPIC_SCHEMA_VERSION, 1, constants.DEFAULT_LANGUAGE_CODE, 0, @@ -827,6 +864,24 @@ def update_name(self, new_name): """ self.name = new_name + def update_abbreviated_name(self, new_abbreviated_name): + """Updates the abbreviated_name of a topic object. + + Args: + new_abbreviated_name: str. The updated abbreviated_name + for the topic. + """ + self.abbreviated_name = new_abbreviated_name + + def update_thumbnail_filename(self, new_thumbnail_filename): + """Updates the thumbnail filename of a topic object. + + Args: + new_thumbnail_filename: str|None. The updated thumbnail filename + for the topic. + """ + self.thumbnail_filename = new_thumbnail_filename + def update_description(self, new_description): """Updates the description of a topic object. diff --git a/core/domain/topic_domain_test.py b/core/domain/topic_domain_test.py index c2c53ee262fe..43c5acd45e6a 100644 --- a/core/domain/topic_domain_test.py +++ b/core/domain/topic_domain_test.py @@ -37,7 +37,7 @@ def setUp(self): self.signup('a@example.com', 'A') self.signup('b@example.com', 'B') self.topic = topic_domain.Topic.create_default_topic( - self.topic_id, 'Name') + self.topic_id, 'Name', 'abbrev') self.topic.subtopics = [ topic_domain.Subtopic(1, 'Title', ['skill_id_1'])] self.topic.next_subtopic_id = 2 @@ -50,10 +50,13 @@ def setUp(self): def test_create_default_topic(self): """Tests the create_default_topic() function.""" - topic = topic_domain.Topic.create_default_topic(self.topic_id, 'Name') + topic = topic_domain.Topic.create_default_topic( + self.topic_id, 'Name', 'abbrev') expected_topic_dict = { 'id': self.topic_id, 'name': 'Name', + 'abbreviated_name': 'abbrev', + 'thumbnail_filename': None, 'description': feconf.DEFAULT_TOPIC_DESCRIPTION, 'canonical_story_references': [], 'additional_story_references': [], @@ -194,10 +197,31 @@ def _assert_valid_topic_id(self, expected_error_substring, topic_id): utils.ValidationError, expected_error_substring): topic_domain.Topic.require_valid_topic_id(topic_id) + def _assert_valid_abbreviated_name( + self, expected_error_substring, name): + """Checks that the topic passes strict validation.""" + with self.assertRaisesRegexp( + utils.ValidationError, expected_error_substring): + topic_domain.Topic.require_valid_abbreviated_name(name) + def test_valid_topic_id(self): self._assert_valid_topic_id('Topic id should be a string', 10) self._assert_valid_topic_id('Topic id abc is invalid', 'abc') + def test_valid_abbreviated_name(self): + self._assert_valid_abbreviated_name( + 'Abbreviated name should be a string.', 10) + self._assert_valid_abbreviated_name( + 'Abbreviated name field should not be empty.', '') + self._assert_valid_abbreviated_name( + 'Abbreviated name field should not exceed 12 characters.', + 'this is a lengthy name.') + + def test_thumbnail_filename_validation(self): + self.topic.thumbnail_filename = 1 + self._assert_validation_error( + 'Expected thumbnail filename to be a string, received 1') + def test_subtopic_title_validation(self): self.topic.subtopics[0].title = 1 self._assert_validation_error('Expected subtopic title to be a string') @@ -411,6 +435,16 @@ def test_update_language_code(self): self.topic.update_language_code('bn') self.assertEqual(self.topic.language_code, 'bn') + def test_update_abbreviated_name(self): + self.assertEqual(self.topic.abbreviated_name, 'abbrev') + self.topic.update_abbreviated_name('name') + self.assertEqual(self.topic.abbreviated_name, 'name') + + def test_update_thumbnail_filename(self): + self.assertEqual(self.topic.thumbnail_filename, None) + self.topic.update_thumbnail_filename('img.png') + self.assertEqual(self.topic.thumbnail_filename, 'img.png') + def test_cannot_add_uncategorized_skill_with_existing_uncategorized_skill( self): self.assertEqual(self.topic.uncategorized_skill_ids, []) diff --git a/core/domain/topic_fetchers.py b/core/domain/topic_fetchers.py index 30dd764b7d1d..5bfa9b0a0125 100644 --- a/core/domain/topic_fetchers.py +++ b/core/domain/topic_fetchers.py @@ -142,6 +142,8 @@ def get_topic_from_model(topic_model): versioned_additional_story_references) return topic_domain.Topic( topic_model.id, topic_model.name, + topic_model.abbreviated_name, + topic_model.thumbnail_filename, topic_model.description, [ topic_domain.StoryReference.from_dict(reference) for reference in versioned_canonical_story_references[ diff --git a/core/domain/topic_fetchers_test.py b/core/domain/topic_fetchers_test.py index edaa671f2941..599c68765bea 100644 --- a/core/domain/topic_fetchers_test.py +++ b/core/domain/topic_fetchers_test.py @@ -49,7 +49,8 @@ def setUp(self): 'subtopic_id': 1 })] self.save_new_topic( - self.TOPIC_ID, self.user_id, 'Name', 'Description', + self.TOPIC_ID, self.user_id, 'Name', 'abbrev', + 'img.png', 'Description', [self.story_id_1, self.story_id_2], [self.story_id_3], [self.skill_id_1, self.skill_id_2], [], 1 ) @@ -100,6 +101,7 @@ def test_cannot_get_topic_from_model_with_invalid_schema_version(self): model = topic_models.TopicModel( id='topic_id', name='name', + abbreviated_name='abbrev', canonical_name='canonical_name', next_subtopic_id=1, language_code='en', @@ -121,6 +123,7 @@ def test_cannot_get_topic_from_model_with_invalid_schema_version(self): model = topic_models.TopicModel( id='topic_id_2', name='name 2', + abbreviated_name='abbrev', canonical_name='canonical_name_2', next_subtopic_id=1, language_code='en', @@ -146,8 +149,8 @@ def test_get_topic_by_id(self): def test_get_topic_by_version(self): topic_id = topic_services.get_new_topic_id() self.save_new_topic( - topic_id, self.user_id, 'topic name', 'Description', - [], [], [], [], 1) + topic_id, self.user_id, 'topic name', + 'abbrev', 'img.png', 'Description', [], [], [], [], 1) changelist = [topic_domain.TopicChange({ 'cmd': topic_domain.CMD_UPDATE_TOPIC_PROPERTY, diff --git a/core/domain/topic_jobs_one_off_test.py b/core/domain/topic_jobs_one_off_test.py index 91ff785b2f85..4c4a0dd7bd93 100644 --- a/core/domain/topic_jobs_one_off_test.py +++ b/core/domain/topic_jobs_one_off_test.py @@ -53,7 +53,7 @@ def test_migration_job_does_not_convert_up_to_date_topic(self): # Create a new topic that should not be affected by the # job. topic = topic_domain.Topic.create_default_topic( - self.TOPIC_ID, name='A name') + self.TOPIC_ID, name='A name', abbreviated_name='abbrev') topic.add_subtopic(1, title='A subtitle') topic_services.save_new_topic(self.albert_id, topic) self.assertEqual( @@ -85,7 +85,7 @@ def test_migration_job_skips_deleted_topic(self): and does not attempt to migrate. """ topic = topic_domain.Topic.create_default_topic( - self.TOPIC_ID, name='A name') + self.TOPIC_ID, name='A name', abbreviated_name='abbrev') topic_services.save_new_topic(self.albert_id, topic) # Delete the topic before migration occurs. @@ -121,8 +121,8 @@ def test_migration_job_converts_old_topic(self): """ # Generate topic with old(v1) subtopic data. self.save_new_topic_with_subtopic_schema_v1( - self.TOPIC_ID, self.albert_id, 'A name', 'a name', '', - [], [], [], 2) + self.TOPIC_ID, self.albert_id, 'A name', 'abbrev', + 'a name', '', [], [], [], 2) topic = ( topic_fetchers.get_topic_by_id(self.TOPIC_ID)) self.assertEqual(topic.subtopic_schema_version, 1) @@ -154,8 +154,8 @@ def _mock_logging_function(msg): # The topic model created will be invalid due to invalid language code. self.save_new_topic_with_subtopic_schema_v1( - self.TOPIC_ID, self.albert_id, 'A name', 'a name', '', - [], [], [], 2, language_code='invalid_language_code') + self.TOPIC_ID, self.albert_id, 'A name', 'abbrev', + 'a name', '', [], [], [], 2, language_code='invalid_language_code') job_id = ( topic_jobs_one_off.TopicMigrationOneOffJob.create_new()) diff --git a/core/domain/topic_services.py b/core/domain/topic_services.py index 7772f744f92e..7bcab01fc74d 100644 --- a/core/domain/topic_services.py +++ b/core/domain/topic_services.py @@ -160,6 +160,8 @@ def _create_topic(committer_id, topic, commit_message, commit_cmds): model = topic_models.TopicModel( id=topic.id, name=topic.name, + abbreviated_name=topic.abbreviated_name, + thumbnail_filename=topic.thumbnail_filename, canonical_name=topic.canonical_name, description=topic.description, language_code=topic.language_code, @@ -299,12 +301,18 @@ def apply_change_list(topic_id, change_list): if (change.property_name == topic_domain.TOPIC_PROPERTY_NAME): topic.update_name(change.new_value) + elif (change.property_name == + topic_domain.TOPIC_PROPERTY_ABBREVIATED_NAME): + topic.update_abbreviated_name(change.new_value) elif (change.property_name == topic_domain.TOPIC_PROPERTY_DESCRIPTION): topic.update_description(change.new_value) elif (change.property_name == topic_domain.TOPIC_PROPERTY_LANGUAGE_CODE): topic.update_language_code(change.new_value) + elif (change.property_name == + topic_domain.TOPIC_PROPERTY_THUMBNAIL_FILENAME): + topic.update_thumbnail_filename(change.new_value) elif (change.cmd == subtopic_page_domain.CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY): subtopic_page_id = ( @@ -412,6 +420,8 @@ def _save_topic(committer_id, topic, commit_message, change_list): topic_model.description = topic.description topic_model.name = topic.name + topic_model.abbreviated_name = topic.abbreviated_name + topic_model.thumbnail_filename = topic.thumbnail_filename topic_model.canonical_story_references = [ reference.to_dict() for reference in topic.canonical_story_references ] diff --git a/core/domain/topic_services_test.py b/core/domain/topic_services_test.py index 78376299a7c3..ae0f975df512 100644 --- a/core/domain/topic_services_test.py +++ b/core/domain/topic_services_test.py @@ -54,7 +54,7 @@ def setUp(self): 'subtopic_id': 1 })] self.save_new_topic( - self.TOPIC_ID, self.user_id, 'Name', 'Description', + self.TOPIC_ID, self.user_id, 'Name', 'abbrev', None, 'Description', [self.story_id_1, self.story_id_2], [self.story_id_3], [self.skill_id_1, self.skill_id_2], [], 1 ) @@ -151,6 +151,7 @@ def test_cannot_get_topic_from_model_with_invalid_schema_version(self): model = topic_models.TopicModel( id='topic_id', name='name', + abbreviated_name='abbrev', canonical_name='canonical_name', next_subtopic_id=1, language_code='en', @@ -172,6 +173,7 @@ def test_cannot_get_topic_from_model_with_invalid_schema_version(self): model = topic_models.TopicModel( id='topic_id_2', name='name 2', + abbreviated_name='abbrev', canonical_name='canonical_name_2', next_subtopic_id=1, language_code='en', @@ -224,7 +226,7 @@ def test_get_all_skill_ids_assigned_to_some_topic(self): 'Moved skill to subtopic.') topic_id = topic_services.get_new_topic_id() self.save_new_topic( - topic_id, self.user_id, 'Name 2', 'Description', + topic_id, self.user_id, 'Name 2', 'abbrev', None, 'Description', [], [], [self.skill_id_1, 'skill_3'], [], 1 ) self.assertEqual( @@ -438,6 +440,16 @@ def test_update_topic(self): 'property_name': topic_domain.TOPIC_PROPERTY_DESCRIPTION, 'old_value': 'Description', 'new_value': 'New Description' + }), topic_domain.TopicChange({ + 'cmd': topic_domain.CMD_UPDATE_TOPIC_PROPERTY, + 'property_name': topic_domain.TOPIC_PROPERTY_ABBREVIATED_NAME, + 'old_value': '', + 'new_value': 'short name' + }), topic_domain.TopicChange({ + 'cmd': topic_domain.CMD_UPDATE_TOPIC_PROPERTY, + 'property_name': topic_domain.TOPIC_PROPERTY_THUMBNAIL_FILENAME, + 'old_value': '', + 'new_value': 'thumbnail.png' })] topic_services.update_topic_and_subtopic_pages( self.user_id_admin, self.TOPIC_ID, changelist, @@ -445,6 +457,8 @@ def test_update_topic(self): topic = topic_fetchers.get_topic_by_id(self.TOPIC_ID) topic_summary = topic_services.get_topic_summary_by_id(self.TOPIC_ID) self.assertEqual(topic.description, 'New Description') + self.assertEqual(topic.abbreviated_name, 'short name') + self.assertEqual(topic.thumbnail_filename, 'thumbnail.png') self.assertEqual(topic.version, 3) self.assertEqual(topic_summary.version, 3) @@ -1037,8 +1051,8 @@ def test_cannot_save_new_topic_with_existing_name(self): with self.assertRaisesRegexp( Exception, 'Topic with name \'Name\' already exists'): self.save_new_topic( - 'topic_2', self.user_id, 'Name', 'Description 2', - [], [], [], [], 1) + 'topic_2', self.user_id, 'Name', 'abbrev', None, + 'Description 2', [], [], [], [], 1) def test_update_topic_language_code(self): topic = topic_fetchers.get_topic_by_id(self.TOPIC_ID) @@ -1143,10 +1157,10 @@ def test_cannot_assign_role_with_invalid_role(self): def test_deassign_user_from_all_topics(self): self.save_new_topic( - 'topic_2', self.user_id, 'Name 2', 'Description 2', + 'topic_2', self.user_id, 'Name 2', 'abbrev', None, 'Description 2', [], [], [], [], 1) self.save_new_topic( - 'topic_3', self.user_id, 'Name 3', 'Description 3', + 'topic_3', self.user_id, 'Name 3', 'abbrev', None, 'Description 3', [], [], [], [], 1) topic_services.assign_role( @@ -1243,6 +1257,7 @@ def test_migrate_subtopic_to_latest_schema(self): model = topic_models.TopicModel( id='topic_id', name='name', + abbreviated_name='abbrev', canonical_name='Name', next_subtopic_id=1, language_code='en', @@ -1285,6 +1300,7 @@ def test_migrate_story_reference_to_latest_schema(self): model = topic_models.TopicModel( id='topic_id', name='name', + abbreviated_name='abbrev', canonical_name='Name', next_subtopic_id=1, language_code='en', diff --git a/core/domain/voiceover_services_test.py b/core/domain/voiceover_services_test.py index c16416d3116a..7bd8fb17b1d2 100644 --- a/core/domain/voiceover_services_test.py +++ b/core/domain/voiceover_services_test.py @@ -70,7 +70,7 @@ def setUp(self): exp_services.save_new_exploration(self.owner_id, exp) topic = topic_domain.Topic.create_default_topic( - topic_id=self.TOPIC_ID, name='topic') + topic_id=self.TOPIC_ID, name='topic', abbreviated_name='abbrev') topic_services.save_new_topic(self.owner_id, topic) story = story_domain.Story.create_default_story( diff --git a/core/storage/topic/gae_models.py b/core/storage/topic/gae_models.py index e0f6bdddd4c9..52e41233c73a 100644 --- a/core/storage/topic/gae_models.py +++ b/core/storage/topic/gae_models.py @@ -51,6 +51,10 @@ class TopicModel(base_models.VersionedModel): name = ndb.StringProperty(required=True, indexed=True) # The canonical name of the topic, created by making `name` lowercase. canonical_name = ndb.StringProperty(required=True, indexed=True) + # The abbreviated name of the topic. + abbreviated_name = ndb.StringProperty(indexed=True, default='') + # The thumbnail filename of the topic. + thumbnail_filename = ndb.StringProperty(indexed=True) # The description of the topic. description = ndb.TextProperty(indexed=False) # This consists of the list of objects referencing canonical stories that diff --git a/core/storage/topic/gae_models_test.py b/core/storage/topic/gae_models_test.py index 185a5468f8bb..a4f8b5df74e3 100644 --- a/core/storage/topic/gae_models_test.py +++ b/core/storage/topic/gae_models_test.py @@ -43,7 +43,8 @@ def test_get_deletion_policy(self): def test_has_reference_to_user_id(self): self.save_new_topic( - 'topic_id', 'owner_id', 'name', 'description', [], [], [], [], 0) + 'topic_id', 'owner_id', 'name', 'abbrev', None, + 'description', [], [], [], [], 0) self.assertTrue( topic_models.TopicModel.has_reference_to_user_id('owner_id')) self.assertFalse( @@ -61,6 +62,7 @@ def test_that_subsidiary_models_are_created_when_new_model_is_saved(self): topic = topic_models.TopicModel( id=self.TOPIC_ID, name=self.TOPIC_NAME, + abbreviated_name='abbrev', canonical_name=self.TOPIC_CANONICAL_NAME, subtopic_schema_version=feconf.CURRENT_SUBTOPIC_SCHEMA_VERSION, story_reference_schema_version=( @@ -91,7 +93,8 @@ def test_that_subsidiary_models_are_created_when_new_model_is_saved(self): def test_get_by_name(self): topic = topic_domain.Topic.create_default_topic( topic_id=self.TOPIC_ID, - name=self.TOPIC_NAME + name=self.TOPIC_NAME, + abbreviated_name='abbrev' ) topic_services.save_new_topic(feconf.SYSTEM_COMMITTER_ID, topic) self.assertEqual( diff --git a/core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts.ts b/core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts similarity index 61% rename from core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts.ts rename to core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts index d4b1ee35bf8a..2e952c2ae5b3 100644 --- a/core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts.ts +++ b/core/templates/dev/head/components/entity-creation-services/topic-creation.service.ts @@ -42,11 +42,19 @@ angular.module('oppia').factory('TopicCreationService', [ '$scope', '$uibModalInstance', function($scope, $uibModalInstance) { $scope.topicName = ''; - $scope.isTopicNameEmpty = function(topicName) { - return (topicName === ''); + $scope.abbreviatedTopicName = ''; + $scope.isTopicValid = function() { + return ( + $scope.topicName !== '' && + $scope.abbreviatedTopicName !== '' && + $scope.topicName.length <= 20 && + $scope.abbreviatedTopicName.length <= 12); }; - $scope.save = function(topicName) { - $uibModalInstance.close(topicName); + $scope.save = function(topicName, abbreviatedTopicName) { + $uibModalInstance.close({ + topicName: topicName, + abbreviatedTopicName: abbreviatedTopicName + }); }; $scope.cancel = function() { $uibModalInstance.dismiss('cancel'); @@ -55,26 +63,31 @@ angular.module('oppia').factory('TopicCreationService', [ ] }); - modalInstance.result.then(function(topicName) { - if (topicName === '') { + modalInstance.result.then(function(topic) { + if (topic.topicName === '') { throw Error('Topic name cannot be empty'); } + if (topic.abbreviatedTopicName === '') { + throw Error('Abbreviated name cannot be empty'); + } topicCreationInProgress = true; AlertsService.clearWarnings(); $rootScope.loadingMessage = 'Creating topic'; - $http.post('/topic_editor_handler/create_new', {name: topicName}) - .then(function(response) { - $timeout(function() { - $window.location = UrlInterpolationService.interpolateUrl( - TOPIC_EDITOR_URL_TEMPLATE, { - topic_id: response.data.topicId - } - ); - }, 150); - }, function() { - $rootScope.loadingMessage = ''; - }); + $http.post('/topic_editor_handler/create_new', { + name: topic.topicName, + abbreviated_name: topic.abbreviatedTopicName + }).then(function(response) { + $timeout(function() { + $window.location = UrlInterpolationService.interpolateUrl( + TOPIC_EDITOR_URL_TEMPLATE, { + topic_id: response.data.topicId + } + ); + }, 150); + }, function() { + $rootScope.loadingMessage = ''; + }); }); } }; diff --git a/core/templates/dev/head/domain/topic/TopicObjectFactory.ts b/core/templates/dev/head/domain/topic/TopicObjectFactory.ts index d16d15d450f6..43ddf7b788a1 100644 --- a/core/templates/dev/head/domain/topic/TopicObjectFactory.ts +++ b/core/templates/dev/head/domain/topic/TopicObjectFactory.ts @@ -32,6 +32,7 @@ import { Subtopic, SubtopicObjectFactory } from export class Topic { _id: string; _name: string; + _abbreviatedName: string; _description: string; _languageCode: string; _canonicalStoryReferences: Array; @@ -40,15 +41,17 @@ export class Topic { _nextSubtopicId: number; _version: number; _subtopics: Array; + _thumbnailFilename: string; skillSummaryObjectFactory: SkillSummaryObjectFactory; subtopicObjectFactory: SubtopicObjectFactory; storyReferenceObjectFactory: StoryReferenceObjectFactory; constructor( - id: string, name: string, description: string, languageCode: string, - canonicalStoryReferences: Array, + id: string, name: string, abbreviatedName: string, description: string, + languageCode: string, canonicalStoryReferences: Array, additionalStoryReferences: Array, uncategorizedSkillIds: Array, nextSubtopicId: number, version: number, subtopics: Array, + thumbnailFilename: string, // TODO(#7165): Replace any with exact type. skillIdToDescriptionMap: any, skillSummaryObjectFactory: SkillSummaryObjectFactory, @@ -56,6 +59,7 @@ export class Topic { storyReferenceObjectFactory: StoryReferenceObjectFactory) { this._id = id; this._name = name; + this._abbreviatedName = abbreviatedName; this._description = description; this._languageCode = languageCode; this._canonicalStoryReferences = canonicalStoryReferences; @@ -69,6 +73,7 @@ export class Topic { this._nextSubtopicId = nextSubtopicId; this._version = version; this._subtopics = cloneDeep(subtopics); + this._thumbnailFilename = thumbnailFilename; this.subtopicObjectFactory = subtopicObjectFactory; this.storyReferenceObjectFactory = storyReferenceObjectFactory; } @@ -86,6 +91,22 @@ export class Topic { this._name = name; } + getAbbreviatedName(): string { + return this._abbreviatedName; + } + + setAbbreviatedName(abbreviatedName: string): void { + this._abbreviatedName = abbreviatedName; + } + + setThumbnailFilename(thumbnailFilename: string): void { + this._thumbnailFilename = thumbnailFilename; + } + + getThumbnailFilename(): string { + return this._thumbnailFilename; + } + getDescription(): string { return this._description; } @@ -116,6 +137,10 @@ export class Topic { issues.push('Topic name should not be empty.'); } + if (!this._abbreviatedName) { + issues.push('Abbreviated name should not be empty.'); + } + let subtopics = this._subtopics; let canonicalStoryIds = this.getCanonicalStoryIds(); let additionalStoryIds = this.getAdditionalStoryIds(); @@ -169,6 +194,14 @@ export class Topic { return issues; } + prepublishValidate(): Array { + let issues = []; + if (!this._thumbnailFilename) { + issues.push('Topic should have a thumbnail.'); + } + return issues; + } + getSkillIds(): Array { let topicSkillIds = cloneDeep( this._uncategorizedSkillSummaries.map((skillSummary: SkillSummary) => { @@ -366,6 +399,8 @@ export class Topic { copyFromTopic(otherTopic: Topic): void { this._id = otherTopic.getId(); this.setName(otherTopic.getName()); + this.setAbbreviatedName(otherTopic.getAbbreviatedName()); + this.setThumbnailFilename(otherTopic.getThumbnailFilename()); this.setDescription(otherTopic.getDescription()); this.setLanguageCode(otherTopic.getLanguageCode()); this._version = otherTopic.getVersion(); @@ -419,11 +454,13 @@ export class TopicObjectFactory { }); return new Topic( topicBackendDict.id, topicBackendDict.name, + topicBackendDict.abbreviated_name, topicBackendDict.description, topicBackendDict.language_code, canonicalStoryReferences, additionalStoryReferences, topicBackendDict.uncategorized_skill_ids, topicBackendDict.next_subtopic_id, topicBackendDict.version, - subtopics, skillIdToDescriptionDict, this.skillSummaryObjectFactory, + subtopics, topicBackendDict.thumbnail_filename, + skillIdToDescriptionDict, this.skillSummaryObjectFactory, this.subtopicObjectFactory, this.storyReferenceObjectFactory ); } @@ -432,8 +469,8 @@ export class TopicObjectFactory { // the actual topic is fetched from the backend. createInterstitialTopic(): Topic { return new Topic( - null, 'Topic name loading', 'Topic description loading', - 'en', [], [], [], 1, 1, [], {}, + null, 'Topic name loading', 'Topic abbreviated name loading', + 'Topic description loading', 'en', [], [], [], 1, 1, [], '', {}, this.skillSummaryObjectFactory, this.subtopicObjectFactory, this.storyReferenceObjectFactory ); diff --git a/core/templates/dev/head/domain/topic/TopicObjectFactorySpec.ts b/core/templates/dev/head/domain/topic/TopicObjectFactorySpec.ts index 2d3e35235e87..1c3bf2bc5657 100644 --- a/core/templates/dev/head/domain/topic/TopicObjectFactorySpec.ts +++ b/core/templates/dev/head/domain/topic/TopicObjectFactorySpec.ts @@ -30,6 +30,8 @@ describe('Topic object factory', () => { let sampleTopicBackendObject = { id: 'sample_topic_id', name: 'Topic name', + abbreviated_name: 'abbrev', + thumbnail_filename: 'img.png', description: 'Topic description', version: 1, uncategorized_skill_ids: ['skill_1', 'skill_2'], @@ -70,11 +72,13 @@ describe('Topic object factory', () => { it('should validate the topic', () => { _sampleTopic.setName(''); + _sampleTopic.setAbbreviatedName(''), _sampleTopic.addCanonicalStory('story_2'); _sampleTopic.getSubtopics()[0].addSkill('skill_1', ''); expect(_sampleTopic.validate()).toEqual([ 'Topic name should not be empty.', + 'Abbreviated name should not be empty.', 'The story with id story_2 is present in both canonical ' + 'and additional stories.', 'The skill with id skill_1 is duplicated in the topic' diff --git a/core/templates/dev/head/domain/topic/topic-domain.constants.ajs.ts b/core/templates/dev/head/domain/topic/topic-domain.constants.ajs.ts index 6058893b0995..5debcb5408f7 100644 --- a/core/templates/dev/head/domain/topic/topic-domain.constants.ajs.ts +++ b/core/templates/dev/head/domain/topic/topic-domain.constants.ajs.ts @@ -66,6 +66,15 @@ angular.module('oppia').constant( angular.module('oppia').constant( 'TOPIC_PROPERTY_NAME', TopicDomainConstants.TOPIC_PROPERTY_NAME); + +angular.module('oppia').constant( + 'TOPIC_PROPERTY_ABBREVIATED_NAME', + TopicDomainConstants.TOPIC_PROPERTY_ABBREVIATED_NAME); + +angular.module('oppia').constant( + 'TOPIC_PROPERTY_THUMBNAIL_FILENAME', + TopicDomainConstants.TOPIC_PROPERTY_THUMBNAIL_FILENAME); + angular.module('oppia').constant( 'TOPIC_PROPERTY_DESCRIPTION', TopicDomainConstants.TOPIC_PROPERTY_DESCRIPTION); diff --git a/core/templates/dev/head/domain/topic/topic-domain.constants.ts b/core/templates/dev/head/domain/topic/topic-domain.constants.ts index b61c8b0d43a5..a6ebc41a573b 100644 --- a/core/templates/dev/head/domain/topic/topic-domain.constants.ts +++ b/core/templates/dev/head/domain/topic/topic-domain.constants.ts @@ -47,6 +47,8 @@ export class TopicDomainConstants { CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY = 'update_subtopic_page_property'; public static TOPIC_PROPERTY_NAME = 'name'; + public static TOPIC_PROPERTY_ABBREVIATED_NAME = 'abbreviated_name'; + public static TOPIC_PROPERTY_THUMBNAIL_FILENAME = 'thumbnail_filename'; public static TOPIC_PROPERTY_DESCRIPTION = 'description'; public static TOPIC_PROPERTY_LANGUAGE_CODE = 'language_code'; diff --git a/core/templates/dev/head/domain/topic/topic-update.service.ts b/core/templates/dev/head/domain/topic/topic-update.service.ts index 235c33b56bc2..b9a6d3ae8444 100644 --- a/core/templates/dev/head/domain/topic/topic-update.service.ts +++ b/core/templates/dev/head/domain/topic/topic-update.service.ts @@ -29,25 +29,27 @@ require('domain/topic/topic-domain.constants.ajs.ts'); angular.module('oppia').factory('TopicUpdateService', [ 'ChangeObjectFactory', 'UndoRedoService', - 'CMD_ADD_SUBTOPIC', - 'CMD_DELETE_ADDITIONAL_STORY', 'CMD_DELETE_CANONICAL_STORY', - 'CMD_DELETE_SUBTOPIC', 'CMD_MOVE_SKILL_ID_TO_SUBTOPIC', - 'CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC', 'CMD_REMOVE_UNCATEGORIZED_SKILL_ID', - 'CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY', 'CMD_UPDATE_SUBTOPIC_PROPERTY', - 'CMD_UPDATE_TOPIC_PROPERTY', 'SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_AUDIO', + 'CMD_ADD_SUBTOPIC', 'CMD_DELETE_ADDITIONAL_STORY', + 'CMD_DELETE_CANONICAL_STORY', 'CMD_DELETE_SUBTOPIC', + 'CMD_MOVE_SKILL_ID_TO_SUBTOPIC', 'CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC', + 'CMD_REMOVE_UNCATEGORIZED_SKILL_ID', 'CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY', + 'CMD_UPDATE_SUBTOPIC_PROPERTY', 'CMD_UPDATE_TOPIC_PROPERTY', + 'SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_AUDIO', 'SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_HTML', 'SUBTOPIC_PROPERTY_TITLE', - 'TOPIC_PROPERTY_DESCRIPTION', 'TOPIC_PROPERTY_LANGUAGE_CODE', - 'TOPIC_PROPERTY_NAME', function( + 'TOPIC_PROPERTY_ABBREVIATED_NAME', 'TOPIC_PROPERTY_DESCRIPTION', + 'TOPIC_PROPERTY_LANGUAGE_CODE', 'TOPIC_PROPERTY_NAME', + 'TOPIC_PROPERTY_THUMBNAIL_FILENAME', function( ChangeObjectFactory, UndoRedoService, - CMD_ADD_SUBTOPIC, - CMD_DELETE_ADDITIONAL_STORY, CMD_DELETE_CANONICAL_STORY, - CMD_DELETE_SUBTOPIC, CMD_MOVE_SKILL_ID_TO_SUBTOPIC, - CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC, CMD_REMOVE_UNCATEGORIZED_SKILL_ID, - CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY, CMD_UPDATE_SUBTOPIC_PROPERTY, - CMD_UPDATE_TOPIC_PROPERTY, SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_AUDIO, + CMD_ADD_SUBTOPIC, CMD_DELETE_ADDITIONAL_STORY, + CMD_DELETE_CANONICAL_STORY, CMD_DELETE_SUBTOPIC, + CMD_MOVE_SKILL_ID_TO_SUBTOPIC, CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC, + CMD_REMOVE_UNCATEGORIZED_SKILL_ID, CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY, + CMD_UPDATE_SUBTOPIC_PROPERTY, CMD_UPDATE_TOPIC_PROPERTY, + SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_AUDIO, SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_HTML, SUBTOPIC_PROPERTY_TITLE, - TOPIC_PROPERTY_DESCRIPTION, TOPIC_PROPERTY_LANGUAGE_CODE, - TOPIC_PROPERTY_NAME) { + TOPIC_PROPERTY_ABBREVIATED_NAME, TOPIC_PROPERTY_DESCRIPTION, + TOPIC_PROPERTY_LANGUAGE_CODE, TOPIC_PROPERTY_NAME, + TOPIC_PROPERTY_THUMBNAIL_FILENAME) { // Creates a change using an apply function, reverse function, a change // command and related parameters. The change is applied to a given // topic. @@ -70,7 +72,7 @@ angular.module('oppia').factory('TopicUpdateService', [ _applyChange(topic, CMD_UPDATE_TOPIC_PROPERTY, { property_name: propertyName, new_value: angular.copy(newValue), - old_value: angular.copy(oldValue) + old_value: angular.copy(oldValue) || null }, apply, reverse); }; @@ -124,6 +126,45 @@ angular.module('oppia').factory('TopicUpdateService', [ }); }, + /** + * Changes the abbreviated name of a topic and records the change in the + * undo/redo service. + */ + setAbbreviatedTopicName: function(topic, abbreviatedName) { + var oldAbbreviatedName = angular.copy(topic.getAbbreviatedName()); + _applyTopicPropertyChange( + topic, TOPIC_PROPERTY_ABBREVIATED_NAME, + abbreviatedName, oldAbbreviatedName, + function(changeDict, topic) { + // Apply + var name = _getNewPropertyValueFromChangeDict(changeDict); + topic.setAbbreviatedName(name); + }, function(changeDict, topic) { + // Undo. + topic.setAbbreviatedName(oldAbbreviatedName); + }); + }, + + /** + * Changes the thumbnail filename of a topic and records the change in the + * undo/redo service. + */ + setThumbnailFilename: function(topic, thumbnailFilename) { + var oldThumbnailFilename = angular.copy(topic.getThumbnailFilename()); + _applyTopicPropertyChange( + topic, TOPIC_PROPERTY_THUMBNAIL_FILENAME, + thumbnailFilename, oldThumbnailFilename, + function(changeDict, topic) { + // Apply + var thumbnailFilename = ( + _getNewPropertyValueFromChangeDict(changeDict)); + topic.setThumbnailFilename(thumbnailFilename); + }, function(changeDict, topic) { + // Undo. + topic.setThumbnailFilename(oldThumbnailFilename); + }); + }, + /** * Changes the description of a topic and records the change in the * undo/redo service. diff --git a/core/templates/dev/head/pages/topic-editor-page/editor-tab/topic-editor-tab.directive.html b/core/templates/dev/head/pages/topic-editor-page/editor-tab/topic-editor-tab.directive.html index d9d150de9fb3..ca1f2c8957ce 100644 --- a/core/templates/dev/head/pages/topic-editor-page/editor-tab/topic-editor-tab.directive.html +++ b/core/templates/dev/head/pages/topic-editor-page/editor-tab/topic-editor-tab.directive.html @@ -1,5 +1,25 @@
+
+ +

+ Upload a 150 x 150 px thumbnail image for the topic +

+
+
+
+
+ + + +
+
+
+
+
+ + + + Please use at most 12 characters for the abbreviated topic name. + +
+