UI modifications for the new editors and dashboard (oppia#5601)

* Made UI changes to skill and topic editors * made review and more ui changes * Fixed backend tests * Made more ui changes * lint fix * More ui changes * final ui changes * Made UI changes * made review changes * disabled flag * made code review changes * disabled flag * made review changes
dmoneyballer · Sep 15, 2018 · 058e3a0 · 058e3a0
1 parent f378256
commit 058e3a0
Show file tree

Hide file tree

Showing 49 changed files with 617 additions and 322 deletions.
diff --git a/assets/i18n/en.json b/assets/i18n/en.json
@@ -525,6 +525,7 @@
     "I18N_TOPNAV_SIGN_IN": "Sign in",
     "I18N_TOPNAV_SIGN_IN_WITH_GOOGLE": "Sign in with Google",
     "I18N_TOPNAV_TEACH_WITH_OPPIA": "Teach with Oppia",
+    "I18N_TOPNAV_TOPICS_AND_SKILLS_DASHBOARD": "Topics and Skills Dashboard",
     "I18N_TOTAL_SUBSCRIBERS_TEXT": "You have a total of <[totalSubscribers]> subscribers.",
     "I18N_UNSUBSCRIBE_BUTTON_TEXT": "Unsubscribe",
     "I18N_WORKED_EXAMPLE": "Worked Example"

diff --git a/assets/i18n/qqq.json b/assets/i18n/qqq.json
@@ -525,6 +525,7 @@
 	"I18N_TOPNAV_SIGN_IN": "Text displayed in the top right corner of the page. - When clicked, the user is redirected to the sign in page where he can access his account.\n{{Identical|Sign in}}",
 	"I18N_TOPNAV_SIGN_IN_WITH_GOOGLE": "Text displayed inside the Sign In dropdown. - When clicked, the user is signed in with Google Oauth - The text in the button needs to be less than 25 characters long",
 	"I18N_TOPNAV_TEACH_WITH_OPPIA": "Text displayed inside drop-down menu - The list is shown after the user hovers over the I18N_TOPNAV_ABOUT button. When the option is clicked, the teach with oppia page is loaded which contains information specific for teachers who would like to create explorations.",
+	"I18N_TOPNAV_TOPICS_AND_SKILLS_DASHBOARD": "Text displayed in the dropdown on the navbar for reaching the topics and skills dashboard.",
 	"I18N_TOTAL_SUBSCRIBERS_TEXT": "Text displayed under the subscribers tab in creator dashboard. Tells the creator the total number of subscribers he/she has.",
 	"I18N_UNSUBSCRIBE_BUTTON_TEXT": "The text that appears on the unsubscribe button, which allows users to unsubscribe from creators.\n{{Identical|Unsubscribe}}",
 	"I18N_WORKED_EXAMPLE": "The text that is displayed on the button to view another worked example when a skill concept card is displayed in the exploration player."

diff --git a/core/controllers/base.py b/core/controllers/base.py
@@ -201,6 +201,8 @@ def __init__(self, request, response):  # pylint: disable=super-init-not-called
         self.values['is_moderator'] = user_services.is_at_least_moderator(
             self.user_id)
         self.values['is_admin'] = user_services.is_admin(self.user_id)
+        self.values['is_topic_manager'] = (
+            user_services.is_topic_manager(self.user_id))
         self.values['is_super_admin'] = self.is_super_admin
 
         if self.request.get('payload'):

diff --git a/core/controllers/library_test.py b/core/controllers/library_test.py
@@ -55,6 +55,7 @@ def test_library_handler_demo_exploration(self):
         self.assertEqual({
             'iframed': False,
             'is_admin': False,
+            'is_topic_manager': False,
             'is_moderator': False,
             'is_super_admin': False,
             'activity_list': [],

diff --git a/core/controllers/topic_editor.py b/core/controllers/topic_editor.py
@@ -42,7 +42,7 @@ class TopicEditorStoryHandler(base.BaseHandler):
     display in topic editor page.
     """
 
-    @acl_decorators.can_edit_topic
+    @acl_decorators.can_view_any_topic_editor
     def get(self, topic_id):
         """Handles GET requests."""
 
@@ -97,7 +97,7 @@ class TopicEditorQuestionHandler(base.BaseHandler):
     summaries for display in topic editor page.
     """
 
-    @acl_decorators.can_edit_topic
+    @acl_decorators.can_view_any_topic_editor
     def get(self, topic_id):
         """Handles GET requests."""
         if not feconf.ENABLE_NEW_STRUCTURES:
@@ -191,7 +191,7 @@ def _require_valid_version(
                 'which is too old. Please reload the page and try again.'
                 % (subtopic_page_version, version_from_payload))
 
-    @acl_decorators.can_edit_subtopic_page
+    @acl_decorators.can_view_any_topic_editor
     def get(self, topic_id, subtopic_id):
         """Handles GET requests."""
 

diff --git a/core/controllers/topic_editor_test.py b/core/controllers/topic_editor_test.py
@@ -118,9 +118,8 @@ def test_get(self):
             self.login(self.TOPIC_MANAGER_EMAIL)
             response = self.testapp.get(
                 '%s/%s?cursor=' % (
-                    feconf.TOPIC_EDITOR_QUESTION_URL, self.topic_id
-                ), expect_errors=True)
-            self.assertEqual(response.status_int, 401)
+                    feconf.TOPIC_EDITOR_QUESTION_URL, self.topic_id))
+            self.assertEqual(response.status_int, 200)
             self.logout()
 
             topic_services.assign_role(

diff --git a/core/controllers/topics_and_skills_dashboard.py b/core/controllers/topics_and_skills_dashboard.py
@@ -146,26 +146,24 @@ class NewSkillHandler(base.BaseHandler):
     def post(self):
         if not feconf.ENABLE_NEW_STRUCTURES:
             raise self.PageNotFoundException
-        topic_id = self.payload.get('topic_id')
-
-        if topic_id is not None:
-            topic = topic_services.get_topic_by_id(topic_id, strict=False)
-            if topic is None:
-                raise self.InvalidInputException
 
         description = self.payload.get('description')
+        linked_topic_ids = self.payload.get('linked_topic_ids')
+        new_skill_id = skill_services.get_new_skill_id()
+        if linked_topic_ids is not None:
+            topics = topic_services.get_topics_by_ids(linked_topic_ids)
+            for topic in topics:
+                if topic is None:
+                    raise self.InvalidInputException
+                topic_services.add_uncategorized_skill(
+                    self.user_id, topic.id, new_skill_id)
 
         skill_domain.Skill.require_valid_description(description)
 
-        new_skill_id = skill_services.get_new_skill_id()
         skill = skill_domain.Skill.create_default_skill(
             new_skill_id, description)
         skill_services.save_new_skill(self.user_id, skill)
 
-        if topic_id is not None:
-            topic_services.add_uncategorized_skill(
-                self.user_id, topic_id, new_skill_id)
-
         self.render_json({
             'skillId': new_skill_id
         })
diff --git a/core/controllers/topics_and_skills_dashboard_test.py b/core/controllers/topics_and_skills_dashboard_test.py
@@ -169,7 +169,10 @@ def test_skill_creation_in_invalid_topic(self):
 
             json_response = self.post_json(
                 '%s' % feconf.NEW_SKILL_URL,
-                {'description': 'Skill Description', 'topic_id': 'topic'},
+                {
+                    'description': 'Skill Description',
+                    'linked_topic_ids': ['topic']
+                },
                 csrf_token=csrf_token, expect_errors=True,
                 expected_status_int=400)
             self.assertEqual(json_response['status_code'], 400)
@@ -184,7 +187,10 @@ def test_skill_creation_in_valid_topic(self):
 
             json_response = self.post_json(
                 '%s' % feconf.NEW_SKILL_URL,
-                {'description': 'Skill Description', 'topic_id': self.topic_id},
+                {
+                    'description': 'Skill Description',
+                    'linked_topic_ids': [self.topic_id]
+                },
                 csrf_token=csrf_token)
             skill_id = json_response['skillId']
             self.assertEqual(len(skill_id), 12)

diff --git a/core/domain/topic_services.py b/core/domain/topic_services.py
@@ -201,6 +201,23 @@ def get_topic_by_id(topic_id, strict=True, version=None):
             return None
 
 
+def get_topics_by_ids(topic_ids):
+    """Returns a list of topics matching the IDs provided.
+
+    Args:
+        topic_ids: list(str). List of IDs to get topics for.
+
+    Returns:
+        list(Topic|None). The list of topics corresponding to given ids
+            (with None in place of topic ids corresponding to deleted topics).
+    """
+    all_topic_models = topic_models.TopicModel.get_multi(topic_ids)
+    topics = [
+        get_topic_from_model(topic_model) if topic_model is not None else None
+        for topic_model in all_topic_models]
+    return topics
+
+
 def get_topic_by_name(topic_name):
     """Returns a domain object representing a topic.
 
@@ -625,6 +642,9 @@ def delete_topic(committer_id, topic_id, force_deletion=False):
         committer_id, feconf.COMMIT_MESSAGE_TOPIC_DELETED,
         force_deletion=force_deletion)
 
+    # Delete the summary of the topic (regardless of whether
+    # force_deletion is True or not).
+    delete_topic_summary(topic_id)
     topic_model = topic_models.TopicModel.get(topic_id)
     for subtopic in topic_model.subtopics:
         subtopic_page_services.delete_subtopic_page(
@@ -638,10 +658,6 @@ def delete_topic(committer_id, topic_id, force_deletion=False):
     topic_memcache_key = _get_topic_memcache_key(topic_id)
     memcache_services.delete(topic_memcache_key)
 
-    # Delete the summary of the topic (regardless of whether
-    # force_deletion is True or not).
-    delete_topic_summary(topic_id)
-
 
 def delete_topic_summary(topic_id):
     """Delete a topic summary model.

diff --git a/core/domain/topic_services_test.py b/core/domain/topic_services_test.py
@@ -119,6 +119,17 @@ def test_get_topic_by_id(self):
         topic = topic_services.get_topic_by_id(self.TOPIC_ID)
         self.assertEqual(topic.to_dict(), expected_topic)
 
+    def test_get_topics_by_id(self):
+        expected_topic = self.topic.to_dict()
+        topics = topic_services.get_topics_by_ids([self.TOPIC_ID])
+        self.assertEqual(topics[0].to_dict(), expected_topic)
+        self.assertEqual(len(topics), 1)
+
+        topics = topic_services.get_topics_by_ids([self.TOPIC_ID, 'topic'])
+        self.assertEqual(topics[0].to_dict(), expected_topic)
+        self.assertIsNone(topics[1])
+        self.assertEqual(len(topics), 2)
+
     def test_commit_log_entry(self):
         topic_commit_log_entry = (
             topic_models.TopicCommitLogEntryModel.get_commit(self.TOPIC_ID, 1)

diff --git a/core/templates/dev/head/components/SkillCreationService.js b/core/templates/dev/head/components/SkillCreationService.js
@@ -27,15 +27,16 @@ oppia.factory('SkillCreationService', [
     var skillCreationInProgress = false;
 
     return {
-      createNewSkill: function(description) {
+      createNewSkill: function(description, linkedTopicIds) {
         if (skillCreationInProgress) {
           return;
         }
         skillCreationInProgress = true;
         AlertsService.clearWarnings();
         $rootScope.loadingMessage = 'Creating skill';
         $http.post('/skill_editor_handler/create_new', {
-          description: description
+          description: description,
+          linked_topic_ids: linkedTopicIds
         }).then(function(response) {
           $timeout(function() {
             $window.location = UrlInterpolationService.interpolateUrl(

diff --git a/core/templates/dev/head/components/SummaryListHeaderDirective.js b/core/templates/dev/head/components/SummaryListHeaderDirective.js
@@ -21,6 +21,7 @@ oppia.directive('summaryListHeader', [
     return {
       restrict: 'E',
       scope: {
+        isSortingDisabled: '&disableSorting',
         getIndex: '&index',
         getSummary: '&summary',
         getShortSummary: '&shortSummary',

diff --git a/core/templates/dev/head/components/summary_list_header_directive.html b/core/templates/dev/head/components/summary_list_header_directive.html
@@ -14,10 +14,13 @@
 
   summary-list-header .oppia-example-header-block {
     overflow: hidden;
-    padding-left: 24px;
     white-space: nowrap;
   }
 
+  summary-list-header .left-padding {
+    padding-left: 24px;
+  }
+
   summary-list-header .oppia-example-header {
     float: left;
     overflow: hidden;
@@ -34,7 +37,7 @@
     }
   }
 </style>
-<div class="oppia-example-header-block">
+<div class="oppia-example-header-block" ng-class="{'left-padding': !isSortingDisabled()}">
   <div class="oppia-example-header">
     <span ng-if="!isActive()" ng-attr-title="<[getSummary()]>">
       <[getShortSummary()]>

diff --git a/core/templates/dev/head/components/top_navigation_bar/TopNavigationBarDirective.js b/core/templates/dev/head/components/top_navigation_bar/TopNavigationBarDirective.js
@@ -52,6 +52,7 @@ oppia.directive('topNavigationBar', [
             $scope.profilePictureDataUrl = dataUrl;
           });
           $scope.isAdmin = GLOBALS.isAdmin;
+          $scope.isTopicManager = GLOBALS.isTopicManager;
           $scope.isModerator = GLOBALS.isModerator;
           $scope.isSuperAdmin = GLOBALS.isSuperAdmin;
           $scope.logoutUrl = GLOBALS.logoutUrl;

diff --git a/core/templates/dev/head/components/top_navigation_bar/top_navigation_bar_directive.html b/core/templates/dev/head/components/top_navigation_bar/top_navigation_bar_directive.html
@@ -123,6 +123,12 @@
             <span translate="I18N_TOPNAV_LEARNER_DASHBOARD"></span>
           </a>
         </li>
+        <li>
+          <a ng-if="isAdmin || isTopicManager" ng-click="closeSubmenuIfNotMobile($event)" href="/topics_and_skills_dashboard"
+             class="protractor-test-dashboard-link">
+            <span translate="I18N_TOPNAV_TOPICS_AND_SKILLS_DASHBOARD"></span>
+          </a>
+        </li>
         <li>
           <a ng-click="closeSubmenuIfNotMobile($event)" href="/notifications_dashboard"
              class="protractor-test-notifications-link">

diff --git a/core/templates/dev/head/pages/base.html b/core/templates/dev/head/pages/base.html
@@ -91,6 +91,7 @@
         SITE_FEEDBACK_FORM_URL: JSON.parse('{{SITE_FEEDBACK_FORM_URL|js_string}}'),
         iframed: JSON.parse('{{iframed|js_string}}'),
         isAdmin: JSON.parse('{{is_admin|js_string}}'),
+        isTopicManager: JSON.parse('{{is_topic_manager|js_string}}'),
         isModerator: JSON.parse('{{is_moderator|js_string}}'),
         isSuperAdmin: JSON.parse('{{is_super_admin|js_string}}'),
         loginUrl: JSON.parse('{{login_url|js_string}}'),

diff --git a/core/templates/dev/head/pages/skill_editor/SkillEditorNavbarBreadcrumbDirective.js b/core/templates/dev/head/pages/skill_editor/SkillEditorNavbarBreadcrumbDirective.js
@@ -0,0 +1,40 @@
+// Copyright 2016 The Oppia Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS-IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * @fileoverview Controller for the navbar breadcrumb of the skill editor.
+ */
+oppia.directive('skillEditorNavbarBreadcrumb', [
+  'UrlInterpolationService', function(UrlInterpolationService) {
+    return {
+      restrict: 'E',
+      scope: {},
+      templateUrl: UrlInterpolationService.getDirectiveTemplateUrl(
+        '/pages/skill_editor/skill_editor_navbar_breadcrumb_directive.html'),
+      controller: [
+        '$scope', 'SkillEditorStateService',
+        function($scope, SkillEditorStateService) {
+          var skill = SkillEditorStateService.getSkill();
+          $scope.getTruncatedDescription = function() {
+            var truncatedDescription = skill.getDescription().substr(0, 40);
+            if (skill.getDescription().length > 40) {
+              truncatedDescription += '...';
+            }
+            return truncatedDescription;
+          };
+        }
+      ]
+    };
+  }
+]);
diff --git a/core/templates/dev/head/pages/skill_editor/SkillEditorNavbarDirective.js b/core/templates/dev/head/pages/skill_editor/SkillEditorNavbarDirective.js
@@ -79,6 +79,7 @@ oppia.directive('skillEditorNavbar', [
                 $scope.skillRights.setPublic();
                 SkillEditorStateService.setSkillRights(
                   $scope.skillRights);
+                AlertsService.addSuccessMessage('Skill Published.');
               });
           };
 
@@ -103,6 +104,7 @@ oppia.directive('skillEditorNavbar', [
 
             modalInstance.result.then(function(commitMessage) {
               SkillEditorStateService.saveSkill(commitMessage);
+              AlertsService.addSuccessMessage('Changes Saved.');
             });
           };
 

diff --git a/core/templates/dev/head/pages/skill_editor/editor_tab/SkillDescriptionEditorDirective.js b/core/templates/dev/head/pages/skill_editor/editor_tab/SkillDescriptionEditorDirective.js
@@ -32,25 +32,17 @@ oppia.directive('skillDescriptionEditor', [
         '$scope',
         function($scope) {
           $scope.skill = SkillEditorStateService.getSkill();
+          $scope.tmpSkillDescription = $scope.skill.getDescription();
           $scope.skillRights = SkillEditorStateService.getSkillRights();
-          $scope.skillDescriptionEditorIsShown = false;
-
-          $scope.openSkillDescriptionEditor = function() {
-            if ($scope.canEditSkillDescription()) {
-              $scope.skillDescriptionEditorIsShown = true;
-              $scope.tmpSkillDescription = $scope.skill.getDescription();
-            }
-          };
-
-          $scope.closeSkillDescriptionEditor = function() {
-            $scope.skillDescriptionEditorIsShown = false;
-          };
 
           $scope.canEditSkillDescription = function() {
             return $scope.skillRights.canEditSkillDescription();
           };
 
           $scope.saveSkillDescription = function(newSkillDescription) {
+            if (newSkillDescription === $scope.skill.getDescription()) {
+              return;
+            }
             if (SkillObjectFactory.hasValidDescription(
               newSkillDescription)) {
               $scope.skillDescriptionEditorIsShown = false;

diff --git a/core/templates/dev/head/pages/skill_editor/editor_tab/add_misconception_modal_directive.html b/core/templates/dev/head/pages/skill_editor/editor_tab/add_misconception_modal_directive.html
@@ -22,7 +22,10 @@ <h3>Add Misconception</h3>
           <span class="oppia-editor-description">Use a name that will be easy for question creators to understand.</span>
         </div>
         <div>
-          <input class="form-control" type="text" ng-model="misconceptionName">
+          <input class="form-control" type="text" ng-model="misconceptionName" maxlength="30">
+          <span ng-if="misconceptionName.length === 30" class="help-block" style="color: red; font-size: smaller">
+            <em>Please use at most 30 characters.</em>
+          </span>
         </div>
       </div>
       <div class="oppia-misconception-editor-section">

diff --git a/core/templates/dev/head/pages/skill_editor/editor_tab/misconception_editor_directive.html b/core/templates/dev/head/pages/skill_editor/editor_tab/misconception_editor_directive.html
@@ -31,7 +31,10 @@
     <div ng-if="isEditable() && nameEditorIsOpen">
       <div class="oppia-rule-details-header">
         <strong>Misconception Name</strong>
-        <input class="form-control" type="text" ng-model="container.misconceptionName">
+        <input class="form-control" type="text" ng-model="container.misconceptionName" maxlength="30">
+        <span ng-if="container.misconceptionName.length === 30" class="help-block" style="color: red; font-size: smaller">
+          <em>Please use at most 30 characters.</em>
+        </span>
       </div>
 
       <div class="oppia-rule-save-cancel-buttons">