[221] Don't send cancelled or returned line items #236
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
changed the base branch from
master
to
221-create-sync-logs-for-replace-transaction-jobs
nvandoorn
force-pushed
the
221-handle-multiple-refunds-in-a-row
branch
from
9be7fc4 to
564efe5
Compare
nvandoorn
changed the base branch from
221-create-sync-logs-for-replace-transaction-jobs
to
master
nvandoorn
force-pushed
the
221-handle-multiple-refunds-in-a-row
branch
from
564efe5 to
58df5d3
Compare
nvandoorn
changed the title
nvandoorn
force-pushed
the
221-handle-multiple-refunds-in-a-row
branch
from
58df5d3 to
6877c9a
Compare
forkata
reviewed
Jul 24, 2023
| @@ -146,7 +146,8 @@ def valid_line_items(line_items) | |||
| # The API appears to error when sent line items with no quantity... | |||
| # but why would you do that anyway. | |||
| line_items.reject do |line_item| | |||
| line_item.quantity.zero? | |||
| line_item.quantity.zero? || | |||
| !line_item.inventory_units.where.not(state: 'returned').exists? | |||
There was a problem hiding this comment.
Could we write this as
line_item.inventory_units.returned.any?
| @@ -116,7 +116,7 @@ def line_items_params(line_items) | |||
| line_items: valid_line_items(line_items).map do |line_item| | |||
| { | |||
| id: line_item.id, | |||
| quantity: line_item.quantity, | |||
| quantity: line_item.inventory_units.where.not(state: ['returned', 'canceled']).count, | |||
There was a problem hiding this comment.
Since we're doing this a few times, should we consider pulling it into a helper so we don't have to repeat it?
benjaminwil
approved these changes
Jul 31, 2023
Prior to this commit, all line items would be reported to TaxJar, including returned line items. To fix this, we check that a line item has at least one inventory unit with a state other than 'returned'. Co-authored-by: Alistair Norman <[email protected]>
Prior to this change, each request to TaxJar sent all line items. We should exclude line items that have been returned or cancelled so the total of the line items still adds up to the order's payment total. Co-authored-by: Noah Silvera <[email protected]>
Prior to this change, the query for inventory units that are not cancelled or returned was repeated three times so we move the query into a private method. Co-authored-by: Alistair Norman <[email protected]>
nvandoorn
force-pushed
the
221-handle-multiple-refunds-in-a-row
branch
from
6877c9a to
3128fb8
Compare
|
Latest revision rebases this on-top of master and moves the repeated query into a private method. |
forkata
approved these changes
Aug 11, 2023
benjaminwil
approved these changes
Aug 11, 2023
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.
Prior to this change, each request to TaxJar sent all line items. We
should exclude line items that have been returned or cancelled so the
total of the line items still adds up to the order's payment total.