Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INT 2085: Optimize transaction backfill order search #218

Conversation

sethobey
Copy link
Contributor

@sethobey sethobey commented Nov 10, 2021

Context

Current BackfillTransactions:class observer uses SearchCriteria to retrieve orders within given date and store constraints, but does not consider whether the order is actually "syncable" before batching that order into a bulk operation. The consumer Backfill::class then determines whether or not we should sync the transaction. This means that potentially many unnecessary operations are created only to determine that the orders within should not be synced.

Description

This PR will now check within BackfillTransactions:class whether or not "force" option was enabled, and if force is not enabled, only syncable orders will be included in operations. While adding this functionality, BackfillTransactions.php was refactored, unit and integration tested.

Also corrects failing Transaction::class unit tests

Performance

The same total number of order-date comparisons will now be paid upfront instead spread across consumer processes, but there should be a net decrease in number of operations for the same request.

Testing

Automated Testing

  • Adds Taxjar\SalesTax\Test\Unit\Observer\BackfillTransactionsTest.php
  • Adds Taxjar\SalesTax\Test\Integration\Observer\BackfillTransactionsTest.php

Manual Testing

  1. Enable Taxjar transaction sync
  2. Create/invoice/ship an order (optionally create credit memo as well)
  3. Observe order is synced with Taxjar
  4. Trigger backfill transaction sync (not forced) with date range inclusive of order
  5. Observe created order not included in any created operations
  6. Trigger forced backfill transaction sync with date range inclusive of order
  7. Observe created order is now included in created operations

Versions

  • Magento 2.4
  • Magento 2.3
  • Magento 2.2
  • Magento 2.1
  • Magento Open Source (CE)
  • Magento Commerce (EE)
  • Magento B2B
  • Magento Cloud
  • PHP 7.x
  • PHP 5.x

@sethobey sethobey changed the base branch from develop to transaction-backfill-feature November 10, 2021 00:27
@sethobey sethobey changed the title INT-2085: Optimize transaction backfill order search INT 2085: Optimize transaction backfill order search Nov 10, 2021
@sethobey sethobey force-pushed the INT-2085-optimize-backfill-search-criteria branch 2 times, most recently from b8b365e to 14d5cc1 Compare November 10, 2021 08:57
@sethobey sethobey force-pushed the INT-2085-optimize-backfill-search-criteria branch from 14d5cc1 to 34edb3c Compare November 11, 2021 19:58
@sethobey sethobey marked this pull request as ready for review November 11, 2021 19:58
Copy link
Contributor

@dallendalton dallendalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running a backfill without force enab

Observer/BackfillTransactions.php Outdated Show resolved Hide resolved
Observer/BackfillTransactions.php Show resolved Hide resolved
@sethobey sethobey force-pushed the INT-2085-optimize-backfill-search-criteria branch 2 times, most recently from b733fb5 to 21d3b01 Compare November 17, 2021 07:55
@sethobey sethobey force-pushed the INT-2085-optimize-backfill-search-criteria branch from 21d3b01 to 93e0078 Compare November 17, 2021 18:57
Model/Logger.php Outdated Show resolved Hide resolved
Observer/BackfillTransactions.php Outdated Show resolved Hide resolved
Observer/BackfillTransactions.php Outdated Show resolved Hide resolved
@sethobey sethobey merged commit 64dab32 into transaction-backfill-feature Nov 18, 2021
@sethobey sethobey deleted the INT-2085-optimize-backfill-search-criteria branch November 18, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants