Spec requiring tax category when creating products #12038
Conversation
We observed an error when an instance requires a tax category and we tried to create products via the DFC API: * openfoodfoundation#11212 But I found that the error only appears after changing the instance config without declaring a tax category as default. The right setup as in the spec does work. The spec passes. I don't think that this needs any fix. We shouldn't assign any tax category just because it's required. The instance manager needs to select a default.
The logic doesn't change but I simplified it and added more detailed specs.
Best viewed ignoring whitespaces. Products don't require a tax category by default. And when you activate it via Spree::Config then it's automatically reset at the end of the spec. We don't need this helper to do it.
These shared examples were used in only one spec file. It's much easier to read having all the related specs in one file instead of hiding some in a helper module. It's also one less file to load whenever we run specs.
mkllnk
added
the
technical changes only
| expect { | ||
| variant.tax_category = nil | ||
| }.to_not change { | ||
| variant.tax_category | ||
| } |
There was a problem hiding this comment.
I am not following what we are testing here ? Are we just checking that a tax category can't be set to null if a default one exists ?
There was a problem hiding this comment.
Are we just checking that a tax category can't be set to null if a default one exists ?
Yes, exactly. Well, I don't actually know what would be saved to the database and it doesn't really matter but the getter method always returns the default in this case.
drummer83
added
pr-staged-au
|
Hmm, sorry, I think I am missing something here @mkllnk. On all three staging servers I get an error 404:
Could you please provide some hints? Thanks! |
|
I'm not exactly sure what you did, but here are some hints:
|
|
My test notes above were aiming at the normal admin UX though. I wanted to be sure that I didn't break anything. The API itself is well covered with specs. I'm happy for you to test it as well but I thought it may be too much effort. |
Ah, thanks! That's probably it! I checked that the id wasn't already taken and thought that the new variant would get this id assigned. Thanks for clarification! I will try this within the next days - weekend is coming. 😉 |
drummer83
added
no-staging-AU
|
Hi @mkllnk, I have tested this now. After staging
Both have been tested for variants as well as for products. According to your comment here it is ok to see the error when a tax category is required but no default tax category is set. 👍 Great! This one is ready to go! 🎉 |
What? Why?
It doesn't really fix the error described in the issue but it shows that the error was caused by a misconfiguration of the instance.
Instead I discovered some unnecessary spec code and cleaned up a little.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates