Skip to content

Comments

reviewer tools listing content approval and rejection#24386

Merged
eviljeff merged 5 commits intomozilla:masterfrom
eviljeff:15988-reviewer-tools-content-rejection
Feb 3, 2026
Merged

reviewer tools listing content approval and rejection#24386
eviljeff merged 5 commits intomozilla:masterfrom
eviljeff:15988-reviewer-tools-content-rejection

Conversation

@eviljeff
Copy link
Member

@eviljeff eviljeff commented Jan 29, 2026

Fixes: mozilla/addons#15988

Description

Updates the existing Approve Content reviewer action to use the relevant ContentAction, with an alternative action that sends and email if the listing is currently Rejected. Also adds a new Reject Listing (behind a waffle) to replace the existing reject versions action that is used.

Context

The new Reject Listing Content action is behind a waffle as there is currently no way to developers to request a new listing review (mozilla/addons#15989), and if they missed the email, no explanation in devhub (mozilla/addons#16025). The Approve Listing Content action is backwards compatible (as is the Approve Rejected Listing Content action, and would be available if the waffle switch was enabled and then disabled again).

The change to show the content review link everywhere was because this tool is the only way for a listing to be rejected, or approved after a rejection. (If the plan to only carry out content review in Cinder goes ahead then we'd likely drop the content review tool in the reviewer tools and move the specific actions into the main review tool for listed channel.)

Re: activity classes. In #24308 I added APPROVE_LISTING_CONTENT but I've since concluded it was redundant - it was the same as the existing APPROVE_CONTENT class, really. So I've taken over APPROVE_CONTENT activity for initial content review, and repurposed APPROVE_LISTING_CONTENT as APPROVE_REJECTED_LISTING_CONTENT to differentiate when a listing is restored after a rejection (so we can expose it in the reviewer tools, etc).

REJECT_CONTENT will be obsolete once the waffle switch is permanently on, and we remove support from the reject functions for content review rejection (but is kind of different to REJECT_LISTING_CONTENT, as the former is necessarily associated to which versions were rejected for the content review.)

Testing

Approve Listing Content

  • Go to an add-ons content review page, from the content review queue in the reviewer tools.
    • shouldn't matter if it's not in the content review queue or not for the actions, but you can verify it "left" the queue
  • Use the Approve Listing Content action.
    • if cinder_policy_review_reasons_enabled waffle is on (which it is on dev/stage, but isn't on prod) there should be an Approve policy to select (only one)
  • Content should be marked as approved as before (it will now set AddonApprovalCounter.last_content_review_pass to True, but that's transparent)
  • No emails should go out.

Reject Listing Content

  • Repeat - can be the same add-on if you like - but with Reject Listing Content action
    • enable content_rejection_enabled waffle switch to show this instead of Reject Multiple Versions
  • if cinder_policy_review_reasons_enabled waffle is on there should be the same list of policies to choose from as Reject and Force Disable
    • this will change once Operations/T&S decide which policies should lead to listing rejection, and which to add-on disable, but for now we still need it to work.
    • if the waffle switch is off there should be the list of ReviewActionReason's that prefill the comments box - same as Reject and Force Disable
  • Listing should be set to Rejected (see devhub, reviewer tools, and listing is non-public)
  • Email should be sent to the developer(s)

Approve Rejected Listing Content

  • Go to the content review page for an add-on with status Rejected (e.g. the one you just rejected in the previous steps)
  • Use the Approve Rejected Listing Content action.
  • Listing should be re-enabled - back to Approved or whatever
  • Email should be sent to the developer(s) notifying them.

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 force-pushed the 15988-reviewer-tools-content-rejection branch from 45a19af to 326731b Compare January 29, 2026 12:28
@eviljeff eviljeff changed the title 15988 reviewer tools content rejection reviewer tools listing content approval and rejection Jan 29, 2026
@eviljeff eviljeff force-pushed the 15988-reviewer-tools-content-rejection branch 2 times, most recently from 43f6085 to a561ce7 Compare January 29, 2026 18:29
@eviljeff eviljeff marked this pull request as ready for review January 29, 2026 18:51
@eviljeff eviljeff requested a review from diox January 29, 2026 18:51
@eviljeff
Copy link
Member Author

Mmm, pretty sure Listing rejection won't clear the add-on from the content review queue.... mozilla/addons#15989 is going to have to change how reviews are requested so it can be addressed with that (unless you think it needs to be in this PR)

Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

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

As it stands, when you use the ContentActionApproveListingContent action (and reject too in this PR with the waffle switch enabled) the resulting activity is not attached to a version, and therefore not displayed anywhere in reviewer tools. It used to be, so that would be a regression.

We should either attach that to the current_version like it used to happen when not using the ContentAction stuff, or put those activity logs in addon_important_changes_actions.


class APPROVE_REJECTED_LISTING_CONTENT(_LOG):
id = 211
format = _('{addon} rejected listing content approved.')
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the string we expose to developers should be that verbose. Maybe they don't need that level of detail and we could just use {addon} listing content approved ? (and have a reviewer_format for more-specific internal info for reviewers with the full string)

Copy link
Member Author

Choose a reason for hiding this comment

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

7f9fa49 - short still has the longer string because I couldn't remember (nor establish conclusively with a cursory search) if we exposed it to reviewers too. I also tinkered with the wording of the other two classes so the copy was consistent.

@eviljeff
Copy link
Member Author

eviljeff commented Feb 2, 2026

put those activity logs in addon_important_changes_actions.

#24396 adds this

@eviljeff eviljeff force-pushed the 15988-reviewer-tools-content-rejection branch from 7f9fa49 to d260c5a Compare February 3, 2026 10:59
@eviljeff
Copy link
Member Author

eviljeff commented Feb 3, 2026

rebased after #24396

@eviljeff eviljeff requested a review from diox February 3, 2026 12:18
@eviljeff eviljeff merged commit 7475d61 into mozilla:master Feb 3, 2026
45 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]: Allow reviewers to change content listing state in the reviewer tools

2 participants