-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
Only use custom routes #237
base: main
Are you sure you want to change the base?
Conversation
Using both devise generated routes and custom ones is confusing. As an example: when failing authentication, the devise route would send the user to "new_spree_user_session_path", however we want our users to be redirected to "/login." This commit deprecates the route and sends users to "/login."
5dfb2e7
to
bf56a64
Compare
@@ -0,0 +1,36 @@ | |||
# frozen_string_literal: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced by where this file is placed. Why it's in app/controllers
? This kind of helper is more something for libs
IMO, similar to what we do in core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it there because it plays nice with autoload, maybe we could put it in app/controllers/concerns/solidus_auth_devise/deprecated_routes.rb
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, better!
It will be removed in solidus_auth_devise v3. | ||
If you want to continue using this route please define it in your application code: | ||
|
||
Spree::Core::Engine.routes.draw do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works only for the frontend right? For backend routes it will also have the namespace :admin do
part. Not that we need it now, just wanted to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backend we already skip: :all
so it should be already streamlined, luckily 🍀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't find this skip: app
for backend routes, can you please point it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got what you mean now. Still, the with_options deprecated_route: true do
could be used in the future on a backend route. I'm just saying that if that happens, this concern will be printing a non-accurate message, because it only works with frontend routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but I trust we'll remove everything when releasing v3.0
bf56a64
to
242a515
Compare
242a515
to
ef6c4ba
Compare
Summary
Using both devise generated routes and custom ones is confusing.
As an example: when failing authentication, the devise route would
send the user to "new_spree_user_session_path", however we want our
users to be redirected to "/login." This commit deprecates the route
and sends users to "/login."
This PR maintains full support for existing routes allowing stores to copy them in their routes file if they want to continue using them.
Visiting
/user/spree_user/sign_up
the logs will show this deprecation message:Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):