Only credit back allowed amount #1603

gmacdougall · 2016-11-22T14:40:57Z

This fixes an issue with payment refunding. To reproduce:

Give a user $100 in store credit
Purchase an item with the user for less than $100.
Refund a portion of the payment through the admin
Attempt to cancel the order

This will result in an exception as it will attempt to give the user more
store credit than they could potentially have available.

This modifies the store credit payment cancel to give back the maximum allowed
rather than the full amount.

Additionally, this gets rid of some terrible logic which only works with currencies
that have two decimal places which multiplies and divides things by 100.

bbuchalter · 2016-11-22T15:41:08Z

core/spec/models/spree/payment_spec.rb

+
+    context "when there are offsets" do
+      before do
+        payment.offsets.create(


Prefer create! to ensure errors are raised in test setup.

bbuchalter · 2016-11-22T15:42:22Z

core/app/models/spree/payment_method/store_credit.rb

@@ -67,7 +67,7 @@ def credit(amount_in_cents, auth_code, gateway_options = {})
        originator = gateway_options[:originator]

        store_credit.credit(
-          amount_in_cents / 100.0.to_d,
+          ::Money.new(amount_in_cents, currency).to_d,


Is the .to_d still required now that we're returning a Fixnum from amount_in_cents?

This is creating a RubyMoney in order to reverse the amount_in_cents logic. Example:

[1] pry(main)> ::Money.new(1000, 'USD').to_d.to_s => "10.0" [2] pry(main)> ::Money.new(1000, 'JPY').to_d.to_s => "1000.0"

bbuchalter · 2016-11-22T15:44:24Z

core/app/models/spree/payment_method/store_credit.rb

-        amount_in_cents = (store_credit_event.amount * 100).round
-        credit(amount_in_cents, auth_code)
+        credit(
+          store_credit_event.originator.credit_allowed_in_cents,


Can the originator here ever be something other than a payment? I assume so, and cause this will break.

As this is going through the PaymentMethod I would hope not, but I should raise a more appropriate exception in that case.

I just assumed since a polymorphic association is used, we should expect it possible that something other than a payment method be the originator. Maybe that should be changed to not be polymorphic?

bbuchalter · 2016-11-22T15:45:11Z

core/spec/models/spree/payment_method/store_credit_spec.rb

@@ -267,15 +267,17 @@
        create(:store_credit_capture_event,


The spacing here needs some help. Perhaps in a separate commit?

bbuchalter · 2016-11-22T15:45:50Z

core/spec/models/spree/payment_method/store_credit_spec.rb


      it_behaves_like "a spree payment method"

      it "refunds the capture amount" do
        expect { subject }.to change{ store_credit.reload.amount_remaining }.
-                              from(original_amount - captured_amount).
-                              to(original_amount)
+          from(90).to(100)


This is a great improvement, thank you!

bbuchalter · 2016-11-22T15:49:41Z

core/app/models/spree/payment_method/store_credit.rb

+          auth_code,
+          currency,
+          action_originator: originator
+        ) if amount_in_cents > 0


Can we add a regression test for this guard?

gmacdougall · 2016-11-23T16:57:53Z

Going to do some additional work on this. There's an issue with cancelling an order containing a fully refunded store credit payment.

gmacdougall · 2016-11-23T19:57:56Z

I have corrected the error which occurred when cancelling an order which was previously fully refunded.

cbrunsdon · 2016-11-25T11:57:27Z

core/app/models/spree/payment.rb

+    #
+    # @return [Fixnum] The amount of this payment minus offets and refunds
+    #   in cents.
+    def credit_allowed_in_cents


Whats the rational here for returning the in_cents rather than the money object and letting the caller do with it as it wishes?

@gmacdougall could you follow up here?

This will not attempt to credit store credit for zero dollar amount, which will trigger errors.

Previously there was some unsafe math where numbers were multiplied and divided by 100 in the store credit section. This is a step towards getting rid of that.

When a payment is cancelled, previously it would attempt to return the whole amount to the store credit even if it had been entirely or partially refunded.

cbrunsdon

My feedback is resolved, thanks 👍

gmacdougall · 2017-10-04T18:50:33Z

This was superseded by #2111

gmacdougall force-pushed the store-credit-allowed-amount branch 2 times, most recently from f898e19 to b8abebe Compare November 22, 2016 15:16

bbuchalter reviewed Nov 22, 2016

View reviewed changes

gmacdougall force-pushed the store-credit-allowed-amount branch 2 times, most recently from c8314ff to fbc2b60 Compare November 22, 2016 20:25

bbuchalter approved these changes Nov 22, 2016

View reviewed changes

gmacdougall mentioned this pull request Nov 22, 2016

Store credit cancel TommyJohnWear/solidus#6

Merged

gmacdougall force-pushed the store-credit-allowed-amount branch from fbc2b60 to d73f8f0 Compare November 23, 2016 13:25

gmacdougall force-pushed the store-credit-allowed-amount branch from d73f8f0 to d24ca62 Compare November 23, 2016 19:28

cbrunsdon reviewed Nov 25, 2016

View reviewed changes

Only attempt to credit positive amount

6e5dc51

This will not attempt to credit store credit for zero dollar amount, which will trigger errors.

gmacdougall force-pushed the store-credit-allowed-amount branch from d24ca62 to 793f44e Compare July 3, 2017 17:09

Gregor MacDougall added 2 commits July 3, 2017 13:12

Add credit allowed as Spree::Money

7c4cc74

Previously there was some unsafe math where numbers were multiplied and divided by 100 in the store credit section. This is a step towards getting rid of that.

Only credit back allowed amount

04eccbe

When a payment is cancelled, previously it would attempt to return the whole amount to the store credit even if it had been entirely or partially refunded.

gmacdougall force-pushed the store-credit-allowed-amount branch from 793f44e to 04eccbe Compare July 3, 2017 17:12

gmacdougall added the Code review needed label Jul 5, 2017

cbrunsdon approved these changes Jul 5, 2017

View reviewed changes

jhawthorn approved these changes Oct 4, 2017

View reviewed changes

gmacdougall closed this Oct 4, 2017

gmacdougall deleted the store-credit-allowed-amount branch October 4, 2017 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only credit back allowed amount #1603

Only credit back allowed amount #1603

gmacdougall commented Nov 22, 2016 •

edited

Loading

bbuchalter Nov 22, 2016

gmacdougall Nov 22, 2016

bbuchalter Nov 22, 2016

gmacdougall Nov 22, 2016

bbuchalter Nov 22, 2016

gmacdougall Nov 22, 2016

bbuchalter Nov 22, 2016

bbuchalter Nov 22, 2016

bbuchalter Nov 22, 2016

bbuchalter Nov 22, 2016

gmacdougall Nov 22, 2016

gmacdougall commented Nov 23, 2016

gmacdougall commented Nov 23, 2016

cbrunsdon Nov 25, 2016

bbuchalter May 26, 2017

cbrunsdon left a comment

gmacdougall commented Oct 4, 2017

Only credit back allowed amount #1603

Only credit back allowed amount #1603

Conversation

gmacdougall commented Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmacdougall commented Nov 23, 2016

gmacdougall commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrunsdon left a comment

Choose a reason for hiding this comment

gmacdougall commented Oct 4, 2017

gmacdougall commented Nov 22, 2016 •

edited

Loading