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

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: Other
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: markh, Assigned: mhoye)

Tracking

Production

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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.
Flags: needinfo?(josh)
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?
Flags: needinfo?(josh) → needinfo?(dkl)
please attach it as a patch here and ask for review from a suggested reviewer.
Flags: needinfo?(dkl)
Created attachment 8704655 [details] [diff] [review]
Improve the information dump
Assignee: nobody → josh
Attachment #8704655 - Flags: review?(dkl)

Updated

a year ago
Attachment #8704655 - Flags: feedback?(mhoye)
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.
Attachment #8704655 - Flags: review?(dkl) → review+
(Assignee)

Comment 5

a year ago
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.
Assignee: josh → mhoye
(Assignee)

Comment 6

a year ago
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 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 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.
Attachment #8712278 - Flags: review-
(Assignee)

Comment 9

a year ago
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.
Attachment #8704655 - Attachment is obsolete: true
Attachment #8712278 - Attachment is obsolete: true
Attachment #8704655 - Flags: feedback?(mhoye)
Attachment #8713239 - Flags: review?(josh)
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
Attachment #8713239 - Flags: review?(josh) → feedback+
(Assignee)

Comment 11

a year ago
Created attachment 8713250 [details] [diff] [review]
1188236.v3.patch

Thanks, corrections made.
Attachment #8713239 - Attachment is obsolete: true
Attachment #8713250 - Flags: review?(josh)
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"
Attachment #8713250 - Flags: review?(josh) → feedback+
(Assignee)

Comment 13

a year ago
Created attachment 8713323 [details] [diff] [review]
1188236.v4.patch

Fixed here.
Attachment #8713250 - Attachment is obsolete: true
Committed

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   3ec68d9..95adf55  master -> master
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.