-
Notifications
You must be signed in to change notification settings - Fork 42
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
Pass location.search parameter through history search. #2573
Conversation
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 code hadn't changed in 3 years. Do we understand why it's suddenly broken? The PR description describes what is changing but does not describe why it fixes this bug.
Do we need to adjust how search
is passed to history.push
on line 175 as well? Anywhere else?
line#404 was changed from "pathname: params.id ? /users/preview/${params.id} : '/users'," to "pathname: params.id ? /users/preview/${params.id}${location.search} : '/users'," during August in the scope of PR #2533 (https://issues.folio.org/browse/UIU-2928) I am not sure why passing the search params in the path has lead to these bugs, but for sure the change in current PR is fixing all the 3 bugs -
I don't see there is a need to update line#175. I have tested the create user record flow and seems to be working fine. For some reason, today's P1 bug(UIU-2971) seems to report that the url queries are mal formed on initial load for several API calls like manualblocks, feefines and more.. The reason being :{id} in userId==:{id} of query is picking the search params in a broken way. |
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.
Thanks for the extra details, @Terala-Priyanka. I finally understand what's happening here: history.push(string)
accepts a string with a query string but history.push(object)
requires the object to have separate pathname
and search
properties.
Kudos, SonarCloud Quality Gate passed! |
This is for those visitors who might come back here .. |
Purpose
Please note that this fix addressess a P1 issue reported for Poppy-bugfix.
UIU-2971 - Pass location.search parameter through history search.
These changes address the fix for both https://issues.folio.org/browse/UIU-2971 and https://issues.folio.org/browse/UIU-2947
Approach
TODOS and Open Questions
Learning
Screencast
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.