chicago-tool-library / circulate

A lending library management system
Other
82 stars 66 forks source link

Require email confirmation before "borrowing" items #1626

Closed crismali closed 2 months ago

crismali commented 3 months ago

What it does

This adds a check to prevent users from placing holds on items before confirming their email address (via devise). It also adds some warning toast messages to encourage users to confirm.

Why it is important

Handles #1370

UI Change Screenshot

Trying to sign in without a confirmed email address: Screenshot 2024-08-20 at 9 26 07 AM

Admin member profile when unconfirmed: Screenshot 2024-08-16 at 4 37 33 PM

Resend confirmation instructions page: Screenshot 2024-08-07 at 3 22 18 PM

Admin users index with a user that has updated their email but hasn't confirmed the new email: Screenshot_2024-08-16_at_5_00_34 PM

Implementation notes

It doesn't look like members can create loans directly, so I prevented members with unconfirmed email addresses from being able to make holds. Admins could still create loans for these members, so maybe we want to add a message on the admin side that warns them about creating loans for these members?

Users that haven't confirmed their email addresses cannot sign in at all. Users can update their email and have to confirm the new email, but can still log in with the old email until the new one is confirmed. Not sure if we want to bring back the message that encourages people to confirm their email when they update it?

Right now the migration will mark all existing users as verified so these changes should only impact new members. I'm not sure if there's a heuristic we could use to make this smarter.

Take a look and let me know what you think!

phinze commented 2 months ago

This is looking really good!

Admins could still create loans for these members, so maybe we want to add a message on the admin side that warns them about creating loans for these members?

I think something on the admin side member page to indicate that the user has not confirmed their email address is a good idea. But I also think it's correct that admins should be able to create loans for them.

phinze commented 2 months ago

I'm flushing out a few Circulate tasks now, so I can pick up the two outstanding tweaks and bring this home. 🤝

phinze commented 2 months ago

After discussing this with staff, we have two changes to make here:

And one change to investigate:

crismali commented 2 months ago

I've made some of the changes we talked about the other night. A new question is how much do we want to surface folks who have changed their email addresses but haven't confirmed the new ones yet. I've changed the members so that when they change the member email they're also updated the user unconfirmed email so there shouldn't be in any place where a user's email being changed directly. I've also made it so the message on the admin member page appears if the user has never been confirmed or if they just haven't confirmed their new email.

What do folks think? Should we call this out more for members or admins?

crismali commented 2 months ago

@jim They should all be able to log in with this. The migration makes it so that if you're already an existing user you're automatically confirmed even though that hasn't actually happened.