Add translator role for the exploration (M1.1 Lesson translation dashboard) #4959

DubeySandeep · 2018-05-20T09:53:39Z

Explanation

Milestone 1.1:
- Adding “translator” role for the exploration.
- Adding new entity translator_ids in ExplorationSummary and ActivityRights object.
- Adding translator_ids in ExplorationSummaryModel and ExplorationRightsModel.
- Writing @acl_decorators for can_translate_exploration.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes.
The PR explanation includes the words "Fixes #bugnum: ...".
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

codecov-io · 2018-05-20T11:07:21Z

Codecov Report

Merging #4959 into develop will increase coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4959      +/-   ##
===========================================
+ Coverage    44.48%   44.56%   +0.07%     
===========================================
  Files          386      387       +1     
  Lines        23306    23345      +39     
  Branches      3790     3800      +10     
===========================================
+ Hits         10367    10403      +36     
- Misses       12939    12942       +3

Impacted Files	Coverage Δ
...v/head/pages/learner_dashboard/LearnerDashboard.js	`2.27% <0%> (-0.02%)`	⬇️
...ev/head/domain/utilities/ImageFileObjectFactory.js	`100% <0%> (ø)`
...lates/dev/head/services/AssetsBackendApiService.js	`69.56% <0%> (+9.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a148f4...5562d02. Read the comment docs.

tjiang11

Thanks @DubeySandeep , sorry for the delay. I think this looks great, though it'd be best if @seanlip also takes a look since this PR's predecessor was originally rejected, and I'm not all that familiar with the ACL infrastructure.

Also, I'm not sure if this is complete since the ACL decorator isn't applied? Or is that to be done later?

tjiang11 · 2018-05-23T00:48:31Z

core/domain/acl_decorators.py

@@ -438,6 +438,34 @@ def test_can_edit(self, exploration_id, **kwargs):
    return test_can_edit


+def can_translate_exploration(handler):


Just checking, where is this decorator going to be eventually placed (since it doesn't seem to be used in this PR other than in tests)?

This will be used for translator's controllers (for uploading audio, making audio related changes in exploration). This is similar to can_edit_exploration which is used in ExplorationHandler of editor.py

seanlip

This looks good in general. I spotted some small things but nothing serious.

I assume the actions assigned to the role will go in a separate PR, with the migration of the exploration to split out the translatable components into a separate substructure?

seanlip · 2018-05-23T07:28:30Z

core/domain/acl_decorators_test.py

+        self.assertEqual(response.status_int, 401)
+        self.logout()
+
+    def test_admin_can_edit_private_exploration(self):


you mean translate?

seanlip · 2018-05-23T07:28:48Z

core/domain/acl_decorators_test.py

+        self.assertEqual(response.status_int, 200)
+        self.logout()
+
+    def test_translator_can_translate_assigned_exploration(self):


maybe add another test to check they can't translate an exp that they aren't assigned to

You mean to add "test_user_without_translator_role_of_exploration_cannot_translate_exploration"?

seanlip · 2018-05-23T07:30:36Z

core/domain/rights_manager.py

        if owner_viewer:
            raise utils.ValidationError(
                'A user cannot be both an owner and a viewer: %s' %
                owner_viewer)
+        if editor_translator:


You forgot viewer_translator; there should be 4-choose-2 checks.

seanlip · 2018-05-23T07:30:59Z

core/domain/rights_manager.py

+            user_id: str or None. Id of the user.
+
+        Returns:
+            bool. Whether user is in activity translator.


Done! (similar changes in above docstrings)

seanlip · 2018-05-23T07:35:45Z

core/domain/rights_manager.py

+    if activity_rights is None:
+        return False
+
+    if role_services.ACTION_EDIT_OWNED_ACTIVITY not in user.actions:


The flow here seems a bit off to me. It assumes that it's impossible for a user to have ACTION_EDIT_ANY_ACTIVITY but not ACTION_EDIT_OWNED_ACTIVITY. While currently true, this is an implicit assumption.

So perhaps you should check the last couple of things (any activity, any public activity) first, so that you don't erroneously exit early.

I found a similar flow for can_edit_activity in the above definition, should I change that function too?

Yes, I think so.

DubeySandeep · 2018-05-23T15:19:27Z

Adding @nithusha21 for reviewing acl_decorators_test.py files (for get_json usecase).

seanlip

All looks good except the missing test I mentioned. I defer to @nithusha21 on the get_json part of the review.

Thanks!

seanlip · 2018-05-23T18:31:52Z

core/domain/rights_manager.py

@@ -169,7 +175,7 @@ def is_owner(self, user_id):
            user_id: str or None. Id of the user.

        Returns:
-            bool. Whether user is in activity owners.
+            bool. Whether user is an activity owners.


owners --> owner, ditto below.

nithusha21

The acl_decorator tests in general LGTM. Just one comment I had.

nithusha21 · 2018-05-24T07:31:56Z

core/domain/acl_decorators_test.py

@@ -206,43 +216,46 @@ def setUp(self):
        rights_manager.publish_collection(self.owner, self.published_col_id)

    def test_guest_is_redirected_to_login_page(self):
-        response = self.testapp.get(
-            '/mock/%s' % self.published_col_id, expect_errors=True)
+        with self.swap(self, 'testapp', self.mock_testapp):


You only need to swap when calling get_json. Here there is no need to swap. (ditto at other 302 responses)

Also just to note, @seanlip only 302 responses cannot be tested using get_json because it's a redirect and returns a html. So for now I think let's test 302 by directly calling self.testapp.get (or mock_testapp).

I don't think I understand (conceptually, at least). If we make a GET JSON request, and it redirects, it seems to me that it should redirect to a JSON handler. Unless it redirects because the user doesn't have permission or something (maybe they aren't logged in)? Then in such cases I guess we should 404...

If this is the case, can we update the "check logged in" decorator to do different things based on whether the handler has json return type or not?

@nithusha21& @seanlip, I found this:
here it says it will raise 401

oppia/core/controllers/base.py

Lines 102 to 103 in f11089b

class NotLoggedInException(Exception):

"""Error class for users that are not logged in (error code 401)."""

but here it says it will send a redirect uri

oppia/core/controllers/base.py

Lines 447 to 450 in f11089b

if isinstance(exception, self.NotLoggedInException):

self.redirect(

current_user_services.create_login_url(self.request.uri))

return

seanlip · 2018-05-24T19:38:00Z

The added test looks good to me, thanks @DubeySandeep. LGTM from my perspective. I think you just need to get confirmation from @nithusha21 on the get_json stuff.

@nithusha21 can we file an issue to ensure 302 behaves correctly for json responses (see my note above)?

nithusha21 · 2018-05-25T04:43:59Z

The get_json parts LGTM. I'll file an issue regarding 302 responses. I think your point is valid about sending some other response codes for not logged in situations.

DubeySandeep requested review from anmolshkl and tjiang11 May 20, 2018 09:53

DubeySandeep force-pushed the Milestone1.1 branch from a5ee46c to 486c962 Compare May 20, 2018 09:58

DubeySandeep assigned anmolshkl and tjiang11 May 20, 2018

Add translator role for the exploration

2346d04

DubeySandeep force-pushed the Milestone1.1 branch from 486c962 to 2346d04 Compare May 20, 2018 11:07

tjiang11 approved these changes May 23, 2018

View reviewed changes

tjiang11 requested a review from seanlip May 23, 2018 05:54

seanlip reviewed May 23, 2018

View reviewed changes

Made review changes

d09247e

DubeySandeep requested a review from nithusha21 May 23, 2018 15:19

seanlip reviewed May 23, 2018

View reviewed changes

Added more acl tests

53c3cd8

nithusha21 reviewed May 24, 2018

View reviewed changes

Added review changes in test

5562d02

nithusha21 mentioned this pull request May 25, 2018

For situations where user is not logged in, a 302 (redirect) response is obtained #4978

Closed

tjiang11 merged commit 9cb6e30 into oppia:develop May 27, 2018

nalinbhardwaj mentioned this pull request Aug 25, 2018

Fix #4978 : change JSON response on user not logged in #5589

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add translator role for the exploration (M1.1 Lesson translation dashboard) #4959

Add translator role for the exploration (M1.1 Lesson translation dashboard) #4959

DubeySandeep commented May 20, 2018

codecov-io commented May 20, 2018 •

edited

tjiang11 left a comment

tjiang11 May 23, 2018

DubeySandeep May 23, 2018

seanlip left a comment

seanlip May 23, 2018

DubeySandeep May 23, 2018

seanlip May 23, 2018

DubeySandeep May 23, 2018

seanlip May 23, 2018

DubeySandeep May 23, 2018

seanlip May 23, 2018

seanlip May 23, 2018

DubeySandeep May 23, 2018

seanlip May 23, 2018

DubeySandeep May 23, 2018

seanlip May 23, 2018

DubeySandeep commented May 23, 2018

seanlip left a comment

seanlip May 23, 2018

DubeySandeep May 23, 2018

nithusha21 left a comment

nithusha21 May 24, 2018

seanlip May 24, 2018

DubeySandeep May 24, 2018 •

edited

seanlip commented May 24, 2018

nithusha21 commented May 25, 2018

		@@ -438,6 +438,34 @@ def test_can_edit(self, exploration_id, **kwargs):
		return test_can_edit


		def can_translate_exploration(handler):

	class NotLoggedInException(Exception):
	"""Error class for users that are not logged in (error code 401)."""

	if isinstance(exception, self.NotLoggedInException):
	self.redirect(
	current_user_services.create_login_url(self.request.uri))
	return

Add translator role for the exploration (M1.1 Lesson translation dashboard) #4959

Add translator role for the exploration (M1.1 Lesson translation dashboard) #4959

Conversation

DubeySandeep commented May 20, 2018

Explanation

Checklist

codecov-io commented May 20, 2018 • edited

Codecov Report

tjiang11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep commented May 23, 2018

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithusha21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep May 24, 2018 • edited

Choose a reason for hiding this comment

seanlip commented May 24, 2018

nithusha21 commented May 25, 2018

codecov-io commented May 20, 2018 •

edited

DubeySandeep May 24, 2018 •

edited