Fix #5466: Added check to enforce that all name of controllers end wi…

…th "Handler" or "Page" (#5878) * Added line breaks * Moved test to base_test * Modified code * Modified code * Changed condition * Modified render_downloadable test * Check for class name test * Fixed test case * Remove conflicts * Change condition of html response * Made lint changes
oppia · Nov 28, 2018 · d7c44b2 · d7c44b2
1 parent 13074c3
commit d7c44b2
Show file tree

Hide file tree

Showing 24 changed files with 210 additions and 44 deletions.
diff --git a/core/controllers/admin.py b/core/controllers/admin.py
@@ -308,7 +308,7 @@ def post(self):
         self.render_json({})
 
 
-class AdminJobOutput(base.BaseHandler):
+class AdminJobOutputHandler(base.BaseHandler):
     """Retrieves job output to show on the admin page."""
 
     GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
@@ -322,22 +322,22 @@ def get(self):
         })
 
 
-class AdminTopicsCsvDownloadHandler(base.BaseHandler):
+class AdminTopicsCsvFileDownloader(base.BaseHandler):
     """Retrieves topic similarity data for download."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_DOWNLOADABLE
+
     @acl_decorators.can_access_admin_page
     def get(self):
-        self.response.headers['Content-Type'] = 'text/csv'
-        self.response.headers['Content-Disposition'] = (
-            'attachment; filename=topic_similarities.csv')
-        self.response.write(
-            recommendations_services.get_topic_similarities_as_csv())
+        self.render_downloadable_file(
+            recommendations_services.get_topic_similarities_as_csv(),
+            'topic_similarities.csv', 'text/csv')
 
 
 class DataExtractionQueryHandler(base.BaseHandler):
     """Handler for data extraction query."""
 
-    GET_HANDLER_ERROR_RETURN_TYPE = 'json'
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
 
     @acl_decorators.can_access_admin_page
     def get(self):

diff --git a/core/controllers/base.py b/core/controllers/base.py
@@ -281,6 +281,13 @@ def render_json(self, values):
         json_output = json.dumps(values, cls=utils.JSONEncoderForHTML)
         self.response.write('%s%s' % (feconf.XSSI_PREFIX, json_output))
 
+    def render_downloadable_file(self, values, filename, content_type):
+        """Prepares downloadable content to be sent to the client."""
+        self.response.headers['Content-Type'] = content_type
+        self.response.headers['Content-Disposition'] = (
+            'attachment; filename=%s' % (filename))
+        self.response.write(values)
+
     def _get_logout_url(self, redirect_url_on_logout):
         """Prepares and returns logout url which will be handled
         by LogoutPage handler.
