Email all individual owners when a new version is published

[carols10cents]

trafficstars

Extracted from #815

Now that we've been requiring verified email addresses for publishing, it's more likely this feature will work well.

This is a big task that's more complicated than it looks! Please ask questions if anything is confusing! This task would be great for someone looking to get experience with just about every part of crates.io: the frontend, the backend API, the database, email sending.

What should happen:

• When any owner of a crate (either by being an individual owner or having permission through a team) publishes a new version of a crate they own,

• Email all individual owners of that crate that have verified email addresses to notify them of the new version

  • We could email all team owners as well, but we'd have to query GitHub for that each time and try to match up team members to crates.io accounts, seems complex. Let's start with individual owners.
  • We could email the person who published the crate as well if they have permissions because of a team, so that they're notified more quickly if they didn't authorize that publish with their credentials. However, there'd be no way to turn this email off because we don't have a record on which to store that preference? I'm torn about whether we say "the person who did the publish always gets an email even if you've turned notifications off" or whether we say "people who have permissions via teams never get emails even if they're the one who published the crate". Open to arguments.
  • The email should include the name of the crate, the version published, who published it, the date and time they published it, instructions on what to do if the publish wasn't authorized, and a link to the page to opt out of the emails. Here's an example of an email from rubygems.org:

• Let each owner of a crate opt out of getting these notifications for each crate they're an individual owner of. Rubygems.org has this preference page:

Implementation instructions

Roughly trying to separate these into pieces that can be submitted and reviewed as separate PRs; let me know if you run into any problems with the way I've split the tasks up.

• [x] Backend: email preference storing (addressed in https://github.com/rust-lang/crates.io/pull/1901)

  • Create a migration (search for diesel migration generate in this guide) that adds a column to the crate_owners table... this isn't great because teams are also stored in this table and teams don't have emails, but I think this is the best spot for now. Name the column something like email_notifications, of type boolean, and set the default to true. In the database dump configuration, set this new column to private (we shouldn't include this value in public database dumps).
  • Create a function that updates the email notifications field with a given value; it'll look something like the owner_remove function.
  • Create a new API route in the router that's a PUT request to /me/email_notifications or similar and calls a new controller function that I think would fit in the user/me controller, called update_email_notifications or similar
  • That controller should get a list of crate IDs and on/off values from radio buttons in the frontend about whether the current user wants to get email notifications for that crate or not. Iterate through the crate IDs, fetch any crate owner records for the current user and the given IDs (ignore IDs for crates that the current user doesn't actually own), and update the email_notifications column of those records with the values sent. Updating the values unconditionally is fine, I think. Ignore any crates that the current user owns that we didn't get any radio button value for.
  • So whether you have email notifications for a crate you own on or off should be private information. That makes me inclined to include this information in with the GET /me request and the encodable_private response that uses the EncodablePrivate struct. Minimally, this should be a vector of (crate name, crate id, email notification value) tuples, I think. Enough to display the form with the radio buttons described below.
  • Write tests for the new update controller action and the new info in the /me response in with the other user tests.

• [x] Frontend: settings (addressed in https://github.com/rust-lang/crates.io/pull/1913)

  • Given that I think we should include the email setting values in with the /me API request, I think the frontend should add a new section to https://crates.io/me whose HTML is here, as opposed to creating a whole separate page for email notifications.
  • This section should list all crates the current user owns as listed in the API response. My ember knowledge is still lacking, so I'm not sure how to describe how to do this the right way.
  • There should be a form with radio buttons for each crate id and whether emails are on/off, and a button to save all the settings that sends the radio button values to the backend /me/email_notifications controller described above.
  • If there's a good way to test this in the ember tests, please add some. I don't think there's going to be too much frontend logic though?

• [ ] Backend: email sending (no PR yet)

  • After the crate has been successfully added to the index, call a new notify_owners function. So use ? on repo.commit_and_push, and instead return the result from notify_owners in this add_crate function.
  • In add_crate, the Crate parameter is of this type, which currently doesn't have the owners available on it, nor should it because that is what gets serialized into the index. The add_crate function doesn't currently use a database connection. Therefore, I'm thinking add_crate should have a new parameter that's a vector of email addresses. Then add_crate can call notify_owners with the email addresses, the krate.name, krate.vers, and chrono::Utc::now().
  • So in the publish controller, before git::add_crate is called, we'll need to get the unique set of verified email addresses of the named owners plus the verified email address of the person who published the crate, if we decide we want to do that (see bolded bullet point at the top where I'm conflicted about this). We already have the set of crate owners available, which is a Vec of Owners. So iterate through the owners, and for user owners, if their email_notifications value is true (the new column described above), collect the present verified email addresses into a HashSet (to get unique ones) and (maybe, see above) add the publisher's verified email address.
  • The new notify_owners function will probably look like try_send_user_confirm_email and can go in that file. Use the screenshot above of the rubygems email as a general guide for what should be in the email, and ask questions here if you have any.
  • We don't have a great way to write automated tests for emails sent from background jobs, but do write tests for gathering the email addresses of the owners.

[DSpeckhals]

Thank you for making the implementation instructions so clear and easy to follow! I just put up a PR for the first point described here.

On another note, would it be possible to make these points a check box to clarify to those just coming here what's been completed and what's left to do? Thanks a lot!

[carols10cents]

Good call! I added check marks and checked off the one you're working on, thank you!

[DSpeckhals]

Is there a strong preference here of using a radio input vs. a checkbox input? On first thought, it seems like a checkbox would be a little better...mainly because it's just a boolean value.

[carols10cents]

Is there a strong preference here of using a radio input vs. a checkbox input? On first thought, it seems like a checkbox would be a little better...mainly because it's just a boolean value.

I'm not sure! I'll admit that I'm mostly copying rubygems' interface, which we don't have to do if we don't want to :)

[DSpeckhals]

The last step for this (actually sending the email) looks like it's the last thing left to do now. Unless someone else is already on it, I'll see if I can get it done soon. The sooner the better because the associated profile settings are already visible to users.

[MightyPork]

Came to the bugtracker to report that the e-mails are not sent, but ... it's just not implemented?

(Is this section in the settings what this issue is about?)

[liangyuanpeng]

I just got the error when i verify my email address.

Maybe my email not your scope, i use 163 email. And i change email to gmail, but still send message to 163 email.

---

Verify my email address from gmail, still wrong.

[jtgeibel]

@liangyuanpeng it looks like you have a currently verified gmail addresses for your account. If you're still having issues, please open a separate issue so that this doesn't get lost. Thanks.

[mdtro]

Hi @carols10cents! I'd like to pick this one up to finish the last remaining task of sending out email notifications. 😄 I'm excited to dig in on it this weekend. It's been awhile since this issue has had some activity. Are you still a good point of contact/mentor for this issue?

I did notice email_notifications has moved to the CrateOwner now. To get to these details, what would be your recommended approach?

I was going to attempt to implement a function on User similar to owning to return a boolean depending on if the user had email_notifications enabled for the particular crate. Thoughts?

[paolobarbolini]

See #2705 @mdtro

[mdtro]

See #2705 @mdtro

Ah thanks, @paolobarbolini! Sorry I missed that PR while I was reading through the issue.

[0xSaksham]

The service is still not available, I would love to work on it/

[paolobarbolini]

There's still #2705, although it requires a rebase

[0xSaksham]

I'll take a look. Thanks!