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.

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 part of #6550: Write tests for controllers/editor #6849

Merged
merged 12 commits into from
Jun 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions core/controllers/acl_decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ def test_can_play(self, exploration_id, **kwargs):
exploration_rights = rights_manager.get_exploration_rights(
exploration_id, strict=False)

if exploration_rights is None:
raise self.PageNotFoundException

if rights_manager.check_can_access_activity(
self.user, exploration_rights):
return handler(self, exploration_id, **kwargs)
Expand Down Expand Up @@ -170,6 +173,9 @@ def test_can_play(self, collection_id, **kwargs):
collection_rights = rights_manager.get_collection_rights(
collection_id, strict=False)

if collection_rights is None:
raise self.PageNotFoundException

if rights_manager.check_can_access_activity(
self.user, collection_rights):
return handler(self, collection_id, **kwargs)
Expand Down Expand Up @@ -211,6 +217,10 @@ def test_can_download(self, exploration_id, **kwargs):

exploration_rights = rights_manager.get_exploration_rights(
exploration_id, strict=False)

if exploration_rights is None:
raise self.PageNotFoundException

if rights_manager.check_can_access_activity(
self.user, exploration_rights):
return handler(self, exploration_id, **kwargs)
Expand Down Expand Up @@ -251,6 +261,10 @@ def test_can_view_stats(self, exploration_id, **kwargs):

exploration_rights = rights_manager.get_exploration_rights(
exploration_id, strict=False)

if exploration_rights is None:
raise self.PageNotFoundException

if rights_manager.check_can_access_activity(
self.user, exploration_rights):
return handler(self, exploration_id, **kwargs)
Expand Down
26 changes: 20 additions & 6 deletions core/controllers/acl_decorators_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ def setUp(self):
self.set_admins([self.ADMIN_USERNAME])
self.owner = user_services.UserActionsInfo(self.owner_id)
self.mock_testapp = webtest.TestApp(webapp2.WSGIApplication(
[webapp2.Route('/mock/<collection_id>', self.MockHandler)],
[webapp2.Route(
'/mock_play_collection/<collection_id>', self.MockHandler)],
debug=feconf.DEBUG,
))
self.save_new_valid_exploration(
Expand All @@ -151,33 +152,46 @@ def setUp(self):

def test_guest_can_access_published_collection(self):
with self.swap(self, 'testapp', self.mock_testapp):
response = self.get_json('/mock/%s' % self.published_col_id)
response = self.get_json(
'/mock_play_collection/%s' % self.published_col_id)
self.assertEqual(response['collection_id'], self.published_col_id)

def test_guest_cannot_access_private_collection(self):
with self.swap(self, 'testapp', self.mock_testapp):
self.get_json(
'/mock/%s' % self.private_col_id, expected_status_int=404)
'/mock_play_collection/%s' % self.private_col_id,
expected_status_int=404)

def test_admin_can_access_private_collection(self):
self.login(self.ADMIN_EMAIL)
with self.swap(self, 'testapp', self.mock_testapp):
response = self.get_json('/mock/%s' % self.private_col_id)
response = self.get_json(
'/mock_play_collection/%s' % self.private_col_id)
self.assertEqual(response['collection_id'], self.private_col_id)
self.logout()

def test_owner_can_access_private_collection(self):
self.login(self.OWNER_EMAIL)
with self.swap(self, 'testapp', self.mock_testapp):
response = self.get_json('/mock/%s' % self.private_col_id)
response = self.get_json(
'/mock_play_collection/%s' % self.private_col_id)
self.assertEqual(response['collection_id'], self.private_col_id)
self.logout()

def test_logged_in_user_cannot_access_not_owned_private_collection(self):
self.login(self.user_email)
with self.swap(self, 'testapp', self.mock_testapp):
self.get_json(
'/mock/%s' % self.private_col_id, expected_status_int=404)
'/mock_play_collection/%s' % self.private_col_id,
expected_status_int=404)
self.logout()

def test_cannot_access_collection_with_invalid_collection_id(self):
self.login(self.OWNER_EMAIL)
with self.swap(self, 'testapp', self.mock_testapp):
self.get_json(
'/mock_play_collection/invalid_collection_id',
expected_status_int=404)
self.logout()


Expand Down
65 changes: 20 additions & 45 deletions core/controllers/editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import imghdr
import logging

from constants import constants
from core.controllers import acl_decorators
from core.controllers import base
from core.domain import config_domain
Expand Down Expand Up @@ -84,12 +83,6 @@ class ExplorationPage(EditorHandler):
@acl_decorators.can_play_exploration
def get(self, exploration_id):
"""Handles GET requests."""
if exploration_id in constants.DISABLED_EXPLORATION_IDS:
self.render_template(
'pages/error-pages/disabled-exploration.mainpage.html',
iframe_restriction=None)
return

exploration_rights = rights_manager.get_exploration_rights(
exploration_id)

Expand Down Expand Up @@ -197,11 +190,9 @@ def put(self, exploration_id):
except utils.ValidationError as e:
raise self.InvalidInputException(e)

try:
Copy link
Member

Choose a reason for hiding this comment

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