@@ -410,18 +417,21 @@ def _render_exception_json_or_html(self, return_type, values):
             values: dict. The key-value pairs to include in the response.
         """
 
-        if return_type == feconf.HANDLER_TYPE_JSON:
-            self.render_json(values)
-        elif return_type == feconf.HANDLER_TYPE_HTML:
+        method = self.request.environ['REQUEST_METHOD']
+
+        if return_type == feconf.HANDLER_TYPE_HTML and (
+                method == 'GET'):
             self.values.update(values)
             if 'iframed' in self.values and self.values['iframed']:
                 self.render_template(
                     'pages/error/error_iframed.html', iframe_restriction=None)
             else:
                 self.render_template('pages/error/error.html')
         else:
-            logging.warning('Not a recognized return type: '
-                            'defaulting to render JSON.')
+            if return_type != feconf.HANDLER_TYPE_JSON and (
+                    return_type != feconf.HANDLER_TYPE_DOWNLOADABLE):
+                logging.warning('Not a recognized return type: '
+                                'defaulting to render JSON.')
             self.render_json(values)
 
     def _render_exception(self, error_code, values):

diff --git a/core/controllers/base_test.py b/core/controllers/base_test.py
@@ -17,6 +17,7 @@
 """Tests for generic controller behavior."""
 
 import datetime
+import inspect
 import json
 import os
 import re
@@ -298,6 +299,37 @@ def test_special_char_escaping(self):
         self.assertNotIn('马', response.body)
 
 
+class RenderDownloadableTest(test_utils.GenericTestBase):
+
+    class MockHandler(base.BaseHandler):
+        """Mock handler that subclasses BaseHandler and serves a response
+        that is of a 'downloadable' type.
+        """
+        def get(self):
+            """Handles GET requests."""
+            file_contents = 'example'
+            self.render_downloadable_file(
+                file_contents, 'example.pdf', 'text/plain')
+
+    def setUp(self):
+        super(RenderDownloadableTest, self).setUp()
+
+        # Modify the testapp to use the mock handler.
+        self.testapp = webtest.TestApp(webapp2.WSGIApplication(
+            [webapp2.Route('/mock', self.MockHandler, name='MockHandler')],
+            debug=feconf.DEBUG,
+        ))
+
+    def test_downloadable(self):
+        response = self.testapp.get('/mock')
+        self.assertEqual(response.status_int, 200)
+        self.assertEqual(
+            response.content_disposition,
+            'attachment; filename=example.pdf')
+        self.assertEqual(response.body, 'example')
+        self.assertEqual(response.content_type, 'text/plain')
+
+
 class LogoutPageTest(test_utils.GenericTestBase):
 
     def test_logout_page(self):
@@ -513,7 +545,7 @@ def test_every_method_has_decorator(self):
         handlers_checked = []
 
         for route in main.URLS:
-            # URLS = MAPREDUCE_HANDLERS + other handers. MAPREDUCE_HANDLERS
+            # URLS = MAPREDUCE_HANDLERS + other handlers. MAPREDUCE_HANDLERS
             # are tuples. So, below check is to handle them.
             if isinstance(route, tuple):
                 continue
@@ -589,3 +621,68 @@ def test_get_items(self):
                 'param2=value%20with%20%26%20%2B%20-%20/&'
                 'param3=value%20with%20.%20%%20@%20123%20=%20!%20%3C%3E')
             self.assertDictContainsSubset(params, result)
+
+
+class ControllerClassNameTest(test_utils.GenericTestBase):
+
+    def test_controller_class_names(self):
+        """This function checks that all controller class names end with
+        either 'Handler', 'Page' or 'FileDownloader'.
+        """
+        # A mapping of returned handler types to expected name endings.
+        handler_type_to_name_endings_dict = {
+            feconf.HANDLER_TYPE_HTML: 'Page',
+            feconf.HANDLER_TYPE_JSON: 'Handler',
+            feconf.HANDLER_TYPE_DOWNLOADABLE: 'FileDownloader',
+        }
+        num_handlers_checked = 0
+        for url in main.URLS:
+            # URLS = MAPREDUCE_HANDLERS + other handlers. MAPREDUCE_HANDLERS
+            # are tuples. So, below check is to pick only those which have
+            # a RedirectRoute associated with it.
+            if isinstance(url, main.routes.RedirectRoute):
+                clazz = url.handler
+                num_handlers_checked += 1
+                all_base_classes = [base_class.__name__ for base_class in
+                                    (inspect.getmro(clazz))]
+                # Check that it is a subclass of 'BaseHandler'.
+                if 'BaseHandler' in all_base_classes:
+                    class_return_type = clazz.GET_HANDLER_ERROR_RETURN_TYPE
+                    # Check that any class with a get handler has a
+                    # GET_HANDLER_ERROR_RETURN_TYPE that's one of
+                    # the allowed values.
+                    if 'get' in clazz.__dict__.keys():
+                        self.assertIn(
+                            class_return_type,
+                            handler_type_to_name_endings_dict)
+                    class_name = clazz.__name__
+                    file_name = inspect.getfile(clazz)
+                    line_num = inspect.getsourcelines(clazz)[1]
+                    allowed_class_ending = handler_type_to_name_endings_dict[
+                        class_return_type]
+                    # Check that the name of the class ends with
+                    # the proper word if it has a get function.
+                    if 'get' in clazz.__dict__.keys():
+                        message = (
+                            'Please ensure that the name of this class '
+                            'ends with \'%s\'' % allowed_class_ending)
+                        error_message = (
+                            '%s --> Line %s: %s'
+                            % (file_name, line_num, message))
+                        self.assertTrue(
+                            class_name.endswith(allowed_class_ending),
+                            msg=error_message)
+
+                    # Check that the name of the class ends with 'Handler'
+                    # if it does not has a get function.
+                    else:
+                        message = (
+                            'Please ensure that the name of this class '
+                            'ends with \'Handler\'')
+                        error_message = (
+                            '%s --> Line %s: %s'
+                            % (file_name, line_num, message))
+                        self.assertTrue(class_name.endswith('Handler'),
+                                        msg=error_message)
+
+        self.assertGreater(num_handlers_checked, 150)
diff --git a/core/controllers/collection_editor.py b/core/controllers/collection_editor.py
@@ -75,6 +75,8 @@ def get(self, collection_id):
 class EditableCollectionDataHandler(CollectionEditorHandler):
     """A data handler for collections which supports writing."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     def _require_valid_version(self, version_from_payload, collection_version):
         """Check that the payload version matches the given collection
         version.
@@ -141,6 +143,8 @@ def put(self, collection_id):
 class CollectionRightsHandler(CollectionEditorHandler):
     """Handles management of collection editing rights."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_edit_collection
     def get(self, collection_id):
         """Gets the editing rights for the given collection.
@@ -234,6 +238,8 @@ def put(self, collection_id):
 class ExplorationMetadataSearchHandler(base.BaseHandler):
     """Provides data for exploration search."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.open_access
     def get(self):
         """Handles GET requests."""

