solidus

solidus copied to clipboard

The code is basically a cut & paste of the relevant parts from the gem solidus_auth_devise.

The rationale for this change is that the orders history page better belongs inside the solidus main project, because the feature does not have much to do with the actual authentication process, and because it allows projects that have an existing authentication system (ie. don't use solidus_auth_devise) to have the orders page feature out of the box.

Closes #4010

It makes sense but we should find a way to handle duplication of routes/controllers/views for users that will update solidus but not solidus_auth_devise, where we should also remove those files if they are already here. We should conditionally avoid including those files in some way? maybe Solidus version number?

Also, we are thinking about moving all the controllers/views/helpers from solidus_auth_devise into core, keeping solidus_auth_devise as solely an authentication system, so it's better to wait until we finalize this decision. Any thought is welcome.

By merging this PR stores will end up having the same route/action/view defined in both solidus_frontend and solidus_auth_devise. Do you see a possible solution to this?

@kennyadsl regarding your question about routes, what you wrote is certainly true, but I think I am missing something as I don't see why this can be a problem. Can you explain the reason?

Sure, I'm thinking that if a user upgrades to latest Solidus (that will include code from solidus_auth_devise) he will have the same code defined in both solidus and solidus_auth_devise. If we want to change something in core I'm afraid some changes (like changes on views) could not be doable since they will be overridden from the same files in the extension since they should be loaded later, right?

I think I understand your concerns. Looking at a concrete example, spree_auth_devise file lib/frontend/spree/users/show.html.erb overwrites the new file frontend/app/views/spree/users/show.html.erb.

One way to look at this, is to consider the new files in Solidus codebase as sensible defaults that are naturally overwritten by the current version of solidus_auth_devise. After all, their code is the same right now, so no harm done.

Issues arise when one of the two files changes.

Solidus changes will be masked by spree_auth_devise file as long as that file exist also in the gem. I think this first scenario is not a big issue, again as I'd rather consider Solidus file as (hidden) default, at least initially. spree_auth_devise is still in charge.

IMHO a more significant issue is when the file inside spree_auth_devise gets updated, as the updates should probably also be manually backported to Solidus' file.

I think one way to minimize code backporting may be to address all current issues in spree_auth_devise, this should give us some stability, and basically freeze that gem's development. As soon as a new version of Solidus with the new file is released, a new version of spree_auth_devise stripped of the very same file can be released. The new gem version would require the new Solidus release, of course.

What do you think? Does this strategy seem too clumsy to you?

@spaghetticode your proposed solution makes sense but no one can ensure us that user will update solidus_auth_devise after the Solidus update.

We should think at least to some warning or suggestion into the changelog entry that asks users to update solidus_auth_devise along with solidus. If we go in this direction, I'd prefer to ask/force Solidus users to update solidus_auth_devise once, by moving all the code unrelated to authentication from solidus_auth_devise to solidus in a single version release.

@kennyadsl 👍 on deprecations/warnings when an old version of solidus_auth_devise is found.

This looks like a good change that we can roll into v3.0, but it requires a little bit of coordination.

solidus_auth_devise is currently pinned to Solidus versions less than v3.0. In order to avoid conflicts where someone is running a version of solidus_auth_devise that defines this controller on a version of Solidus that also does, we'll have to merge this change into v3.0 and cut a new version of solidus_auth_devise at the same time that removes the controller and bumps the version requirement to Solidus >= 3.0.

I've been experimenting a bit with this PR, and I think it is safe to merge anytime, as the code in solidus_auth_devise will always take precedence over the one defined here. I tested this in both Rails 5.2 and 6.0 (Zeitwerk) and basically what happens is that extension directories come first in autoload_paths (Rails 5.2) and root_dirs (Zeitwerk) compared to the ones defined in Solidus. For example, this is autoload_paths from a sandbox application:

/Users/andrea/solidus/sandbox_5.2/app/assets
/Users/andrea/solidus/sandbox_5.2/app/channels
/Users/andrea/solidus/sandbox_5.2/app/controllers
/Users/andrea/solidus/sandbox_5.2/app/helpers
/Users/andrea/solidus/sandbox_5.2/app/jobs
/Users/andrea/solidus/sandbox_5.2/app/mailers
/Users/andrea/solidus/sandbox_5.2/app/models
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/solidus_auth_devise-2.5.2/lib/decorators/backend/controllers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/solidus_auth_devise-2.5.2/lib/decorators/frontend/controllers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/solidus_auth_devise-2.5.2/app/mailers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/solidus_auth_devise-2.5.2/app/models
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/solidus_auth_devise-2.5.2/lib/controllers/backend
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/solidus_auth_devise-2.5.2/lib/controllers/frontend
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/devise-4.7.3/app/controllers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/devise-4.7.3/app/helpers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/devise-4.7.3/app/mailers
/Users/andrea/solidus/frontend/app/assets
/Users/andrea/solidus/frontend/app/controllers
/Users/andrea/solidus/frontend/app/helpers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/canonical-rails-0.2.11/app/helpers
/Users/andrea/solidus/backend/app/assets
/Users/andrea/solidus/backend/app/controllers
/Users/andrea/solidus/backend/app/helpers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/font-awesome-rails-4.7.0.5/app/assets
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/font-awesome-rails-4.7.0.5/app/helpers
/Users/andrea/solidus/api/app/controllers
/Users/andrea/solidus/api/app/helpers
/Users/andrea/solidus/core/app/assets
/Users/andrea/solidus/core/app/controllers
/Users/andrea/solidus/core/app/helpers
/Users/andrea/solidus/core/app/jobs
/Users/andrea/solidus/core/app/mailers
/Users/andrea/solidus/core/app/models
/Users/andrea/solidus/core/app/models/concerns
/Users/andrea/solidus/core/app/subscribers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/activestorage-5.2.4.4/app/assets
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/activestorage-5.2.4.4/app/controllers
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/activestorage-5.2.4.4/app/controllers/concerns
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/activestorage-5.2.4.4/app/javascript
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/activestorage-5.2.4.4/app/jobs
/Users/andrea/.rvm/gems/ruby-2.6.3/gems/activestorage-5.2.4.4/app/models
/Users/andrea/solidus/sandbox5/test/mailers/previews

So, for example, Spree::UsersController will be found first in solidus_auth_devise and the one in solidus_frontend will never be loaded.

The code in this PR becomes alive only when its counterpart is removed from solidus_auth_devise.

@spaghetticode What about the routes? Don't we have a conflict of both core and solidus_auth_devise defining the same one?

@kennyadsl I didn't find any issue with that when I tested the feature. The last routing file loaded will simply override the previous route, if they're the same.

Closing as we've deprecated solidus_frontend. We could move it to solidus_starter_frontend, though.