[150] Asynchronously process transaction backfills #195
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Noah-Silvera
requested review from
AlistairNorman,
adammathys,
benjaminwil,
forkata,
jarednorman,
nvandoorn and
senemsoy
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
44fabcf to
d8817f5
Compare
forkata
reviewed
Jul 4, 2022
app/jobs/super_good/solidus_taxjar/backfill_transaction_sync_batch_job.rb
Show resolved
Hide resolved
AlistairNorman
approved these changes
Jul 6, 2022
| @@ -2,11 +2,10 @@ | |||
|
|
|||
| RSpec.describe SuperGood::SolidusTaxjar::BackfillTransactions do | |||
| describe ".call" do | |||
There was a problem hiding this comment.
I know it's not in this PR but this should be describe "#call" do because it's an instance method. That being said since we aren't passing anything to the initializer anymore it could also just be a class method instead.
Noah-Silvera
force-pushed
the
150-add-taxjar-backfill-to-tabs
branch
2 times, most recently
from
8e8e641 to
816b91a
Compare
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
d8817f5 to
56f0af1
Compare
Noah-Silvera
changed the base branch from
150-add-taxjar-backfill-to-tabs
to
master
|
Sorry just a rebase on master since the base branch was merged! No changes yet |
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
56f0af1 to
2b493fb
Compare
forkata
approved these changes
Aug 12, 2022
|
Tests still need some attention, but otherwise |
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
2b493fb to
9130fe3
Compare
|
Just a push to fix the tests! |
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
9130fe3 to
e8a6b60
Compare
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
60c5cc1 to
beeec8a
Compare
This variable is no longer needed for test setup and can be removed. Co-authored-by: Nick Van Doorn <[email protected]>
Transaction backfills have the potential to encompass a large number of orders, so we should execute them asychronously, and direct the user to the in progress batch display page. Co-authored-by: Nick Van Doorn <[email protected]> Co-authored-by: Chris Todorov <[email protected]>
The ID of the transaction sync batch can change when the whole suite is run on CI, so we should just check for a number regex instead of a hardcoded number.
Noah-Silvera
force-pushed
the
origin/150-asynchronously-process-transaction-backfills
branch
from
beeec8a to
15c1e9f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the goal of this PR?
Transaction backfills have the potential to encompass a large number of orders, so we should execute them asychronously, and direct the user to the in progress batch display page.
How do you manually test these changes? (if applicable)
Merge Checklist