If overriding tax_category, it only makes sense to update tax_categor… #11802
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
|
Changes preview: Legend: |
…y_id as well fixes confusing display of Tax category as "None" in admin, while it actually uses "Default" ideally, it would be better to update the tax_category_id in the DB and re-update if TaxCategory#is_default changes
mrbrdo
force-pushed
the
fix_tax_category_default
branch
from
cdf9696
to
c6cf6ea
Compare
mrbrdo
referenced
this pull request
Nov 22, 2022
When Tax Category was deleted all Variants using that category would raise ActiveRecord::RecordNotFound when `#tax_category` was called on Variant object.
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.
…y_id as well
fixes confusing display of Tax category as "None" in admin, while it actually uses "Default"
ideally, it would be better to update the tax_category_id in the DB and re-update any time TaxCategory#is_default changes
I can also implement the "ideal" solution as described but it will require to add a migration (which means when updating spree, migrations need to be copied again, and ran).Also in this case, is there any reason why Product#tax_category_id would actually be nil, or can we make it mandatory? In the current implementation it will always return the default category. Is it possible to create (and use) a product without having any tax categories?
On the other hand, I assume we always want Variant#tax_category_id to fallback to Product#tax_category_id unless explicitly set.
After some thinking, need a comment on this. Using this solution, the tax_category_id will also be set in the DB after the product is updated (in the backend) due to how Rails forms work. This means that if default TaxCategory is changed, these products will keep using the same TaxCategory as before. In the current implementation, they would switch to the new default, which may be the desired behavior.
But on the other hand having
tax_category_idnot reflecttax_category, and having it show up asNonein the backend both sounds wrong. Honestly for the last 6 months I have been clicking to change it toDefault(the name of my default TaxCategory) on every new product, since I wasn't clear whatNonewould do.