Closed Bug 1609266 Opened 5 years ago Closed 5 years ago

Implement mozilla-specific Phabricator email notifier

Categories

(Conduit :: Phabricator, enhancement, P2)

Product:
Conduit
Component:
Phabricator
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mhentges, Assigned: mhentges)

References

(Blocks 2 open bugs)

Details

(Keywords: access, conduit-triaged)

Attachments

(10 files, 1 obsolete file)

combined-desktop-screenshot.png
131.97 KB, image/png
Details
Doesn't look bad on Android Gmail either :)
119.69 KB, image/jpeg
Details
Bug 1609266 - Adds endpoint for email events.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - Adds endpoint for the current feed status
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - Don't email users unless they've enabled Mozilla notifications
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - Adds timestamp to each email event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - Allow using the email API endpoints as any user when running in dev r?dkl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - Improve docker build performance
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - catches email type errors r?dkl
47 bytes, text/x-phabricator-request
Details | Review
Bug 1609266 - Support group reviewers r?dkl
47 bytes, text/x-phabricator-request
Details | Review

We want to improve the emails sent from Phabricator for when differentials change:

  • Remove noise ("what is all this policy, visibility and 'secure revision' text about?")
  • Explain the important event that has happened ("why should I care about this email?")
  • Make them more actionable ("as a receiver of this email, what do I need to do about it?")
  • Batch the notification for a stack changing into a single email, rather than sending an email for each diff in the stack

Phabricator hasn't built its email templating system to be configurable, so it will probably be cleaner to implement a separate piece of software that consumes the Phabricator feed and sends email accordingly.

Ticket workflow:

This system will overlap with the existing Phabricator email system as it improves, so we'll need to slowly replace the old with the new as this improves.

  1. Implement emails for all required differential events
  2. Consult team and stakeholders to confirm quality
  3. Run a "beta test" where additional users can opt into using the new system, confirm quality
  4. Switch everyone over to new system, stop sending emails about differentials from the old system

Questions

  • What events do we want to send emails for? I think that it's just the following, is that correct?
    • Differential approved/rejected/closed
    • Comments added
    • Reviewer added/removed
    • Code changed
  • Do we want to show differential comments inline in emails still? I'm guessing "yes"
  • What information do we want to show/omit from emails for secure bugs? I'm guessing just show differential ID, link, action (approved/rejected/commented/code changed)?
  • How should we allow people to opt into the "beta test"? Should we add a checkbox in the config UI in Phabricator, or perhaps users can notify me via slack (not sure what the volume will be)? Alternatively, would this be a private beta test?
  • When running the "beta test", should we disable the old email system for opted-in users?
  • Should we email if a revision is submitted as a draft, and it has reviewers?
    • I personally use the draft feature to make patches visible to teammates with the intention of having feedback before I finalize my changes, so I would prefer that reviewers are notified of my patch. However, this might not be compatible with the upstream Phabricator draft workflow.
Flags: needinfo?(smacleod)
Assignee: nobody → mhentges
Keywords: conduit-triaged
Priority: -- → P2

Also an ops question: should I directly talk to Amazon SES to send email, or do our machines have something like postfix installed locally?

I meant to add an NI for ckolos with the above comment, I'll do it now.

Progress update:

  • I understand enough of the Phabricator API to see how it'll allow us to consume a feed and look up associated information for transactions on that feed 👍. The only remaining unknown here is: if we decide we want to show inline comments (with context), we'll need to fetch the associated source code, which I'm not sure how that works yet. I'll investigate when we decide we want inline comments.
  • I have text-based mockups for each of the events I'm guessing we'll want. I need to make html/email mockups instead, at which point I'll post them here to make sure they have all the information we care about.
  • I fixed a small issue with the feed.query_id API, which is arguably neato
  • As I start converting the email mockups to programmatically-inflated templates, I'll need to start considering the data model and what will be stored in the local db.
  • Then I just need to "draw the rest of the owl".
Flags: needinfo?(ckolos)

SES is used directly by phabricator via a key/secret pair.

Flags: needinfo?(ckolos)

There's going to be a period where both Phabricator and this new tool (phabricator-emails) will send mail - we'll just generate a new key/secret pair for phabricator-emails, yeah?

