Fix Location Storage #231
Draft
Fix Location Storage #231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
With the deprecation of #redirect_back_or_default in solidus 4.0, we can utilize Devise helpers store_location_for and stored_location_for to provide the same functionality. Co-Authored-By: Elia Schito <[email protected]>
cpfergus1
force-pushed
the
elia+connor/fix-location-storage
branch
from
11b3f3a to
c38010d
Compare
This method was utilized for redirect_back_or_default. It is no longer required but we still need to store a location if we get to this point
Previously we would fall back on authorization to determine if a user should be able to access a certain endpoint. We should be authenticating the user first prior to checking if the specific user is authorized to access a certain endpoint Co-Authored-By: Elia Schito <[email protected]>
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."
Devise utilizes spree_user scope when executing after_sign_in_path. This is an important distinction when storing the user location as the user would not be redirected as expected if we use spree_current_user. Co-Authored-By: Elia Schito <[email protected]>
cpfergus1
force-pushed
the
elia+connor/fix-location-storage
branch
from
c38010d to
fcd8eff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes the new store location feature introduced in #228
Motivation and Context
We replaced the method #redirect_back_or_default and the class user_last_url_storer because Devise already provides functionality similar to redirect_back_or_default. After emerging PR#228 We noticed that the redirects were not working as intended and investigated the cause. We found a couple of issues that we missed and needed to be rectified, specifically:
We need to Authenticate the user and not rely on authorization
Utilize
spree_userinstead ofspree_current_usersince that is the scope being utilizedUse implemented custom paths instead of devise helpers for certain routes
How Has This Been Tested?
The current test suite covers the changes made in the PR - quadrupled checked 👍
Types of changes
New feature (non-breaking change which adds functionality)Breaking change (fix or feature that would cause existing functionality to not work as expected)Checklist: