The default bug view has changed. See this FAQ.

"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.