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

fix: handle OPF submit payment sequence error #19851

Merged
merged 5 commits into from
Jan 20, 2025
Merged

Conversation

Matejk00
Copy link
Contributor

Closes: CXSPA-9192

@Matejk00 Matejk00 requested a review from a team as a code owner January 10, 2025 11:25
@github-actions github-actions bot marked this pull request as draft January 10, 2025 11:25
@Matejk00 Matejk00 marked this pull request as ready for review January 10, 2025 11:29
Copy link

cypress bot commented Jan 10, 2025

spartacus    Run #46727

Run Properties:  status check passed Passed #46727  •  git commit e38351f147 ℹ️: Merge e7f7111b0ef71acd78b9860e0886f8f5b67875e6 into f334ee17e07977b304f0941ea737...
Project spartacus
Branch Review feature/CXSPA-9192
Run status status check passed Passed #46727
Run duration 03m 54s
Commit git commit e38351f147 ℹ️: Merge e7f7111b0ef71acd78b9860e0886f8f5b67875e6 into f334ee17e07977b304f0941ea737...
Committer Mateusz Kolasa
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 125
View all changes introduced in this branch ↗︎

*
* It will resolve with `true` if no values are emitted.
*/
last(() => true, true),
Copy link
Contributor

@FollowTheFlo FollowTheFlo Jan 13, 2025

Choose a reason for hiding this comment

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

if i understand, in scenario where error is thrown from this.opfPaymentFacade.submitPayment, we return 'true' rather than catching the error?
should we create a logic that if error occurs we returns 'false'?
In GooglePayService (line 406) we have

  catchError(() => {
              return of(false);
            })

It is the logic we wrote in dociumentation
https://help.sap.com/docs/SAP_COMMERCE_CLOUD_PUBLIC_CLOUD/0996ba68e5794b8ab51db8d25d4c9f8a/1fba7ed786b44700a21435ddc07bbc98.html

opfPaymentFacade.submit({
...
}).subscribe(success=>{
    // true when order has been successful placed
});
}

Copy link
Contributor Author

@Matejk00 Matejk00 Jan 16, 2025

Choose a reason for hiding this comment

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

Thanks @FollowTheFlo for the suggestion.

This situation arises from using the submit function as a global function, rather than invoking it within the context of a Spartacus.

Screenshot 2025-01-16 at 11 14 48

Key point here isn't about error handling, but rather about managing empty emissions. If this.opfPaymentFacade.submitPayment emits no values, we want to ensure we handle such case, otherwise we are getting an error in a browser console: https://stackoverflow.com/questions/41131476/emptyerror-no-elements-in-sequence telling that emission is empty.

The last operator is designed to emit the last value from an observable sequence or a default value if the sequence is empty. By providing true as a default value, we prevent sequence errors in the console when there are no emitted values. This approach helps maintain a consistent output and avoids any unexpected behavior.

In contrast, in the catchError logic we are explicitly handling potential errors that may occur during the payment submission process. Returning of(false) to indicate failure is appropriate for error conditions. But, in my case we are not going into the error state, so that is why I can't use such proposal.

Hope it clarifies the difference. In case of additional questions please let me know :)

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm, as discussed even though there is no catch in the pipe, partner can catch it from Promise type global function.

@github-actions github-actions bot marked this pull request as draft January 16, 2025 10:36
Copy link
Contributor

@FollowTheFlo FollowTheFlo left a comment

Choose a reason for hiding this comment

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

lgtm, see comments

@Matejk00 Matejk00 marked this pull request as ready for review January 20, 2025 10:29
@Matejk00 Matejk00 merged commit 61251c3 into develop Jan 20, 2025
28 checks passed
@Matejk00 Matejk00 deleted the feature/CXSPA-9192 branch January 20, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants