Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes devise initializer from dummy generator #727

Merged
merged 1 commit into from Jan 26, 2016
Merged

Removes devise initializer from dummy generator #727

merged 1 commit into from Jan 26, 2016

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Jan 21, 2016

The dummy app generator adds a devise initializer that's disabled in most of the time.

The official solidus_auth_devise gem has it's own devise initializer, that never gets generated in the dummy app, because the generator skips existing devise initializers (what's good, to prevent overwriting in the real host app).

I don't know if this will have any impact on other existing extension, but IMO it shouldn't unless they depend on devise itself.

@jhawthorn
Copy link
Contributor

Sounds reasonable to me 👍

CC @adammathys who I know pointed this out before

@tvdeyen
Copy link
Member Author

tvdeyen commented Jan 21, 2016

I plan to let all Devise controllers inherit from Spree::BaseController, to make it much easier to extend them from solidus_i18n

@cbrunsdon
Copy link
Contributor

I'm also 👍 this but I don't know this area of the code very well. CC'ing @brendandeere and @forkata as well who know it better than myself.

@jhawthorn
Copy link
Contributor

One thing we'll have to check before merging is whether this breaks extensions which depend on auth_devise

@forkata
Copy link
Contributor

forkata commented Jan 22, 2016

I just tested this with solidus_auth_devise and indeed it will install it's devise initializer if this isn't in place.

I also did a quick test with a gem that depends on solidus_auth_devise like @jhawthorn suggested (solidus_virtual_gift_card) and he is right in that case no initializer will be generated and it throws the devise warnings. I believe this was put in place to just prevent those warnings from the original comment here d3d33ae.

Having said that, I do think this should be addressed in solidus_auth not in core, however I don't know how one might go about that. Maybe we need a way for extensions to hook into the dummy app generator :)

@tvdeyen
Copy link
Member Author

tvdeyen commented Jan 22, 2016

@forkata could rake test_app from solidus_virtual_gift_card also run the solidus_auth_devise generator, like in a "real app"? Or the generator from solidus_virtual_gift_card could run the solidus_auth_devise generator, if it's not already present.

TBH I really don't see the core have to know anything about something it does not depend on.

@cbrunsdon
Copy link
Contributor

solidus_virtual_gift_card will error out trying to run tests against this change with the devise token missing (not just throw a warning). Before I'd be for merging this in I think we need a plan for what we're going to do on gems such as those.

I agree this shouldn't be in core, but I'd like an idea on what we're going to do for those other gems.

@cbrunsdon
Copy link
Contributor

Sorry, @forkata corrected me IRL, it is in fact just warning in the test_app, so it won't break specs. I'm 👍 on this change then, we should deal with it eventually but by no means a priority IMO.

@mamhoff
Copy link
Contributor

mamhoff commented Jan 23, 2016

👍

The dummy app generator adds a devise initializer that's disabled in most of the time.

The official solidus_auth_devise gem has it's own devise initializer, that never gets generated in the dummy app, because the generator skips existing devise initializers (what's good, to prevent overwriting in the real host app).
@cbrunsdon
Copy link
Contributor

Rebased over master for the rspec fixes, trying build again.

cbrunsdon added a commit that referenced this pull request Jan 26, 2016
Removes devise initializer from dummy generator
@cbrunsdon cbrunsdon merged commit bcb4385 into solidusio:master Jan 26, 2016
@tvdeyen
Copy link
Member Author

tvdeyen commented Jan 27, 2016

Great. Thanks

@mamhoff mamhoff deleted the remove-devise-initializer branch May 24, 2016 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants