Merge branch 'develop' into new_base

oppia · Nov 23, 2018 · bff0c62 · bff0c62
2 parents 737ee27 + 029eac6
commit bff0c62
Show file tree

Hide file tree

Showing 84 changed files with 843 additions and 348 deletions.
diff --git a/constants.py b/constants.py
@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-#pylint: disable=invalid-name
+# pylint: disable=invalid-name
 
 """Loads constants for backend use."""
 

diff --git a/core/controllers/base_test.py b/core/controllers/base_test.py
@@ -142,7 +142,7 @@ def test_root_redirect_rules_for_logged_in_learners(self):
     def test_root_redirect_rules_for_users_with_no_user_contribution_model(
             self):
         self.login(self.TEST_LEARNER_EMAIL)
-        # delete the UserContributionModel.
+        # Delete the UserContributionModel.
         user_id = user_services.get_user_id_from_username(
             self.TEST_LEARNER_USERNAME)
         user_contribution_model = user_models.UserContributionsModel.get(

diff --git a/core/controllers/editor_test.py b/core/controllers/editor_test.py
@@ -1883,7 +1883,7 @@ def test_draft_not_updated_validation_error(self):
             '%s.%s' % (self.owner_id, self.EXP_ID2))
         self.assertEqual(
             exp_user_data.draft_change_list, self.DRAFT_CHANGELIST)
