mozilla / addons

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

[Bug]: Confirm approval action is sending an email to the reporter #15089

Open ioanarusiczki opened 1 month ago

ioanarusiczki commented 1 month ago

What happened?

While checking https://github.com/mozilla/addons/issues/15068

At one point Confirm approval action is available for https://reviewers.addons.allizom.org/en-US/reviewers/review-listed/1031553 The version is reported for add-on policies and flagged for HR Using Confirm Approval + checking the DSA report would generate an email which is sent to the reporter

confirm approval plus a report attached

What did you expect to happen?

There is already a dedicated policy which is sending the email above to a reporter from Resolve Reports + Approve.

Maybe for Confirm approval the DSA section should not be displayed (as for Clear pending rejection, Set Needs Human Review or Reviewer Reply)

Is there an existing issue for this?

┆Issue is synchronized with this Jira Task

eviljeff commented 1 month ago

Originally, the intent was for all abuse report related actions to be "bolted onto" the existing reviewer actions - the thinking was that a reviewer could complete both tasks at once - confirm approval on the version, and dismiss the report at the same time (thinking: if a version was okay to be approved, the add-on must be okay, so the abuse report can also be dismissed).

But we've since added dedicated actions for resolving abuse reports as the add-on is okay, and it's probably more confusing to combine functionality here, especially as it would send out an email (to the reporter though, not the developer).

@wagnerand agree?

wagnerand commented 1 month ago

It's hard to say which is the best approach.

If I understand correctly, this is about confirming an (auto-)approval, which never sends an email to the developer. Versions that are pending manual approval/signing are usually not public yet, so I am wondering if they can realistically receive abuse reports.

eviljeff commented 1 month ago

It's hard to say which is the best approach.

It's either agree, and we remove the option to resolve an abuse report with "Confirm Approval"; or wontfix. If you're on the fence we'll go with wontfix and we can reopen later if you change your mind.

If I understand correctly, this is about confirming an (auto-)approval, which never sends an email to the developer.

Yes, this is about the Confirm Approval reviewer tools action, which relates to auto-approved versions (that could have received abuse reports in the time period between them being signed automatically, and someone confirming that approval.)

Versions that are pending manual approval/signing are usually not public yet, so I am wondering if they can realistically receive abuse reports.

That's a different action, Approve. The resolution of abuse reports/jobs is also available with that action, iirc, because abuse reports could have accrued for earlier approved versions. (If you disagree with the functionality for the Approve action, please file a separate issue)

wagnerand commented 1 month ago

It's either agree, and we remove the option to resolve an abuse report with "Confirm Approval"; or wontfix. If you're on the fence we'll go with wontfix and we can reopen later if you change your mind.

It's not that simple. Another option is to remove the "Resolve Appeals" action in this case.

I would like to her @abyrne-moz's thoughts on this as well.

The overall workflow documentation how to handle and resolve abuse reports, appeals and esclations for all scenarios that I had requested a while ago would probably help us make a decision here. What is the status of that?

eviljeff commented 1 month ago

It's either agree, and we remove the option to resolve an abuse report with "Confirm Approval"; or wontfix. If you're on the fence we'll go with wontfix and we can reopen later if you change your mind.

It's not that simple. Another option is to remove the "Resolve Appeals" action in this case.

It's "Resolve Reports", but I don't see how removing it that would be a fix to this issue. This issue is "it's confusing that a reviewer tools action that doesn't send an email to a developer sends an email to a reporter", not "it's confusing there are two ways to resolve reports/jobs". If you think we want to make that change please file an issue - though I'd argue against it as the "Resolve Reports" action allows the reviewer to resolve reports as invalid, in addition to resolving them as approve.

The overall workflow documentation how to handle and resolve abuse reports, appeals and esclations for all scenarios that I had requested a while ago would probably help us make a decision here. What is the status of that?

Can you link to the ticket for it?

wagnerand commented 4 weeks ago

It's "Resolve Reports", but I don't see how removing it that would be a fix to this issue. This issue is "it's confusing that a reviewer tools action that doesn't send an email to a developer sends an email to a reporter", not "it's confusing there are two ways to resolve reports/jobs". If you think we want to make that change please file an issue - though I'd argue against it as the "Resolve Reports" action allows the reviewer to resolve reports as invalid, in addition to resolving them as approve.

Ah, thanks for that clarification.

Can you link to the ticket for it?

I am not sure there is a ticket, but there is a conversation including a boilerplate document to fill out: https://mozilla.slack.com/archives/C078CL8A2HK/p1719341827297949

eviljeff commented 4 weeks ago

I am not sure there is a ticket, but there is a conversation including a boilerplate document to fill out: https://mozilla.slack.com/archives/C078CL8A2HK/p1719341827297949

Ah, I see the four of us didn't work on the solid documentation after all 😆