Add passwordless for authentication

esmale commented 4 months ago

What does this PR do?

I've added two gems to the application: passwordless for authentication, and letter_opener in order to easily trigger/access emails in development, since passwordless authentication relies upon email. I hope the application is set up with some kind of email delivery service to actually send emails. :-)

Instead of removing the Speaker model/table completely the way I initially intended while working on this at RailsConf, I backed away from that and just created the User model with an email address, and then set up the Speaker model to belong to a User. I assume that in the admin section of the site you'd be able to associate new User records to old Speaker records as necessary.

The ability to add Speakers has been removed, and instead when a person registers to join the site they also have to give their name, and the controller will create both the User and Speaker records and tie them together.

The ability to add new Proposals is also adjusted:

You no longer specify the speaker, it's always assumed to be the currently logged in user
The ability to edit proposals is limited to the proposals that you (the currently logged in user) own

I also removed the need to complete the captcha for these two actions. Since these actions cannot be performed unless logged in, it felt unnecessary. But I haven't removed the gem from the Gemfile yet, in case I'm wrong.

I've added and updated the test suite to account for a lot of these changes, but there are still some tests and application code changes that need to be made before this PR is truly complete. Primarily in the area of limiting the ability for updating Submissions to only the owner of the associated Proposal.

That said, I felt like this PR was far enough along that I wanted to get it in front of you to see what feedback you had.

I also had a could of questions about some corner cases to see what you would like:

Should the ability to create Events also be locked behind being logged in?
What do you think about putting the captcha on the login and registration forms?

Why was this work done? Is there a related Issue?

This PR addresses this issue: Speakerline is completely open to edit

Where should a reviewer start?

Are there any manual testing steps?

Screenshots

Deployment instructions

Database changes

There are a couple of new migrations that add:

a new users table
a new passwordless_sessions table
adds a user_id column to the speakers table

New ENV variables

nodunayo commented 3 months ago

Hi David! Thank you for this! Things are a bit busy right now, and I'm travelling for the next week, but I hope to properly read through and respond in the next couple weeks.

nodunayo commented 2 months ago

Hello!

Sorry it's taken me so long to get back to this. Thank you so so much for doing this! I really appreciate it. Finally taking a look now.

The app does not have an email service set up! There's been no reason to have it thus far
Can you tell me a little bit more about why we're retaining the Speaker model and having a User model, given they'll both be inextricably linked?
You're correct on the captcha being able to be removed now
I don't think creating Events should be locked as organisers may want to add their events and then notify speakers to add their proposals
We can keep the captha on the login and registration forms, but is it really necessary?

esmale commented 2 months ago

Email service: Right, just wanted to point out that this would be necessary to set up

Speaker and User model: I went through a variety of possible ways to approach this:

Rename the Speaker model/table to User, add an email field. This felt bad because these User records would be invalid until such a time that you gave them an email address.
Remove the Speaker model/table and all associations to it, and create a brand new User model/table, with its own associations to other records. This was the direction I initially went, but it got to a point where it felt excessively invasive to the rest of the app, since so much of the architecture ties back to Speakers in the current implementation. The way it felt at the time, I'd have to reassign every existing proposal back to you, which would then muddy the waters if someone were to visit the site and try searching for a talk by the speaker instead of via a topic.
So keeping the Speaker model in place is what I ended up with. It's not a perfect solution because, as you said, the Speaker and User models are inextricably linked and overlapping in what they represent. However, it felt like in the short term it preserves the data integrity of the application, and gives time in which you could attempt to get the speakers whose proposals are already on Speakerline to register on the app. And once you're at the point where you're satisfied with how many people have done that, we could go a step farther to merge the Speaker and User models into just a User model.

Captcha: Is keeping the captcha on the registration and login forms necessary? I don't know, that's why I was asking you. :-) I don't have any concerns right now with removing the captcha. It's easy enough to add back in at a later date if trolls start abusing the login system.

nodunayo commented 2 weeks ago

@esmale So sorry for the delay on this! I think I'm going to have to pick it up later on in the year or in the New Year! Will get back to you when I can.

nodunayo / speakerline