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

Only use custom routes #237

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

elia
Copy link
Member

@elia elia commented Dec 23, 2022

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:

DEPRECATION WARNING: This route is deprecated: "/user/spree_user/sign_up".
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
  devise_scope :spree_user do
    get "/user/spree_user/sign_up", to: spree/user_registrations#new, ...
  end
end

Please check your application for places in which this route was generated.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.
  • I have opened a PR to update the guides.
  • I have updated the README to account for my changes.

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."
@elia elia force-pushed the elia+kennyadsl/fix-routes branch from 5dfb2e7 to bf56a64 Compare December 23, 2022 12:53
@@ -0,0 +1,36 @@
# frozen_string_literal: true
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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
Copy link
Member

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.

Copy link
Member Author

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 🍀

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Member Author

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

@elia elia force-pushed the elia+kennyadsl/fix-routes branch from bf56a64 to 242a515 Compare December 23, 2022 14:41
@elia elia force-pushed the elia+kennyadsl/fix-routes branch from 242a515 to ef6c4ba Compare December 23, 2022 15:09
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.

3 participants