SUPESC-280: Added a result code check when a payment/details API call was performed.
#39
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
herasimenko1987
changed the title
herasimenko1987
changed the title
herasimenko1987
changed the title
herasimenko1987
changed the title
payment/details api call was performed.
romanhavrylko
approved these changes
Dec 13, 2021
src/SprykerEco/Zed/Adyen/Business/Handler/Redirect/OnlineTransferRedirectHandler.php
Outdated
Show resolved
Hide resolved
dmiseev
approved these changes
Dec 13, 2021
…gfix/supesc-280-adyen-sends-information-about-refused-authorization-which-needs-to-be-checked
dmytro-sokolovskyi
changed the title
payment/details api call was performed.payment/details API call was performed.
dmytro-sokolovskyi
suggested changes
Jan 12, 2022
There was a problem hiding this comment.
- I think, you should also mention changes
AdyenFacade::handleOnlineTransferResponseFromAdyen()andAdyenFacade::handleCreditCard3dResponseFromAdyen()in the change log. - Release Type looks like minor not patch.
- The bug MUST be tested. Please, add coverage for this case in FacadeTest.
src/SprykerEco/Zed/Adyen/Business/Handler/Redirect/OnlineTransferRedirectHandler.php
Outdated
Show resolved
Hide resolved
…esc-280-adyen-sends-information-about-refused-authorization-which-needs-to-be-checked � Conflicts: � tests/SprykerEcoTest/Zed/Adyen/Business/BaseSetUpTest.php
dmytro-sokolovskyi
suggested changes
Aug 16, 2022
There was a problem hiding this comment.
- We shouldn't describe
GatewayControllerin the change log. - Please, use the template to describe changes and avoid the technical details. Describe the business logic changes.
- Please, think a bit about how to improve PR/RG title based on our requirements.
- Fix Travis.
tests/SprykerEcoTest/Zed/Adyen/_support/_generated/AdyenZedTesterActions.php
Outdated
Show resolved
Hide resolved
added 2 commits
August 16, 2022 16:56
…-authorization-which-needs-to-be-checked' of github.com:spryker-eco/adyen into bugfix/supesc-280-adyen-sends-information-about-refused-authorization-which-needs-to-be-checked
|
Please fix change logs markup. methods should be |
kkicheglovspryker
approved these changes
Sep 28, 2022
DmitryLymarenko
suggested changes
Sep 28, 2022
| @@ -85,6 +85,15 @@ public function handle(AdyenRedirectResponseTransfer $redirectResponseTransfer): | |||
| return $redirectResponseTransfer; | |||
| } | |||
|
|
|||
| $resultCodeLower = strtolower($responseTransfer->getPaymentDetailsResponseOrFail()->getResultCode()); | |||
| if ($resultCodeLower === $this->config->getAdyenPaymentStatusRefused()) { | |||
There was a problem hiding this comment.
$this->config->getAdyenPaymentStatusRefused() call result could be moved to a variable to not call it twice
| * | ||
| * @return \Generated\Shared\Transfer\AdyenRedirectResponseTransfer | ||
| */ | ||
| protected function createRefusedRedirectResponseTransfer(OrderTransfer $orderTransfer): AdyenRedirectResponseTransfer |
There was a problem hiding this comment.
this method duplicates code from createRedirectResponseTransfer
result code could be passed as an argument instead
| */ | ||
| public function testHandleOnlineTransferResponseFromAdyenWithRefusedStatus(): void | ||
| { | ||
| //Arrange |
There was a problem hiding this comment.
Suggested change
| //Arrange | |
| // Arrange |
same for similar cases
| $redirectResponseTransfer = $this->createRefusedRedirectResponseTransfer($orderTransfer); | ||
|
|
||
| //Act | ||
| $result = $facade->handleOnlineTransferResponseFromAdyen($redirectResponseTransfer); |
There was a problem hiding this comment.
result naming is unacceptable
facade returns AdyenRedirectResponseTransfer
what is the reason you use result?
| $result = $facade->handleOnlineTransferResponseFromAdyen($redirectResponseTransfer); | ||
|
|
||
| //Assert | ||
| $this->assertEquals(static::REDIRECT_RESPONSE_RESULT_CODE_REFUSED, $result->getResultCode()); |
There was a problem hiding this comment.
never use assertEquals on non-object types
assertSame must be used instead
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.
Developer(s): @herasimenko1987, @Zingeon
Ticket: https://spryker.atlassian.net/browse/SUPESC-280
Release Group: https://release.spryker.com/release-groups/view/3863
merge: squash
Release Table
Module Adyen
Change log
Improvements
AdyenFacade::handleOnlineTransferResponseFromAdyen()so it handles refused authorisation from the API.AdyenFacade::handleCreditCard3dResponseFromAdyen()so it handles refused authorisation from the API.AdyenConfig::getAdyenPaymentStatusRefused().