Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

Merged
merged 12 commits into from
Nov 28, 2018
Merged

Fix #5466: Added check to enforce that all name of controllers end with "Handler" or "Page" #5878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bff1fca
Added line breaks
Sep 13, 2018
dc28ba3
Moved test to base_test
Nov 3, 2018
c2cb23e
Modified code
Nov 10, 2018
843d180
Modified code
Nov 10, 2018
e630d23
Changed condition
Nov 12, 2018
67634f9
Modified render_downloadable test
Nov 13, 2018
41c3ff5
Check for class name test
Nov 16, 2018
737ee27
Fixed test case
Nov 16, 2018
d570f1d
Remove conflicts
Nov 23, 2018
84d54cc
Change condition of html response
Nov 26, 2018
0f7b1ff
Made lint changes
Nov 27, 2018
af4b4e1
Merge branch 'develop' into new_base
Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Modified code
  • Loading branch information
Rishav Chakraborty committed Nov 16, 2018
commit c2cb23e8f1cc335f5f5c4da1f3fe195e09f7c796
9 changes: 5 additions & 4 deletions core/controllers/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,21 +322,22 @@ def get(self):
})


class AdminTopicsCsvDownloadFileDownloader(base.BaseHandler):
class AdminTopicsCsvFileDownloader(base.BaseHandler):
"""Retrieves topic similarity data for download."""

GET_HANDLER_ERROR_RETURN_TYPE = 'downloadable'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_DOWNLOADABLE

@acl_decorators.can_access_admin_page
def get(self):
self.render_downloadable_file(
recommendations_services.get_topic_similarities_as_csv())
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):
Expand Down
6 changes: 3 additions & 3 deletions core/controllers/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,11 @@ 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):
def render_downloadable_file(self, values, filename, content_type):
"""Prepares downloadable content to be sent to the client."""
self.response.headers['Content-Type'] = 'text/csv'
self.response.headers['Content-Type'] = content_type
self.response.headers['Content-Disposition'] = (
'attachment; filename=topic_similarities.csv')
'attachment; filename=%s' % (filename))
self.response.write(values)

def _get_logout_url(self, redirect_url_on_logout):
Expand Down
138 changes: 84 additions & 54 deletions core/controllers/base_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
"""Tests for generic controller behavior."""

