solidusio / solidus_auth_devise

🔑 Devise authentication for your Solidus store.
http://solidus.io
BSD 3-Clause "New" or "Revised" License
52 stars 124 forks source link

Dynamic devise class_name within routes #101

Closed karlentwistle closed 6 years ago

karlentwistle commented 7 years ago

Make it possible to complete the process of overriding Spree.user_class while using solidus_auth_devise. Before this PR if a developer took advantage of setting their own Spree.user_class - for example.

User = Class.new(Spree::User)

initializer "spree.auth.environment", after: :load_config_initializers do |app|
  Spree.user_class = "::User"
end

The hardcoded 'Spree::User' (string) within the routes file would override this customisation and continue to incorrectly use Spree::User rather than User. Then meant when using devise built in helpers the user class returned would be incorrect - for example.

Before this PR current_spree_user.class => Spree::User After this PR current_spree_user.class => User

cbrunsdon commented 7 years ago

That failure looks unrelated, re-running tests.

Also this looks like its a good change to me, @brendandeere? You know this code better than I do.

tvdeyen commented 7 years ago

Restarted the build again. Still randomly fails for unrelated reasons.

qr8r commented 7 years ago

:+1: For using the built in solidus user class hook.

A custom user class would still need to implement the api expected by SolidusAuthDevise in order to function proplerly, but that seems like a reasonable expectation of someone providing a custom user class.

It might be worth looking into this hook and changing when it is run. If you explicitly set a user_class in config/initializers/solidus.rb, I believe the auth devise gem would still override that setting with its own user class.

It could be worth while to make auth devise set it's default value at a different time which still allows a developer to override the default from within an initializer file, rather than specifying an :after clause.

tvdeyen commented 6 years ago

@karlentwistle would you mind to rebase on current master? That should fix the failing build

karlentwistle commented 6 years ago

Sorry for the delay I just returned from honeymoon. I have just rebased off master.