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

delegate reject actions to ContentActions too #23023

Conversation

eviljeff
Copy link
Member

@eviljeff eviljeff commented Jan 24, 2025

Fixes: mozilla/addons#15306

Depends on #23004 & needs rebasing after that is merged.

Description

Implements the last remaining actions that could need a hold for 2nd level approval - version rejections.

Context

To ease implementation, I've focused on the simple case - the initial rejection of one or more versions - so ignored auto rejections and rejections that happen via blocks (and renamed the existing reject_multiple_versions function). Auto rejections don't need another hold because it was already considered when the initial decision was made; blocks have their own hold criteria. Follow-up work would be to continue migrating the remaining code that changes addon/version states in the reviewer tools to their equivalent ContentAction

Testing

Similarly to #15306, the change is how the action ends up being processed in the reviewer tools, but this time for all the reject actions:

  • For a "normal" add-on, select reject action from reviewer tools, and choose a reason.
  • verify that the version was disabled; and the email we send out contains the reason text
  • check the activity log created is linked to the addon, the version rejected, decision, policy, and reason.

Repeat with an add-on that would be considered high impact - e.g. a Recommended or Notable add-on.

  • select reject from the reviewer tools, etc.
  • verify that the version was not disabled, and the review history includes that the action was held. (no email to developer either)
  • Go to 2nd level approval queue and release the hold - proceed with the action.
  • verify that the version was disabled; and the email we send out contains the reason text
  • check the activity log created is linked to the addon, version rejected, decision, policy, and reason.

This can be further repeated with multiple version rejection; delayed rejections; and content rejections - they should all work as before when the add-on isn't high impact, but be held for review if the add-on is.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@eviljeff eviljeff changed the title 1982 delegate reject actions to content actions too delegate reject actions to ContentActions too Jan 27, 2025
@eviljeff eviljeff marked this pull request as ready for review January 27, 2025 08:25
@eviljeff eviljeff force-pushed the 1982-delegate-reject-actions-to-content-actions-too branch from 4471c28 to e92e4b8 Compare January 27, 2025 14:00
raise NotImplementedError
def __init__(self, decision):
super().__init__(decision)
self.content_review = decision.metadata.get('content_review', False)
Copy link
Member Author

Choose a reason for hiding this comment

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

fwiw, I did consider using separate Action classes ("ContentActionRejectContent" and "ContentActionRejectContentDelayed"), but it's the same actual outcome, with the emails sent in either case so didn't seem worth the extra complexity - especially as content reviews are being changed soon(?).

At that point we may want a different ContentAction class - and perhaps set AMO_DISABLE_ADDON instead as the cinder action as it's more what the outcome is (the listing being removed) than a version rejection(?)

@eviljeff eviljeff requested a review from diox January 28, 2025 11:03
class HELD_ACTION_REJECT_VERSIONS(_LOG):
id = 199
action_class = 'reject'
format = _('{addon} {version} rejection held for further review.')
Copy link
Member

Choose a reason for hiding this comment

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

It won't be shown to developers (it's an admin event), so it doesn't need to be translated. Same for the others below (and also REQUEST_LEGAL above if you want to do a drive-by)

Comment on lines +1427 to +1430
AddonReviewerFlags.objects.update_or_create(
addon=self.addon,
defaults={auto_approval_disabled_until_next_approval_flag: True},
)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of the ContentAction ? It feels like a consequence of the action, something that we wouldn't do if the action is "cancelled" after being held up for approval.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't block merging though.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that in either case we'd want to stop any further auto approvals. It also prevents any approvals while the rejection is held.

Copy link
Member

Choose a reason for hiding this comment

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

mmm yeah preventing any approvals while the rejection is held certainly makes sense.

@diox diox merged commit bee1ba3 into mozilla:master Jan 28, 2025
49 checks passed
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.

[Task]: delegate reject version review actions to CinderActions too
2 participants