As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 1188236 - "Congratulations on having your first patch approved" email should be clearer about how to get the patch landed.
: "Congratulations on having your first patch approved" email should be clearer...
Classification: Other
Component: Extensions: Other (show other bugs)
: Production
: Unspecified Unspecified
: -- normal (vote)
: ---
Assigned To: Mike Hoye [:mhoye]
Depends on:
  Show dependency treegraph
Reported: 2015-07-27 21:31 PDT by Mark Hammond [:markh]
Modified: 2016-01-28 14:43 PST (History)
5 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---

Improve the information dump (1.55 KB, patch)
2016-01-06 07:52 PST, Josh Matthews [:jdm]
dkl: review+
Details | Diff | Splinter Review
1188236.patch - Rewritten text and updated links for the contributor email template. (3.46 KB, patch)
2016-01-26 11:00 PST, Mike Hoye [:mhoye]
dkl: review-
Details | Diff | Splinter Review
1188236.v2.patch (3.88 KB, patch)
2016-01-28 09:04 PST, Mike Hoye [:mhoye]
josh: feedback+
Details | Diff | Splinter Review
1188236.v3.patch (3.89 KB, patch)
2016-01-28 10:04 PST, Mike Hoye [:mhoye]
josh: feedback+
Details | Diff | Splinter Review
1188236.v4.patch (3.92 KB, patch)
2016-01-28 12:40 PST, Mike Hoye [:mhoye]
no flags Details | Diff | Splinter Review

Description User image Mark Hammond [:markh] 2015-07-27 21:31:13 PDT
Via #firefox, I saw a user confused about the email and what actions they should take.

In particular, there is a link to 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 User image Josh Matthews [:jdm] 2016-01-05 08:19:11 PST
I propose the following change:

Dave, what's required to get this committed to the right place?
Comment 2 User image Byron Jones ‹:glob› 2016-01-05 22:05:46 PST
please attach it as a patch here and ask for review from a suggested reviewer.
Comment 3 User image Josh Matthews [:jdm] 2016-01-06 07:52:51 PST
Created attachment 8704655 [details] [diff] [review]
Improve the information dump
Comment 4 User image David Lawrence [:dkl] 2016-01-06 08:08:55 PST
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 User image Mike Hoye [:mhoye] 2016-01-06 08:46:14 PST
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 User image Mike Hoye [:mhoye] 2016-01-26 11:00:41 PST
Created attachment 8712278 [details] [diff] [review]
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 User image Josh Matthews [:jdm] 2016-01-26 12:30:25 PST
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.

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 User image David Lawrence [:dkl] 2016-01-27 15:20:44 PST
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 User image Mike Hoye [:mhoye] 2016-01-28 09:04:26 PST
Created attachment 8713239 [details] [diff] [review]

OK, let's give this a shot. If git-diff isn't lying to me about whitespace, this should do the trick.
Comment 10 User image Josh Matthews [:jdm] 2016-01-28 09:37:48 PST
Comment on attachment 8713239 [details] [diff] [review]

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

::: extensions/ContributorEngagement/template/en/default/contributor/email.txt.tmpl
@@ +17,5 @@
>  [%+ urlbase %]attachment.cgi?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


@@ +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

Comment 11 User image Mike Hoye [:mhoye] 2016-01-28 10:04:10 PST
Created attachment 8713250 [details] [diff] [review]

Thanks, corrections made.
Comment 12 User image Josh Matthews [:jdm] 2016-01-28 12:35:18 PST
Comment on attachment 8713250 [details] [diff] [review]

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 User image Mike Hoye [:mhoye] 2016-01-28 12:40:28 PST
Created attachment 8713323 [details] [diff] [review]

Fixed here.
Comment 14 User image David Lawrence [:dkl] 2016-01-28 14:43:57 PST

To ssh://
   3ec68d9..95adf55  master -> master

Note You need to log in before you can comment on or make changes to this bug.