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

Update link to ratings in reviewer tools for disabled addons. #15751

Merged
merged 7 commits into from Nov 3, 2020

Conversation

vishalol
Copy link
Contributor

Fixes mozilla/addons#6223

If the addon is public, X reviews points to the public ratings page
If the addon is not public and user has the Ratings:Moderate permission, pointing to /admin/models/ratings/rating/?addon=. For users that don't have that permission (ie regular reviewers, they would still see that text, but without a link.

  • This PR relates to an existing open issue and there are no existing
    PRs open for the same issue.
  • Add Fixes #ISSUENUM at the top of your PR.
  • Add a description of the changes introduced in this PR.
  • The change has been successfully run locally.

@vishalol
Copy link
Contributor Author

@eviljeff Please review

@eviljeff eviljeff requested review from a team and bobsilverberg and removed request for a team October 16, 2020 13:37
{# Using num=count so we don't change an L10n string. #}
{% if not addon.is_disabled or action_allowed(amo.permissions.RATINGS_MODERATE) %}
{% set review_url = url('addons.ratings.list', addon.slug) %}
{% if addon.is_disabled %}
Copy link
Member

Choose a reason for hiding this comment

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

we want this always when the user has the permission

Copy link
Member

Choose a reason for hiding this comment

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

see the original comment in the issue - the two alternatives are having the permission and not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to only linkify if the user has permission Ratings:Moderate, otherwise leave it unlinked. Is it right ?

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

we also need some tests that prove your changes work.

TestReview.test_user_ratings can be added to, perhaps, to check the link is there. And there should be a new test to check the link isn't there when the user doens't have the correct permission.

Comment on lines 97 to 107
{# Using num=count so we don't change an L10n string. #}
{% if acl_is_review_moderator %}
{% set review_url = url('admin:ratings_rating_changelist')|urlparams(addon=addon.pk) %}
<a href="{{ review_url|absolutify }}"><strong>{{
ngettext('{num} review', '{num} reviews', num)|format_html(num=count)
}}</strong></a>
{% else %}
<strong>{{
ngettext('{num} review', '{num} reviews', num)|format_html(num=count)
}}</strong>
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to have to define the same string twice, so we should also define it separately, like:

Suggested change
{# Using num=count so we don't change an L10n string. #}
{% if acl_is_review_moderator %}
{% set review_url = url('admin:ratings_rating_changelist')|urlparams(addon=addon.pk) %}
<a href="{{ review_url|absolutify }}"><strong>{{
ngettext('{num} review', '{num} reviews', num)|format_html(num=count)
}}</strong></a>
{% else %}
<strong>{{
ngettext('{num} review', '{num} reviews', num)|format_html(num=count)
}}</strong>
{% endif %}
{# Using num=count so we don't change an L10n string. #}
{% set review_url_text = ngettext('{num} review', '{num} reviews', num)|format_html(num=count) %}
{% if acl_is_review_moderator %}
{% set review_url = url('admin:ratings_rating_changelist')|urlparams(addon=addon.pk) %}
<a href="{{ review_url|absolutify }}"><strong>{{ review_url_text }}</strong></a>
{% else %}
<strong>{{ review_url_text }}</strong>
{% endif %}

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

This is looking good, r+

@eviljeff
Copy link
Member

@wagnerand before I merge this, is it really only Ratings:Moderate permission that's needed to access the admin tool? (That's what the issue says now, but no special admin perm/checks?)

@wagnerand
Copy link
Member

@eviljeff
Copy link
Member

eviljeff commented Nov 3, 2020

@eviljeff I believe so (https://github.com/mozilla/addons-server/blob/master/src/olympia/constants/permissions.py#L215:L215), unless I am missing something?

I thought maybe a check that the user was able to actually access django admin (is_staff?) was needed too but okay

@eviljeff eviljeff merged commit ab278dc into mozilla:master Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants