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

Allow reviewers and developers to reply to older versions #22661

Merged
merged 8 commits into from
Sep 16, 2024

Conversation

diox
Copy link
Member

@diox diox commented Sep 11, 2024

@diox diox changed the title Allow reviewer replies any version Allow reviewers and developers to reply to older versions Sep 12, 2024
@diox diox marked this pull request as ready for review September 12, 2024 12:08
Comment on lines +150 to +160
assert response.status_code == 404

user = UserProfile.objects.get(username='reviewer')
self.grant_permission(user, 'Addons:ViewDeleted')
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the check on version being the latest version forced me to fix the test_reply_to_deleted_version_is_400 test below, which was expecting a 400... but it really should have expected a 404 instead. Digging further, I found that developers and reviewers without Addons:ViewDeleted were able to view activities on deleted versions, which was not consistent with every other API/view. So I fixed that, which in turn forced me to adjust a few more tests.

{% set can_request_review=addon.can_request_review() %}
{% set can_cancel=(not addon.is_disabled and addon.status==amo.STATUS_NOMINATED) %}
{% if full_info and check_addon_ownership(request.user, addon, allow_developer=True) and version.channel == amo.CHANNEL_LISTED and (can_request_review or can_cancel) %}
{% if full_info and version.channel == amo.CHANNEL_LISTED and (can_request_review or can_cancel) and check_addon_ownership(request.user, addon, allow_developer=True) %}
Copy link
Member Author

Choose a reason for hiding this comment

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

We were redefining those 2 variables for each version, which is pointless and caused extra queries. I've moved their definition to the view.

Comment on lines -135 to -139
{% else %}
<div>
<a href="#" class="review-history-show" data-version="{{ latest_version_in_channel_including_disabled.id }}">{{ _('Leave a reply') }}</a>
</div>
{% endif %}
Copy link
Member Author

Choose a reason for hiding this comment

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

This part no longer makes sense since we're displaying the form on every version now.

@diox diox requested a review from eviljeff September 12, 2024 12:33
src/olympia/reviewers/utils.py Outdated Show resolved Hide resolved
@diox diox force-pushed the allow-reviewer-replies-any-version branch from 5843736 to bf4db19 Compare September 16, 2024 11:35
@diox diox merged commit 1e288cb into mozilla:master Sep 16, 2024
31 checks passed
KevinMind pushed a commit that referenced this pull request Sep 17, 2024
* Allow reviewers and developers to reply to older versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reviewers should be able to reply to older versions in a channel for an add-on
2 participants