Why delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_user_exploration_data()'s failure cases are handled by its decorator, i.e can_edit_exploration

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a reference to the ExplorationUserData model there. What if it's just not present for some reason? Should still catch this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But how can I create an exception here? It won't throw an exception if ExplorationUserData model is None -- it will work fine. I think the possible reasons for raising an exception is covered by its decorator.

Copy link
Member

Choose a reason for hiding this comment

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

There's a line in that function that does:

    exp_user_data = user_models.ExplorationUserDataModel.get(
        user_id, exploration_id)

What happens if that model instance doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns None and the code below it works fine even if the model is None

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, thanks for explaining. Yes, what you did sounds fine, then.

exploration_data = exp_services.get_user_exploration_data(
self.user_id, exploration_id)
except:
raise self.PageNotFoundException
exploration_data = exp_services.get_user_exploration_data(
self.user_id, exploration_id)

self.values.update(exploration_data)
self.render_json(self.values)

Expand Down Expand Up @@ -243,7 +234,7 @@ def put(self, exploration_id):
new_member_id = user_services.get_user_id_from_username(
new_member_username)
if new_member_id is None:
raise Exception(
raise self.InvalidInputException(
'Sorry, we could not find the specified user.')

rights_manager.assign_role_for_exploration(
Expand Down Expand Up @@ -399,10 +390,7 @@ class ExplorationFileDownloader(EditorHandler):
@acl_decorators.can_download_exploration
def get(self, exploration_id):
"""Handles GET requests."""
try:
exploration = exp_services.get_exploration_by_id(exploration_id)
except:
raise self.PageNotFoundException
exploration = exp_services.get_exploration_by_id(exploration_id)

version_str = self.request.get('v', default_value=exploration.version)
output_format = self.request.get('output_format', default_value='zip')
Expand Down Expand Up @@ -439,10 +427,10 @@ class StateYamlHandler(EditorHandler):
@acl_decorators.can_play_exploration
def post(self, unused_exploration_id):
"""Handles POST requests."""
try:
state_dict = self.payload.get('state_dict')
width = self.payload.get('width')
except Exception:
state_dict = self.payload.get('state_dict')
width = self.payload.get('width')

if not width or not state_dict:
raise self.PageNotFoundException

self.render_json({
Expand All @@ -460,11 +448,8 @@ class ExplorationSnapshotsHandler(EditorHandler):
def get(self, exploration_id):
"""Handles GET requests."""

try:
snapshots = exp_services.get_exploration_snapshots_metadata(
exploration_id)
except:
raise self.PageNotFoundException
snapshots = exp_services.get_exploration_snapshots_metadata(
exploration_id)

# Patch `snapshots` to use the editor's display name.
snapshots_committer_ids = [
Expand Down Expand Up @@ -518,11 +503,8 @@ class ExplorationStatisticsHandler(EditorHandler):
@acl_decorators.can_view_exploration_stats
def get(self, exploration_id):
"""Handles GET requests."""
try:
current_exploration = exp_services.get_exploration_by_id(
exploration_id)
except:
raise self.PageNotFoundException
current_exploration = exp_services.get_exploration_by_id(
exploration_id)

self.render_json(stats_services.get_exploration_stats(
exploration_id, current_exploration.version).to_frontend_dict())
Expand All @@ -536,11 +518,8 @@ class StateRulesStatsHandler(EditorHandler):
@acl_decorators.can_view_exploration_stats
def get(self, exploration_id, escaped_state_name):
"""Handles GET requests."""
try:
current_exploration = exp_services.get_exploration_by_id(
exploration_id)
except:
raise self.PageNotFoundException
current_exploration = exp_services.get_exploration_by_id(
exploration_id)

state_name = utils.unescape_encoded_uri_component(escaped_state_name)
if state_name not in current_exploration.states:
Expand Down Expand Up @@ -570,7 +549,8 @@ def get(self, exp_id):
exp_issues = stats_services.get_exp_issues(exp_id, exp_version)
if exp_issues is None:
raise self.PageNotFoundException(
'Invalid exploration ID %s' % (exp_id))
'Invalid version %s for exploration ID %s'
% (exp_version, exp_id))
unresolved_issues = []
for issue in exp_issues.unresolved_issues:
if issue.is_valid:
Expand Down Expand Up @@ -762,11 +742,7 @@ class StateAnswerStatisticsHandler(EditorHandler):
@acl_decorators.can_view_exploration_stats
def get(self, exploration_id):
"""Handles GET requests."""
try:
current_exploration = (
exp_services.get_exploration_by_id(exploration_id))
except:
raise self.PageNotFoundException
current_exploration = exp_services.get_exploration_by_id(exploration_id)

top_state_answers = stats_services.get_top_state_answer_stats_multi(
exploration_id, current_exploration.states)
Expand All @@ -788,9 +764,8 @@ class TopUnresolvedAnswersHandler(EditorHandler):
@acl_decorators.can_edit_exploration
def get(self, exploration_id):
"""Handles GET requests for unresolved answers."""
try:
state_name = self.request.get('state_name')
except Exception:
state_name = self.request.get('state_name')
if not state_name:
raise self.PageNotFoundException

unresolved_answers_with_frequency = (
Expand Down
Loading