ywwg / ffagc

Firefly Art Grant Core Website
0 stars 1 forks source link

Add Devise User #73

Closed Katee closed 7 years ago

Katee commented 7 years ago

This creates a new model user which will be used to unify Admin, Artist and Voter. Users can register using devise at http://localhost:3000/users/sign_up (currently these pages are unstyled, that will happen in a future PR.)

I also added the letter_opener gem, in development emails will open up in the browser when they are "sent". To see this in action try running FactoryGirl.create(:user) in the rails console.

Gotchas:

Future PRs:

Related issues:

ywwg commented 7 years ago

We should have a conversation about The Future of FFAGC before you do too much major work, just so you have a sense of the roadmap I have in mind. There are assumptions that the current framework makes that I would like to upend at some point.

We will need the concept of user roles, which should be ORable (someone can be a voter and an admin, for instance). I assume that's something that can be added later?

Katee commented 7 years ago

That is my plan, please see https://github.com/ywwg/ffagc/issues/63.

I will either add a simple roles array to User so each user can be some subset of [:admin, :voter, :artist] or use a more intense solutions such as CanCanCan that more finely defines "abilities" for various users/roles.

Katee commented 7 years ago

I'm definitely interested in the future plans too. My current plan is to get this project in a better spot by adding reasonable validations, writing specs, and removing duplicate code.

Katee commented 7 years ago

This PR is probably easier to read commit by commit. Most of the lines come from the commit adding the devise views.

ywwg commented 7 years ago

I've been thinking about this PR and the general design plan to unify the user types, and unfortunately I think this addition of the new user type is the wrong place to start. If we do this first, then we have two user models at the same time and conflicting frontend choices, which means we need to maintain both versions (keep the deprecated one working while still improving the new one) until the new one is feature complete.

I think a better plan would be to start by writing a shim for the current three-table model so that various bits of the frontend can be rewritten so they can inquire about user permissions in a similar way to how the eventual backend model will work. So instead of "if admin_logged_in || voter_logged_in", there would be some sort of API like [totally pseudocode] "if user.permission_contains ['admin', 'voter']". The various calls can be replaced piecemeal and don't need to be done in one fell swoop. Once all the logic has been rewritten to use the shim API, then the backend model can be replaced with a simple swap -- replace the old model with the new and remove the shim.

Doing the migration that way will keep the codebase cleaner by avoiding having conflicting coexisting models, and will ensure that the master branch stays runnable and deployable.

Redoing the user model is a really heavy lift for this codebase because this transition will be hard to manage. But frankly it's low on my list of priorities compared with fixing some of the UX issues. I've tweaked some of the priority labels in the Issues list to give an idea of where I'd prefer to focus development effort right now.