-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor(cypress): verify payment status after payment redirection #6187
base: main
Are you sure you want to change the base?
refactor(cypress): verify payment status after payment redirection #6187
Conversation
Review changes with SemanticDiff. Analyzed 1 of 1 files. Overall, the semantic diff is 11% smaller than the GitHub diff.
|
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.
other than that, looks good to me!
Co-authored-by: Pa1NarK <69745008+pixincreate@users.noreply.github.com>
@pixincreate I have made all the suggested changes. |
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.
other than that, looks good to me!
Co-authored-by: Pa1NarK <69745008+pixincreate@users.noreply.github.com>
bd3174d
Hey @Gnanasundari24 @Sakilmostak , please review the PR now |
if (statusCode >= 400 && statusCode < 500) { | ||
console.error(`Client Error: ${statusCode} - ${paymentStatus || "Payment status not available"}`); | ||
throw new ClientError(`Client error occurred. Status code: ${statusCode}`); | ||
} else if (statusCode >= 500) { | ||
console.error(`Server Error: ${statusCode} - ${paymentStatus || "Payment status not available"}`); | ||
throw new ServerError(`Server error occurred. Status code: ${statusCode}`); | ||
} else if (!paymentStatus) { | ||
console.error(`Error: Payment status is undefined. Unexpected error.`); | ||
throw new Error(`Payment status is undefined. Unexpected error.`); |
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.
From what I understand, this would not work as expected.
The reason being, you will get a 2xx
response with a url
in next_action
and the paymentStatus
would be requires_customer_action
.
The possibility that @Gnanasundari24 mentioned was about getting a 4xx
during redirection. You would not get a status_code over there. For example:
But you can definitely see the 4xx
or a 5xx
when you open inspect tab.
So, you would have either have to throw an error directly like what is done in commit e1f1e44
(#6187) or read what is mentioned in the screen which I do not think is possible in a line or 2.
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.
Check if latest commit resolve it
Hey @Ankesh2004 Kindly look into the comments! |
console.error(`Error: Payment status is undefined. This might indicate a 4xx or 5xx error.`); | ||
throw new Error(`Payment status is undefined. There may have been a client or server error (4xx/5xx). Check network logs or UI.`); |
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.
would be great if you could make it take a screenshot when redirection fails.
to do so, you can use cy.screenshot()
command.
if (paymentStatus === 'requires_customer_action') { | ||
console.log(`Payment requires customer action. Proceeding to next step.`); | ||
return; |
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 is redundant. we get requires_customer_action
as the status in the same way we get for any other status. so it will not matter.
} | ||
|
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 and below made changes in the latest commit is also not necessary. can be reverted.
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.
@pixincreate Done with all reviews corrections. look now
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.
almost there!
class ClientError extends Error { | ||
constructor(message) { | ||
super(message); | ||
this.name = "ClientError"; // 4xx errors | ||
} | ||
} | ||
|
||
class ServerError extends Error { | ||
constructor(message) { | ||
super(message); | ||
this.name = "ServerError"; // 5xx errors | ||
} | ||
} | ||
|
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 can be deleted as it is redundant now
|
||
|
||
|
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.
you can run prettier . --write
once
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.
@pixincreate Have a look now
@pixincreate please review my comments up there |
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.
you should move your changes to L416
. and it should look something like below:
if (redirection_url.host.endsWith(expected_url.host)) {
cy.wait(WAIT_TIME);
cy.window()
.its("location") // after 10 sec wait, the end url will be `hyperswitch.io/status=<payment_status>`
.then((location) => {
const url_params = new URLSearchParams(location.search);
const payment_status = url_params.get("status");
// now, do the validations
});
} else {
...
const urlParams = new URLSearchParams(redirection_url.search); | ||
const paymentStatus = urlParams.get('status'); |
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.
i tested your changes and it does not work. the reason being, redirection_url
is not hyperswitch.io
at this moment. a wait of at least 10 sec is needed.
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.
I have the wait time and necessary changes . @pixincreate Review it
Hey @Ankesh2004, I request you to revert the latest commit by executing below command in the terminal: git revert 2429fef Then, refer to this comment: #6187 (review) In short, you'll have to move the validations that is done outside of the conditional statements to inside it i.e., after: if (redirection_url.host.endsWith(expected_url.host)) {
// add them here |
Hey @Ankesh2004 Any update on the changes! |
Type of Change
This pull request Resolve #5915
Description
This PR refactors the verifyReturnUrl function to validate the redirection_url and ensure that the payment status is either succeeded or processing. If the status is invalid, the test will now fail, preventing false positives in redirection tests like 3DS or bank redirects.
###Key Changes:
Additional Changes
Motivation and Context
This change improves the reliability of Cypress redirection tests by ensuring that the final redirection URL reflects a valid payment status, preventing false positives and ensuring tests behave as expected for flows like 3DS and bank redirects.
How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy