Reviewer assignment functionality for community contribution #8720
Conversation
DubeySandeep
requested review from
aks681,
ankita240796,
kevinlee12,
nithusha21,
sagangwee,
seanlip and
vojtechjelinek
as code owners
|
Hi @DubeySandeep. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
core/controllers/admin.py
Outdated
| if utils.is_supported_audio_language_code(language_code): | ||
| if user_services.can_review_translation_suggestions( | ||
| new_reviewer_user_id, language_code=language_code): | ||
| raise self.InvalidInputException( | ||
| 'User %s already has rights to review translation in ' | ||
| 'language code %s' % ( | ||
| new_reviewer_username, language_code)) | ||
| else: | ||
| user_services.allow_user_review_translation_in_language( | ||
| new_reviewer_user_id, language_code) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| 'Invalid language_code: %s' % language_code) |
There was a problem hiding this comment.
Consider refactoring this to use guard clauses to reduce nesting, e.g.
| if utils.is_supported_audio_language_code(language_code): | |
| if user_services.can_review_translation_suggestions( | |
| new_reviewer_user_id, language_code=language_code): | |
| raise self.InvalidInputException( | |
| 'User %s already has rights to review translation in ' | |
| 'language code %s' % ( | |
| new_reviewer_username, language_code)) | |
| else: | |
| user_services.allow_user_review_translation_in_language( | |
| new_reviewer_user_id, language_code) | |
| else: | |
| raise self.InvalidInputException( | |
| 'Invalid language_code: %s' % language_code) | |
| if not utils.is_supported_audio_language_code(language_code): | |
| raise self.InvalidInputException( | |
| 'Invalid language_code: %s' % language_code) | |
| if user_services.can_review_translation_suggestions( | |
| new_reviewer_user_id, language_code=language_code): | |
| raise self.InvalidInputException( | |
| 'User %s already has rights to review translation in ' | |
| 'language code %s' % ( | |
| new_reviewer_username, language_code)) | |
| user_services.allow_user_review_translation_in_language( | |
| new_reviewer_user_id, language_code) |
Here and other areas where the nesting is more than 2 clauses deep.
core/controllers/admin.py
Outdated
| if user_id is None: | ||
| raise self.InvalidInputException( | ||
| 'Invalid username: %s' % username) | ||
| if removal_type == 'all': |
There was a problem hiding this comment.
Can these removal_type(s) also be made to be constants?
core/controllers/admin.py
Outdated
| @acl_decorators.can_access_admin_page | ||
| def get(self): | ||
| method = self.request.get('method') | ||
| if method == 'username': |
There was a problem hiding this comment.
Can these method strings be made to be constants?
core/controllers/admin.py
Outdated
| if removal_type == 'all': | ||
| user_services.remove_user_from_community_reviewer(user_id) | ||
| elif removal_type == 'specific': | ||
| review_type = self.payload.get('review_type') | ||
| if review_type == constants.REVIEWABLE_ITEM_TRANSLATION: | ||
| language_code = self.payload.get('language_code') | ||
| if user_services.can_review_translation_suggestions( | ||
| user_id, language_code=language_code): | ||
| user_services.remove_translation_review_rights_in_language( | ||
| user_id, language_code) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| '%s does not have rights to review translation in ' | ||
| 'language %s.' % (username, language_code)) | ||
| elif review_type == constants.REVIEWABLE_ITEM_VOICEOVER: | ||
| language_code = self.payload.get('language_code') | ||
| if user_services.can_review_voiceover_applications( | ||
| user_id, language_code=language_code): | ||
| user_services.remove_voiceover_review_rights_in_language( | ||
| user_id, language_code) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| '%s does not have rights to review voiceover in ' | ||
| 'language %s.' % (username, language_code)) | ||
| elif review_type == constants.REVIEWABLE_ITEM_QUESTION: | ||
| if user_services.can_review_question_suggestions(user_id): | ||
| user_services.remove_user_question_review_rights( | ||
| user_id) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| '%s does not have rights to review question.' % ( | ||
| username)) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| 'Invalid type: %s' % review_type) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| 'Invalid removal_type: %s' % removal_type) |
There was a problem hiding this comment.
Another area where introducing guard clauses to reduce nesting would greatly improve readability.
core/storage/user/gae_models.py
Outdated
|
|
||
| @classmethod | ||
| def get_question_reviewer_user_id(cls): | ||
| """Returns the Id of the users who has rights to review questions. |
core/storage/user/gae_models.py
Outdated
| """Returns the Id of the users who has rights to review questions. | ||
|
|
||
| Returns: | ||
| list(str). A list of user's Ids who has rights to review questions. |
| function(userCommunityRights) { | ||
| userCommunityRights | ||
| .can_review_translation_for_language_codes.forEach( | ||
| function(languageCode) { | ||
| ctrl.userCanReviewTranslationSuggestionsInLanguages | ||
| .push( | ||
| LanguageUtilService.getAudioLanguageDescription( | ||
| languageCode)); |
There was a problem hiding this comment.
The deep nesting is difficult to read. Consider extracting one of the functions.
| ctrl.userCanReviewTranslationSuggestionsInLanguages | ||
| .length > 0 || | ||
| ctrl.userCanReviewVoiceoverSuggestionsInLanguages | ||
| .length > 0 || | ||
| ctrl.userCanReviewQuestions) { |
There was a problem hiding this comment.
This conditional is difficult to read. Consider extracting this to a variable or method with an explicit name.
| <strong>Review rights:</strong> | ||
| <div ng-if="$ctrl.userCanReviewTranslationSuggestionsInLanguages.length > 0"> | ||
| Translation in languages: | ||
| <span ng-repeat="langaugeCode in $ctrl.userCanReviewTranslationSuggestionsInLanguages"> |
There was a problem hiding this comment.
typn in language*. Here and below.
There was a problem hiding this comment.
@sagangwee, I've resolved all your comments. I'll re-assign you as a review after doing a self-review.
core/controllers/admin.py
Outdated
| if utils.is_supported_audio_language_code(language_code): | ||
| if user_services.can_review_translation_suggestions( | ||
| new_reviewer_user_id, language_code=language_code): | ||
| raise self.InvalidInputException( | ||
| 'User %s already has rights to review translation in ' | ||
| 'language code %s' % ( | ||
| new_reviewer_username, language_code)) | ||
| else: | ||
| user_services.allow_user_review_translation_in_language( | ||
| new_reviewer_user_id, language_code) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| 'Invalid language_code: %s' % language_code) |
core/controllers/admin.py
Outdated
| if user_id is None: | ||
| raise self.InvalidInputException( | ||
| 'Invalid username: %s' % username) | ||
| if removal_type == 'all': |
core/controllers/admin.py
Outdated
| if removal_type == 'all': | ||
| user_services.remove_user_from_community_reviewer(user_id) | ||
| elif removal_type == 'specific': | ||
| review_type = self.payload.get('review_type') | ||
| if review_type == constants.REVIEWABLE_ITEM_TRANSLATION: | ||
| language_code = self.payload.get('language_code') | ||
| if user_services.can_review_translation_suggestions( | ||
| user_id, language_code=language_code): | ||
| user_services.remove_translation_review_rights_in_language( | ||
| user_id, language_code) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| '%s does not have rights to review translation in ' | ||
| 'language %s.' % (username, language_code)) | ||
| elif review_type == constants.REVIEWABLE_ITEM_VOICEOVER: | ||
| language_code = self.payload.get('language_code') | ||
| if user_services.can_review_voiceover_applications( | ||
| user_id, language_code=language_code): | ||
| user_services.remove_voiceover_review_rights_in_language( | ||
| user_id, language_code) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| '%s does not have rights to review voiceover in ' | ||
| 'language %s.' % (username, language_code)) | ||
| elif review_type == constants.REVIEWABLE_ITEM_QUESTION: | ||
| if user_services.can_review_question_suggestions(user_id): | ||
| user_services.remove_user_question_review_rights( | ||
| user_id) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| '%s does not have rights to review question.' % ( | ||
| username)) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| 'Invalid type: %s' % review_type) | ||
| else: | ||
| raise self.InvalidInputException( | ||
| 'Invalid removal_type: %s' % removal_type) |
core/controllers/admin.py
Outdated
| @acl_decorators.can_access_admin_page | ||
| def get(self): | ||
| method = self.request.get('method') | ||
| if method == 'username': |
core/storage/user/gae_models.py
Outdated
|
|
||
| @classmethod | ||
| def get_question_reviewer_user_id(cls): | ||
| """Returns the Id of the users who has rights to review questions. |
core/storage/user/gae_models.py
Outdated
| """Returns the Id of the users who has rights to review questions. | ||
|
|
||
| Returns: | ||
| list(str). A list of user's Ids who has rights to review questions. |
| <strong>Review rights:</strong> | ||
| <div ng-if="$ctrl.userCanReviewTranslationSuggestionsInLanguages.length > 0"> | ||
| Translation in languages: | ||
| <span ng-repeat="langaugeCode in $ctrl.userCanReviewTranslationSuggestionsInLanguages"> |
| ctrl.userCanReviewTranslationSuggestionsInLanguages | ||
| .length > 0 || | ||
| ctrl.userCanReviewVoiceoverSuggestionsInLanguages | ||
| .length > 0 || | ||
| ctrl.userCanReviewQuestions) { |
| function(userCommunityRights) { | ||
| userCommunityRights | ||
| .can_review_translation_for_language_codes.forEach( | ||
| function(languageCode) { | ||
| ctrl.userCanReviewTranslationSuggestionsInLanguages | ||
| .push( | ||
| LanguageUtilService.getAudioLanguageDescription( | ||
| languageCode)); |
core/domain/suggestion_services.py
Outdated
| language_codes = ( | ||
| community_rights.can_review_translation_for_language_codes) | ||
| for suggestion in all_suggestions: | ||
| if suggestion.change.language_code in language_codes: |
There was a problem hiding this comment.
I think this can be done through IN query in the model layer. I'll check and update this!
There was a problem hiding this comment.
Marking this as needing resolution.
Also consider a list comprehension as an alternative way to write this: user_reviewable_suggestions = [suggestion for suggestion in all_suggestions if ...]
There was a problem hiding this comment.
Okay, I realized that we cant use IN query here as the lanaguge_codes are inside change (json) property. Done! (list comprehension)
| @@ -37,20 +37,6 @@ export class CommunityDashboardConstants { | |||
| description: 'Translate the text in the lessons to break the ' + | |||
| 'language barrier for non-English speaker.', | |||
| customizationOptions: ['language', 'sort'] | |||
| }, | |||
There was a problem hiding this comment.
Removed this as it's not required as of now!
|
@seanlip & @sagangwee Can you please review this PR? |
There was a problem hiding this comment.
Hi @DubeySandeep -- I've done a thorough pass on the controller and domain layers. Sending this your way so that you can take a look while I review the rest. Thanks!
(Good news is that all the comments are pretty minor. In general the PR looks great!)
assets/constants.ts
Outdated
| "REVIEWABLE_ITEM_VOICEOVER": "voiceover", | ||
| "REVIEWABLE_ITEM_QUESTION": "question", | ||
|
|
||
| "REVIEW_REMOVE_TYPE_ALL": "all", |
There was a problem hiding this comment.
The naming here is confusing, but finding a good name is a bit hard. I thought about it and suggest the following because really what you're describing is an action:
ACTION_REMOVE_ALL_REVIEW_RIGHTS
ACTION_REMOVE_SPECIFIC_REVIEW_RIGHT
What do you think?
assets/constants.ts
Outdated
|
|
||
| "REVIEW_REMOVE_TYPE_ALL": "all", | ||
| "REVIEW_REMOVE_TYPE_SPECIFIC": "specific", | ||
| "VIEW_METHOD_USERNAME": "username", |
There was a problem hiding this comment.
ACTION_VIEW_REVIEWERS_BY_USERNAME perhaps (or similar)? Ditto below.
There was a problem hiding this comment.
@sealip This variable is also being used for viewing the user's rights on the platform (i.e, to check what's the user role.)
ACTION_VIEW_RIGHTS_BY_USERNAME & ACTION_VIEW_USERNAMES_FOR_RIGHTS (this still doesn't look like a set of option.)
There was a problem hiding this comment.
Yeah, it is hard to name this. Something like USER_FILTER_CRITERION_ROLE or USER_FILTER_CRITERION_USERNAME might work.
| @@ -2506,6 +2506,17 @@ def test_can_accept_suggestion( | |||
| self.user_id, suggestion.score_category): | |||
| return handler(self, target_id, suggestion_id, **kwargs) | |||
|
|
|||
| if suggestion.suggestion_type == ( | |||
There was a problem hiding this comment.
How does this interplay with the clause above ("check_user_can_review_in_category")?
Also should it be elif instead of if?
There was a problem hiding this comment.
check_user_can_review_in_category will never be true as we don't store the user's score. I've left it as is so that we can remove the whole existing score structure in one PR!
Also should it be elif instead of if?
I've tried to build a if-else block for the "suggestion_type" check, the above checks will "return" if they pass so this if-else block can be written, independent. I'm fine with making it elif, let me know if that's better?
There was a problem hiding this comment.
OK to separate them out if they are conceptually different, but add a comment to clarify what's going on with check_user_can_review_in_category and a TODO pointing to an issue if you plan to remove it later.
There was a problem hiding this comment.
As of now, the check_user_can_review_in_category is working fine with the score category functionality (though it's not enabled.). Also, as they are conceptually different then I don't think there needs to be a comment to explain as I think it's clear if someone has enough score in a given category then they can review the suggestion (This would be same once we will have the new scoring system.) [Am I missing something?]
There was a problem hiding this comment.
Perhaps. As soon as a reviewer leaves comments on your code, it means that there is at least one reader to whom the distinction is not clear. So I would suggest answering the question within the code itself, for the benefit of other readers of the code in the future. Adopt this as a general practice.
More specifically: you explained in a reply to the original comment that the condition will never be true and that the whole scoring structure will be removed in a future PR. Both of these points individually seem like important information that future readers of the code won't know anything about, and so it should be documented (unless I misinterpreted your comment above).
There was a problem hiding this comment.
the condition will never be true and that the whole scoring structure will be removed in a future PR
Yeah, these conditions are True but I don't think a reader has to know this to understand the new changes OR the new changes are not made considering "these" conditions i.e, it will work without considering the above condition.
core/controllers/acl_decorators.py
Outdated
| """Checks whether the user can view reviewable suggestions. | ||
|
|
||
| Args: | ||
| target_type: str. The targeted entity type of suggestion. |
There was a problem hiding this comment.
The entity type of the target of the suggestion.
core/controllers/acl_decorators.py
Outdated
| @@ -2514,6 +2525,49 @@ def test_can_accept_suggestion( | |||
| return generate_decorator_for_handler | |||
|
|
|||
|
|
|||
| def can_view_reviewable_suggestions(handler): | |||
| """Decorator to check whether user can view reviewable suggestions. | |||
There was a problem hiding this comment.
Is it "the user's reviewable suggestions", or a general list of every reviewable suggestion? If the former, the word "their" should be somewhere in the description, for clarity.
Ditto below for the inner function.
There was a problem hiding this comment.
No it's all the suggestion which user can review in a given category it will not include the user's suggestion as user cannot review their own suggestion (though they can check the status of their own suggestion through a different handler.)
There was a problem hiding this comment.
Ah, I see. Then say: whether the user can view the list of suggestions that they are allowed to review (or similar). At the moment the phrasing is not precise enough.
(It's not clear to me whether this includes every suggestion in the given category that's not by the user, or whether it only includes a subset. It may be worth clarifying this.)
| language_code: str. The code of the language. | ||
| """ | ||
| user_community_rights = get_user_community_rights(user_id) | ||
| user_community_rights.can_review_translation_for_language_codes.remove( |
There was a problem hiding this comment.
Will this error if it's not in the list? If so you need a Raises section in the docstring.
There was a problem hiding this comment.
We have this check in the acl and this method won't get called if the language code isn't in the list.
There was a problem hiding this comment.
See above -- add info to args if you're expecting the caller to make that check. Otherwise, by default, the assumption is that any valid language code is a valid input.
Ditto below.
core/domain/user_services.py
Outdated
| _update_user_community_rights(user_community_rights) | ||
|
|
||
|
|
||
| def allow_user_review_voiceover_in_language(user_id, language_code): |
There was a problem hiding this comment.
allow_user_to_...; ditto below and elsewhere.
core/domain/user_services.py
Outdated
| user_community_rights = get_user_community_rights(user_id) | ||
| language_list = ( | ||
| user_community_rights.can_review_voiceover_for_language_codes) | ||
| language_list.append(language_code) |
There was a problem hiding this comment.
The various comments above re validation apply here and below, too.
There was a problem hiding this comment.
Ditto as above i.e, these checks are in the acl and this won't get called in that case. (and we do have a validation check in the domain object for the same.)
core/domain/user_services.py
Outdated
| Returns: | ||
| list(str.) A list of usernames. | ||
| """ | ||
| reviewers_id = [] |
core/domain/user_services.py
Outdated
| if review_type == constants.REVIEWABLE_ITEM_TRANSLATION: | ||
| reviewers_id = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_translation_reviewer_user_id(language_code)) |
There was a problem hiding this comment.
Method name should say user_ids, right? (plural)
Ditto below.
There was a problem hiding this comment.
Completed the review. As mentioned before, this is a great PR -- thanks @DubeySandeep!
core/storage/user/gae_models.py
Outdated
|
|
||
| @classmethod | ||
| def has_reference_to_user_id(cls, user_id): | ||
| """Check whether UserCommunityRightsModel exists for user. |
core/storage/user/gae_models.py
Outdated
| return cls.get_by_id(user_id) is not None | ||
|
|
||
| @classmethod | ||
| def get_translation_reviewer_user_id(cls, language_code): |
There was a problem hiding this comment.
get_translation_reviewer_user_ids, ditto below.
core/storage/user/gae_models.py
Outdated
|
|
||
| @classmethod | ||
| def get_translation_reviewer_user_id(cls, language_code): | ||
| """Returns the ID of the users who has rights to review translation in |
There was a problem hiding this comment.
ID --> IDs
has --> have
translation --> translations
core/storage/user/gae_models.py
Outdated
| language_code: str. The code of the language. | ||
|
|
||
| Returns: | ||
| list(str). A list of user's IDs who has rights to review translation |
There was a problem hiding this comment.
A list of IDs of users who have rights
translation --> translations
core/storage/user/gae_models.py
Outdated
| reviewer_models = ( | ||
| cls.query( | ||
| cls.can_review_translation_for_language_codes == language_code) | ||
| .fetch()) |
There was a problem hiding this comment.
Can you do a keys_only query if all you need is the IDs? Might save datastore bandwidth.
There was a problem hiding this comment.
I had tried that, but I'm not sure why that didn't work. I'll check and update.
There was a problem hiding this comment.
Ah, I remember! It returned a Keys object not list and it was hard to convert it to usernames. I'll recheck.
There was a problem hiding this comment.
I think you can do that easily enough -- key.id or key.id() or similar, IIRC. Please do check.
Note that this method should return user IDs, not usernames.
| <div ng-if="$ctrl.userIsReviewer"> | ||
| <strong>Review rights:</strong> | ||
| <div ng-if="$ctrl.userCanReviewTranslationSuggestionsInLanguages.length > 0"> | ||
| Translation in languages: |
| </span> | ||
| </div> | ||
| <div ng-if="$ctrl.userCanReviewQuestions"> | ||
| Questions: Allowed |
There was a problem hiding this comment.
I think just "Questions" will do, for consistency.
| ctrl.switchToReviewTab(ctrl.SUGGESTION_TYPE_QUESTION); | ||
| } else if (ctrl.userIsLoggedIn) { | ||
| ctrl.switchToContributionsTab(ctrl.SUGGESTION_TYPE_QUESTION); | ||
| ctrl.userDeatilsLoading = false; |
| } | ||
| if (ctrl.reviewTabs.length > 0) { | ||
| ctrl.canReviewTranslation = true; | ||
| ctrl.reviewTabActive = false; |
There was a problem hiding this comment.
reviewTabIsActive
Also I'm a bit confused. Why is this inactive when reviewTabs.length > 0 and active otherwise? It seems counterintuitive.
There was a problem hiding this comment.
Earlier it uses to represent whether the review tab is active and it was marked false just to mark it true after fetching data from the backend, but not the structure is changed so I've made it simpler.
| }); | ||
| } | ||
| if (ctrl.reviewTabs.length > 0) { | ||
| ctrl.canReviewTranslation = true; |
There was a problem hiding this comment.
(I found this variable wasn't required and it was there of no use, so removed.)
| @@ -2506,6 +2506,17 @@ def test_can_accept_suggestion( | |||
| self.user_id, suggestion.score_category): | |||
| return handler(self, target_id, suggestion_id, **kwargs) | |||
|
|
|||
| if suggestion.suggestion_type == ( | |||
There was a problem hiding this comment.
check_user_can_review_in_category will never be true as we don't store the user's score. I've left it as is so that we can remove the whole existing score structure in one PR!
Also should it be elif instead of if?
I've tried to build a if-else block for the "suggestion_type" check, the above checks will "return" if they pass so this if-else block can be written, independent. I'm fine with making it elif, let me know if that's better?
core/controllers/acl_decorators.py
Outdated
| @@ -2514,6 +2525,49 @@ def test_can_accept_suggestion( | |||
| return generate_decorator_for_handler | |||
|
|
|||
|
|
|||
| def can_view_reviewable_suggestions(handler): | |||
| """Decorator to check whether user can view reviewable suggestions. | |||
There was a problem hiding this comment.
No it's all the suggestion which user can review in a given category it will not include the user's suggestion as user cannot review their own suggestion (though they can check the status of their own suggestion through a different handler.)
core/controllers/acl_decorators.py
Outdated
| """Checks whether the user can view reviewable suggestions. | ||
|
|
||
| Args: | ||
| target_type: str. The targeted entity type of suggestion. |
core/controllers/admin.py
Outdated
| raise self.InvalidInputException( | ||
| 'Invalid username: %s' % new_reviewer_username) | ||
|
|
||
| review_item = self.payload.get('review_item') |
core/controllers/admin.py
Outdated
| 'User %s already has rights to review translation in ' | ||
| 'language code %s' % ( | ||
| new_reviewer_username, language_code)) | ||
| user_services.allow_user_review_translation_in_language( |
core/domain/email_manager_test.py
Outdated
|
|
||
| def test_send_assigned_question_reviewer_email(self): | ||
| expected_email_subject = ( | ||
| 'You have been invited to review Oppia question') |
core/domain/email_manager_test.py
Outdated
| expected_email_html_body = ( | ||
| 'Hi translator,<br><br>' | ||
| 'The Oppia team has removed you from the translation reviewer role ' | ||
| 'in the Hindi-language. You won\'t be able to review translation ' |
core/domain/email_manager_test.py
Outdated
| self.assertEqual( | ||
| sent_email_model.intent, feconf.EMAIL_INTENT_ONBOARD_REVIEWER) | ||
|
|
||
| def test_remove_translation_reviewer_email_when_can_send_emails_is_false( |
core/domain/email_manager_test.py
Outdated
| 'The Oppia team has removed you from the translation reviewer role ' | ||
| 'in the Hindi-language. You won\'t be able to review translation ' | ||
| 'suggestions made by contributors in the Hindi language any more, ' | ||
| 'but you can still contribute translation through the Community ' |
| <div ng-if="$ctrl.userIsReviewer"> | ||
| <strong>Review rights:</strong> | ||
| <div ng-if="$ctrl.userCanReviewTranslationSuggestionsInLanguages.length > 0"> | ||
| Translation in languages: |
| @@ -2506,6 +2506,17 @@ def test_can_accept_suggestion( | |||
| self.user_id, suggestion.score_category): | |||
| return handler(self, target_id, suggestion_id, **kwargs) | |||
|
|
|||
| if suggestion.suggestion_type == ( | |||
There was a problem hiding this comment.
OK to separate them out if they are conceptually different, but add a comment to clarify what's going on with check_user_can_review_in_category and a TODO pointing to an issue if you plan to remove it later.
core/controllers/acl_decorators.py
Outdated
| @@ -2514,6 +2525,49 @@ def test_can_accept_suggestion( | |||
| return generate_decorator_for_handler | |||
|
|
|||
|
|
|||
| def can_view_reviewable_suggestions(handler): | |||
| """Decorator to check whether user can view reviewable suggestions. | |||
There was a problem hiding this comment.
Ah, I see. Then say: whether the user can view the list of suggestions that they are allowed to review (or similar). At the moment the phrasing is not precise enough.
(It's not clear to me whether this includes every suggestion in the given category that's not by the user, or whether it only includes a subset. It may be worth clarifying this.)
| Returns: | ||
| list(UserCommunityRights). A list of UserCommunityRights objects. | ||
| """ | ||
| reviewer_models = user_models.UserCommunityRightsModel.get_all() |
There was a problem hiding this comment.
OK, but please file a TODO/issue and self-assign it, and track it in your action items.
core/domain/user_services.py
Outdated
| user_community_rights = get_user_community_rights(user_id) | ||
| language_list = ( | ||
| user_community_rights.can_review_translation_for_language_codes) | ||
| language_list.append(language_code) |
There was a problem hiding this comment.
The docstring for "args" should say that ("Callers should ensure that the language code is not already in the list.")
| language_code: str. The code of the language. | ||
| """ | ||
| user_community_rights = get_user_community_rights(user_id) | ||
| user_community_rights.can_review_translation_for_language_codes.remove( |
There was a problem hiding this comment.
See above -- add info to args if you're expecting the caller to make that check. Otherwise, by default, the assumption is that any valid language code is a valid input.
Ditto below.
core/storage/user/gae_models.py
Outdated
| reviewer_models = ( | ||
| cls.query( | ||
| cls.can_review_translation_for_language_codes == language_code) | ||
| .fetch()) |
There was a problem hiding this comment.
I think you can do that easily enough -- key.id or key.id() or similar, IIRC. Please do check.
Note that this method should return user IDs, not usernames.
| role: null, | ||
| username: '', | ||
| isValid: function() { | ||
| if (this.method === VIEW_METHOD_ROLE) { |
There was a problem hiding this comment.
Perhaps change the name to "filter criterion" or similar
I tried using the existing terminology. Should I change it?
Hmm. I think yes, in this case, since it should add clarity.
assets/constants.ts
Outdated
|
|
||
| "REVIEW_REMOVE_TYPE_ALL": "all", | ||
| "REVIEW_REMOVE_TYPE_SPECIFIC": "specific", | ||
| "VIEW_METHOD_USERNAME": "username", |
| @@ -2506,6 +2506,17 @@ def test_can_accept_suggestion( | |||
| self.user_id, suggestion.score_category): | |||
| return handler(self, target_id, suggestion_id, **kwargs) | |||
|
|
|||
| if suggestion.suggestion_type == ( | |||
There was a problem hiding this comment.
As of now, the check_user_can_review_in_category is working fine with the score category functionality (though it's not enabled.). Also, as they are conceptually different then I don't think there needs to be a comment to explain as I think it's clear if someone has enough score in a given category then they can review the suggestion (This would be same once we will have the new scoring system.) [Am I missing something?]
core/controllers/acl_decorators.py
Outdated
| @@ -2514,6 +2525,49 @@ def test_can_accept_suggestion( | |||
| return generate_decorator_for_handler | |||
|
|
|||
|
|
|||
| def can_view_reviewable_suggestions(handler): | |||
| """Decorator to check whether user can view reviewable suggestions. | |||
| language_code: str. The code of the language. | ||
| """ | ||
| user_community_rights = get_user_community_rights(user_id) | ||
| user_community_rights.can_review_translation_for_language_codes.remove( |
core/domain/user_services.py
Outdated
| user_community_rights = get_user_community_rights(user_id) | ||
| language_list = ( | ||
| user_community_rights.can_review_translation_for_language_codes) | ||
| language_list.append(language_code) |
core/storage/user/gae_models.py
Outdated
| reviewer_models = ( | ||
| cls.query( | ||
| cls.can_review_translation_for_language_codes == language_code) | ||
| .fetch()) |
| role: null, | ||
| username: '', | ||
| isValid: function() { | ||
| if (this.method === VIEW_METHOD_ROLE) { |
| Returns: | ||
| list(UserCommunityRights). A list of UserCommunityRights objects. | ||
| """ | ||
| reviewer_models = user_models.UserCommunityRightsModel.get_all() |
There was a problem hiding this comment.
@DubeySandeep Thanks. Took another a pass, just a few comments.
core/controllers/suggestion_test.py
Outdated
| self.login(self.REVIEWER_EMAIL) | ||
|
|
||
| csrf_token = self.get_new_csrf_token() | ||
|
|
||
| suggestion_to_accept = self.get_json( | ||
| '%s?author_id=%s' % ( | ||
| feconf.SUGGESTION_LIST_URL_PREFIX, | ||
| self.author_id))['suggestions'][0] | ||
|
|
||
| suggestion = suggestion_services.get_suggestion_by_id( | ||
| suggestion_to_accept['suggestion_id']) | ||
|
|
||
| self.assertEqual( | ||
| suggestion.status, suggestion_models.STATUS_IN_REVIEW) | ||
|
|
||
| csrf_token = self.get_new_csrf_token() |
There was a problem hiding this comment.
The blank lines can be removed here to differentiate the "arrange" block, or add a comment before each test block if you want to keep the blank lines.
There was a problem hiding this comment.
I had copied the test structure from the above block, I fixed those as well.
core/controllers/suggestion_test.py
Outdated
| with self.swap(constants, 'ENABLE_NEW_STRUCTURE_VIEWER_UPDATES', True): | ||
| self.put_json('%s/skill/%s/%s' % ( | ||
| feconf.SUGGESTION_ACTION_URL_PREFIX, | ||
| suggestion_to_accept['target_id'], | ||
| suggestion_to_accept['suggestion_id']), { | ||
| 'action': u'accept', | ||
| 'commit_message': u'commit message', | ||
| 'review_message': u'Accepted!', | ||
| 'skill_id': self.skill_id | ||
| }, csrf_token=csrf_token) | ||
|
|
||
| suggestion = suggestion_services.get_suggestion_by_id( | ||
| suggestion_to_accept['suggestion_id']) |
There was a problem hiding this comment.
This can probably be the test "act" block.
There was a problem hiding this comment.
1032 -- 1044 was act block and last two lines were assert block, Done!
core/controllers/suggestion_test.py
Outdated
| self.assertEqual( | ||
| suggestion.status, suggestion_models.STATUS_ACCEPTED) | ||
|
|
||
| self.logout() |
There was a problem hiding this comment.
This can be the "assert" block.
core/domain/suggestion_services.py
Outdated
| suggestion for suggestion in all_suggestions if ( | ||
| suggestion.change.language_code in language_codes) |
There was a problem hiding this comment.
Are the parentheses necessary if the statement is already inside brackets?
There was a problem hiding this comment.
Parens aren't necessary here -- @DubeySandeep please also deindent this line and the line below by 4 to match the opening bracket indentation. (Thanks @sagangwee for the catch!)
There was a problem hiding this comment.
Fixed! (I was considering if as a seprate block and use parenthesis to break lines.)
| this.category === REVIEW_CATEGORY_TRANSLATION || | ||
| this.category === REVIEW_CATEGORY_VOICEOVER) { |
There was a problem hiding this comment.
Since this check is made thrice, consider extracting to a method.
| voiceover: [], | ||
| question: true | ||
| }; | ||
|
|
There was a problem hiding this comment.
Can remove this blank line to differentiate the setup from the action.
| @@ -2506,6 +2506,17 @@ def test_can_accept_suggestion( | |||
| self.user_id, suggestion.score_category): | |||
| return handler(self, target_id, suggestion_id, **kwargs) | |||
|
|
|||
| if suggestion.suggestion_type == ( | |||
There was a problem hiding this comment.
Perhaps. As soon as a reviewer leaves comments on your code, it means that there is at least one reader to whom the distinction is not clear. So I would suggest answering the question within the code itself, for the benefit of other readers of the code in the future. Adopt this as a general practice.
More specifically: you explained in a reply to the original comment that the condition will never be true and that the whole scoring structure will be removed in a future PR. Both of these points individually seem like important information that future readers of the code won't know anything about, and so it should be documented (unless I misinterpreted your comment above).
| raise self.InvalidInputException( | ||
| 'Invalid username: %s' % new_reviewer_username) | ||
|
|
||
| review_category = self.payload.get('review_category') |
There was a problem hiding this comment.
If no review_category is provided, what happens? Please ensure there is a test for this.
In general, take care to validate handlers very strictly; data coming from outside can be anything.
There was a problem hiding this comment.
If no review_category is provided, what happens?
It will be None by default and will fail at line 689.
Please ensure there is a test for this.
We have a test for invalid review_category in admin_test.py at line 1283. I'll add a test for no "review_category"
|
|
||
| @acl_decorators.can_access_admin_page | ||
| def put(self): | ||
| username = self.payload.get('username') |
There was a problem hiding this comment.
Here and elsewhere, I think you need checks to ensure that the keys exist, or default them to None and treat those cases explicitly by raising a 400.
There was a problem hiding this comment.
For no value the default value is None and I have handled the case for None below
| reviewer_rights_message = ( | ||
| 'review %s %s made by contributors in the %s language' % ( | ||
| review_item, review_item_type, language_description)) | ||
| else: |
There was a problem hiding this comment.
No, I think the check at the top covers it.
core/domain/email_manager_test.py
Outdated
| email_manager.send_email_to_new_community_reviewer( | ||
| self.translation_reviewer_id, 'invalid_category') | ||
|
|
||
| def test_all_category_keys_present_in_new_reviewer_email_data_constant( |
There was a problem hiding this comment.
For both the new constants, you should loop over the keys and validate the schema of the value.
There was a problem hiding this comment.
Done! (I've all the schema test for once constant in one test should I create a separate test for each value in the constant?)
core/domain/user_services.py
Outdated
| Args: | ||
| user_id: str. The unique ID of the user. | ||
| language_code: str. The code of the language. Callers should ensure that | ||
| the user have rights to review translations in the given |
core/domain/user_services.py
Outdated
| language code. | ||
| """ | ||
| user_community_rights = get_user_community_rights(user_id) | ||
| language_set = set( |
There was a problem hiding this comment.
Same thing, throughout this function, and elsewhere: distinguish between languages and language codes.
core/domain/user_services.py
Outdated
| user_community_rights_model.delete() | ||
|
|
||
|
|
||
| def get_community_reviewer_usernames(review_type, language_code=None): |
There was a problem hiding this comment.
If you are using "review category" (based on the naming of the constants) then let's use that throughout instead of review type.
There was a problem hiding this comment.
I had changed the review_type to a category all over the places, it was left here only! Fixed!
core/storage/user/gae_models.py
Outdated
| dict. Dictionary of the data from UserCommunityRightsModel. | ||
| """ | ||
| rights_model = cls.get_by_id(user_id) | ||
| if rights_model is not None: |
There was a problem hiding this comment.
It's better to frame this as:
if rights_model is None:
return {}
return {{big_dict}}
I.e. have the earlier clauses exit early.
|
Please note that backend tests are failing. |
There was a problem hiding this comment.
@seanlip @sagangwee, I've addressed the review comments PTAL!
Also, the backend test was failing earlier because of coverage issue with one line in admin.py which I've added now!
core/controllers/suggestion_test.py
Outdated
| self.login(self.REVIEWER_EMAIL) | ||
|
|
||
| csrf_token = self.get_new_csrf_token() | ||
|
|
||
| suggestion_to_accept = self.get_json( | ||
| '%s?author_id=%s' % ( | ||
| feconf.SUGGESTION_LIST_URL_PREFIX, | ||
| self.author_id))['suggestions'][0] | ||
|
|
||
| suggestion = suggestion_services.get_suggestion_by_id( | ||
| suggestion_to_accept['suggestion_id']) | ||
|
|
||
| self.assertEqual( | ||
| suggestion.status, suggestion_models.STATUS_IN_REVIEW) | ||
|
|
||
| csrf_token = self.get_new_csrf_token() |
There was a problem hiding this comment.
I had copied the test structure from the above block, I fixed those as well.
core/controllers/suggestion_test.py
Outdated
| with self.swap(constants, 'ENABLE_NEW_STRUCTURE_VIEWER_UPDATES', True): | ||
| self.put_json('%s/skill/%s/%s' % ( | ||
| feconf.SUGGESTION_ACTION_URL_PREFIX, | ||
| suggestion_to_accept['target_id'], | ||
| suggestion_to_accept['suggestion_id']), { | ||
| 'action': u'accept', | ||
| 'commit_message': u'commit message', | ||
| 'review_message': u'Accepted!', | ||
| 'skill_id': self.skill_id | ||
| }, csrf_token=csrf_token) | ||
|
|
||
| suggestion = suggestion_services.get_suggestion_by_id( | ||
| suggestion_to_accept['suggestion_id']) |
There was a problem hiding this comment.
1032 -- 1044 was act block and last two lines were assert block, Done!
core/controllers/suggestion_test.py
Outdated
| self.assertEqual( | ||
| suggestion.status, suggestion_models.STATUS_ACCEPTED) | ||
|
|
||
| self.logout() |
| Returns: | ||
| list(UserCommunityRights). A list of UserCommunityRights objects. | ||
| """ | ||
| reviewer_models = user_models.UserCommunityRightsModel.get_all() |
core/domain/user_services.py
Outdated
| user_community_rights: UserCommunityRights. The updated | ||
| UserCommunityRights object of the user. | ||
| """ | ||
| if user_community_rights.can_review_atleast_one_item(): |
There was a problem hiding this comment.
Done!
I thought I commented on this before?
That was somewhere in the docstring!
core/domain/email_manager_test.py
Outdated
| email_manager.send_email_to_new_community_reviewer( | ||
| self.translation_reviewer_id, 'invalid_category') | ||
|
|
||
| def test_all_category_keys_present_in_new_reviewer_email_data_constant( |
There was a problem hiding this comment.
Done! (I've all the schema test for once constant in one test should I create a separate test for each value in the constant?)
core/domain/email_manager_test.py
Outdated
| email_manager.send_email_to_removed_community_reviewer( | ||
| self.translation_reviewer_id, 'invalid_category') | ||
|
|
||
| def test_all_category_keys_present_in_removed_reviewer_email_data_constant( |
core/domain/email_manager_test.py
Outdated
|
|
||
| def test_all_category_keys_present_in_new_reviewer_email_data_constant( | ||
| self): | ||
| self.assertEqual(len(email_manager.NEW_REVIEWER_EMAIL_DATA), 3) |
core/domain/suggestion_services.py
Outdated
| suggestion for suggestion in all_suggestions if ( | ||
| suggestion.change.language_code in language_codes) |
There was a problem hiding this comment.
Fixed! (I was considering if as a seprate block and use parenthesis to break lines.)
core/domain/user_services.py
Outdated
| Args: | ||
| user_id: str. The unique ID of the user. | ||
| language_code: str. The code of the language. Callers should ensure that | ||
| the user have rights to review translations in the given |
There was a problem hiding this comment.
Hi @DubeySandeep, only trivial comments left. No idea if you'll be able to see this since the main page of this PR is not loading properly, but I left 8 commits (one in email_manager_test.py, one optional one in suggestion_services.py, and six in user_services.py. Thanks!
| constants.REVIEW_CATEGORY_TRANSLATION, | ||
| constants.REVIEW_CATEGORY_VOICEOVER]) | ||
|
|
||
| self.assertEqual(sorted(email_manager.NEW_REVIEWER_EMAIL_DATA[ |
There was a problem hiding this comment.
Don't do three separate checks. Do a loop through all keys to check the three usual things and to check that exactly one of rights_message and rights_message_template is contained.
That way, this validation will scale if more review categories are added, without you needing to modify the tests in the future.
Ditto below.
| suggestion.change.language_code in language_codes) | ||
| ] | ||
| suggestion for suggestion in all_suggestions if | ||
| suggestion.change.language_code in language_codes] |
There was a problem hiding this comment.
Optional nit: slightly nicer to move the "if" to the front of this line, since it starts a new clause.
| @@ -1870,16 +1870,15 @@ def allow_user_to_review_translation_in_language(user_id, language_code): | |||
| Args: | |||
| user_id: str. The unique ID of the user. | |||
| language_code: str. The code of the language. Callers should ensure that | |||
| the user does not have rights to review translations in the given | |||
| the user does not has rights to review translations in the given | |||
There was a problem hiding this comment.
has --> have. The previous version was correct. The "singular form" is already there in "does not".
There was a problem hiding this comment.
Done! (Thanks for the explanation. :) )
| @@ -1906,16 +1905,15 @@ def allow_user_to_review_voiceover_in_language(user_id, language_code): | |||
| Args: | |||
| user_id: str. The unique ID of the user. | |||
| language_code: str. The code of the language. Callers should ensure that | |||
| the user does not have rights to review voiceovers in the given | |||
| the user does not has rights to review voiceovers in the given | |||
There was a problem hiding this comment.
Ditto.
(Note that "already has" above is correct. But "has" is wrong here.)
| @@ -1940,7 +1938,7 @@ def allow_user_to_review_question(user_id): | |||
|
|
|||
| Args: | |||
| user_id: str. The unique ID of the user. Callers should ensure that | |||
| the given user does not have rights to review questions. | |||
| the given user does not has rights to review questions. | |||
|
|
||
| Args: | ||
| review_type: str. The type of of review item. One of translation, | ||
| voiceover or question. | ||
| review_category: str. str. The category which for in which user can |
There was a problem hiding this comment.
The review category to find the list of reviewers for.
| reviewer_ids = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_voiceover_reviewer_user_ids(language_code)) | ||
| elif review_type == constants.REVIEW_CATEGORY_QUESTION: | ||
| elif review_category == constants.REVIEW_CATEGORY_QUESTION: |
There was a problem hiding this comment.
Error here if language_code is not None.
| reviewer_ids = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_voiceover_reviewer_user_ids(language_code)) | ||
| elif review_type == constants.REVIEW_CATEGORY_QUESTION: | ||
| elif review_category == constants.REVIEW_CATEGORY_QUESTION: | ||
| reviewer_ids = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_question_reviewer_user_ids()) |
There was a problem hiding this comment.
Missing "else raise Exception" clause at end.
| constants.REVIEW_CATEGORY_TRANSLATION, | ||
| constants.REVIEW_CATEGORY_VOICEOVER]) | ||
|
|
||
| self.assertEqual(sorted(email_manager.NEW_REVIEWER_EMAIL_DATA[ |
| suggestion.change.language_code in language_codes) | ||
| ] | ||
| suggestion for suggestion in all_suggestions if | ||
| suggestion.change.language_code in language_codes] |
| @@ -1870,16 +1870,15 @@ def allow_user_to_review_translation_in_language(user_id, language_code): | |||
| Args: | |||
| user_id: str. The unique ID of the user. | |||
| language_code: str. The code of the language. Callers should ensure that | |||
| the user does not have rights to review translations in the given | |||
| the user does not has rights to review translations in the given | |||
There was a problem hiding this comment.
Done! (Thanks for the explanation. :) )
| @@ -1906,16 +1905,15 @@ def allow_user_to_review_voiceover_in_language(user_id, language_code): | |||
| Args: | |||
| user_id: str. The unique ID of the user. | |||
| language_code: str. The code of the language. Callers should ensure that | |||
| the user does not have rights to review voiceovers in the given | |||
| the user does not has rights to review voiceovers in the given | |||
| @@ -1940,7 +1938,7 @@ def allow_user_to_review_question(user_id): | |||
|
|
|||
| Args: | |||
| user_id: str. The unique ID of the user. Callers should ensure that | |||
| the given user does not have rights to review questions. | |||
| the given user does not has rights to review questions. | |||
|
|
||
| Args: | ||
| review_type: str. The type of of review item. One of translation, | ||
| voiceover or question. | ||
| review_category: str. str. The category which for in which user can |
| reviewer_ids = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_voiceover_reviewer_user_ids(language_code)) | ||
| elif review_type == constants.REVIEW_CATEGORY_QUESTION: | ||
| elif review_category == constants.REVIEW_CATEGORY_QUESTION: |
| reviewer_ids = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_voiceover_reviewer_user_ids(language_code)) | ||
| elif review_type == constants.REVIEW_CATEGORY_QUESTION: | ||
| elif review_category == constants.REVIEW_CATEGORY_QUESTION: | ||
| reviewer_ids = ( | ||
| user_models.UserCommunityRightsModel | ||
| .get_question_reviewer_user_ids()) |
Explanation
Reviewer assignment functionality for community contribution.
Checklist
python -m scripts.pre_commit_linterandpython -m scripts.run_frontend_tests.