Include Devise routes/controllers/views directly in the project #153

kennyadsl · 2021-03-25T10:56:28Z

Description

This PR explores if it makes still sense to have all the views needed for the frontend copied over the host application except for the things related to the authentication. The rationale is that if people want to extend their application they probably also want to change those parts of the application.

Also, it was technically very difficult to make SolidusAuthDevise work without all those things copied because it requires its
components (controller, routes, views) only when SolidusSupport.frontend_available? returns true. This method returns false using this frontend, even using a monkey_patch, because some calls are made during the engines initialization, when the decorators are not yet loaded.

Cons of this approach are:

views/controllers/routes are duplicated here and in solidus_auth_devise
at this point it's not possible to install solidus_starter_fronted without solidus_auth_devise. Well, it's possible but al the devise things need to be manually removed from the host application after the installation.

Motivation and Context

We want to provide a smooth installation process, and now it was impossible to have a store working with the auth provided by solidus_auth_devise.

The project will ship with a working Authentication system that uses Devise. Don't you like it? Well, you can just remove the extra code from your project after it has been copied and use what you prefer.

With another iteration, we can explore adding an install configuration (--without-authentication) that does not copy auth-related things if needed.

How Has This Been Tested?

I tested this creating a new rails store following the README instructions updated with this PR.
Some specs are passing, some don't but that's because we need to change some things that solidus_auth_devise views changed, but it's expected.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

Fix current specs
Add specs from SolidusAuthDevise
Fix TODOs introduced
Update solidus_compare step hash
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

It makes no sense to have all the view needed for the frontend copied over the host application except for the things related to the authentication. If people want to extend their application they probably also want to change those things. Also, it was technically very difficult to make SolidusAuthDevise work without all those things copied because it requires its components (controller, routes, views) only when SolidusSupport.frontend_available? returns true. This method retunrs false using this frontend, even using a monkey_patch, because some calls are made during the engines intialization, when the decorators are not yet loaded.

hound · 2021-03-25T10:56:51Z

app/controllers/spree/users_controller.rb

+    @user = Spree::User.new(user_params)
+    if @user.save
+
+      if current_order


Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

hound · 2021-03-25T10:56:51Z

app/controllers/spree/users_controller.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+class Spree::UsersController < Spree::StoreController


Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

hound · 2021-03-25T10:56:51Z

app/controllers/spree/checkout_controller.rb

+      %w(registration update_registration).include?(params[:action])
+    end
+
+    def check_authorization


Lint/DuplicateMethods: Method Spree::CheckoutController#check_authorization is defined at both app/controllers/spree/checkout_controller.rb:247 and app/controllers/spree/checkout_controller.rb:278.

hound · 2021-03-25T10:56:51Z

app/controllers/spree/checkout_controller.rb

+    end
+
+    def skip_state_validation?
+      %w(registration update_registration).include?(params[:action])


Style/PercentLiteralDelimiters: %w-literals should be delimited by [ and ].

hound · 2021-03-25T10:56:51Z

app/controllers/spree/checkout_controller.rb

+        permit(:email)
+    end
+
+    def skip_state_validation?


Lint/DuplicateMethods: Method Spree::CheckoutController#skip_state_validation? is defined at both app/controllers/spree/checkout_controller.rb:160 and app/controllers/spree/checkout_controller.rb:274.

hound · 2021-03-25T10:56:52Z

app/controllers/spree/user_confirmations_controller.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class Spree::UserConfirmationsController < Devise::ConfirmationsController


Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.

hound · 2021-03-25T10:56:53Z

config/routes.rb

+    path_names: { sign_out: 'logout' },
+    path_prefix: :user,
+    router_name: :spree
+  })


Layout/IndentFirstHashElement: Indent the right brace the same as the first position after the preceding left parenthesis.

hound · 2021-03-25T10:56:53Z

config/routes.rb

@@ -30,4 +32,35 @@

  get '/unauthorized', to: 'home#unauthorized', as: :unauthorized
  get '/cart_link', to: 'store#cart_link', as: :cart_link
