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

Decorate Spree::UsersController when already existing from Solidus Frontend #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spaghetticode
Copy link
Member

Ref #123

With solidusio/solidus#2695 we're starting to include the order history page (/users/show) in Solidus frontend. At the moment that code is not used, since the one included in solidus_auth_devise takes precedence.

This PR starts the removal of the controller code so that, when available, the version included in Solidus at frontend/app/controllers/spree/users_controller.rb is used instead.

Once all supported versions of Solidus include that page, we will be able to get rid of the else branch with the class definition, together with the ERB view at lib/views/frontend/spree/users/show.html.erb.

@spaghetticode spaghetticode self-assigned this Jan 22, 2021
This commit allows to decorate the class `Spree::UsersController`
when already present (ie. when it's already included in the gem
`solidus_frontend`) or to define the class from scratch when it's
not (ie. when using a version of Solidus where `solidus_frontend`
does not include it).

See solidusio/solidus#2695 for the PR that adds the account page
to Solidus Frontend.

Once all supported versions of Solidus include the account page,
we will be able to get rid of the class definition, together with
the ERB view at `lib/views/frontend/spree/users/show.html.erb`.
@spaghetticode spaghetticode force-pushed the spaghetticode/remove-account-page branch from 22a99a3 to 8da3e8e Compare January 22, 2021 14:55
if defined?(Spree::UsersController)
Spree::UsersController.prepend(self)
else
class Spree::UsersController < Spree::StoreController
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spaghetticode What about keeping the controller in the past directory and duplicating the ifs? I'm just thinking that people may expect to look for this controller in that folder and not finding it. On the other side, I understand that the controller code will be split into two different files and people can't know that this file exists. Well, unless we tell them with a code comment! 😬

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that work with Zeitwerk?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does, I was more worried about this version to be honest. With what I proposed, we'll have Spree::UsersController class defined into lib/controllers/frontend/spree/users_controller.rb.

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.

4 participants