Revoke transaction fee if there is an amount to be credited in order

[apricot12]

What? Why?

Closes #3490

It seems, the revoking of transaction fee for certain situations is handled by :

    def ensure_correct_adjustment
      revoke_adjustment_eligibility if ['failed', 'invalid', 'void'].include?(state) 
      return if adjustment.try(:finalized?)

      if adjustment
        adjustment.originator = payment_method
        adjustment.label = adjustment_label
        adjustment.save
      elsif payment_method.present?
        payment_method.create_adjustment(adjustment_label, self, true)
        adjustment.reload
      end
    end

But I'm facing a a problem :
Once I add || can_credit? to the revoke_adjustment_eligibility if ['failed', 'invalid', 'void'].include?(state) condition, only the first credit is succesful without a transaction fee applied and all subsequent credits have the fee applied.

And even though 'credit' seems to be a valid state, adding it to the state conditions seems to have no effect.

We can ensure that the payment method fee adjustment is not created by adding the condition return if order.payment_state == 'credit_owed to the ensure_correct_adjustment method.

What should we test?

• There should be no transaction fee/payment method fee applied for crediting an amount.

Release notes

• Transaction fee is not applied when crediting an order.

Changelog Category: Technical changes

[apricot12]

@jibees Since you've worked on this block of code before, maybe you'll have some insight?

[jibees]

Hey @apricot12 !
I've worked on it, and yes, it's tricky. Take everything I said here with a grain of salt.

I don't think this code is good, because of all the failing tests here. What I would do:

• Create an automatic test, that recreate the described scenario in the linked issue. I think the place to do this is spec/models/spree/payment_spec.rb on line #383 and should look like this (this code isn't right, take it just as a canva) :

        context "if payment method has any adjustments", :debug do
          let!(:payment_method) { create(:payment_method, calculator: ::Calculator::FlatRate.new(preferred_amount: 10)) }

          before do
            payment.update!(payment_method: payment_method)
            payment.save!
          end

          it "should not applied any transaction fees" do
            payment.credit!
            expect(order.all_adjustments.payment_fee.eligible.length).to eq(0)
          end
        end

• Then, correct the test. I don't think the test (can_credit?) is the right one since it only test that the payment allow some credit. I since it's more around the method credit on file app/models/spree/payment/processing.rb#78.

Sorry, I don't know if it's really helpful, but here's my 2 cents.

[mkllnk]

Good work on finding the right code part to do this. I had an idea for an improvement (comment below). And I think that this should be covered by a spec. It's always a good idea to write a spec to re-produce a bug and then fix it. In this case, a model spec should be enough.

[mkllnk]

This should work in most cases, but checking the state could go wrong. In theory, we could trigger another payment while credit is owed. Then we would owe even more. It doesn't seem to make sense at first but maybe we want to check if a payment goes through before adding an item.

Anyway, I think that the underlying assumption is that we don't pay transaction fees on refunds, no matter what the state of the order is. I would guess that we can check amount <= 0.

It also looks to me as if revoke_adjustment_eligibility would make sense in this case. So maybe it could look like this:

    def ensure_correct_adjustment
      revoke_adjustment_eligibility if failed_or_refund?
      ...
    end

    def failed_or_refund?
      ['failed', 'invalid', 'void'].include?(state) || amount <= 0
    end

[apricot12]

@mkllnk revoke_adjustment_eligibility looked like the obvious choice here to me too, but it doesnt get executed when we use conditions like amount <=0, order.payment_state == 'credit_owed or order.payment_state == paid.

[mkllnk]

I don't understand. The revoke_adjustment_eligibility should finalise the adjustment and the next line returns if the adjustment if finalised:

openfoodnetwork/app/models/spree/payment.rb

Lines 170 to 178 in 1cbb0d8

	def revoke_adjustment_eligibility
	return unless adjustment.try(:reload)
	return if adjustment.finalized?
	adjustment.update(
	eligible: false,
	state: "finalized"
	)
	end

openfoodnetwork/app/models/spree/payment.rb

Lines 143 to 144 in 1cbb0d8

	revoke_adjustment_eligibility if ['failed', 'invalid', 'void'].include?(state)
	return if adjustment.try(:finalized?)

Can you write a spec first? Then we have an example to experiment with and you can show me what you mean.

[apricot12]

@mkllnk After using debugger, it's at line 175 that the execution of revoke_adjustment_eligibility stops when using argument amount <= 0:

openfoodnetwork/app/models/spree/payment.rb

Lines 175 to 176 in 7219384

	return unless adjustment.try(:reload)
	return if adjustment.finalized?

But as you can see from the specs, both of these conditions are supposed to be passing. I'm not sure what exactly is going on in this scenario? Is it because once the adjustment is made when you credit, it go's into the closed state?

[mkllnk]

Thanks for the spec! That's very helpful. It looks like we don't have an adjustment yet when this code is called. The states 'failed', 'invalid', 'void' refer to a changing payment while a refund payment is a new one. I didn't think of that. So my previous suggestion doesn't work.

I'm wondering if we prevent the adjustment creation instead. We code change CalculatedAdjustments but then it needs to know the difference between payments and other things with adjustments. Not good.

Maybe it's enough to modify this method here further down to avoid the creation of an adjustment:

openfoodnetwork/app/models/spree/payment.rb

Line 150 in 7219384

elsif payment_method.present?

-      elsif payment_method.present?
+      elsif amount.positive? &&  payment_method.present?

I saw in CalculatedAdjustments that some adjustments are mandatory and then we need to add a 0 amount adjustment but we can assume that no transaction fees are mandatory at this stage. Not adding an adjustment is much easier.

Would that work?

[apricot12]

@mkllnk Yes not creating the adjustment itself definitely works, and from what I've seen of the related code not adding payment adjustment when refunding shouldn't really affect anything else.

OH so when we're credit some amount back, It's a whole new payment! I didn't think of it that way though it seems so obvious to me now.

I'll update the commits and change up the specs a little bit 👍

[mkllnk]

Almost there.

[mkllnk]

Looks good to me.

[Matt-Yorkley]

Great! I rebased and added a couple of "review-commits" to tighten up the tests a bit and increase the clarity. I left some notes in the commit messages 👌

What do you think of this change: 8084ad0?

There's an old saying in Software Development:

"Today, only you and god understand your code. In six months, only god will understand your code."

I try to keep this in mind when I'm writing bits of code, and ask myself: "if I come back to this in 6 months when I've completely forgotten all the surrounding context, will it make sense or will I have to find the issue and read all the notes before I can figure out why it's there?" Trying to make my code self-explanatory and idiot-proof is a valuable strategy, especially because the idiot is usually me, in the future. 😅

[Matt-Yorkley]

👍

[apricot12]

@Matt-Yorkley Haha that change is actually great. It provides all the context one would need from just from the name!

I'll keep it in mind to make everything as idiot proof as possible in future.

[filipefurtad0]

Hey @apricot12 ,

Before this PR:

• removing an item
• issuing a partial refund
• charges a second transaction fee

After this PR:

We can see that the second fee is not charged. Great work 💪