From 3fcfd5739680fa2b5c01ddb4057a762ba70a54d6 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 11 Oct 2022 12:47:08 +0100 Subject: [PATCH] implement admin reviewer unreject versions action (#19728) * implement admin reviewer unreject versions action * don't subscribe the reviewer to the review * move status setting code to Addon model --- src/olympia/addons/models.py | 16 + src/olympia/addons/tests/test_models.py | 23 ++ src/olympia/constants/activity.py | 11 + src/olympia/reviewers/forms.py | 45 ++- src/olympia/reviewers/tests/test_forms.py | 133 +++++++-- src/olympia/reviewers/tests/test_utils.py | 348 ++++++++++++++-------- src/olympia/reviewers/tests/test_views.py | 80 +++++ src/olympia/reviewers/utils.py | 62 +++- 8 files changed, 547 insertions(+), 171 deletions(-) diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index 1a197ddfbf..0673eece30 100644 --- a/src/olympia/addons/models.py +++ b/src/olympia/addons/models.py @@ -1235,6 +1235,22 @@ class Addon(OnChangeMixin, ModelBase): self.update_version(ignore=ignore_version) + def update_nominated_status(self, user): + # Update the addon status to nominated if there are versions awaiting review + if ( + self.status == amo.STATUS_NULL + and self.versions.filter( + file__status=amo.STATUS_AWAITING_REVIEW, channel=amo.CHANNEL_LISTED + ).exists() + ): + log.info( + 'Changing add-on status [%s]: %s => %s (%s).' + % (self.id, self.status, amo.STATUS_NOMINATED, 'unrejecting versions') + ) + self.update(status=amo.STATUS_NOMINATED) + activity.log_create(amo.LOG.CHANGE_STATUS, self, self.status, user=user) + self.update_version() + @staticmethod def attach_related_versions(addons, addon_dict=None): if addon_dict is None: diff --git a/src/olympia/addons/tests/test_models.py b/src/olympia/addons/tests/test_models.py index 511e2e1d0a..6bc3971ce6 100644 --- a/src/olympia/addons/tests/test_models.py +++ b/src/olympia/addons/tests/test_models.py @@ -2104,6 +2104,29 @@ class TestUpdateStatus(TestCase): amo.STATUS_NULL ) # No listed versions so now NULL + def test_update_nominated_status(self): + addon = addon_factory( + status=amo.STATUS_NULL, file_kw={'status': amo.STATUS_DISABLED} + ) + first_version = addon.versions.last() + version_factory(addon=addon, file_kw={'status': amo.STATUS_APPROVED}) + user = user_factory() + # if add-on has no files that are AWAITING_REVIEW, the status shouldn't change + addon.update_nominated_status(user) + assert addon.reload().status == amo.STATUS_NULL + + addon.update(status=amo.STATUS_DISABLED) + first_version.file.update(status=amo.STATUS_AWAITING_REVIEW) + # and neither should the status change if the addon status isn't NULL + addon.update_nominated_status(user) + addon.refresh_from_db() # this clears the fk relations too + assert addon.status == amo.STATUS_DISABLED + + addon.update(status=amo.STATUS_NULL) + # success case - has version that is awaiting review and incomplete addon status + addon.update_nominated_status(user) + assert addon.reload().status == amo.STATUS_NOMINATED + class TestGetVersion(TestCase): fixtures = [ diff --git a/src/olympia/constants/activity.py b/src/olympia/constants/activity.py index 8d252cb95b..8cf9505d7d 100644 --- a/src/olympia/constants/activity.py +++ b/src/olympia/constants/activity.py @@ -844,6 +844,17 @@ class RESTRICTED(_LOG): format = _('{user} restricted.') +class UNREJECT_VERSION(_LOG): + # takes add-on, version + id = 171 + action_class = 'reject' + format = _('{addon} {version} unrejected.') + short = _('Unrejected') + keep = True + review_queue = True + reviewer_review_action = True + + LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG] # Make sure there's no duplicate IDs. assert len(LOGS) == len({log.id for log in LOGS}) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index 85e8957f3f..4059c94e05 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -112,11 +112,28 @@ class VersionsChoiceWidget(forms.SelectMultiple): """ actions_filters = { - amo.STATUS_APPROVED: ('confirm_multiple_versions', 'block_multiple_versions'), - amo.STATUS_AWAITING_REVIEW: ( - 'reject_multiple_versions', - 'approve_multiple_versions', - ), + amo.CHANNEL_UNLISTED: { + amo.STATUS_APPROVED: ( + 'block_multiple_versions', + 'confirm_multiple_versions', + ), + amo.STATUS_AWAITING_REVIEW: ( + 'approve_multiple_versions', + 'reject_multiple_versions', + ), + amo.STATUS_DISABLED: ('unreject_multiple_versions',), + }, + amo.CHANNEL_LISTED: { + amo.STATUS_APPROVED: ( + 'block_multiple_versions', + 'reject_multiple_versions', + ), + amo.STATUS_AWAITING_REVIEW: ( + 'approve_multiple_versions', + 'reject_multiple_versions', + ), + amo.STATUS_DISABLED: ('unreject_multiple_versions',), + }, } def create_option(self, *args, **kwargs): @@ -126,13 +143,12 @@ class VersionsChoiceWidget(forms.SelectMultiple): obj = option['label'] status = obj.file.status if obj.file else None versions_actions = getattr(self, 'versions_actions', None) - if versions_actions and obj.channel == amo.CHANNEL_UNLISTED: - # For unlisted, some actions should only apply to approved/pending - # versions, so we add our special `data-toggle` class and the - # right `data-value` depending on status. + if versions_actions: + # Add our special `data-toggle` class and the right `data-value` depending + # on status. option['attrs']['class'] = 'data-toggle' option['attrs']['data-value'] = '|'.join( - self.actions_filters.get(status, ()) + ('',) + self.actions_filters[obj.channel].get(status, ()) + ('',) ) # Just in case, let's now force the label to be a string (it would be # converted anyway, but it's probably safer that way). @@ -218,7 +234,7 @@ class ReviewForm(forms.Form): if action: if not action.get('comments', True): self.fields['comments'].required = False - if action.get('versions', False): + if action.get('multiple_versions', False): self.fields['versions'].required = True if not action.get('requires_reasons', False): self.fields['reasons'].required = False @@ -255,18 +271,19 @@ class ReviewForm(forms.Form): # if the relevant actions are available, otherwise we don't really care # about this field. versions_actions = [ - k for k in self.helper.actions if self.helper.actions[k].get('versions') + k + for k in self.helper.actions + if self.helper.actions[k].get('multiple_versions') ] if versions_actions: if self.helper.version: channel = self.helper.version.channel else: channel = amo.CHANNEL_LISTED - statuses = (amo.STATUS_APPROVED, amo.STATUS_AWAITING_REVIEW) self.fields['versions'].widget.versions_actions = versions_actions self.fields['versions'].queryset = ( self.helper.addon.versions(manager='unfiltered_for_relations') - .filter(channel=channel, file__status__in=statuses) + .filter(channel=channel) .no_transforms() .select_related('file') .select_related('autoapprovalsummary') diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index fe2ee54e06..0d5b5240e2 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -61,6 +61,16 @@ class TestReviewForm(TestCase): ) assert list(actions.keys()) == ['reply', 'super', 'comment'] + # If an admin reviewer we also show unreject_multiple_versions + self.grant_permission(self.request.user, 'Reviews:Admin') + actions = self.get_form().helper.get_actions() + assert list(actions.keys()) == [ + 'unreject_multiple_versions', + 'reply', + 'super', + 'comment', + ] + def test_actions_addon_status_deleted(self): # If the add-on is deleted we only show reply, comment and # super review. @@ -84,7 +94,18 @@ class TestReviewForm(TestCase): 'comment', ] - # The add-on is already disabled so we don't show + # If an admin reviewer we also show unreject_multiple_versions + self.grant_permission(self.request.user, 'Reviews:Admin') + actions = self.get_form().helper.get_actions() + assert list(actions.keys()) == [ + 'reject_multiple_versions', + 'unreject_multiple_versions', + 'reply', + 'super', + 'comment', + ] + + # The add-on is already disabled so we don't show unreject_multiple_versions or # reject_multiple_versions, but reply/super/comment are still present. actions = self.set_statuses_and_get_actions( addon_status=amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED @@ -266,14 +287,24 @@ class TestReviewForm(TestCase): assert form.fields['versions'].required is False assert list(form.fields['versions'].queryset) == [self.addon.current_version] - def test_versions_queryset_contains_pending_files_for_listed(self): + def test_versions_queryset_contains_pending_files_for_listed( + self, select_data_value='reject_multiple_versions|' + ): + # We hide some of the versions using JavaScript + some data attributes on each + #