search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
128 stars 41 forks source link

[Task]: verify Cinder override decisions work as expected #15144

Closed eviljeff closed 1 month ago

eviljeff commented 1 month ago

Description

Cinder has an override function to make another decision on a job where a decision was already made (which closed the job). AMO has supported this from the beginning but it wasn't called out as supported feature so may not have been fully QA'd.

Acceptance Criteria

  ### Milestones/checkpoints
  - [ ] A job for an add-on that was resolved with (policy leading to) ADDON_DISABLE can be overridden with a different decision to APPROVE; the add-on owner gets a correct email informing them their add-on is now re-enabled because we reconsidered our decision.
  - [ ] an APPROVE can be overridden with a ADDON_DISABLE and the developer gets disable email (as if the override was the first decision, because they wouldn't have been aware of the the APPROVE)
  - [ ] a policy can be overridden with a different policy that still leads to ADDON_DISABLE - the developer gets the disable email with the new policies
  - [ ] (lower priority) repeat with non-addon content types

Checks

┆Issue is synchronized with this Jira Task

ioanarusiczki commented 1 month ago

So far I tried the following:

eviljeff commented 1 month ago

From looking at the webhook response I've identified the problem - Cinder is not passing the job id of the overridden job, only the decision id.

ioanarusiczki commented 4 weeks ago

I've a strange thing going on with emails. They stopped being sent to real inboxes so I've decided to check this on fakemail.

It took a while to understand what is going on in there, it seems that an email keeps being generated for the same action, for example at submission.

For example for an extension https://stage.cinder.nonprod.webservices.mozgcp.net/investigate/amo_addon/634059/decisions?filters=%7B%7D initially approved from Cinder

same email at addon submission is generated twice (as far as I've checked) https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196169/change/ https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196173/change/

the auto-approval email is again generated at least twice https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196174/change/ https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196179/change/

first email at approve from Cinder sent to reporter https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196178/change/ https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196180/change/ https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196182/change/ https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196183/change/

and the emails sent after override to disable content reporter https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196184/change/ developer https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196185/change/ reporter https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196187/change/ developer https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196186/change/ reporter https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196189/change/ developer https://addons-internal.dev.mozaws.net/en-US/admin/models/amo/fakeemail/196188/change/

When checking https://github.com/mozilla/addons/issues/15104 I had to do an auto-approval, block, delete block, force enable and unreject -> on stage I've received in the real inbox all emails. On dev, I did not see today emails sent for auto-approval, block or force enable (they're on dev, don't remember seeing them more than once but I wouldn't be 100% sure)

ioanarusiczki commented 4 weeks ago

forgot -> at the moment it's a bit confusing what's going on so I can't tell if the correct emails are sent or not. What's different than before at override it's that the actions have an effect, for example if something is disabled then overridden with approve, content is re-enabled.

https://stage.cinder.nonprod.webservices.mozgcp.net/investigate/amo_addon/634054/decisions?filters=%7B%7D https://stage.cinder.nonprod.webservices.mozgcp.net/investigate/amo_addon/634053/decisions?filters=%7B%7D

eviljeff commented 4 weeks ago

@ioanarusiczki it seems the duplicate emails are caused by (or a symptom of) #15160 - though I understand it may be hard/impractical to complete QA on this change with all the extra email noise in the meantime!

ioanarusiczki commented 3 weeks ago

I tried this on -dev and -stage today

Scenario 1 : content is disabled, override with approve , I tried with an extension, theme, rating, user

Scenario2: an approval overridden with disable, tested with a theme, an extension, a collection

Scenario3: a disable decision overridden with another policy that disables, tested with an extension and a theme.

That should be all expected. I didn't make screenshots with the emails because 1. would make this tltr 2. I think it's the correct emails sent.

I did not try overriding appeals (I'll check if that's possible tomorrow). Only now while writing test results down, I've realized maybe that's possible too.

ioanarusiczki commented 3 weeks ago

There's no such thing for appeals 🙃😄

I forgot to add that I've seen an error message together with success! in Cinder while testing this -> but as I said, things are happening as it's expected.

image