diff --git a/core/controllers/collection_editor_test.py b/core/controllers/collection_editor_test.py
@@ -107,8 +107,8 @@ def test_editable_collection_handler_get(self):
         response = self.testapp.get(
             '%s/%s' % (
                 feconf.COLLECTION_EDITOR_DATA_URL_PREFIX,
-                self.COLLECTION_ID))
-        self.assertEqual(response.status_int, 302)
+                self.COLLECTION_ID), expect_errors=True)
+        self.assertEqual(response.status_int, 401)
 
         # Check that whitelisted users can access the data
         # from the editable_collection_data_handler.

diff --git a/core/controllers/concept_card_viewer.py b/core/controllers/concept_card_viewer.py
@@ -18,11 +18,14 @@
 from core.controllers import base
 from core.domain import acl_decorators
 from core.domain import skill_services
+import feconf
 
 
 class ConceptCardDataHandler(base.BaseHandler):
     """A card that shows the explanation of a skill's concept."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_view_skill
     def get(self, skill_id):
         """Handles GET requests."""

diff --git a/core/controllers/creator_dashboard.py b/core/controllers/creator_dashboard.py
@@ -396,7 +396,7 @@ def post(self):
         })
 
 
-class UploadExploration(base.BaseHandler):
+class UploadExplorationHandler(base.BaseHandler):
     """Uploads a new exploration."""
 
     @acl_decorators.can_upload_exploration

diff --git a/core/controllers/editor.py b/core/controllers/editor.py
@@ -87,6 +87,8 @@ def _require_valid_version(version_from_payload, exploration_version):
 class EditorLogoutHandler(base.BaseHandler):
     """Handles logout from editor page."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.open_access
     def get(self):
         """Checks if exploration is published and redirects accordingly."""
@@ -446,12 +448,12 @@ def put(self, exploration_id):
         })
 
 
-class ExplorationDownloadHandler(EditorHandler):
+class ExplorationFileDownloader(EditorHandler):
     """Downloads an exploration as a zip file, or dict of YAML strings
     representing states.
     """
 
-    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_DOWNLOADABLE
 
     @acl_decorators.can_download_exploration
     def get(self, exploration_id):
