crates.io

crates.io copied to clipboard

I believe this should complete the last task in #1895

A few notes on the implementation:

• I decided to write a custom query for this task, since listing owners and then retrieving the email one by one didn't feel clean and would have been an N+1 query
• I didn't use an HashSet to deduplicate emails, instead I used the DISTINCT keyword in the SQL query
• I had to change the db_new_user implementation to generate a different email based on the username, otherwise I wouldn't be able to test if all of the owners would be counted in by owners_with_notification_email
• ~~I put all emails in the To header since crate owners are public anyway~~

I manually tested this locally by setting this to true.

Rendered email:

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @carols10cents (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Thanks for the PR @paolobarbolini! I haven't had a chance to review the code in-depth, but I have a few initial points I would like to raise.

I put all emails in the To header since crate owners are public anyway

The verified email address is not made publicly available, so I think we should hide email addresses from other recipients.

There are also some aspects that the team needs to consider from an ops perspective:

• This will increase our outbound email volume. We should calculate estimates (based on average volume of crate publishes * average count of crate authors) to determine if we will need to upgrade our plan.
• We currently only send an email for verification and for owner invites. The additional volume may mean that we need to keep a closer eye on bounce rates and have processes in place to handle/disable addresses that become invalid.

(Our agenda for this week is already pretty busy, but I hope to raise these points in a meeting soon.)

The verified email address is not made publicly available, so I think we should hide email addresses from other recipients.

Ah, right. I'll change it in the following days.

When it comes to bounces, it could probably be handled with a Mailgun Webhook?

Ah, right. I'll change it in the following days.

Changed it. While I was at it I rebased it to the master branch.

:umbrella: The latest upstream changes (presumably #2962) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Rebased on the latest master

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

:umbrella: The latest upstream changes (presumably #3062) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@paolobarbolini sorry for the long radio silence on this topic. we've discussed this in the team meeting today and we'd love to see an MVP of this merged in the near future.

I haven't looked at the implementation yet in detail, but one question that came up was how we would handle owners that are GitHub Teams. do you have any thoughts on this topic? for the MVP implementation we could consider sending the emails only to regular user owners for now.

would be great if you could also get this PR rebased. I hope the conflicts are not too bad 😞

Rebased

one question that came up was how we would handle owners that are GitHub Teams. do you have any thoughts on this topic?

It looks like the GitHub API does expose the organization contact email for organizations with a public contact email, so it could probably be backfilled, at least for some of them. Here's an example:

• https://api.github.com/orgs/rust-lang hidden contact email
• https://api.github.com/orgs/lettre public contact email

The biggest issue will probably be, other than the fact that not all organizations have a public email address, with letting organizations unsubscribe from notifications emails, since as far as I know you can't login using the organization account.

:umbrella: The latest upstream changes (presumably #3483) made this pull request unmergeable. Please resolve the merge conflicts.

I squashed all of the previous commits and rebased. #3483 made it so that the email configuration is parsed at startup, so in order to be able to send emails from the background job, I made a separate commit which adds Emails to Environment

Is there any work being done on this PR, @paolobarbolini ?

Is there any work being done on this PR, @paolobarbolini ?

I haven't kept up with the rebasing but other than that either there hasn't been any feedback or there are things like monitoring bounces which should be done separately.

@paolobarbolini I've tried to rebase this PR over the weekend, but with all the index syncing changes it was a bit harder than I had hoped. I still used your PR as a starting point for https://github.com/rust-lang/crates.io/pull/9341, so thank you for working on this, and sorry again that it took so long for this to land :)