search

mozilla / addons

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

Stop exposing reviewer names to developers in activity emails #8638

Closed bobsilverberg closed 2 years ago

bobsilverberg commented 2 years ago

This is a spin-off from mozilla/addons#8636 and is about removing and/or anonymizing the names of reviewers from emails sent to developers as a result of activities. The templates for these emails can be found in src/olympia/activity/templates/activity/emails/. Depending on how these templates are used, we may be able to remove the reviewer's name, or we may have to add logic to just anonymize the reviewer's name when the email is being sent to an add-on developer.

For QA: The following changes have been made:

  1. In the email template for activity emails sent from reviewers, the text was changed so it no longer opens with {{ author }}, a reviewer at addons.mozilla.org, found an issue ... and instead opens with A reviewer at addons.mozilla.org found an issue ....
  2. Emails sent from reviewers to add-on developers should not show the name of the reviewer in the "From" line, and instead should show "An add-on reviewer".
  3. Other emails (i.e., any emails not to an add-on developer, and emails to add-on developers but not from reviewers) should continue to show the name of the user who initiated the email in the "From" line.
  4. Although I expect it to be covered by the above, there should be no cases in which an email to an add-on developer contains the name of a reviewer, either in the body of the email or in the "From" line.
bobsilverberg commented 2 years ago

I have reviewed all of the code that could potentially send an email from a reviewer to a developer, and have identified these cases:

  1. When process_email is called (which happens in response to receiving an email), it sends out a bunch of emails (via notify_about_activity_log), some of which could be from a reviewer, going to a developer, so we need to address that case. There is a path for sending emails to developers, and we can use that to anonymize the sender of the email.

  2. There is an API available to add a note to a review of a version, which is undocumented (I’ll file an issue to fix that), and is used by devhub when a developer is replying to a review. The current use wouldn’t seem to allow an email from a reviewer (as it’s being used on devhub), but there’s nothing to prevent this API being used in the future to send an email from a reviewer. Fortunately, this also eventually calls notify_about_activity_log, so our changes to that will address this use case.

  3. When a developer submits source code when editing a version on devhub, a notification is sent to staff, reviewers and other authors. Once again, this eventually calls notify_about_activity_log, so our changes to that will address this use case.

  4. When a reviewer creates a reply in reviewer tools, a notification is sent to add-on developers. Once again, this eventually calls notify_about_activity_log, so our changes to that will address this use case.

  5. There are a number of other actions in reviewer tools that send notifications to add-on developers, all of which use notify_email in ReviewBase. This calls send_activity_mail directly, which will bypass our changes to notify_about_activity_log, but these also all use templates which exclude reviewer information, and the author of the email is set to be the generic ADDONS_EMAIL, so there is no need for further anonymization.

  6. There is a command, send_pending_rejection_last_warning_notifications, which also calls notify_email. This passes a template into notify_email which excludes reviewer information, so there is no need for further anonymization.

Regarding the anonymization, when sending email to a developer, do we only want to anonymize information about the sender when the sender is a reviewer, or in all cases?

ioanarusiczki commented 2 years ago

@bobsilverberg

This e-mail has been generated for all my tests with one exception -> when I made a reply to a new Theme (New Queue) I did not see an e-mail sent. It only worked for the themes from the Updated queue.

As a note here , I did not see an e-mail being generated to notify the developer about the addon being disabled after being blocked but I don't know if that's been working before either.

bobsilverberg commented 2 years ago

@bobsilverberg

This e-mail has been generated for all my tests with one exception -> when I made a reply to a new Theme (New Queue) I did not see an e-mail sent. It only worked for the themes from the Updated queue.

Is this something that worked before this? I'm fairly certain I didn't change any code that would affect when emails are sent, just the content of emails. If this is something you believe worked before and is now broken, can you open a new issue for it please?

As a note here , I did not see an e-mail being generated to notify the developer about the addon being disabled after being blocked but I don't know if that's been working before either.

As noted above, nothing about whether an email is sent or not has been changed, so my assumption would be that an email wasn't being sent before either. Is this something you can check?

wagnerand commented 2 years ago

As a note here , I did not see an e-mail being generated to notify the developer about the addon being disabled after being blocked but I don't know if that's been working before either.

Fwiw, this is the desired behavior. Internally, there is still a rejection happening as part of the block, and I wasn't sure that would trigger an email, but thank you for confirming it does not.

ioanarusiczki commented 2 years ago

@bobsilverberg I don't know if this worked before but I'm going to file it since it's working from all the other queues where reply is available.

ioanarusiczki commented 2 years ago

@bobsilverberg Hah, no need to, I re-checked right before filing the new theme issue but I've seen the e-mail being generated on -dev and -stage. Maybe it had delays yesterday, I don't know.