fix: improve redirect parameter handling for success page #20764
Conversation
|
@guptadeepak8 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
mrge reviewed 2 files and found no issues. View the review in mrge.io
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (04/18/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (04/18/25)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (07/01/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
@guptadeepak8 visual demo is important here - can you add , making it draft until then.
|
previous:- changes:- Screen.Recording.2025-04-18.162056.mp4 |
There was a problem hiding this comment.
mrge reviewed 2 files and found no issues. View the review in mrge.io
| @@ -185,7 +194,7 @@ export const useBookingSuccessRedirect = () => { | |||
| ...query, | |||
| ...bookingExtraParams, | |||
| }, | |||
| searchParams: new URLSearchParams(searchParams.toString()), | |||
| searchParams: filterEmbedParameter(searchParams, isEmbed), | |||
There was a problem hiding this comment.
Instead of making it embed specific we should handle removing the param in getBookingRedirectExtraParams and unit test that function as that function has the responidbility of building what params to send
There was a problem hiding this comment.
@guptadeepak8 Thanks for the PR !! I have left a suggestion to implement it in a better way
Making it draft until then |
a844625 to
2ce25f5
Compare
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
|
This PR is being marked as stale due to inactivity. |
|
@TusharBhatt1 pls review this |
E2E results are ready! |
| @@ -184,6 +189,7 @@ export const useBookingSuccessRedirect = () => { | |||
| query: { | |||
| ...query, | |||
| ...bookingExtraParams, | |||
| isEmbed, | |||
There was a problem hiding this comment.
I don't think it is implemented correctly @Devanshusharma2005 could you confirm?
getNewSearchParams seems to have added a new property isEmbed(optional) but that isn't passed here. isEmbed is being passed in query object and we don't read isEmbed from query object in getNewSearchParams
There was a problem hiding this comment.
Also, I asked Devin to work on a much more exhaustive approach with unit tests to handle all embed related params
#22028 @Devanshusharma2005 Maybe you want to pick it up?
Summary by mrge
Improved redirect handling by removing the unused "embed" parameter before forwarding users to the success page.
What does this PR do?
Improved query parameter handling by removing the unused embed param before forwarding to success page
Mandatory Tasks (DO NOT REMOVE)