Send email on crate owner invitation

[carols10cents]

If there's an email address for the person being invited (which we don't require yet)

It'd be awesome if the email included a one-click link to accept the invitation without logging in (so with a token like the email verification works now)

Related:

• We shouldn't let you add someone as an owner without their consent #924 (the owner invitation system so far)
• Add some sort of notification that you have invitations to own a crate #1116 (showing a notification on crates.io)

Instructions

Invitations get created in the krate.owner_add method.

Currently, if you add someone as an owner multiple times, it just updates the one existing invitation record. We don't want to send multiple emails in this situation, as this would be an easy vector for spamming someone.

You can tell whether a row was inserted or not when you say on conflict do update by adding a returning method call to request that the query returns some of the data, getting the result with the get_result method, and calling the optional method to tell diesel it's ok if it doesn't get a result. If the query did nothing (because that record already existed), nothing will be returned and this diesel statement will result in a None value. Here's an example where we do this:

So after creating an invitation, and only if that invitation record was created (rather than only updated because it already existed), send the invited user an email.

Get the email address by first getting the User (owner here is an Owner::User, change the underscore in that match pattern to a variable name to be able to use the inner User that Owner::User holds)

Get the email address for that User NOT through the email field; that's left over from before we verified email addresses. Instead, use the verified_email method. If the user doesn't have a verified email address, don't send the email.

Send the email by calling a new function you'll need to create in src/email.rs. The new function should swallow all errors like send_user_confirm_email does by using let _ =, but should call send_email like try_send_user_confirm_email does. I think we only need one function in the case of sending owner invitation emails, rather than the two functions we have for verifying your email address, because we always want creating the invitation to succeed even if we can't send an email about it.

I would merge a PR that sends an email that always has the same content; something along the lines of "You've been invited to become the owner of a crate! Please visit https://crates.io/me/pending-invites to accept or reject this invitation."

The enhancements I could see making to that email would be:

• Including the name of the crate you're being invited to own
• Adding the username of the person who sent the invite
• Adding a URL with a randomly-generated token (that we'd need to store in a new column on the invitations table) so that you could click on that link to accept the invitation (this would need to be a new URL and controller action added)

but I would rather have a PR with the basic implementation of this feature than no PR.

We don't have a great way to write tests that check emails are sent :-/ You should be able to test this out locally; the emails will be "sent" to a file in /tmp/.

[Andy-Bell]

Hi, I would be willing to have a look at this if you want, but may need a bit of help with it.

Thanks

[joshtriplett]

Several people have observed this, and people have the impression that there are such emails. At the very least until there is a mechanism for this the UI needs an update to explicitly say "there's no notification, let the proposed owner know yourself, here's the link to send them".

[arshiamufti]

I'm currently working on this! Planned behaviour (after discussion with @carols10cents) is to dispatch the email in krate.rs and allow this operation to silently fail. That is, adding an owner can succeed even if sending the invitation email fails.

[nkanderson]

@carols10cents I'm working through the initial changes to the query in krate.owner_add, and have something that seems to be working, but diverges from the implementation you have outlined above.

The following returns None if the entry already exists, and Some(CrateOwnerInvitation){...} if it's a new entry:

let maybe_inserted = insert_into(crate_owner_invitations::table)
                    .values(&NewCrateOwnerInvitation {
                        invited_user_id: owner.id(),
                        invited_by_user_id: req_user.id,
                        crate_id: self.id,
                    })
                    .on_conflict_do_nothing()
                    .get_result::<CrateOwnerInvitation>(conn)
                    .optional()?;

I was attempting to chain on_conflict, do_update, and returning, but it seemed that do_update required a call to set for at least a nominal update, and as far as I could tell, returning was conflicting with get_result (perhaps in the CrateOwnerInvitation type in get_result?).

Assuming the above query is working as I've described, does that seem alright?

[carols10cents]

@nkanderson whoops! You're right-- I wrote do update but this spot doesn't do an update, it does nothing. So yes, I think what you have is correct!

[nkanderson]

Hi @carols10cents, I'm still chipping away at this one, but had a quick question for you - the first bit of this (send email with basic info) was pretty straightforward, and I think I could open a PR for it. Should I go ahead and do that? I'm up for continuing the work to implement the token part of it as well, but don't want to block on the improvement of having any email sent. If you'd rather review it all at once though, I can keep going and open a PR with the whole thing once it's ready.

[carols10cents]

@nkanderson Opening a PR for a smaller part is fine with me! Please do! :)