import datetime
import importlib
import inspect
import json
import os
Expand Down Expand Up @@ -300,6 +299,33 @@ 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."""
text = 'example'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text is vague -- consider file_contents instead.

self.render_downloadable_file(text, '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.assertTrue(response.content_disposition.startswith('attachment'))
self.assertEqual(response.body, 'example')


class LogoutPageTest(test_utils.GenericTestBase):

def test_logout_page(self):
Expand Down Expand Up @@ -597,58 +623,62 @@ class ControllerClassNameTest(test_utils.GenericTestBase):

def test_controller_class_names(self):
"""This function checks that all controller class names end with
either 'Handler' or 'Page'.
either 'Handler', 'Page' or 'FileDownloader'.
"""
allowed_handler_error_return_type = [feconf.HANDLER_TYPE_JSON,
feconf.HANDLER_TYPE_HTML,
feconf.HANDLER_TYPE_DOWNLOADABLE]
# 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',
}
for url in main.URLS:
if not isinstance(url, tuple):
handler = str(url.handler)[8:-2].rsplit('.', 1)[1]
class_path = str(url.handler)[8:-2].rsplit('.', 1)[0]
file_name = class_path.replace('.', '/') + ".py"
module = importlib.import_module(class_path)
clazz = getattr(module, handler)
all_base_classes = [base_class.__name__ for base_class in
(inspect.getmro(clazz))]
if 'BaseHandler' in all_base_classes:
class_return_type = clazz.GET_HANDLER_ERROR_RETURN_TYPE
# Check that the name of the class ends with 'Handler'
# if it renders JSON.
if class_return_type == 'json' or (
not 'get' in clazz.__dict__.keys()):
message = (
'Please ensure that the name of this class '
'ends with \'Handler\'')
error_message = (
'%s --> Line %s: %s'
% (file_name, inspect.getsourcelines(clazz)[-1],
message))
self.assertTrue(handler.endswith('Handler'),
msg=error_message)

# Check that the name of the class ends with 'Page' if
# it has a get function that renders a HTML page.
elif class_return_type == 'html' and (
'get' in clazz.__dict__.keys()):
message = (
'Please ensure that the name of this class '
'ends with \'Page\'')
error_message = (
'%s --> Line %s: %s'
% (file_name, inspect.getsourcelines(clazz)[-1],
message))
self.assertTrue(handler.endswith('Page'),
msg=error_message)

# Check that the name of the class ends with
# 'FileDownloader' if it has a get function
# that prepares downloadable content.
elif class_return_type == 'downloadable' and (
'get' in clazz.__dict__.keys()):
message = (
'Please ensure that the name of this class '
'ends with \'FileDownloader\'')
error_message = (
'%s --> Line %s: %s'
% (file_name, inspect.getsourcelines(clazz)[-1],
message))
self.assertTrue(handler.endswith('FileDownloader'),
msg=error_message)
# URLS = MAPREDUCE_HANDLERS + other handers. MAPREDUCE_HANDLERS
# are tuples. So, below check is to handle them.
if isinstance(url, tuple):
continue
else:
clazz = url.handler
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,
allowed_handler_error_return_type)
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)
6 changes: 3 additions & 3 deletions core/controllers/collection_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def get(self, collection_id):
class EditableCollectionDataHandler(CollectionEditorHandler):
"""A data handler for collections which supports writing."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
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
Expand Down Expand Up @@ -143,7 +143,7 @@ def put(self, collection_id):
class CollectionRightsHandler(CollectionEditorHandler):
"""Handles management of collection editing rights."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_edit_collection
def get(self, collection_id):
Expand Down Expand Up @@ -238,7 +238,7 @@ def put(self, collection_id):
class ExplorationMetadataSearchHandler(base.BaseHandler):
"""Provides data for exploration search."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.open_access
def get(self):
Expand Down
2 changes: 1 addition & 1 deletion core/controllers/concept_card_viewer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
class ConceptCardDataHandler(base.BaseHandler):
"""A card that shows the explanation of a skill's concept."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_view_skill
def get(self, skill_id):
Expand Down
26 changes: 12 additions & 14 deletions core/controllers/editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def _require_valid_version(version_from_payload, exploration_version):
class EditorLogoutHandler(base.BaseHandler):
"""Handles logout from editor page."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.open_access
def get(self):
Expand Down Expand Up @@ -448,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):
Expand All @@ -468,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' % (
utils.to_ascii(exploration.title.replace(' ', '')), version)
filename = 'oppia-%s-v%s%s' % (
utils.to_ascii(exploration.title.replace(' ', '')), version, '.zip')

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))
Expand Down Expand Up @@ -633,7 +631,7 @@ class FetchIssuesHandler(EditorHandler):
exploration. This removes the invalid issues and returns the remaining
unresolved ones.
"""
GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_view_exploration_stats
def get(self, exp_id):
Expand All @@ -655,7 +653,7 @@ def get(self, exp_id):
class FetchPlaythroughHandler(EditorHandler):
"""Handler used for retrieving a playthrough."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_view_exploration_stats
def get(self, unused_exploration_id, playthrough_id):
Expand Down Expand Up @@ -828,7 +826,7 @@ def post(self, exploration_id):
class StateAnswerStatisticsHandler(EditorHandler):
"""Returns basic learner answer statistics for a state."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_view_exploration_stats
def get(self, exploration_id):
Expand All @@ -848,7 +846,7 @@ def get(self, exploration_id):
class TopUnresolvedAnswersHandler(EditorHandler):
"""Returns a list of top N unresolved answers."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_edit_exploration
def get(self, exploration_id):
Expand Down
4 changes: 2 additions & 2 deletions core/controllers/email_dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def get(self):
class EmailDashboardDataHandler(base.BaseHandler):
"""Query data handler."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_manage_email_dashboard
def get(self):
Expand Down Expand Up @@ -128,7 +128,7 @@ def _validate(self, data):
class QueryStatusCheck(base.BaseHandler):
"""Handler for checking status of individual queries."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_manage_email_dashboard
def get(self):
Expand Down
2 changes: 1 addition & 1 deletion core/controllers/feedback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def post(self, exploration_id):
class ThreadListHandlerForTopics(base.BaseHandler):
"""Handles listing of suggestions threads linked to topics."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_edit_topic
def get(self, topic_id):
Expand Down
4 changes: 2 additions & 2 deletions core/controllers/learner_dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class LearnerDashboardIdsHandler(base.BaseHandler):
the activities currently being pursued, and the activities present in
the playlist.
"""
GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_access_learner_dashboard
def get(self):
Expand All @@ -147,7 +147,7 @@ def get(self):
class LearnerDashboardFeedbackThreadHandler(base.BaseHandler):
"""Gets all the messages in a thread."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_access_learner_dashboard
def get(self, thread_id):
Expand Down
2 changes: 1 addition & 1 deletion core/controllers/library.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def get(self):
class LibraryGroupIndexHandler(base.BaseHandler):
"""Provides data for categories such as top rated and recently published."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.open_access
def get(self):
Expand Down
2 changes: 1 addition & 1 deletion core/controllers/question_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def delete(self, question_id, skill_id):
class EditableQuestionDataHandler(base.BaseHandler):
"""A data handler for questions which supports writing."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_view_question_editor
def get(self, question_id):
Expand Down
2 changes: 1 addition & 1 deletion core/controllers/reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ def get(self, exploration_id):
class PretestHandler(base.BaseHandler):
"""Provides subsequent pretest questions after initial batch."""

GET_HANDLER_ERROR_RETURN_TYPE = 'json'
GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON

@acl_decorators.can_play_exploration
def get(self, exploration_id):
Expand Down
Loading