jdtornow / challah

User authorization and session management in Rails.
MIT License
18 stars 7 forks source link

Extract status enum to separate concern #32

Closed philtr closed 7 years ago

philtr commented 7 years ago

I wanted to be able to add another item to the status enum ("pending"), but it doesn't look like Rails provides a way to do that once you've already defined an enum. This moves all the enum-related logic to a separate concern for the user model that can be toggled with an option.

An alternate solution would be something like this:

# We could set the statuses in the initializer
Challah.options[:statuses] = { pending: 0, active: 1, inactive: 2 }

# Or only set the additional new ones
Challah.options[:additional_statuses] = [:pending]

Thoughts?

philtr commented 7 years ago

I'm going to close this; upon further thinking I am going to need something more than this anyway, since in my case a pending user should be able to log in but not have permission to access certain resources. I am going to add a separate column to my user model. 🍻

jdtornow commented 7 years ago

Hey @philtr - I do like this idea, and it seems like a good idea to be more flexible on those initial statuses, even if you don't need it for your current project. I like the solution here. Any objections to merging it in for future use?

philtr commented 7 years ago

@jdtornow the only problem I had with this (that I discovered after I submitted my PR) was that if you defined your custom enum in a different order it wouldn't work and I didn't debug it. So you might want to look into that before merging.

jdtornow commented 7 years ago

That's a good point @philtr. I kind of like your second proposal idea of adding an option setting to allow for more than the standard statuses:

Challah.options[:additional_statuses] = [:pending]

Then, we could add a method into User that determines whether the status lets the user authenticate. Something like valid_for_session? or similar, that right now would just return true if the user is active. But, could be overridden in the app's User model to check for other statuses or logic. Not sure that would help your current use case, but seems like a good idea to add.

Thoughts on that approach?

philtr commented 7 years ago

@jdtornow Yeah, I like it! I push a couple commits that do the very thing. There was already a User#valid_session? method, so I just hooked everything into that instead of using User#active?

jdtornow commented 7 years ago

Awesome! Thanks for doing that, I think this is nice and flexible.