1701722 - Phabricator review-request emails are mistakenly sent as "(Secure bug)"

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P2)

Description

[Daniel Holbert [:dholbert]]

This morning I received some phabricator mail for a regular mundane bug (bug 1701645) and patch (https://phabricator.services.mozilla.com/D110112), but the email didn't have any of the details, and instead was written as if it were a secure bug / secure revision.

Specifically, the email looked like this:
Subject:

D110112: (secure bug 1701645)

Email body:

D110112: (secure revision)
Associated with bug 1701645: (secure bug)
🔄 Gijs requested a review 🔬 A review is needed
Reviewers:
dholbert

It seems like we've got some sort of issue with mistakenly classifying bugs/revisions as secure when sending email, sometimes?

(One guess at a possible cause: I do notice that phabricator revisions are hidden (private-only-to-the-author) for a short period of time after they're posted; maybe there's some sort of race condition between exiting that "hidden" phase and the email notifications going out, which results in the email-bot mistakenly thinking that the revision/bug are secure?)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot of the phabricator email I received for this review

Comment 2

[Daniel Holbert [:dholbert]]

Actually, it looks like all (or most?) review requests that I've gotten since Friday March 26th have had this issue.

The first email I see with this issue was for https://phabricator.services.mozilla.com/D109904

Comment 3

[David Lawrence [:dkl]]

Mitch can you take a look a this?

Comment 4

[Mitchell Hentges [:mhentges] 🦀]

Hmm, I'll take a look.

(One guess at a possible cause: I do notice that phabricator revisions are hidden (private-only-to-the-author) for a short period of time after they're posted; maybe there's some sort of race condition between exiting that "hidden" phase and the email notifications going out, which results in the email-bot mistakenly thinking that the revision/bug are secure?)

We only email that a revision's been created once phab-bot has finished checking if it's secure or not, so this shouldn't be the case? I'll dig in.

Comment 5

[Mitchell Hentges [:mhentges] 🦀]

Oh, it's because as of new moz-phab WIP behaviour, it sends a "Needs Review" status, which is parsed before phab-bot sets the secure/insecure status.
Phab Emails shouldn't send "Needs Review" until phab-bot has set the secure/insecure status.

Comment 6

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Screenshot from 2021-03-29 18-59-13.png

Hmm, looking into this a little bit more, that new moz-phab WIP behaviour also breaks the existing Phabricator emails.
If you're on the built-in Phab emails, and someone submits a patch with r?<you>, the regular Phabricator emails tell you that it's a secure revision (see the attached screenshot).

So, to me it appears that the fix here is to adjust moz-phab to not set "Needs Review" on brand new patches.
Zeid, what do you think?

Comment 7

[:glob ✱]

This wasn't an intentional moz-phab change, will fix.

Comment 8

[:glob ✱]

Attachment: Bug 1701722 - Don't explicitly request review on new revisions; r=zeid

Let the Phabricator sort out the correct status for new revisions to avoid
emails being sent before phabbugs has processed the revision.

Comment 9

[:glob ✱]

https://github.com/mozilla-conduit/review/commit/c2024fb55e83964cd904b65e9d1fe0183dbcd0ba