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

Merged
merged 12 commits into from
Jun 13, 2019
Merged

Fix part of #6550: Write tests for controllers/editor #6849

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
ab8d5a2
Merge stash
Jun 2, 2019
b317870
Write tests for controllers/editor
Jun 5, 2019
8c1b30b
Merge branch 'develop' into new_editor_test
Jun 5, 2019
73a5688
package-lock
Jun 5, 2019
d417645
Fix lint
Jun 5, 2019
8df5a37
Fix tests
Jun 5, 2019
e406e09
Merge develop
Jun 10, 2019
e645367
Addressed comments
Jun 10, 2019
b74388c
Addressed comments
Jun 10, 2019
c5b6344
Merge branch 'develop' into new_editor_test
Jun 11, 2019
8c31ca0
Added comment
Jun 11, 2019
3fb9f3c
Merge develop
Jun 12, 2019
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
Addressed comments
  • Loading branch information
Rishav Chakraborty committed Jun 10, 2019
commit e645367aa187e6aca8eae6d7e60d30979b957ead
21 changes: 14 additions & 7 deletions core/controllers/acl_decorators_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,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 @@ -139,40 +140,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/invalid_collection_id', expected_status_int=404)
'/mock_play_collection/invalid_collection_id',
expected_status_int=404)
self.logout()


Expand Down
5 changes: 3 additions & 2 deletions core/controllers/editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,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 @@ -562,7 +562,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 version %s' % (exp_version))
'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
110 changes: 53 additions & 57 deletions core/controllers/editor_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,23 @@ def test_exploration_download_handler_for_default_exploration(self):
# Check downloaded dict.
self.assertEqual(self.SAMPLE_JSON_CONTENT, response)

# Check downloading a specific version.
download_url = (
'/createhandler/download/%s?output_format=%s&v=1' %
(exp_id, feconf.OUTPUT_FORMAT_JSON))
response = self.get_json(download_url)
self.assertEqual(['Introduction'], response.keys())

# Check downloading an invalid version results in downloading the
# latest version.
download_url = (
'/createhandler/download/%s?output_format=%s&v=xxx' %
(exp_id, feconf.OUTPUT_FORMAT_JSON))
response = self.get_json(download_url)
self.assertEqual(self.SAMPLE_JSON_CONTENT, response)
self.assertEqual(
['Introduction', 'State A', 'State B'], response.keys())

self.logout()

def test_state_yaml_handler(self):
Expand Down Expand Up @@ -656,7 +673,7 @@ def test_get_exploration_snapshot_history(self):
for snapshot in snapshots:
snapshot.update({
'committer_id': 'owner'
})
})

response = self.get_json('/createhandler/snapshots/%s' % (exp_id))

Expand All @@ -675,7 +692,7 @@ def test_get_exploration_snapshot_history(self):
for snapshot in snapshots:
snapshot.update({
'committer_id': 'owner'
})
})

response = self.get_json('/createhandler/snapshots/%s' % (exp_id))

Expand Down Expand Up @@ -762,39 +779,6 @@ def test_record_user_saw_tutorial(self):
self.logout()


class StateAnswerStatisticsHandlerTests(test_utils.GenericTestBase):
Copy link
Member

Choose a reason for hiding this comment

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

Why is this deleted?

Consider preemptively justifying any deletions in a comment to the review thread, otherwise reviewers will just ask that question during reviews and that will slow things down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StateAnswerStatisticsHandlerTests was added in PR #6679. That's why I deleted this duplicate test class


def setUp(self):
super(StateAnswerStatisticsHandlerTests, self).setUp()
self.signup(self.OWNER_EMAIL, self.OWNER_USERNAME)

def test_get_with_invalid_exploration_id_raises_error(self):
self.login(self.OWNER_EMAIL)

self.get_json(
'/createhandler/state_answer_stats/invalid_exp_id',
expected_status_int=404)

self.logout()

def test_get_basic_learner_answer_statistics_for_a_state(self):
self.login(self.OWNER_EMAIL)
exp_id = 'eid'
owner_id = self.get_user_id_from_email(self.OWNER_EMAIL)

exploration = self.save_new_valid_exploration(exp_id, owner_id)

answers = stats_services.get_top_state_answer_stats_multi(
exp_id, exploration.states)

response = self.get_json(
'/createhandler/state_answer_stats/%s' % exp_id)

self.assertEqual(response['answers'], answers)

self.logout()


class TopUnresolvedAnswersHandlerTests(test_utils.GenericTestBase):

def setUp(self):
Expand All @@ -809,7 +793,6 @@ def setUp(self):
def test_cannot_get_unresolved_answers_with_no_state_name(self):
self.login(self.OWNER_EMAIL)


