Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Email all individual owners when a new version is published #1895

Open
carols10cents opened this issue Nov 14, 2019 · 5 comments
Open

Email all individual owners when a new version is published #1895

carols10cents opened this issue Nov 14, 2019 · 5 comments

Comments

@carols10cents
Copy link
Member

@carols10cents carols10cents commented Nov 14, 2019

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:

rubygems.org email screenshot

  • 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:

rubygems.org preferences page screenshot

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.

  • Backend: email preference storing (addressed in #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.
  • Frontend: settings (addressed in #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

This comment has been minimized.

Copy link
Contributor

@DSpeckhals DSpeckhals commented Nov 16, 2019

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

This comment has been minimized.

Copy link
Member Author

@carols10cents carols10cents commented Nov 18, 2019

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

@DSpeckhals

This comment has been minimized.

Copy link
Contributor

@DSpeckhals DSpeckhals commented Nov 18, 2019

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

This comment has been minimized.

Copy link
Member Author

@carols10cents carols10cents commented Nov 21, 2019

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 :)

bors added a commit that referenced this issue Nov 22, 2019
…cents

backend: Add user email preference storing

This PR addresses the first step in implementing #1895: "Backend: email preference storing."

This commit is the first step in the process to send crate owners an email notification when a new version of one of their crates is published. A database migration adds a `email_notifications` column to the `crate_owners` table, and thus the property was added to the corresponding `CrateOwner` struct. This new property is defaulted to `true`.

Because a user may not want to receive a version publish notification for all of their crates, an API endpoint was added to allow them to toggle email notifications for each crate. The front end implementation will be in a forthcoming commit, as well as the actual sending of these notifications.
bors added a commit that referenced this issue Dec 5, 2019
…10cents

front-end: Add email notification preferences

This commit adds the front-end capabilities to coincide with the API endpoint that was built on the server. There's also a small bug fixed in the api-token-row; it was throwing an error every time the "/me" route was loaded.

This PR addresses the second item in #1895.

I like how the UI ended up, but it may very well be creators bias 😄. If this doesn't work, then I'll be happy to iterate!

### Screenshot of a crate with notifications on and one that's off
![image](https://user-images.githubusercontent.com/3310769/69435559-c6d7f880-0d0d-11ea-9d30-0b38a4fc7cfb.png)
@DSpeckhals

This comment has been minimized.

Copy link
Contributor

@DSpeckhals DSpeckhals commented Dec 5, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.