Skip to content

Conversation

@cpfergus1
Copy link
Contributor

@cpfergus1 cpfergus1 commented Aug 30, 2022

Description

Removes #redirect_back_or_default in favor of Rails #redirect_back and #redirect_back_or_to

Motivation and Context

Rails introduced redirect_back in rails 5+ and redirect_back_or_to in rails 7+. These methods should be utilized instead of the redirect method created by Soldius, which similarly replicates the functionality of the deprecated method.
The method #redirect_back_or_default will be deprecated in Solidus PR #4533

How Has This Been Tested?

The current test suite covers the changes made in the PR

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:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@waiting-for-dev
Copy link
Contributor

Thanks, @cpfergus1. The change is perfectly legit, but I'm not sure we want to merge changes into the deprecated frontend. cc @kennyadsl @aldesantis

@cpfergus1 cpfergus1 force-pushed the connorferguson/sol-306-deprecate-redirect_back_or_default branch from 49f0ee0 to 36d0496 Compare September 1, 2022 13:00
@waiting-for-dev
Copy link
Contributor

We decided that we're going to keep merging changes until the gem is no longer part of the solidus meta-gem. So, we'll merge this one once the extensions are compatible.

@cpfergus1
Copy link
Contributor Author

cpfergus1 commented Sep 12, 2022

Unlike the starter_frontend gem, this extension does not require solidus_auth_devise except for the sandbox - The updated methods will not be compatible unless solidus_auth_devise is included. When are we expecting to remove this extension from the solidus meta-gem? We may just want to continue to leave it untouched.

@waiting-for-dev
Copy link
Contributor

Hey @cpfergus1, as the only occurrence here has nothing to do with the sign-in process, can't we use Rails' redirect_back/redirect_back_or_to here?

The method #redirect_back_or_default and the class user_last_url_storer will be deprecated in Solidus PR #4533 which would break the current build without this change
@cpfergus1 cpfergus1 force-pushed the connorferguson/sol-306-deprecate-redirect_back_or_default branch from 36d0496 to f1fce78 Compare September 13, 2022 15:45
@cpfergus1
Copy link
Contributor Author

Hey @cpfergus1, as the only occurrence here has nothing to do with the sign-in process, can't we use Rails' redirect_back/redirect_back_or_to here?

Yes, it appears to work fine for this location. Thanks for pointing that out!

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @cpfergus1!! 🙌

if @order.errors.any?
flash[:error] = @order.errors.full_messages.join(", ")
redirect_back_or_default(spree.root_path)
redirect_back(fallback_location: spree.root_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that redirect_back is only soft deprecated, so let's do that. Ideally, we should remember to update to redirect_back_or_to once we remove support for Rails 6.

@waiting-for-dev waiting-for-dev merged commit 25a2568 into solidusio:master Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants