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...
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: Extensions: Other (show other bugs)
: Production
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: Mike Hoye [:mhoye]
:
:
Mentors:
https://github.com/mozilla/webtools-b...
Depends on:
Blocks:
  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: ---


Attachments
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 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 User image Josh Matthews [:jdm] 2016-01-05 08:19:11 PST
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 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.

>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 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]
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 User image Josh Matthews [:jdm] 2016-01-28 09:37:48 PST
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 User image Mike Hoye [:mhoye] 2016-01-28 10:04:10 PST
Created attachment 8713250 [details] [diff] [review]
1188236.v3.patch

Thanks, corrections made.
Comment 12 User image Josh Matthews [:jdm] 2016-01-28 12:35:18 PST
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 User image Mike Hoye [:mhoye] 2016-01-28 12:40:28 PST
Created attachment 8713323 [details] [diff] [review]
1188236.v4.patch

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

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   3ec68d9..95adf55  master -> master

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