Closed Bug 1188236 Opened 4 years ago Closed 4 years ago

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


( :: Extensions, defect)

Not set





(Reporter: markh, Assigned: mhoye)





(1 file, 4 obsolete files)

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.
Flags: needinfo?(josh)
I propose the following change:

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)
Attached patch Improve the information dump (obsolete) — Splinter Review
Assignee: nobody → josh
Attachment #8704655 - Flags: review?(dkl)
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+
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
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.

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-
Attached patch 1188236.v2.patch (obsolete) — Splinter Review
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]

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

Attachment #8713239 - Flags: review?(josh) → feedback+
Attached patch 1188236.v3.patch (obsolete) — Splinter Review
Thanks, corrections made.
Attachment #8713239 - Attachment is obsolete: true
Attachment #8713250 - Flags: review?(josh)
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"
Attachment #8713250 - Flags: review?(josh) → feedback+
Attached patch 1188236.v4.patchSplinter Review
Fixed here.
Attachment #8713250 - Attachment is obsolete: true

To ssh://
   3ec68d9..95adf55  master -> master
Closed: 4 years ago
Resolution: --- → FIXED
Component: Extensions: Other → Extensions
You need to log in before you can comment on or make changes to this bug.