@@ -466,16 +468,14 @@ def get(self, exploration_id):
         width = int(self.request.get('width', default_value=80))
 
         # If the title of the exploration has changed, we use the new title.
-        filename = 'oppia-%s-v%s' % (
+        filename = 'oppia-%s-v%s.zip' % (
             utils.to_ascii(exploration.title.replace(' ', '')), version)
 
         if output_format == feconf.OUTPUT_FORMAT_ZIP:
-            self.response.headers['Content-Type'] = 'text/plain'
-            self.response.headers['Content-Disposition'] = (
-                'attachment; filename=%s.zip' % str(filename))
-            self.response.write(
+            self.render_downloadable_file(
                 exp_services.export_to_zip_file(
-                    exploration_id, version=version))
+                    exploration_id, version=version),
+                filename, 'text/plain')
         elif output_format == feconf.OUTPUT_FORMAT_JSON:
             self.render_json(exp_services.export_states_to_yaml(
                 exploration_id, version=version, width=width))
@@ -631,6 +631,7 @@ class FetchIssuesHandler(EditorHandler):
     exploration. This removes the invalid issues and returns the remaining
     unresolved ones.
     """
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
 
     @acl_decorators.can_view_exploration_stats
     def get(self, exp_id):
@@ -652,6 +653,8 @@ def get(self, exp_id):
 class FetchPlaythroughHandler(EditorHandler):
     """Handler used for retrieving a playthrough."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_view_exploration_stats
     def get(self, unused_exploration_id, playthrough_id):
         """Handles GET requests."""
@@ -823,6 +826,8 @@ def post(self, exploration_id):
 class StateAnswerStatisticsHandler(EditorHandler):
     """Returns basic learner answer statistics for a state."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_view_exploration_stats
     def get(self, exploration_id):
         """Handles GET requests."""
@@ -841,6 +846,8 @@ def get(self, exploration_id):
 class TopUnresolvedAnswersHandler(EditorHandler):
     """Returns a list of top N unresolved answers."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_edit_exploration
     def get(self, exploration_id):
         """Handles GET requests for unresolved answers."""

diff --git a/core/controllers/email_dashboard.py b/core/controllers/email_dashboard.py
@@ -40,6 +40,8 @@ def get(self):
 class EmailDashboardDataHandler(base.BaseHandler):
     """Query data handler."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_manage_email_dashboard
     def get(self):
         cursor = self.request.get('cursor')
@@ -123,9 +125,11 @@ def _validate(self, data):
                 raise self.InvalidInputException('400 Invalid input for query.')
 
 
-class QueryStatusCheck(base.BaseHandler):
+class QueryStatusCheckHandler(base.BaseHandler):
     """Handler for checking status of individual queries."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_manage_email_dashboard
     def get(self):
         query_id = self.request.get('query_id')

diff --git a/core/controllers/feedback.py b/core/controllers/feedback.py
@@ -60,8 +60,11 @@ def post(self, exploration_id):
         self.render_json(self.values)
 
 
-class ThreadListHandlerForTopics(base.BaseHandler):
+class ThreadListHandlerForTopicsHandler(base.BaseHandler):
     """Handles listing of suggestions threads linked to topics."""
+
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_edit_topic
     def get(self, topic_id):
         if not constants.ENABLE_NEW_STRUCTURES:

diff --git a/core/controllers/learner_dashboard.py b/core/controllers/learner_dashboard.py
@@ -128,6 +128,7 @@ class LearnerDashboardIdsHandler(base.BaseHandler):
     the activities currently being pursued, and the activities present in
     the playlist.
     """
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
 
     @acl_decorators.can_access_learner_dashboard
     def get(self):
@@ -146,6 +147,8 @@ def get(self):
 class LearnerDashboardFeedbackThreadHandler(base.BaseHandler):
     """Gets all the messages in a thread."""
 
+    GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
+
     @acl_decorators.can_access_learner_dashboard
     def get(self, thread_id):
         """Handles GET requests."""