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

[nkanderson]

I'm getting into the second phase of this work, and wanted to check in about a couple things and make sure I'm on the right track:

• I'm assuming the user would not need to be logged in to confirm the invite via the token. As such, I was thinking the API endpoint for the token-based confirmation could be something like /confirm/crate_ownership_invitation/:token, under the confirm namespace rather than under me. Does that make sense? Some proof-of-concept work indicates this shouldn't conflict with the existing /confirm/:email_token (presumably unless an email token with the value crate_ownership_invitation is generated 😄).
• Similarly, for the ember / web part, I was thinking of creating a route along the lines of /confirm-invite/:token, and following prior art for the email verification. Seems like the message could indicate success or failure of confirmation, and link to the pending-invites page as the place to manage all invitations.

Does that seem like a reasonable approach? Should any of that be adjusted (like route names, etc.)?

[carols10cents]

* I'm assuming the user would not need to be logged in to confirm the invite via the token. As such, I was thinking the API endpoint for the token-based confirmation could be something like `/confirm/crate_ownership_invitation/:token`, under the `confirm` namespace rather than under `me`. Does that make sense? Some proof-of-concept work indicates this shouldn't conflict with the existing `/confirm/:email_token` (presumably unless an email token with the value `crate_ownership_invitation` is generated 😄).

Hm, I think as someone working on the codebase, I would expect all the crate ownership invitation URLs to be grouped together (that is, have the same prefix) rather than all the token confirmation URLs to be grouped together.

Looking at the URLs and what else is under /me/, most of the stuff in the menu in the upper right hand side is under /me/, except for the email token confirmation and the email confirmation resend. Those probably should have been under /me/, especially because the functions that get called for them are in the user::me module :)

And this is also kind of nitpicky and not super important, but I would describe the action we're talking about adding here as "accepting" an invitation, rather than "confirm" like the email confirming.

What do you think about /me/crate_owner_invitations/accept/:token? I don't feel super strongly about it though.

I'd also accept a separate pull request to move the confirm email and resend email routes under /me/ if you'd like :)

* Similarly, for the ember / web part, I was thinking of creating a route along the lines of `/confirm-invite/:token`, and following prior art for the email verification. Seems like the message could indicate success or failure of confirmation, and link to the pending-invites page as the place to manage all invitations.

Similarly, I think /accept-invite/:token would match my mental model a bit better, but I don't feel super strongly about it. The email verification should be a good model to follow, please ask questions if anything doesn't make sense!

I'm thinking about what I'd like to do next after successfully accepting an invitation to own a crate... would I want to see if I had any other invitations to accept? Would I want to go to the page of the crate I now own? Would I want to go to the list of all crates that I own? I'm not sure! The pending-invitations page is probably fine; we can adjust if we get feedback that something else would be more useful.

If accepting an invitation failed, I think suggesting they go to the pending-invites page to try again is a good idea.

Thanks!!

[nkanderson]

Great, yeah, those route names sound good to me! It's helpful to have the distinction b/t accept and confirm or verify. And it sounds like the /me endpoint namespace is more about associating actions with a specific user, vs. only allowing actions when a user is logged in, so that makes sense.

I think I should be able to modify my proof-of-concept to match what you've outlined above, then I'll move on to writing tests. It'll be my first attempt at writing Rust tests of any complexity, so I may post back here with questions!

[nkanderson]

Hi @carols10cents - I've got a branch on my fork with all of the working code for token-based ownership acceptance, but I've got a couple of questions and not sure if I should open a PR to review there or work through it here. Do you have a preference?

Specifically, I've got a couple of questions in the commits indicated by TODOs, and I wasn't sure where to add in tests since I didn't find much for previous similar code. If you want to work through it here, I can describe and link to questions more specifically.

[carols10cents]

@nkanderson Opening a PR sounds great! You can even open a "draft PR" in GitHub now, which will make sure we don't merge it until you're happy with it. Thanks!

[carols10cents]

This is now completed and will be fully deployed soon! Thank you @nkanderson !!!