Refund and create a new order transaction when an order is recalculated #139

benjaminwil · 2021-12-16T23:23:41Z

What is the goal of this PR?

The goal of this PR is to:

Subscribe to "order_recalculated" Spree::Events.
If the spree_order.total does not equal the amounts as they are in the
matching TaxJar order transactions (closing Allow checking whether TaxJar has up-to-date order totals #106), we need to refund that
transaction and create a new one with the new amounts (closing Custom TaxJar transaction update logic (refund then create) #112).
In all other cases, we currently do nothing with the "order_recalculated"
subscriber.

How do you manually test these changes? (if applicable)

In a sandbox application:

Enable solidus_taxjar reporting.
Ensure a tax rate named "Sales Tax" is setup
Complete an order and pay for it (and then capture the payment)
Mark the order as shipped
Check that spree_order.order_transactions has a record.
Update the order total.
- One easy way to do this would be to check out with two line items and then cancel one of them.
- Order-level adjustments will not work.
See that the order_recalculated event has fired.
Check that spree_order.order_transactions has a second record.
Check the TaxJar reporting dashboard to ensure the reporting totals are correct.

Merge Checklist

Run the manual tests
Update the changelog

Screenshots

N/A

benjaminwil · 2021-12-16T23:52:02Z

While doing manual tests, we realized we've missed one condition:

The #update_transaction reporting subscriber should only happen if the order has already shipped. At least until we've gotten rid of this StandardError, which will be replaced by functionality we just haven't built out yet.

Currently it's impossible to capture a payment if reporting is enabled.

benjaminwil · 2022-02-03T23:16:33Z

I will perform the manual tests tomorrow. But in the meantime, this code is reviewable.

Noah-Silvera

This is some tricky stuff, thanks for diving into it! Overall it looks great, just a couple of comments!

lib/super_good/solidus_taxjar/testing_support/factories/order_transaction_factory.rb

lib/super_good/solidus_taxjar/reporting.rb

app/subscribers/super_good/solidus_taxjar/spree/reporting_subscriber.rb

spec/jobs/super_good/solidus_taxjar/report_transaction_job_spec.rb

benjaminwil · 2022-02-04T17:20:40Z

@Noah-Silvera Thank you for such a thoughtful review!

forkata

Thanks Ben and Co. great work here. Left a few comments around what Noah brought up and I think we should address those concerns before merging this.

lib/super_good/solidus_taxjar/api.rb

app/jobs/super_good/solidus_taxjar/replace_transaction_job.rb

benjaminwil · 2022-02-05T00:58:07Z

@forkata Thank you for the thoughtful review !

app/subscribers/super_good/solidus_taxjar/spree/reporting_subscriber.rb

Noah-Silvera · 2022-02-23T21:40:38Z

spec/super_good/solidus_taxjar/reporting_spec.rb

+  describe "#refund_and_create_transaction" do
+    subject { reporting.refund_and_create_new_transaction(order) }
+
+    it "refunds the transaction and creates a new one in TaxJar" do


Out of curiosity, why aren't we using VCR for this test but we are for the others?

This was the only test case where we explicitly cared about an error class from an external library. (A Taxjar::Error subclass.)

I would like to use VCR more but this is the only case where it seemed absolutely necessary to help our future selves make sense of the extension code being the way it is: @api.create_transaction(order) will not get executed if @api.refund_transaction(order) fails beforehand.

Noah-Silvera · 2022-02-23T21:45:06Z

Thanks for your patience 🐢

stewart

🙇

app/subscribers/super_good/solidus_taxjar/spree/reporting_subscriber.rb

benjaminwil · 2022-03-07T21:50:07Z

During manual testing, I noticed there's an issue with where the re-reported totals do not match the updated order totals. I will need to debug this.

We noticed that the `OrderTransaction` factory could create test failures due to a misuse of `#sequence`. The second order transaction created would always be suffixed with `-1`, the third would be suffixed with `-2`, and so on. This is not how order transactions work in the real world. Co-Authored-By: Alex Blackie <[email protected]>

If a TaxJar transaction already exists for a Solidus order and the order totals have changed, we need to cancel (or refund) the existing TaxJar transaction and create a new one with the latest Solidus order totals on it. If the refund fails for some reason, we *do not* want to create the new transaction. So we're testing that a new transaction is not created in that case. We can use this service method to do that in a child commit. Co-Authored-By: Alex Blackie <[email protected]> Co-Authored-By: Mike Conlin <[email protected]>

If a TaxJar order transaction already exists for a Solidus order, we need to increment the transaction ID used. For example: If an order transaction for Solidus order number R123 already exists in TaxJar with the transaction ID "R123", the next transaction ID will be "R123-1". As of this commit, we're checking for order transactions known to exist and incrementing based on the existing transactions IDs. Co-Authored-By: Mike Conlin <[email protected]>

Previously called `#report_transaction`, this method is no longer going to be the only method that technically reports transactions. So it seems like a good moment to rename it to be more specific. Co-Authored-By: Alex Blackie <[email protected]>

If an order total changes, we need to re-report the transaction with the updated order totals. This commit adds a flow for refunding the inaccurate transaction and creating a new one to replace it. This is TaxJar's recommended way of dealing with out-of-date order transactions. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Alex Blackie <[email protected]> Co-Authored-By: Chris Todorov <[email protected]> Co-Authored-By: Nicholas Van Doorn <[email protected]>

Since we implemented this feature, a new `preferred_reporting_enabled` configuration option has appeared. This commit updates specs to conform to it. It was a very nasty rebase, so we aren't rebasing this. Co-authored-by: Alex Blackie <[email protected]>

Our `ReportingSubscriber#replace_transaction` subscriber should only try to replace an existing transaction if one exists.

The first time that a TaxJar order transaction is created for an order, we know that TaxJar won't already have a transaction on record. We still want to defensively check for a foreign reference to the TaxJar order transaction in the context of an ActiveJob, in case the job is run more than once. In a subsequent change, we'll be re-adding the `NotImplementedError` we removed here in our refund flow. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Noah Silvera <[email protected]> Co-Authored-By: Chris Todorov <[email protected]>

We haven't finished implementing the refund-and-recreate TaxJar order transactions flow for the TaxJar reporting feature. For now, we'll be sure to raise a `NotImplementedError` if we attempt to create a refund transaction for an order we have no record of being previously reported. Co-Authored-By: Adam Mueller <[email protected]> Co-Authored-By: Chris Todorov <[email protected]> Co-Authored-By: Noah Silvera <[email protected]>

It turns out that `Order#paid?` returns `true` even if credit is owed. So we want to explicitly check that the order's payment state is "paid" and not some other value, like "credit_owed". Co-Authored-By: Noah Silvera <[email protected]>

nvandoorn · 2022-03-22T23:47:17Z

Latest push rebases onto master.

Noah-Silvera · 2022-03-23T22:06:43Z

Note: @nvandoorn and I tested this and were able to get it to work when rebased on a fix to the discount calculator #181

We tested it by removing X quantity from a line item and then issuing a refund for the payment

One test we didn't do which we probably should is testing having two separate line items, and removing one entirely.

nvandoorn · 2023-09-12T00:34:07Z

Thanks @joeleonjr. I deleted the comment and rotated the key. Fortunately it's only a test TaxJar trial account.

benjaminwil changed the title ~~Update TaxJar order transactions when "order_recalculate" events are fired~~ Refund and create a new order transaction when an order is recalculated Dec 16, 2021

benjaminwil force-pushed the bw/order-recalculated-subscriber branch from 1ea8154 to 64b59e3 Compare January 21, 2022 23:18

benjaminwil force-pushed the bw/order-recalculated-subscriber branch from 64b59e3 to 6568158 Compare February 3, 2022 23:06

benjaminwil requested review from forkata, Noah-Silvera, senemsoy, adammathys, alexblackie and nvandoorn February 3, 2022 23:15

Noah-Silvera requested changes Feb 4, 2022

View reviewed changes

forkata reviewed Feb 4, 2022

View reviewed changes

lib/super_good/solidus_taxjar/api.rb Show resolved Hide resolved

app/jobs/super_good/solidus_taxjar/replace_transaction_job.rb Show resolved Hide resolved

Noah-Silvera mentioned this pull request Feb 15, 2022

Seed tax rate in dummy app and sandbox #164

Open

benjaminwil force-pushed the bw/order-recalculated-subscriber branch 2 times, most recently from 899465c to 8574280 Compare February 17, 2022 23:43

benjaminwil requested review from forkata and Noah-Silvera February 17, 2022 23:50

Noah-Silvera approved these changes Feb 23, 2022

View reviewed changes

stewart approved these changes Feb 25, 2022

View reviewed changes

Noah-Silvera reviewed Feb 28, 2022

View reviewed changes

app/subscribers/super_good/solidus_taxjar/spree/reporting_subscriber.rb Outdated Show resolved Hide resolved

Noah-Silvera approved these changes Mar 7, 2022

View reviewed changes

Noah-Silvera mentioned this pull request Mar 21, 2022

Don't refund orders with unaccounted for order level adjustments #180

Open

benjaminwil removed the request for review from alexblackie March 21, 2022 22:12

benjaminwil and others added 2 commits March 22, 2022 14:21

benjaminwil and others added 9 commits March 22, 2022 14:21

Only #replace_transaction if it's replaceable

6775f15

Our `ReportingSubscriber#replace_transaction` subscriber should only try to replace an existing transaction if one exists.

Don't replace transaction if credit is owed

2d3636c

It turns out that `Order#paid?` returns `true` even if credit is owed. So we want to explicitly check that the order's payment state is "paid" and not some other value, like "credit_owed". Co-Authored-By: Noah Silvera <[email protected]>

Update CHANGELOG

9a029a0

nvandoorn force-pushed the bw/order-recalculated-subscriber branch from 410c1b5 to 9a029a0 Compare March 22, 2022 23:46

nvandoorn merged commit 416c09b into master Apr 20, 2022

nvandoorn deleted the bw/order-recalculated-subscriber branch April 20, 2022 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refund and create a new order transaction when an order is recalculated #139

Refund and create a new order transaction when an order is recalculated #139

benjaminwil commented Dec 16, 2021 •

edited by nvandoorn

Loading

benjaminwil commented Dec 16, 2021 •

edited

Loading

benjaminwil commented Feb 3, 2022

Noah-Silvera left a comment •

edited

Loading

benjaminwil commented Feb 4, 2022

forkata left a comment

benjaminwil commented Feb 5, 2022

Noah-Silvera Feb 23, 2022

benjaminwil Feb 23, 2022

Noah-Silvera commented Feb 23, 2022

stewart left a comment

benjaminwil commented Mar 7, 2022

nvandoorn commented Mar 22, 2022

Noah-Silvera commented Mar 23, 2022

nvandoorn commented Sep 12, 2023

Refund and create a new order transaction when an order is recalculated #139

Refund and create a new order transaction when an order is recalculated #139

Conversation

benjaminwil commented Dec 16, 2021 • edited by nvandoorn Loading

What is the goal of this PR?

How do you manually test these changes? (if applicable)

Merge Checklist

Screenshots

benjaminwil commented Dec 16, 2021 • edited Loading

benjaminwil commented Feb 3, 2022

Noah-Silvera left a comment • edited Loading

Choose a reason for hiding this comment

benjaminwil commented Feb 4, 2022

forkata left a comment

Choose a reason for hiding this comment

benjaminwil commented Feb 5, 2022

Noah-Silvera Feb 23, 2022

Choose a reason for hiding this comment

benjaminwil Feb 23, 2022

Choose a reason for hiding this comment

Noah-Silvera commented Feb 23, 2022

stewart left a comment

Choose a reason for hiding this comment

benjaminwil commented Mar 7, 2022

nvandoorn commented Mar 22, 2022

Noah-Silvera commented Mar 23, 2022

nvandoorn commented Sep 12, 2023

benjaminwil commented Dec 16, 2021 •

edited by nvandoorn

Loading

benjaminwil commented Dec 16, 2021 •

edited

Loading

Noah-Silvera left a comment •

edited

Loading