Flags: needinfo?(ckolos)

(In reply to Mitchell Hentges [:mhentges] from comment #1)

Questions

  • What events do we want to send emails for? I think that it's just the following, is that correct?
    • Differential approved/rejected/closed
    • Comments added
    • Reviewer added/removed
    • Code changed

Additionally we'll want events for things like the title / summary / bug changing for a revision. Pretty much we want to be firing on the same events that current emails are sent on (whenever a new feed story is added), but we want to be more intelligent about how these events are displayed and possibly combine multiple events into a single email.

  • Do we want to show differential comments inline in emails still? I'm guessing "yes"

Yup.

  • What information do we want to show/omit from emails for secure bugs? I'm guessing just show differential ID, link, action (approved/rejected/commented/code changed)?

We'll need to get rid of all information, so it's pretty much just a link to the revision / event, that is until we add support for the encrypted emails.

  • How should we allow people to opt into the "beta test"? Should we add a checkbox in the config UI in Phabricator, or perhaps users can notify me via slack (not sure what the volume will be)? Alternatively, would this be a private beta test?

I think to get started just manually adding people who tell us they want to try works. We can always do the Phabricator option / something without admin intervention in the near future.

  • When running the "beta test", should we disable the old email system for opted-in users?

I'm thinking no, but telling them how they could might be useful. It should be possible to easily filter each set of emails into their own distinct buckets, so they could just auto filter the old emails in the worst case.

  • Should we email if a revision is submitted as a draft, and it has reviewers?
    • I personally use the draft feature to make patches visible to teammates with the intention of having feedback before I finalize my changes, so I would prefer that reviewers are notified of my patch. However, this might not be compatible with the upstream Phabricator draft workflow.

The draft state in Phabricator has special properties, and trying to send emails while we're in it goes against upstream Phabricator. Feed stories won't be published for events when a revision is in draft and bugzilla does a bunch of fixups to the revision which should come as a single email. It's probably best to stick with the current model and have no emails for draft.

Flags: needinfo?(smacleod)

This is very helpful, thanks!

we want to be more intelligent about how these events are displayed and possibly combine multiple events into a single email.

How do we want to handle condensing multiple patch pushes?
The "easy" way is to send an email for every revision that has been updated. If we were to combine these "revision updated" events, how do we want that to look? Do we want to have a single email with a list of all revisions in a stack that got updated? What's the ideal behaviour here?

Flags: needinfo?(smacleod)

I'm attaching the first email mockup here to show what I've imagined as the philosophy of the email design, comments are welcome.
I'll follow up with associated mockups for each event later this week.

(In reply to Mitchell Hentges [:mhentges] from comment #8)

I'm attaching the first email mockup here to show what I've imagined as the philosophy of the email design, comments are welcome.
I'll follow up with associated mockups for each event later this week.

That looks great!

Glad to see you're using litmus to ensure the massive variety of email client CSS support is exposed - font sizing in particular has been problematic with gmail doing its own thing vs. other clients (Bug 892382, Bug 895449, Bug 917683, Bug 1306695, Bug 1314984, Bug 1518266, ...).

Don't forget we need text/plain versions too.

I think the CTA may be better to be at the top of the email - one of the core issues with Phabricator's emails is there's no immediately obvious differentiation between "FYI" and "CTA" emails. Bringing the button to the top would be one way to address that, and we may want to consider other ways to hit home the difference in intent of the two types. I'd like to see mockups for both types of emails.

We'd also always need to include links to the revision, the bug, a link to /new/ for updates if relevant, and a link to the email prefs so people know how to "unsubscribe".

(In reply to Mitchell Hentges [:mhentges] from comment #7)

How do we want to handle condensing multiple patch pushes?
The "easy" way is to send an email for every revision that has been updated. If we were to combine these "revision updated" events, how do we want that to look? Do we want to have a single email with a list of all revisions in a stack that got updated? What's the ideal behaviour here?

Ya, the main event we want to combine is when new diffs are added. Keeping reviews and comments separate is fine, but a single email for the stack when diffs are updated could work well. Might be good to get user input on what the ideal behaviour would be.

Flags: needinfo?(smacleod)

Huh, Gmail does some weird shenanigans:

  • user-select: none is removed (so line numbers from inline-comment code aren't omitted from copy-paste, which is a bummer)
  • overflow-x: auto has been stripped as well, so code lines wrap (which is really gross, but also what existing phabricator emails do, so 🤷
  • The Android native Gmail client got aggressive at disrupting blockquote, but replacing it with a styled div seems to be enough to work around

I spent today making improvements according to glob's suggestions and improving the inline-diff styling. I still need to add the footer and create a style for replies-to-inline-comments for this event-email to be screenshot-ready.

We'd also always need to include links to the revision, the bug, a link to /new/ for updates if relevant, and a link to the email prefs so people know how to "unsubscribe".

I'll add a consistent link to the revision in the footer as well as a recommendation to "@mhentges for suggestions or to unsubscribe".

We can't link email prefs since we won't have a configurable settings page for the MVP. We can design the emails around being email-client-filterable to allow manual notification preferences (for now), though. Additionally, during the "opt-in phase", we'll have a hardcoded-ish list of users receiving the new emails, and I'll recommend that users "@mhentges" if they don't want to be on this list anymore.

Finally, I'm not sure that a link to /new/ makes sense to be in every email. It's a confusing-enough feature ("even without /new/ I already see 'changes since last comment'? What does it do?") that it's probably mostly used by knowledgeable users. Therefore, if it's not generally useful, it will be "noise" on the email for most users. Besides, we'll already have a link to the revision, why show a /new/ link too?

double submitted comment

Attachment #9122543 - Attachment is obsolete: true

(In reply to Mitchell Hentges [:mhentges] from comment #13)

I'll add a consistent link to the revision in the footer as well as a recommendation to "@mhentges for suggestions or to unsubscribe".

For the pilot period ask people to file bugs with feedback, not to contact you directly.

We can't link email prefs since we won't have a configurable settings page for the MVP.

Unsubscribing relates to how to opt out of all Phabricator emails, not the new email system itself.
Basically mirror the existing content from Phabricator's emails and link to email prefs.
It's important for anything we send through SES.

As the pilot will be opt-in I don't think it's necessary to detail how to opt-out on every email.

Finally, I'm not sure that a link to /new/ makes sense to be in every email. It's a confusing-enough feature ("even without /new/ I already see 'changes since last comment'? What does it do?") that it's probably mostly used by knowledgeable users.

OK, let's not add it to the MVP and get feedback.

Screenshots

I like what you've done to highlight that action is required.
The revision and bug numbers appears to be missing from the content. Both are important to display.

For the pilot period ask people to file bugs with feedback, not to contact you directly.

I'm nervous about this being too heavyweight, as Bugzilla adds significant friction to requesting improvements.
However, I'm not expecting a lot of feedback. If there is potential for a flood of requests, then I agree that pointing that flood towards my Slack inbox would be problematic, and that Bugzilla would make more sense.
Additionally, I suppose direct messages to me are private, and hard to use as references/rationale for a modification.

Perhaps the friction of using Bugzilla is better solved by improving the create-bug workflow, rather than trying to sidestep it.

The revision and bug numbers appears to be missing from the content. Both are important to display.

I'm going to have the revision in the subject of the email, is it necessary for it to be in the content too?

(In reply to Mitchell Hentges [:mhentges] from comment #18)

For the pilot period ask people to file bugs with feedback, not to contact you directly.

I'm nervous about this being too heavyweight, as Bugzilla adds significant friction to requesting improvements.

All recipients of these emails already have bugzilla accounts and are heavy users.

The revision and bug numbers appears to be missing from the content. Both are important to display.

I'm going to have the revision in the subject of the email, is it necessary for it to be in the content too?

Yes.

Had a solid in-person discussion with Steve:

  1. We have three separate improvements we want to make to Phabricator emails: improve usability (less verbosity, make more obvious if actionable), put all emails for the same stack into a single email thread, and batch code push emails (so updating a stack doesn't cause too many emails). It's probably worthwhile to launch of these into production as they're finished, rather than waiting until all three are finished. This will allow us to receive feedback and productize faster.
  2. Stacks can be uniquely and consistently defined by using the lowest revision ID number in the stack.
  3. Set the "email thread" header based on ^ so that email clients thread all events in the same stack
  4. The subject of the thread should be the name of the bug if set [0], or "Stack on Dn" if not
  5. I'm going to determine if it's possible for gmail to filter out individual emails in a thread. This is valuable for devs who want to filter for action-able emails. If this isn't possible, we might need to send two emails for actionable events (one for the thread, one for the filter).

[0] Since we want to be consistent about what bug we associate with a stack, we'll just use the bug associated with the lowest-id'd revision. (We can't just use "most commonly-associated bug" or "most-recently associated bug" since that is more prone to change). If we were inconsistent with the bug that we associated with the stack, then we'd have email-threading issues on gmail because it doesn't honour the "email thread" header.

Notes:

  • If a bug name changes in Bugzilla, the email thread will break in gmail. We cautiously agreed that this is better behaviour than having an obsolete bug name in the subject.

There's going to be a period where both Phabricator and this new tool (phabricator-emails) will send mail - we'll just generate a new key/secret pair for phabricator-emails, yeah?

Of course. Since it's envisioned as a separate service from phabricator, it will maintain its own credentials.

I'm nervous about this being too heavyweight, as Bugzilla adds significant friction to requesting improvements.
However, I'm not expecting a lot of feedback. If there is potential for a flood of requests, then I agree that pointing that flood towards my Slack inbox would be problematic, and that Bugzilla would make more sense.
Additionally, I suppose direct messages to me are private, and hard to use as references/rationale for a modification.

Perhaps the friction of using Bugzilla is better solved by improving the create-bug workflow, rather than trying to sidestep it.

Why not simply put a link in the footer that links to a pre-populated bug template?

Something wrong? Missing? Suggestions for improvement? Click *here* to file a bug

Flags: needinfo?(ckolos)

The concept stage is a great time to get it looked at by SecOps. NI for Julien Vehent to take a look.

It was my understanding that Phabricator was selected in order to not do things bespoke. Have we exhausted other options before going down this path?

Flags: needinfo?(jvehent)

(In reply to Habibullah Pagarkar (use Slack) from comment #22)

It was my understanding that Phabricator was selected in order to not do things bespoke. Have we exhausted other options before going down this path?

The only other option is to ask upstream to address our concerns – some of which they pushed back on, others are on the backlog with no solution in sight.

(In reply to Habibullah Pagarkar (use Slack) from comment #22)

The concept stage is a great time to get it looked at by SecOps. NI for Julien Vehent to take a look.

Simon: could you run a RRA on this?

Flags: needinfo?(jvehent) → needinfo?(sbennetts)
Keywords: access

I've added access as a keyword to look into the accessibility of the mockups.
For example, one potential concern is that the use of emoji to make emails quickly identifiable might be emoji over-use?

(In reply to Mitchell Hentges [:mhentges] from comment #12)

  • The Android native Gmail client got aggressive at disrupting blockquote, but replacing it with a styled div seems to be enough to work around

Not quite. For screen readers, you'll need to add the role="blockquote" attribute to that div so screen readers still know what they're dealing with. If you have to nest them, start with the outer most by adding aria-level="1", and ascending the level number with each deeper indentation.

(In reply to Mitchell Hentges [:mhentges] from comment #25)

For example, one potential concern is that the use of emoji to make emails quickly identifiable might be emoji over-use?

Without a textual representation, I cannot speak to this, but in general, if it is one Emoji to identify each type, things should be fine. Emojis only become problematic if they're 5 or 20 in a row. Because each emoji is represented by a whole set of words like "slightly smiling face" or "red warning flag" or such.

Drive-by comments:

(In reply to Mitchell Hentges [:mhentges] from comment #20)

  1. We have three separate improvements we want to make to Phabricator emails: improve usability (less verbosity, make more obvious if actionable)

I think everyone can agree that this will be helpful. It's too bad upstream hasn't improved emails much AFAICT.

put all emails for the same stack into a single email thread,

I personally have gotten used to the current behaviour of one thread per Diff and prefer it since there is a 1:1 mapping between reviews I need to do and threads and I can use the threads as a TODO list. If we go ahead with this I hope I can opt-out of the new emails and go back to current behaviour.

I think that mail for changes to a different bug in a stack should stay in their own thread. I don't see why one would want to combine bugmail for separate bugs.

and batch code push emails (so updating a stack doesn't cause too many emails).

Hopefully this won't add too much latency to the emails.

Thanks for the comments :)

I think that mail for changes to a different bug in a stack should stay in their own thread.

We have three conflicting requirements:

  1. Reviewers want to know when a patch they're reviewing gets updated
  2. Due to technical limitations, when you change code and submit it, you change the entire stack of patches, not just one.
  3. At the same time, developers want to have each revision's notifications in their own respective threads

This gives us three options, each compromising on a requirement:

  1. Don't send emails when code is updated (how will reviewers know that a revision is ready for re-review?)
  2. Thread emails by their revision. Whenever code is updated, send an email for each revision in the stack (so, 100-patch stacks will send 100 emails every time they're updated)
  3. Put all emails for a stack in a single thread. When code is updated, send a single email saying "this stack was updated"

In the future, perhaps we'll be able to update individual revisions in a stack, and specifically email accordingly.
However, this isn't possible in the near term, and out of the three compromises, #3 seemed the strongest

and batch code push emails (so updating a stack doesn't cause too many emails).

IIRC we'll be looking at under a minute of latency. In the future, we could improve this by having clients communicate when they're done a "code push transaction", allowing the email to be sent immediately, but this is out of scope for now because, hey, email is asynchronous, 1 minute isn't bad :)

RRA Arranged for Thursday 12th - if you dont have an invite (and think you should) then let me know :)

Flags: needinfo?(sbennetts)

Future future reference, I have a document of all mockups being updated regularly here

This endpoint was designed with the goal that consumers would need as little information as possible: all needed information is denormalized.
Consumers should not need to make separate queries to Phabricator (or Bugzilla) to fill in additional information.

A couple notes on rationale:

  • It seems noisy to have both PHP type hints and PHPDoc types. This is becase PHPDoc isn't smart with nullables (but PHP type hinting is with "?type").
    On the flip side, PHP type hinting doesn't have support for arrays of a type, while PHPDoc does. I needed both for my IDE to behave as helpfully as possible.
  • It might seem boilerplate-y to have a bazillion classes to represent the data model (as opposed to implicit dictionaries), but I erred on the side
    of this being more maintainable (and failing faster if there's missing properties) as opposed to leaving failures to runtime.
  • Finally, I decided to separate the "Secure<Event>" types from the "<Event>" types. Though, again, there's more boilerplate, it reduces the amount of
    properties that would be unnecessarily nullable/implicit

When we roll out Phabricator emails, we don't want it to iterate and "catch up" on the feed from the beginning. Instead,
we want it to start from the position of the feed when we deploy the service.

Depends on D68265

Adds a patch to re-use the existing Email Delivery setting to configure Mozilla emails

Depends on D69237

Depends on: 1627306

Uses the "created" date from the underlying event to populate the timestamp field

Depends on D69462

Moves more logic above the "COPY ." step, making dev rebuilds significantly faster.
Adds the debug extension to the "test" image to facilitate unit test debugging.

Reduces iterations of "testUniqueAuthTokenGenerated()", it takes too long when xdebug is installed

Depends on D70302

catch (Exception $e) isn't enough to catch type errors in our email event handler. Since we want to handle errors by reporting and skipping them,
we need to catch type errors too. Catching Throwable makes the handler general enough to catch type errors, too.

Also adds ext-json to composer.json as recommended.

Depends on D71682

The existing implementation assumed that each reviewer was an individual user. However, it's possible to assign groups ("projects") as reviewers.
In this case, each user in the group should be emailed.

Note that it's possible for a user to be in more than one reviewing group, and be a recipient multiple times for the same event.
This issue is captured by bug 1632919.

Depends on D72187

See Also: → 1667229

The new email notifier has been implemented!
It hasn't been rolled out to be the default email implementation (1667229 is the tracker for that), but it's possible to opt into the "beta" by doing the following:

  1. Open Phabricator
  2. Click your avatar in the top-right and click "Settings"
  3. Go to the Email Delivery tab on the left
  4. Change "Email Notifications" to "Mozilla Notifications"
  5. Most importantly: if you run into issues, bug me and/or report them to the Phabricator component in Bugzilla
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: