Skip to content
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

Add AuthorizationRequired State #7471

Merged
merged 56 commits into from
Oct 23, 2023

Conversation

zmaglica
Copy link
Contributor

@zmaglica zmaglica commented Oct 12, 2023

Fixes #7418

Changes proposed in this Pull Request

This PR adds Authentication required state that handles the situation when payment needs to be authorized (3DS). Among others, this PR introduces multiple state changes.

Testing instructions

  • Make sure that all tests passed
  • Make sure to test with a credit card that requires 3DS (4000000000003063)
  • Make sure that you are redirected to the authorization URL.

  • Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

@zmaglica zmaglica marked this pull request as draft October 12, 2023 14:49
Copy link
Contributor

@RadoslavGeorgiev RadoslavGeorgiev left a comment

Choose a reason for hiding this comment

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

Great job so far, here's my initial feedback:

src/Internal/Payment/PaymentContext.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/InitialState.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/ProcessedState.php Show resolved Hide resolved
src/Internal/Service/PaymentProcessingService.php Outdated Show resolved Hide resolved
src/Internal/Service/CheckoutEncryptionService.php Outdated Show resolved Hide resolved
src/Internal/Service/CheckoutEncryptionService.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/VerifyState.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/VerifyState.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/VerifyState.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/ProcessedState.php Outdated Show resolved Hide resolved
@zmaglica
Copy link
Contributor Author

Great job so far, here's my initial feedback:

Thank you for the review. It really helped a lot. I have addressed all comments. Please let me know if there is anything to change or need additional feedback.

Copy link
Contributor

@RadoslavGeorgiev RadoslavGeorgiev left a comment

Choose a reason for hiding this comment

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

I like the direction of this PR, great job adjusting it to the concept! 👏

There are only a couple of topics I'd like to run by you in comments. In the meantime, feel free to proceed with the PR:

  1. Basic checkout still works, but it would be nice to see the new AuthenticationRequiredState handled as well.
  2. Psalm has a few errors, let's address them, and make sure that there are no left-over @throws in transition methods, especially within InitialState.
  3. There are various unused use statements. Let's tidy up the code by removing them.

It might make sense to address as much as possible before starting to fix existing tests, and adding new ones, to avoid having to change fresh tests.

src/Internal/Payment/State/AuthenticationRequiredState.php Outdated Show resolved Hide resolved
src/Internal/Payment/State/AuthenticationRequiredState.php Outdated Show resolved Hide resolved
src/Internal/Service/CheckoutEncryptionService.php Outdated Show resolved Hide resolved
@zmaglica zmaglica changed the title Rpp/7414 authentication required state Add AuthorizationRequired State Oct 18, 2023
@zmaglica zmaglica marked this pull request as ready for review October 18, 2023 16:07
Copy link
Contributor

@RadoslavGeorgiev RadoslavGeorgiev left a comment

Choose a reason for hiding this comment

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

Only a couple of small (new) things I noticed. There are some comments from previous reviews that are not addressed, probably because they were collapsed:

Uploading image.png…

/**
* The state, which indicates that the payment requires authentication (3DS).
*/
class AuthenticationRequiredState extends AbstractPaymentState {
Copy link
Member

Choose a reason for hiding this comment

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

Based on #7471 (comment):

Suggested change
class AuthenticationRequiredState extends AbstractPaymentState {
class PendingAuthenticationState extends AbstractPaymentState {

src/Internal/Payment/State/ProcessedState.php Show resolved Hide resolved
@zmaglica zmaglica force-pushed the rpp/7414-authentication-required-state branch from fc03e6a to d62420c Compare October 23, 2023 12:23
Copy link
Contributor

@RadoslavGeorgiev RadoslavGeorgiev left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

Thank you for bearing with me through all the nitpicks :)

@zmaglica
Copy link
Contributor Author

LGTM! :shipit:

Thank you for bearing with me through all the nitpicks :)

I love nitpicks 😃 Thank you for the extended review 🙇

@zmaglica zmaglica added this pull request to the merge queue Oct 23, 2023
Merged via the queue into develop with commit dd30c93 Oct 23, 2023
27 checks passed
@zmaglica zmaglica deleted the rpp/7414-authentication-required-state branch October 23, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPP / Processing / AuthenticationRequired state
5 participants