+
+  devise_for(:spree_user, {
+    class_name: 'Spree::User',


Layout/IndentFirstHashElement: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

hound · 2021-03-25T10:56:53Z

config/routes.rb

@@ -30,4 +32,35 @@

  get '/unauthorized', to: 'home#unauthorized', as: :unauthorized
  get '/cart_link', to: 'store#cart_link', as: :cart_link
+
+  devise_for(:spree_user, {


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

hound · 2021-03-25T10:56:53Z

lib/solidus_starter_frontend/authentication_helpers.rb

+
+
+    delegate :login_path, :signup_path, :logout_path,
+              to: :spree,


Layout/AlignArguments: Align the arguments of a method call if they span more than one line.

waiting-for-dev · 2021-03-31T14:02:15Z

Also, it was technically very difficult to make SolidusAuthDevise work without all those things copied because it requires its
components (controller, routes, views) only when SolidusSupport.frontend_available? returns true. This method returns false using this frontend, even using a monkey_patch, because some calls are made during the engines initialization, when the decorators are not yet loaded.

We are making available two methods to install solidus_starter_frontend:

1. Copying all solidus_starter_frontend app files over the project, through solidus_starter_frontend gem's executable.

In this case, the executable relies on rails generate solidus:install having been already executed. This generator is responsible for adding solidus_auth_devise to the stack. At the time it gets installed, the frontend is not available (we haven't executed solidus_starter_frontend yet) so solidus_auth_devise doesn't add the frontend stuff:

https://github.com/solidusio/solidus_auth_devise/blob/09841c164e1d0e6ab7afbb80494737fc692eb806/lib/spree/auth/engine.rb#L26

2. Adding solidus_starter_frontend gem to the Gemfile.

For this installation option, the README is currently recommending adding solidus_starter_frontend after bin/rails generate solidus:install has been executed. So, it essentially happens the same as in option 1, but on top of devise resources not being available an error is raised because of this references in solidus_starter_frontend initializer:

https://github.com/nebulab/solidus_starter_frontend/blob/1609b364d670972bbba718b44e406729d25242f7/lib/solidus_starter_frontend/engine.rb#L18-L25

(Spree::Auth::Engine is defined, but it doesn't have created the Controller's classes.)

However, if we add solidus_starter_frontend before executing bin/rails generate solidus:install, i.e. at the same time that we add solidus_auth_devise, solidus_core, etc... to the Gemfile, the installation does succeed and devise routes are found.

What I'd propose to fix the problem is:

Fix instructions for Option 2.
Remove Option 1. Yeah, it sounds drastic. However, if we take a step backward, I think that supporting an installation method where everything gets copied is not a good idea. It lacks any kind of modularity. Instead, we could add a task that allows you to copy only what you need to override, in a way similar to what devise provides. E.g.:

rails generate solidus_starter_frontend:controllers [checkout]

It would provide a better user experience and, on top of that, this issue would be fixed.

kennyadsl · 2021-04-08T09:19:35Z

@waiting-for-dev I'm fine trying the proposed approach! We can always use a generator to copy controllers and everything else by default if needed. 👍 Feel free to upset the installation process!

gsmendoza · 2021-07-29T09:46:49Z

@kennyadsl I'm closing this PR since it is now superseded by #172.

kennyadsl added 2 commits March 25, 2021 11:37

Update install instructions

a19ebc5

kennyadsl self-assigned this Mar 25, 2021

hound bot reviewed Mar 25, 2021

View reviewed changes

waiting-for-dev mentioned this pull request Apr 12, 2021

Remove dependency to test generators #155

Merged

6 tasks

waiting-for-dev mentioned this pull request Jun 11, 2021

Fix installation method as a gem executable #162

Closed

6 tasks

This was referenced Jul 15, 2021

Fix sandbox script #166

Closed

Discourage SolidusStarterFrontend from being run as an engine #167

Closed

gsmendoza mentioned this pull request Jul 29, 2021

Copy routes/controllers/views/tests from auth_devise #172

Merged

4 tasks

gsmendoza closed this Jul 29, 2021

waiting-for-dev deleted the kennyadsl/include-devise branch November 15, 2022 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include Devise routes/controllers/views directly in the project #153

Include Devise routes/controllers/views directly in the project #153

kennyadsl commented Mar 25, 2021 •

edited

Loading

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

hound bot Mar 25, 2021

waiting-for-dev commented Mar 31, 2021

kennyadsl commented Apr 8, 2021

gsmendoza commented Jul 29, 2021

		@@ -0,0 +1,67 @@
		# frozen_string_literal: true

		class Spree::UsersController < Spree::StoreController

		@@ -0,0 +1,18 @@
		# frozen_string_literal: true

		class Spree::UserConfirmationsController < Devise::ConfirmationsController



		delegate :login_path, :signup_path, :logout_path,
		to: :spree,

Include Devise routes/controllers/views directly in the project #153

Include Devise routes/controllers/views directly in the project #153

Conversation

kennyadsl commented Mar 25, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

hound bot Mar 25, 2021

Choose a reason for hiding this comment

waiting-for-dev commented Mar 31, 2021

kennyadsl commented Apr 8, 2021

gsmendoza commented Jul 29, 2021

kennyadsl commented Mar 25, 2021 •

edited

Loading