-        # id is incremented the first time but not the second.
+        # ID is incremented the first time but not the second.
         self.assertEqual(exp_user_data.draft_change_list_id, 2)
         self.assertEqual(
             response, {'status_code': 400,

diff --git a/core/controllers/library_test.py b/core/controllers/library_test.py
@@ -94,7 +94,7 @@ def test_library_handler_demo_exploration(self):
             self.count_jobs_in_taskqueue(
                 taskqueue_services.QUEUE_NAME_ONE_OFF_JOBS), 0)
 
-        # change title and category.
+        # Change title and category.
         exp_services.update_exploration(
             self.editor_id, '0', [exp_domain.ExplorationChange({
                 'cmd': 'edit_exploration_property',

diff --git a/core/controllers/question_editor_test.py b/core/controllers/question_editor_test.py
@@ -59,8 +59,7 @@ def setUp(self):
 
 
 class QuestionCreationHandlerTest(BaseQuestionEditorControllerTest):
-    """Tests returning of new question ids and creating questions.
-    """
+    """Tests returning of new question ids and creating questions."""
 
     def setUp(self):
         """Completes the setup for QuestionSkillLinkHandlerTest."""
@@ -115,8 +114,7 @@ def test_post(self):
 
 
 class QuestionSkillLinkHandlerTest(BaseQuestionEditorControllerTest):
-    """Tests link and unlink question from skills.
-    """
+    """Tests link and unlink question from skills."""
 
     def setUp(self):
         """Completes the setup for QuestionSkillLinkHandlerTest."""
@@ -217,8 +215,7 @@ def test_delete(self):
 
 
 class EditableQuestionDataHandlerTest(BaseQuestionEditorControllerTest):
-    """Tests get, put and delete methods of editable questions data handler.
-    """
+    """Tests get, put and delete methods of editable questions data handler."""
 
     def test_get(self):
         with self.swap(constants, 'ENABLE_NEW_STRUCTURES', True):

diff --git a/core/controllers/resources.py b/core/controllers/resources.py
@@ -43,82 +43,46 @@ def get(self, generator_id):
             raise self.PageNotFoundException
 
 
-class ImageDevHandler(base.BaseHandler):
-    """Handles image retrievals (only in dev -- in production, image files are
-    served from GCS).
+class AssetDevHandler(base.BaseHandler):
+    """Handles image and audio retrievals (only in dev -- in production,
+    image and audio files are served from GCS).
     """
 
-    _IMAGE_PATH_PREFIX = 'image'
+    _SUPPORTED_TYPES = ['image', 'audio']
 
     GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
 
     @acl_decorators.open_access
-    def get(self, exploration_id, encoded_filename):
-        """Returns an image.
+    def get(self, exploration_id, asset_type, encoded_filename):
+        """Returns an asset file.
 
         Args:
-            exploration_id: the id of the exploration.
-            encoded_filename: a string representing the image filename. This
+            exploration_id: str. The id of the exploration.
+            asset_type: str. Type of the asset, either image or audio.
+            encoded_filename: str. The asset filename. This
               string is encoded in the frontend using encodeURIComponent().
         """
         if not constants.DEV_MODE:
             raise self.PageNotFoundException
+        if asset_type not in self._SUPPORTED_TYPES:
+            raise Exception('%s is not a supported asset type.' % asset_type)
         try:
             filename = urllib.unquote(encoded_filename)
             file_format = filename[(filename.rfind('.') + 1):]
+
             # If the following is not cast to str, an error occurs in the wsgi
             # library because unicode gets used.
             self.response.headers['Content-Type'] = str(
-                'image/%s' % file_format)
+                '%s/%s' % (asset_type, file_format))
 
             fs = fs_domain.AbstractFileSystem(
                 fs_domain.ExplorationFileSystem(
                     'exploration/%s' % exploration_id))
-            raw = fs.get('%s/%s' % (self._IMAGE_PATH_PREFIX, filename))
+            raw = fs.get('%s/%s' % (asset_type, filename))
 
             self.response.cache_control.no_cache = None
             self.response.cache_control.public = True
             self.response.cache_control.max_age = 600
             self.response.write(raw)
         except:
             raise self.PageNotFoundException
-
-
-class AudioDevHandler(base.BaseHandler):
-    """Handles audio retrievals (only in dev -- in production, audio files are
-    served from GCS).
-    """
-
-    _AUDIO_PATH_PREFIX = 'audio'
-    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
-
-    @acl_decorators.open_access
-    def get(self, exploration_id, encoded_filename):
-        """Returns an audio file.
-
-        Args:
-            encoded_filename: a string representing the audio filename. This
-              string is encoded in the frontend using encodeURIComponent().
-        """
-        if not constants.DEV_MODE:
-            raise self.PageNotFoundException
-
-        filename = urllib.unquote(encoded_filename)
-        file_format = filename[(filename.rfind('.') + 1):]
-        # If the following is not cast to str, an error occurs in the wsgi
-        # library because unicode gets used.
-        self.response.headers['Content-Type'] = str(
-            'audio/%s' % file_format)
-
-        fs = fs_domain.AbstractFileSystem(
-            fs_domain.ExplorationFileSystem('exploration/%s' % exploration_id))
-
-        try:
-            raw = fs.get('%s/%s' % (self._AUDIO_PATH_PREFIX, filename))
-        except:
-            raise self.PageNotFoundException
-
-        self.response.cache_control.no_cache = None
-        self.response.cache_control.public = True
-        self.response.cache_control.max_age = 600
-        self.response.write(raw)
diff --git a/core/controllers/resources_test.py b/core/controllers/resources_test.py
@@ -23,7 +23,7 @@
 import feconf
 
 
-class ImageDevHandlerTest(test_utils.GenericTestBase):
+class AssetDevHandlerImageTest(test_utils.GenericTestBase):
 
     IMAGE_UPLOAD_URL_PREFIX = '/createhandler/imageupload'
     ASSET_HANDLER_URL_PREFIX = '/assetsdevhandler'
@@ -35,7 +35,7 @@ def _get_image_url(self, exp_id, filename):
 
     def setUp(self):
         """Load a demo exploration and register self.EDITOR_EMAIL."""
-        super(ImageDevHandlerTest, self).setUp()
+        super(AssetDevHandlerImageTest, self).setUp()
 
         exp_services.delete_demo('0')
         self.system_user = user_services.get_system_user()
@@ -223,8 +223,18 @@ def test_bad_extensions_are_detected(self):
 
         self.logout()
 
+    def test_request_invalid_asset_type(self):
+        """Test that requests for invalid asset type is rejected with a 404."""
+        self.login(self.EDITOR_EMAIL)
+
+        response = self.testapp.get(
+            '/assetsdevhandler/0/assets/unknowntype/myfile',
+            expect_errors=True)
+        self.logout()
+        self.assertEqual(response.status_int, 404)
+
 
-class AudioDevHandlerTest(test_utils.GenericTestBase):
+class AssetDevHandlerAudioTest(test_utils.GenericTestBase):
     """Test the upload of audio files to GCS."""
 
     TEST_AUDIO_FILE_MP3 = 'cafe.mp3'
@@ -234,7 +244,7 @@ class AudioDevHandlerTest(test_utils.GenericTestBase):
     AUDIO_UPLOAD_URL_PREFIX = '/createhandler/audioupload'
 
     def setUp(self):
-        super(AudioDevHandlerTest, self).setUp()
+        super(AssetDevHandlerAudioTest, self).setUp()
         exp_services.delete_demo('0')
         self.system_user = user_services.get_system_user()
         exp_services.load_demo('0')

diff --git a/core/domain/acl_decorators.py b/core/domain/acl_decorators.py
@@ -262,7 +262,7 @@ def can_edit_collection(handler):
     """Decorator to check whether the user can edit collection.
 
     Args:
-        handler: The function to be decorated.
+        handler: function. The function to be decorated.
 
     Returns:
         function. The newly decorated function that checks if
@@ -1615,7 +1615,7 @@ def can_delete_question(handler):
     """Decorator to check whether the user can delete a question.
 
     Args:
-        handler. The function to be decorated.
+        handler: function. The function to be decorated.
 
     Returns:
         function. The newly decorated function that now also checks
@@ -1747,7 +1747,7 @@ def can_edit_story(handler):
     topic.
 
     Args:
-        handler. The function to be decorated.
+        handler: function. The function to be decorated.
 
     Returns:
         function. The newly decorated function that now also checks if
@@ -2054,7 +2054,7 @@ def can_delete_topic(handler):
     """Decorator to check whether the user can delete a topic.
 
     Args:
-        handler. The function to be decorated.
+        handler: function. The function to be decorated.
 
     Returns:
         function. The newly decorated function that now also
@@ -2138,7 +2138,7 @@ def can_access_topics_and_skills_dashboard(handler):
     dashboard.
 
     Args:
-        handler. The function to be decorated.
+        handler: function. The function to be decorated.
 
     Returns:
         function. The newly decorated function that also checks if
@@ -2386,6 +2386,21 @@ def generate_decorator_for_handler(handler):
         """
         def test_can_accept_suggestion(
                 self, target_id, suggestion_id, **kwargs):
+            """Returns a (possibly-decorated) handler to test whether a
+            suggestion can be accepted based on the user actions and roles.
+
+            Args:
+                target_id: str. The target id.
+                suggestion_id: str. The suggestion id.
+                **kwargs: *. Keyword arguments.
+
+            Returns:
+                function. The (possibly-decorated) handler for accepting a
+                    suggestion.
+
+            Raises:
+                NotLoggedInException: The user is not logged in.
+            """
             if not self.user_id:
                 raise base.UserFacingExceptions.NotLoggedInException
             user_actions_info = user_services.UserActionsInfo(self.user_id)

diff --git a/core/domain/classifier_domain.py b/core/domain/classifier_domain.py
@@ -69,7 +69,6 @@ class ClassifierTrainingJob(object):
             classification purpose.
         data_schema_version: int. Schema version of the data used by the
             classifier. This depends on the algorithm ID.
-
     """
 
     def __init__(

diff --git a/core/domain/collection_services.py b/core/domain/collection_services.py
@@ -488,13 +488,13 @@ def _get_collection_summary_dicts_from_models(collection_summary_models):
     """Given an iterable of CollectionSummaryModel instances, create a dict
     containing corresponding collection summary domain objects, keyed by id.
 
-    Args：
-        collection_summary_models: An iterable of CollectionSummaryModel
-            instances.
+    Args:
+        collection_summary_models: iterable(CollectionSummaryModel). An
+            iterable of CollectionSummaryModel instances.
 
     Returns:
-        A dict containing corresponding collection summary domain objects, keyed
-        by id.
+        A dict containing corresponding collection summary domain objects,
+        keyed by id.
     """
     collection_summaries = [
         get_collection_summary_from_model(collection_summary_model)
@@ -588,7 +588,7 @@ def apply_change_list(collection_id, change_list):
     Args:
         collection_id: str. ID of the given collection.
         change_list: list(dict). A change list to be applied to the given
-            collection. Each entry in change_list is a dict that represents a
+            collection. Each entry is a dict that represents a
             CollectionChange.
     object.
 
@@ -886,15 +886,15 @@ def update_collection(
     """Updates a collection. Commits changes.
 
     Args:
-    - committer_id: str. The id of the user who is performing the update
-        action.
-    - collection_id: str. The collection id.
-    - change_list: list of dicts, each representing a CollectionChange object.
-        These changes are applied in sequence to produce the resulting
-        collection.
-    - commit_message: str or None. A description of changes made to the
-        collection. For published collections, this must be present; for
-        unpublished collections, it may be equal to None.
+        committer_id: str. The id of the user who is performing the update
+            action.
+        collection_id: str. The collection id.
+        change_list: list(dict). Each entry represents a CollectionChange
+            object. These changes are applied in sequence to produce the
+            resulting collection.
+        commit_message: str or None. A description of changes made to the
+            collection. For published collections, this must be present; for
+            unpublished collections, it may be equal to None.
     """
     is_public = rights_manager.is_collection_public(collection_id)
 

diff --git a/core/domain/email_manager_test.py b/core/domain/email_manager_test.py
@@ -1213,7 +1213,7 @@ def test_correct_email_body_is_sent(self):
             email_manager.send_feedback_message_email(
                 self.editor_id, feedback_messages)
 
-            # check that email body is correct.
+            # Check that email body is correct.
             messages = self.mail_stub.get_sent_messages(to=self.EDITOR_EMAIL)
             self.assertEqual(len(messages), 1)
             self.assertEqual(
@@ -1223,7 +1223,7 @@ def test_correct_email_body_is_sent(self):
                 messages[0].body.decode(),
                 expected_email_text_body)
 
-            # check that email model is correct.
+            # Check that email model is correct.
             all_models = email_models.SentEmailModel.get_all().fetch()
             self.assertEqual(len(all_models), 1)
 
@@ -1299,7 +1299,7 @@ def test_that_suggestion_emails_are_correct(self):
                 self.exploration.title, self.exploration.id, self.new_user_id,
                 self.recipient_list)
 
-            # make sure correct email is sent.
+            # Make sure correct email is sent.
             messages = self.mail_stub.get_sent_messages(to=self.EDITOR_EMAIL)
             self.assertEqual(len(messages), 1)
             self.assertEqual(
@@ -1382,7 +1382,7 @@ def test_that_subscription_emails_are_correct(self):
             email_manager.send_emails_to_subscribers(
                 self.editor_id, self.exploration.id, self.exploration.title)
 
-            # make sure correct email is sent.
+            # Make sure correct email is sent.
             messages = self.mail_stub.get_sent_messages(to=self.NEW_USER_EMAIL)
             self.assertEqual(len(messages), 1)
             self.assertEqual(
@@ -1617,8 +1617,7 @@ def test_that_flag_exploration_emails_are_correct(self):
 
 
 class OnboardingReviewerInstantEmailTests(test_utils.GenericTestBase):
-    """Test that correct email is sent while onboarding reviewers.
-    """
+    """Test that correct email is sent while onboarding reviewers."""
     REVIEWER_USERNAME = 'reviewer'
     REVIEWER_EMAIL = 'reviewer@example.com'
 
@@ -1684,8 +1683,7 @@ def test_that_correct_completion_email_is_sent(self):
 
 
 class NotifyReviewerInstantEmailTests(test_utils.GenericTestBase):
-    """Test that correct email is sent while notifying reviewers.
-    """
+    """Test that correct email is sent while notifying reviewers."""
     REVIEWER_USERNAME = 'reviewer'
     REVIEWER_EMAIL = 'reviewer@example.com'