721206 - Have Bugzilla send email to people who have had their first patch approved by a reviewer

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[David Boswell]

We would like to be able to send an email to people who have just had their first patch approved by a reviewer.  This is a key moment in the progression of a new contributor and there are a number of things we can communicate:

* Congratulations on the achievement and appreciation for the contribution

* Details about how to get their information added to the Credits page

* Details about how to sign-up to the Contributor Directory

* Information about what to do next (making sure the patch is checked in, finding other bugs, etc.)

We can provide the text of the email as an attachment if it is possible to have Bugzilla do this.

For reference, this idea came out of discussions from the Coding Contribute Group.

https://wiki.mozilla.org/Stewards/Coding

Comment 1

[:glob ✱]

yes, this should be possible; please attach the email content.

can you also provide a list of products where first-r+ should trigger this email.

Comment 2

[David Eaves]

+1 This is an awesome bug. Nice work David B. Not sure if I can help, but let me know if I can.

Comment 3

[David Boswell]

(In reply to David Eaves from comment #2)
> +1 This is an awesome bug. Nice work David B. Not sure if I can help, but
> let me know if I can.

Thanks for the offer to help.  Reviewing the draft email when we have it ready and providing feedback would be great.

Comment 4

[David Boswell]

(In reply to Byron Jones ‹:glob› from comment #1)
> can you also provide a list of products where first-r+ should trigger this
> email.

In theory could we send different text according to the products (so someone submitting a webdev patch would get webdev info and a Firefox patch would get Firefox info)?

If not, we could write a more general email.  If so, it could be worth exploring more specific content.

Comment 5

[David Lawrence [:dkl]]

Sure we could have a template that has common information inside for all emails sent such as headers, etc. and then have a custom format template based on product name for each product.

So what we would need is the email content for each product or variation and then we can break out the common bits and create our templates for BMO use.

Dkl

Comment 6

[David Boswell]

Re comment #5: cool :)

I'll work with the Coding Stewards and Webdev Stewards on some draft text and will post for review.  It seems like those are good places to start and it may make sense to roll this out to more products later.

Comment 7

[David Boswell]

I set up an etherpad with some draft text for an email that we can all work on at:

https://etherpad.mozilla.org/patch-approved-bugzilla-email

Feel free to edit things there or make suggestions (too much content, too little, etc).

A couple thoughts:

* To David's point in comment #5, much of this seems like it would fit in a template (the intro and also the info about the Contributor Directory).  Next steps information and where to go for discussions probably varies across teams.  For instance, webdev probably would want their own next steps about getting patches checked in.

* For the list of products to associate the emails to, I'll defer to the Stewards from the different teams but I put a couple suggestions in to get things started.

* Would we want to set the reply-to address to the associated Stewards so they could have a conversation with people?  You could then end the emails with a line like 'Let me know if you have any questions about this'.

Comment 8

[David Boswell]

Attachment: Patch approved email text for coding products

Here is the text to use for the first patch approved email for the following products: core, firefox, toolkit, fennec, testing, mozilla services.

Comment 9

[David Eaves]

Comment on attachment 595497 [details]
Patch approved email text for coding products

>subj: Congratulations on having your first patch approved
>
>Congratulations! Your first patch has been approved! 
>
>First, as a project that relies on a global army of volunteers we're excited you took the time to submit this patch. Second, you're probably asking yourself... what happens next?  
>
>The next step is to get the patch actually checked in to our repository.  To learn more about how to make that happen, check out this post:
>
>http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
>
>A great way to get familiar with Mozilla is to help out on one of our mentored bugs (bugs where someone is specifically available to help you). You can find a list of them here:
>
>https://bugzil.la/sw:mentor
>
>Alternatively, you could join us on our IRC chat server in the "#introduction" channel and ask for suggestions about good bugs to work on. There's more about using our chat server at:
>
>http://irc.mozilla.org/
>
>If you haven't already, consider creating a profile for yourself in the Mozilla Contributor Directory.  This will make it easier to reach out and connect with other Mozillians. You will need someone to 'vouch for' your profile; if you don't know any other Mozillians... why not contact the person who approved your patch?
>
>The directory is here:
>
>https://mozillians.org/
>
>Thanks you again for your help :-)
>
>Josh, Kyle, Dietrich and Brian; Coding Stewards

Comment 10

[David Eaves]

Bonardo.net appears to be down (Sunday Feb 12 @ 11:30pm PST) not sure if this is a temporary issue or not, but it obviously a bigger problem if it is not.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think we should move the blog post to MDN anyways.

Comment 12

[:glob ✱]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #11)
> I think we should move the blog post to MDN anyways.

i agree; the text of the blog is still visible in the google cache (http://tinyurl.com/843jetf)

Comment 13

[Josh Matthews [:jdm]]

I have copied the contents of the post (with minor grammatical and formatting fixes) to https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in. Can we use that link and get this change in production?

Comment 14

[Ed Morley [:emorley]]

(In reply to Josh Matthews [:jdm] from comment #13)
> I have copied the contents of the post (with minor grammatical and
> formatting fixes) to
> https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in.
> Can we use that link and get this change in production?

See also https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Committing_the_patch - suspect the two could be combined?

Comment 15

[Josh Matthews [:jdm]]

Yes, there is definitely work that can be done. However, I'm interested in not delaying this further - we can always modify the wiki content at the link, or even redirect it to a different page, but right now we're not changing the status quo by using the existing content.

Comment 16

[David Lawrence [:dkl]]

Attachment: WIP Patch to send email to patch author on first approval (v1)

This is a WIP patch I wanted to get up for review. Couple things I am still figuring out that you may be able to comment on.

1. Should I move the products/flags to params pages so we can add/remove using the admin UI or is it sufficient for now to have them in constants?

2. Trying to figure out migration. Simple way would be to go through all patches that have an approval set to + and go ahead and mark those authors as having an email sent. This would catch the attachments that had a previous approval+ that was removed and then added back. But probably doesn't matter in that case. Does this sounds sufficient?

Thanks
dkl

Comment 17

[David Lawrence [:dkl]]

Also this patch does not yet implement per product emails and just has the one that was attached to this bug. Do we still need to have an email tailored specifically for each product or can we just use one universal email?

dkl

Comment 18

[:glob ✱]

Comment on attachment 606369 [details] [diff] [review]
WIP Patch to send email to patch author on first approval (v1)

Review of attachment 606369 [details] [diff] [review]:
-----------------------------------------------------------------

i was thinking about the name of this .. how about naming it "ContributorEngagement" rather than just "Contributor"?

> 1. Should I move the products/flags to params pages so we can add/remove
> using the admin UI or is it sufficient for now to have them in constants?

i'm happy to leave them as a constant.  both of those changes will most probably be accompanied by changes to the email content (or new emails), so i don't think we'll gain anything by moving it into the db.

> 2. Trying to figure out migration. Simple way would be to go through all
> patches that have an approval set to + and go ahead and mark those authors
> as having an email sent. This would catch the attachments that had a
> previous approval+ that was removed and then added back. But probably
> doesn't matter in that case. Does this sounds sufficient?

that sounds good.

::: extensions/Contributor/Extension.pm
@@ +21,5 @@
> +our $VERSION = '1.0';
> +
> +BEGIN {
> +    *Bugzilla::User::first_patch_approved = \&_first_patch_approved;
> +}

_first_patch_approved hasn't been defined, resulting in an error when you approve patches.

@@ +29,5 @@
> +    my $dbh = Bugzilla->dbh;
> +
> +    if (!$dbh->bz_column_info('profiles', 'first_patch_approved')) {
> +        $dbh->bz_add_column('profiles', 'first_patch_approved',
> +            {TYPE => 'INT3', NOTNULL => 1, DEFAULT => 0});

rather than a boolean field, i think it would be better to make this a null-able attachment_id field.

then we can add bling visible on the bug page similar to how tag-new-users identifies a user's first patch.

@@ +47,5 @@
> +    my ($object, $timestamp, $new_flags) = @$args{qw(object timestamp new_flags)};
> +
> +    if (!$object->isa('Bugzilla::Attachment')
> +        || !@$new_flags 
> +        || !grep($_ eq $object->bug->product, ENABLED_PRODUCTS))

i think this will read clearer as:

unless ($object->isa('Bugzilla::Attachment') 
        && @$new_flags 
        && grep($_ eq $object->bug->product, ENABLED_PRODUCTS)
        && !$object->attacher->first_patch_approved
) {

note i've also moved the first_patch_approved test here as we can test that before checking the flags.

@@ +61,5 @@
> +        $change =~ s/\([^\)]+\)$//; # get rid of requestee
> +        my ($name, $value) = $change =~ /^(.+)(.)$/;
> +
> +        # Only interested in approval*+ flags
> +        next if !grep($_ eq $name, ENABLED_FLAGS) || $value ne '+';

check the $value first, it's cheaper than doing a grep.

@@ +66,5 @@
> +        
> +        if (!$attachment->attacher->first_patch_approved) {
> +            _send_approval_mail($attachment->attacher, $timestamp);
> +            last;
> +        }

see previous note about doing the attachment->first_patch_approved check earlier.

::: extensions/Contributor/lib/Constants.pm
@@ +26,5 @@
> +    "TestProduct", 
> +);
> +
> +use constant ENABLED_FLAGS => (
> +    "approval-test"

change this to an array of regexes, so we can do qr/^approval/ instead of having to list out every approval flag.

::: extensions/Contributor/template/en/default/contributor/email.txt.tmpl
@@ +5,5 @@
> +  # This Source Code Form is "Incompatible With Secondary Licenses", as
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +[% PROCESS "global/variables.none.tmpl" %]
> +From: [% Param('mailfrom') %]

we should probably have a better sender for the mail than bugzilla-daemon; if someone replies it should go to someone who can help then :)

something like a distro list for contributor engagement?

@@ +16,5 @@
> +
> +The next step is to get the patch actually checked in to our repository.  
> +For more information about how to make that happen, check out this post:
> +
> +http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

@@ +43,5 @@
> +https://mozillians.org/
> +
> +Thanks again for your help :-)
> +
> +Josh, Kyle, Dietrich and Brian; Coding Stewards)

stray closing parenthesis.

Failed test 'extensions/Contributor/template/en/default/contributor/email.txt.tmpl contains invalid bare words (e.g. 'bug')

Comment 19

[Josh Matthews [:jdm]]

I would be fine with my email being used for the reply-to.

Comment 20

[:glob ✱]

(In reply to Josh Matthews [:jdm] from comment #19)
> I would be fine with my email being used for the reply-to.

thanks josh, however i think it would be better to have replies going to a small distribution list rather than a single person, in case you're unavailable to answer questions due to holidays/sickness/...

Comment 21

[David Lawrence [:dkl]]

Attachment: Patch to send email to patch author on first approval (v2)

EMAIL_FROM constant can be changed before commit to appropriate address.

dkl

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to send email to patch author on first approval (v3)

Fixed a couple of issues as discussed in our phone conversation.

dkl

Comment 23

[David Boswell]

(In reply to David Lawrence [:dkl] from comment #17)
> Also this patch does not yet implement per product emails and just has the
> one that was attached to this bug. Do we still need to have an email
> tailored specifically for each product or can we just use one universal
> email?

Ideally we'd be able to tailor the message since the contribution path is different for different areas.  If that isn't possible though, I think there is some commonality that we could use for a more generic email.

Comment 24

[David Lawrence [:dkl]]

We can still do this if needed. Is there a way we can have someone decided on the wording per product, attach to this bug so I can figure out the best way to generate the proper templates?

If the differences between the different products are small I can just use one template as I do now and just put some conditionals inside the template to show different text based on product.

dkl

Comment 25

[David Boswell]

(In reply to David Lawrence [:dkl] from comment #24)
> We can still do this if needed. Is there a way we can have someone decided
> on the wording per product, attach to this bug so I can figure out the best
> way to generate the proper templates?

Sure.  I think this is something that the person in charge of community building planning for each functional area can take on.  For reference:

https://wiki.mozilla.org/Contribute#Functional_Areas

As a start, if you want one other example email quickly, we can ask Luke (who is copied on this) what a webdev-focused email would look like compared with the coding-focus one that's attached.

Comment 26

[Josh Matthews [:jdm]]

The problem with using webdev as another source is that most of their work appears to happen in github these days.

Comment 27

[:glob ✱]

Comment on attachment 607286 [details] [diff] [review]
Patch to send email to patch author on first approval (v3)

r=glob

Bugzilla::Extension::Contributor ==> Bugzilla::Extension::ContributorEngagement

please submit a new patch fixing the package and extension name to match the directory name; you can carry forward my r+ onto this patch.


let's deal with emails for other products in followup bugs.

Comment 28

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #27)
> Bugzilla::Extension::Contributor ==>
> Bugzilla::Extension::ContributorEngagement

crap :)

> please submit a new patch fixing the package and extension name to match the
> directory name; you can carry forward my r+ onto this patch.
> 
> 
> let's deal with emails for other products in followup bugs.

agreed.

dkl

Comment 29

[David Lawrence [:dkl]]

Attachment: Patch to send email to patch author on first approval (v4)

Fixed module naming everywhere. Carrying forward r+. We will need to keep this bug open as we need to schedule this to be pushed during a BMO outage along with some other changes.

dkl

Comment 30

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
added extensions/ContributorEngagement
added extensions/ContributorEngagement/Config.pm
added extensions/ContributorEngagement/Extension.pm
added extensions/ContributorEngagement/lib
added extensions/ContributorEngagement/template
added extensions/ContributorEngagement/lib/Constants.pm
added extensions/ContributorEngagement/template/en
added extensions/ContributorEngagement/template/en/default
added extensions/ContributorEngagement/template/en/default/contributor
added extensions/ContributorEngagement/template/en/default/contributor/email.txt.tmpl
Committed revision 8107.

Comment 31

[:glob ✱]

this is now live.

Comment 32

[David Boswell]

Very exciting.  Thanks to everyone for this.

Will be interesting to see how this improves the experience (I guess we should think through how to measure this -- number of emails sent, number of people who go on to next step, etc.)

Comment 33

[David Eaves]

+1

Is there any way we can look at data around this? Perhaps look at conversion rates for the 6 months before this patch went live vs. the 6months after? And/or maybe have a test group that doesn't get the email.

Too much?

Comment 34

[David Boswell]

(In reply to David Eaves from comment #33)
> Is there any way we can look at data around this? Perhaps look at conversion
> rates for the 6 months before this patch went live vs. the 6months after?
> And/or maybe have a test group that doesn't get the email.

Let's move the discussion about looking at data around this to bug 736109 where we're looking at adding conversion information to the Get Involved dashboard.  If we add having someone's first patch approved as a conversion point, we can see what the conversion rate was before and after this change was put in place.