-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix TaxCategory default not showing up in admin #3759
Fix TaxCategory default not showing up in admin #3759
Conversation
Thanks! It seems to make sense but I can't find in the codebase a single place where we are using the default tax category. I mean in the tax calculation... Can you help me understand if it's better to remove the concept of the default tax category entirely? |
I think it is useful to have a tax category that's the default for creating new products. Beyond that I don't think it has any meaning, but it's still useful for that. |
3764bce
to
8100fc9
Compare
Thanks, I think it's ready to be merged so I'm going to approve. An extra thing that we could do in terms of UX is explaining what's the meaning of the default checkbox in the Tax Category edit page with another hint: Of course, we can also do that with another PR. Let me know if you want to take care of that or I'll open a new issue to keep track of this. Thanks again! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vl3, just left a comment on an extra SQL query that I think we can get rid of.
8100fc9
to
fbc2d16
Compare
@vl3 thanks, looking great! Also looking forward to your next PR. ❤️ |
Description
When visiting New products page, Spree::TaxCategory with is_default set to true is not being shown as the first selected option in the tax category select box. Additionally, the hint of the select box is saying that the default is None despite having other Spree::TaxCategory as default. This PR aims to fix it.
Without change
With change
Checklist: