1188236 - "Congratulations on having your first patch approved" email should be clearer about how to get the patch landed.

Status: RESOLVED FIXED

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

Description

[Mark Hammond [:markh] [:mhammond]]

Via #firefox, I saw a user confused about the email and what actions they should take.

In particular, there is a link to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F which IMO is confusing:

* it describes how to format a patch suitable for requesting review - but if the patch has r+, it probably already meets most of those requirements (although I guess it can't hurt to ask them to re-check the points there.)

* it makes no mention of the requirement to then change the patch commit message to indicate exactly who gave it r+.

* it makes no mention of how to get the patch landed - which for most new contributors will be set the checkin-needed flag, and possibly some other process to request a try run be pushed.

Comment 1

[Josh Matthews [:jdm]]

I propose the following change: https://github.com/mozilla/webtools-bmo-bugzilla/commit/214116ae60af230faff2fb27fadf8b6482e3975f.patch

Dave, what's required to get this committed to the right place?

Comment 2

[:glob ✱]

please attach it as a patch here and ask for review from a suggested reviewer.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Improve the information dump

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 8704655 [details] [diff] [review]
Improve the information dump

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

r=dkl. If mhoye approves, I can commit this.

Comment 5

[Mike Hoye [:mhoye]]

I see some problems here.

- The patch links to a doc flagged as "this is outdated, go over here" at the top, but takes you to a tag so far down the page you don't see the warning.
- If you're lucky enough to see the warning sign, the "over here" doc doesn't speak this question at all, and
- I don't think the original cart-and-horse confusion about "does r+ mean ready or what" and "if not what do I do" is clearly answered by either the email (patch or no) or doc. 

tl;dr we have a few moving parts to our communications here and we should fix them all at once.

Josh, I'm taking this bug so I can look at it end to end, and I'll bring the work back to this bug.

Comment 6

[Mike Hoye [:mhoye]]

Attachment: 1188236.patch - Rewritten text and updated links for the contributor email template.

I think the text of this is a bit better. The links work.

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 8712278 [details] [diff] [review]
1188236.patch - Rewritten text and updated links for the contributor email template.

I like a lot of the modifications! I just have a few concerns listed below:

>all the nits are picked out,
I'd tend towards "addressed" instead.

>whiteboard keywords
There are two separate fields - the whiteboard, and the keywords. I think referring to keywords like this is unnecessarily confusing. Also note that only people who have been assigned to a bug or have editbugs privileges can make this change, so it can still be confusing. Also, checkin-needed doesn't work if a successful try push isn't already present, since the sheriffs refuse to push it without one.

>Contributors with commit access
That is almost certainly nobody who is receiving this email. I'm not convinced that linking to this documentation is most effective at this point.

>https://bugzil.la/sw:mentor
Noooo this is looking for bugzilla bugs with "mentor" in the whiteboard somewhere. That's definitely not the best source for this; we can use the bug_mentor field instead.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8712278 [details] [diff] [review]
1188236.patch - Rewritten text and updated links for the contributor email template.

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

Make sure to remove all trailing whitespace in the next revisions. On the next revision, set the r?(josh) so he can r+ it. I will check it in after that if it checks out technically.

Comment 9

[Mike Hoye [:mhoye]]

Attachment: 1188236.v2.patch

OK, let's give this a shot. If git-diff isn't lying to me about whitespace, this should do the trick.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8713239 [details] [diff] [review]
1188236.v2.patch

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

::: extensions/ContributorEngagement/template/en/default/contributor/email.txt.tmpl
@@ +17,5 @@
>  
>  [%+ urlbase %]attachment.cgi?id=[% attachment.id FILTER uri %]&action=edit
>  
> +Once the patch is correctly formatted and all the nits are addressed,
> +you should ask the person who reviewed your patch to push it to a

s/a/the/

@@ +34,2 @@
>  
> +And you can learn about Mercurial for Mozillians here:

"advanced techniques for using Mercurial", maybe? This isn't a great line as it stands.

@@ +57,5 @@
> +You will need someone to 'vouch' for your profile; if you don't
> +know any other Mozillians well, why not contact the person who
> +approved your patch?
> +
> +Thanks again for your help. Mozilla's success depend on contributions

depends

Comment 11

[Mike Hoye [:mhoye]]

Attachment: 1188236.v3.patch

Thanks, corrections made.

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 8713250 [details] [diff] [review]
1188236.v3.patch

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

Sorry, one last important point that I missed previously.

::: extensions/ContributorEngagement/template/en/default/contributor/email.txt.tmpl
@@ +23,4 @@
>  
> +Once it comes back passing all our tests, your patch will be ready
> +to check in!  To get your patch checked in you can add "checkin-needed"
> +to the whiteboard field, and one of our helpful Code Sheriffs will

"to the Keywords field on the bug summary page"

Comment 13

[Mike Hoye [:mhoye]]

Attachment: 1188236.v4.patch

Fixed here.

Comment 14

[David Lawrence [:dkl]]

Committed

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   3ec68d9..95adf55  master -> master