Allow providing an existing payment during the checkout by elia · Pull Request #4909 · solidusio/solidus

elia · 2023-02-06T18:20:41Z

Summary

This allows modern payment methods (relying on tokens and frontend confirmation) to have a record on solidus' side that can track events and the payment status.

Creating a payment before or during the payment step of the checkout was already possible, but resulted in an additional payment to be created before transitioning to the confirm step and invalidating all previous payments.

This way it can simply be provided via its id, and fetched from the payments currently associated with the order.

If payment methods will handle their own payment creation in the future we can distance ourselves from the credit-card coupled code and have a generic payment interface that can better serve modern payment methods.

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 I have updated the README to account for my changes.
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

codecov · 2023-02-06T21:25:31Z

Codecov Report

Merging #4909 (cabfa3f) into master (019ba27) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head cabfa3f differs from pull request most recent head 99483d4. Consider uploading reports for the commit 99483d4 to get more accurate results

@@           Coverage Diff           @@
##           master    #4909   +/-   ##
=======================================
  Coverage   86.69%   86.70%           
=======================================
  Files         578      578           
  Lines       14673    14679    +6     
=======================================
+ Hits        12721    12727    +6     
  Misses       1952     1952

Impacted Files	Coverage Δ
core/app/models/spree/payment_create.rb	`93.47% <100.00%> (+0.97%)`	⬆️
core/lib/spree/permitted_attributes.rb	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

waiting-for-dev

Thanks for tackling the improvement of the payment system, @elia. That's not an easy task! 🙂

I left a couple of minor comments. However, I'm concerned about the security repercusions of this change. If I understand well, the payment id could come from the attributes submitted by the user. I see that they are scoped from the payments belonging to the order. At the very least, we should add a test for that, ensuring that no other existent payment can be used. However, do you think it could create some mess in the case a user provides the id for a failed payment, or any other edge case that could be exploited?

elia · 2023-02-08T12:26:29Z

If I understand well, the payment id could come from the attributes submitted by the user. I see that they are scoped from the payments belonging to the order.

Yes, this is mainly a tool for payment methods to create the payment before (or while) the payment page is shown.

At the very least, we should add a test for that, ensuring that no other existent payment can be used.

I'll add one 👍

I'm concerned about the security repercussions of this change […] do you think it could create some mess in the case a user provides the id for a failed payment, or any other edge case that could be exploited?

Currently Solidus allows completing an order even with a payment with a status of :checkout and the enforcement is left to the frontend.

I think being able to provide existing payments associated to the order shouldn't change much, except not creating a new one, since the existing payments are already associated and count toward the balance.

@waiting-for-dev

kennyadsl

Second pass!

kennyadsl · 2023-02-08T15:34:58Z

    #   * :source_attributes Attributes used to build the source of this payment. Usually a {CreditCard}
    #     * :wallet_payment_source_id (Integer): The id of a {WalletPaymentSource} to use
    # @param request_env [Hash] rack env of user creating the payment
    # @param payment [Payment] Internal use only. Instead of making a new payment, change the attributes for an existing one.


If this is deprecated now, we should mark it as such in the YARD documentation

kennyadsl · 2023-02-08T15:35:49Z

+
    # @param order [Order] The order for the new payment
    # @param attributes [Hash,ActionController::Parameters] attributes which are assigned to the new payment
    #   * :payment_method_id Id of payment method used for this payment


What about adding documentation for the new id attribute, required in case we want to use an existing payment?

kennyadsl · 2023-02-08T15:38:00Z

+      end
+    end
+
+    context 'with an unknown payment' do


What if we add a test of a payment coming from another order as well?

kennyadsl · 2023-02-08T15:39:06Z

      @order = order
-      @payment = payment
+
+      if payment != PAYMENT_NOT_PROVIDED


Why not just if payment.present? here?

Got it now, thanks @spaghetticode (#3338 (comment))

It's not being used anywhere in solidus, despite the inline comments saying it's for internal use.

This allows modern payment methods (relying on tokens and frontend confirmation) to have a record on solidus' side that can track events and the payment status. Creating a payment before or during the payment step of the checkout was already possible, but resulted in an additional payment to be created before transitioning to the confirm step and invalidating all previous payments. This way it can simply be provided via its id, and fetched from the payments currently associated with the order. If payment methods will handle their own payment creation in the future we can distance ourselves from the credit-card coupled code and have a generic payment interface that can better serve modern payment methods.

For payments `checkout` is the initial state before any attempt at paying is done, so the `checkout` state should not be considered as "valid". Although we're not providing an official HTML frontend with the main gem we're still providing a REST API interface.

elia · 2023-03-21T09:44:09Z

Was split into other PRs

elia self-assigned this Feb 6, 2023

github-actions Bot added the changelog:solidus_core Changes to the solidus_core gem label Feb 6, 2023

elia marked this pull request as ready for review February 6, 2023 21:14

elia requested a review from a team as a code owner February 6, 2023 21:14

waiting-for-dev reviewed Feb 7, 2023

View reviewed changes

Comment thread core/spec/models/spree/payment_create_spec.rb Outdated

Comment thread core/app/models/spree/payment_create.rb

kennyadsl reviewed Feb 7, 2023

View reviewed changes

Comment thread core/app/models/spree/payment_create.rb Outdated

elia force-pushed the elia/existing-payment-support branch 2 times, most recently from 49d3d91 to cabfa3f Compare February 7, 2023 11:29

elia requested review from kennyadsl and waiting-for-dev February 8, 2023 12:26

kennyadsl reviewed Feb 8, 2023

View reviewed changes

elia marked this pull request as draft February 9, 2023 15:42

elia added 2 commits February 13, 2023 18:57

Check for source attributes at the same abstraction level

5f1490f

Deprecate passing a payment to PaymentCreate

e818496

It's not being used anywhere in solidus, despite the inline comments saying it's for internal use.

elia force-pushed the elia/existing-payment-support branch from cabfa3f to 99483d4 Compare February 13, 2023 18:08

github-actions Bot added the changelog:solidus_api Changes to the solidus_api gem label Feb 13, 2023

elia force-pushed the elia/existing-payment-support branch from 99483d4 to c53e2ab Compare February 13, 2023 18:43

elia force-pushed the elia/existing-payment-support branch from c53e2ab to 76f9d30 Compare February 13, 2023 18:46

elia closed this Mar 21, 2023

elia deleted the elia/existing-payment-support branch March 21, 2023 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Allow providing an existing payment during the checkout#4909

Allow providing an existing payment during the checkout#4909
elia wants to merge 4 commits into
masterfrom
elia/existing-payment-support

elia commented Feb 6, 2023 •

edited

Loading

Uh oh!

codecov Bot commented Feb 6, 2023 •

edited

Loading

Uh oh!

waiting-for-dev left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elia commented Feb 8, 2023

Uh oh!

kennyadsl left a comment

Uh oh!

kennyadsl Feb 8, 2023

Uh oh!

kennyadsl Feb 8, 2023

Uh oh!

kennyadsl Feb 8, 2023

Uh oh!

kennyadsl Feb 8, 2023

Uh oh!

kennyadsl Mar 10, 2023

Uh oh!

elia commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

elia commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

codecov Bot commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

waiting-for-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elia commented Feb 8, 2023

Uh oh!

kennyadsl left a comment

Choose a reason for hiding this comment

Uh oh!

kennyadsl Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

kennyadsl Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

kennyadsl Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

kennyadsl Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

kennyadsl Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

elia commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elia commented Feb 6, 2023 •

edited

Loading

codecov Bot commented Feb 6, 2023 •

edited

Loading