Simplify buggy tax rate migration #1062
Merged
Conversation
This file contains 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
This migration did not work, and requires a call to `TaxRate#compute_amount`. We could have fixed the "the does not work", but the `compute_amount` call requires half of Solidus' object graph (including the `Spree::Calculator` STI table as well as `Spree::Order`, `Spree::Shipment`, `Spree::TaxRate` - this is still not exhaustive). We felt adding all of those into the migration was less than ideal, especially if your local shipping rate was taxed using a different system than the default tax calculator. The downside of this is ONLY that in the backend for old shipping orders, you will not be shown the shipping rate tax (included or additional). The reduced-in-complexity migration now just adds the `tax_rate_id` column on `Spree::ShippingRate`.
|
👍 I think the previous migration was just too big and slow to run. Could we also update the CHANGELOG explaining this limitation. |
|
I'm not happy with this just being removed and leaving the stores somehow "broken". Even within the change log entry are no hints on how to get the old state back, besides "updating the shipping rates". Why don't we take the code and move it into a rake task "solidus:recalculate_shipment_taxes"? I totally agree that a migration isn't the right place, but leaving this "unfixed" isn't an option for me either. Or am I too strict here? /c @cbrunsdon |
|
👍 |
|
Aye, good plan here, thanks folks 👍 |
bbuchalter
pushed a commit
to TommyJohnWear/solidus
that referenced
this pull request
Nov 18, 2016
We removed code from a migration in solidusio#1062 - this commit explains the (minimal) consequences of that removal in the changelog.
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.
This migration did not work, and requires a call to
TaxRate#compute_amount.We could have fixed the "the does not work", but the
compute_amountcall requireshalf of Solidus' object graph (including the
Spree::CalculatorSTI table as wellas
Spree::Order,Spree::Shipment,Spree::TaxRate- this is still not exhaustive).We felt adding all of those into the migration was less than ideal, especially if your
local shipping rate was taxed using a different system than the default tax calculator.
The downside of this is ONLY that in the backend for old shipping orders, you will not be
shown the shipping rate tax (included or additional).
The reduced-in-complexity migration now just adds the
tax_rate_idcolumn onSpree::ShippingRate.