self.get_json(
'/createhandler/get_top_unresolved_answers/%s' % self.exp_id,
expected_status_int=404)
Expand Down Expand Up @@ -872,6 +855,10 @@ def _mock_logging_function(msg, *args):
observed_log_messages[0],
'Could not find state: invalid_state_name')

self.assertEqual(
Copy link
Member

Choose a reason for hiding this comment

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

You might as well replace these three assertions with a single self.assertEqual(observed_log_messages, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

observed_log_messages[1],
'Available states: [u\'Introduction\']')

self.logout()

def test_get_learner_answer_statistics_for_state(self):
Expand Down Expand Up @@ -1496,14 +1483,17 @@ def test_put_with_invalid_new_member_raises_error(self):
response = self.get_html_response('/create/%s' % exp_id)
csrf_token = self.get_csrf_token_from_response(response)
exploration = exp_services.get_exploration_by_id(exp_id)
with self.assertRaisesRegexp(
Exception, 'Sorry, we could not find the specified user.'):
self.put_json(
'%s/%s' % (feconf.EXPLORATION_RIGHTS_PREFIX, exp_id),
payload={
'version': exploration.version,
'new_member_username': 'invalid_new_member_username'},
csrf_token=csrf_token)

response = self.put_json(
'%s/%s' % (feconf.EXPLORATION_RIGHTS_PREFIX, exp_id),
payload={
'version': exploration.version,
'new_member_username': 'invalid_new_member_username'},
csrf_token=csrf_token, expected_status_int=400)

self.assertEqual(
response['error'],
'Sorry, we could not find the specified user.')

def test_make_private_exploration_viewable(self):
self.login(self.OWNER_EMAIL)
Expand All @@ -1530,11 +1520,14 @@ def test_put_with_no_specified_changes_raise_error(self):
response = self.get_html_response('/create/%s' % exp_id)
csrf_token = self.get_csrf_token_from_response(response)
exploration = exp_services.get_exploration_by_id(exp_id)
with self.assertRaisesRegexp(
Exception, 'No change was made to this exploration.'):
self.put_json(
'%s/%s' % (feconf.EXPLORATION_RIGHTS_PREFIX, exp_id),
payload={'version': exploration.version}, csrf_token=csrf_token)

response = self.put_json(
'%s/%s' % (feconf.EXPLORATION_RIGHTS_PREFIX, exp_id),
payload={'version': exploration.version}, csrf_token=csrf_token,
expected_status_int=400)

self.assertEqual(
response['error'], 'No change was made to this exploration.')


class UserExplorationEmailsIntegrationTest(BaseEditorControllerTests):
Expand Down Expand Up @@ -1605,11 +1598,14 @@ def test_put_with_invalid_message_type_raises_error(self):
response = self.get_html_response(
'%s/%s' % (feconf.EDITOR_URL_PREFIX, exp_id))
csrf_token = self.get_csrf_token_from_response(response)
with self.assertRaisesRegexp(Exception, 'Invalid message type.'):
self.put_json(
'%s/%s' % (feconf.USER_EXPLORATION_EMAILS_PREFIX, exp_id),
payload={'message_type': 'invalid_message_type'},
csrf_token=csrf_token)

response = self.put_json(
'%s/%s' % (feconf.USER_EXPLORATION_EMAILS_PREFIX, exp_id),
payload={'message_type': 'invalid_message_type'},
csrf_token=csrf_token, expected_status_int=400)

self.assertEqual(response['error'], 'Invalid message type.')

self.logout()


Expand Down Expand Up @@ -1874,7 +1870,7 @@ def setUp(self):

def test_cannot_fetch_issues_with_invalid_version(self):
self.get_json(
'/issuesdatahandler/%s' % (self.EXP_ID),
'/issuesdatahandler/%s' % self.EXP_ID,
params={'exp_version': 2}, expected_status_int=404)

def test_cannot_fetch_playthrough_with_invalid_playthrough_id(self):
Expand All @@ -1885,7 +1881,7 @@ def test_cannot_fetch_playthrough_with_invalid_playthrough_id(self):
def test_fetch_issues_handler(self):
"""Test that all issues get fetched correctly."""
response = self.get_json(
'/issuesdatahandler/%s' % (self.EXP_ID),
'/issuesdatahandler/%s' % self.EXP_ID,
params={'exp_version': 1})
self.assertEqual(len(response), 2)
self.assertEqual(response[0]['issue_type'], 'EarlyQuit')
Expand All @@ -1900,7 +1896,7 @@ def test_invalid_issues_are_not_retrieved(self):
exp_issues_model.put()

response = self.get_json(
'/issuesdatahandler/%s' % (self.EXP_ID),
'/issuesdatahandler/%s' % self.EXP_ID,
params={'exp_version': 1})
self.assertEqual(len(response), 1)
self.assertEqual(response[0]['issue_type'], 'EarlyQuit')
Expand Down