wagnerand
commented
2 years ago Note that this also has impact on the exposure of reviewers, I recommend considering mozilla/addons#8372 before doing further changes here.
Closed kewisch closed 2 years ago
wagnerand
commented
2 years ago Note that this also has impact on the exposure of reviewers, I recommend considering mozilla/addons#8372 before doing further changes here.
bobsilverberg
commented
2 years ago mozilla/addons#8372 is essentially done. The only reason it isn't closed yet is I am waiting to land the final migration and then I'll close it as it's become a tracking bug.
The issue mentioned above, therefore, about showing the original reviewer's name, has become moot, which I gather will make this less complicated. I am going to look into a solution for this.
bobsilverberg
commented
2 years ago My suggestion is that we add a new field to VersionReviewerFlags called pending_rejection_by (or similar), where we record the user who created the original delayed rejection. Then, when we are processing those delayed rejections in reject_multiple_versions, if it's a pending rejection and pending_rejection_by exists, we use the user stored in pending_rejection_by instead of the Mozilla task user.
@diox / @eviljeff WDYT?
diox
commented
2 years ago Does the job, WFM. Obviously need to be a little careful to handle the case where it's not set, and reset it when we drop pending_rejection (might happen in a couple places, searching for pending_rejection=None is probably a good start).
AlexandraMoga
commented
2 years ago There seems to be an issue with scheduled rejections, so this needs to be re-opened: https://sentry.prod.mozaws.net/operations/olympia-dev/issues/18024287/?query=is%3Aunresolved
diox
commented
2 years ago This is trivial to fix, the auto_reject.py command (executed every hour to execute the pending rejections) will need to be updated to clear pending_rejection_by at the same time it updates pending_rejection in this bit of code:
https://github.com/mozilla/addons-server/blob/b815071676a0e119c6848295c2667fbbfeb82f34/src/olympia/reviewers/management/commands/auto_reject.py#L75-L77
AlexandraMoga
commented
2 years ago I've verified this issue on -dev an these are my results.
View in Reviewer tools - the Scheduled delay and the final Rejection are linked to the reviewer name:
View in devhub review history as the add-on developer sees it:
Currently, a delayed rejection is attributed to the reviewer who sent it, but the final rejection when the time expires is attributed to the Mozilla user. I would like to have a way to make this connection without having to piece together activity log entries.
From my view it looks like it would be difficult to just set the original reviewer as the user taking the action, because this would cause the email sent out to note the original reviewer instead of the Mozilla user and the usual
Automatic rejection after grace period ended. I'd be fine changing the rejection template accordingly in case a simple solution to this would be to not show the user if it is an automatic rejection (or the Mozilla user in general). Happy to provide copy for any template changes.I'm also fine solving this in a completely different way.