863686 - "Congratulations" email received for first approval+ instead of first review+

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Josh Matthews [:jdm]]

The email that gets sent to new contributors who have their first patch get a positive r+ can apparently be sent at other times. Reuben says he thinks he received it in response to a patch that got approval+, and he's had tons of review+ set on his patches in the past.

Comment 1

[:glob ✱]

this email isn't sent by bugzilla itself; i'll see if i can find the right person/team.

Comment 2

[:glob ✱]

my bad, i forgot about the ContributorEngagement extension.

reuben, can we get a sample of the email in question, with full headers?

reuben's first_patch_approved_id is set to 739511, which definitely looks wrong.

Comment 3

[Reuben Morais [:reuben]]

Attachment: Message

Comment 4

[:glob ✱]

the email which is sent is triggered by the first attachment flag which starts with "approval" being granted.  review flags are not checked.

in reuben's case, his patch on bug 863665 _was_ his first patch to get an approval+ (in this case approval-mozilla-b2g18+), so things are working as designed.


i suspect the usage of flags has changed since bug 721206 was fixed, so we may have to revisit that extension and update the logic and email content.  i'll email david boswell to kick off getting things updated.

Comment 5

[Josh Matthews [:jdm]]

Wait, is that really the case? The original goal of bug 721206 was to have the message sent for the first review+, not any kind of approval flag. Can we fix that ASAP?

Comment 6

[:glob ✱]

(In reply to Josh Matthews [:jdm] from comment #5)
> Wait, is that really the case? The original goal of bug 721206 was to have
> the message sent for the first review+, not any kind of approval flag. Can
> we fix that ASAP?

oh, wow, yeah, it's always been that way :|

i can see how "first patch approved by a reviewer" could be confused with the approval flag.
i'll rework the extension to instead do "first review+'d patch".

Comment 7

[:glob ✱]

Attachment: patch v1

due to the schema change, this will require commit-push-commit-push.  in the first commit i'll include just adding the new field and population; then drop the old field as part of the second commit.

i also fixed the url in the email, however it's rather long now - https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

i've also added "Firefox for Metro" to the list of applicable products, bringing the complete list to:
    "Core",
    "Firefox",
    "Firefox for Android",
    "Firefox for Metro",
    "Mozilla Services",
    "Testing",
    "Toolkit",

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 746443 [details] [diff] [review]
patch v1

Review of attachment 746443 [details] [diff] [review]:
-----------------------------------------------------------------

We have 'superreview' and 'ui-review' flags as well. Should we consider those? Otherwise looks good and works as expected.

r=dkl

Comment 9

[Josh Matthews [:jdm]]

Those other flags are not really relevant to the first-patch experience we're aiming at here.

Comment 10

[timeless]

Comment on attachment 746443 [details] [diff] [review]
patch v1

Review of attachment 746443 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/ContributorEngagement/template/en/default/contributor/email.txt.tmpl
@@ +10,2 @@
>  To: [% to_user FILTER none %]
>  Subject: Congratulations on having your first patch approved

`approved` here and below are moderately confusing. You're talking about a positive review. The patch could actually get a genuine approval flag too...

@@ +11,5 @@
>  Subject: Congratulations on having your first patch approved
>  Date: [% date FILTER none %]
> +X-Bugzilla-Type: contributor-engagement
> +
> +Congratulations on having your first patch approved, and thank you for your

and `approved` here.

Comment 11

[:glob ✱]

jdm, do you have any thoughts on timeless's feedback?

i agree that using 'approval' could be confusing (it confused us!). swapping it out for "positively reviewed" should clarify things.

Comment 12

[Josh Matthews [:jdm]]

I find "approved" to be much more understandable in context here. There's little likelihood for the actual approval flag to be used, so I don't think the overlap between the terms is going to be a problem.

Comment 13

[:glob ✱]

schema-only code committed:

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/ContributorEngagement/Extension.pm
Committed revision 8784.

i will commit the rest and resolve this bug during this week's push.

Comment 14

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/BMO/Extension.pm
modified extensions/ContributorEngagement/Extension.pm
modified extensions/ContributorEngagement/lib/Constants.pm
modified extensions/ContributorEngagement/template/en/default/contributor/email.txt.tmpl
Committed revision 8789.