Require variant category and supplier when creating new product variants [OFN-12666] #12690
Conversation
wandji20
force-pushed
the
wb-OFN-12666
branch
4 times, most recently
from
fb29bb2
to
70a4e06
Compare
|
@wandji20 hello! can you have a look at rebasing here? Many thanks for one more 🙏 You're on 🔥 |
| select supplier.name, from: 'Producer' | ||
| select taxon.name, from: 'Category' |
There was a problem hiding this comment.
This doesn't actually test the newly added requirements, that the producer and category dropdowns were blank at the beginning.
That would have been nice, but I think this is still ok.
There was a problem hiding this comment.
Changes here were to fix failing specs
I agree it will be nice to check when the variant category and producer values are blank
| placeholder_value: t('admin.products_v3.filters.search_for_producers'))) | ||
| include_blank: t('admin.products_v3.filters.select_producer'), | ||
| placeholder_value: t('admin.products_v3.filters.select_producer'))) | ||
| = error_message_on variant, :supplier |
There was a problem hiding this comment.
Great, thanks for adding error_message_on. It probably should have been added in the beginning.
| # Set new variant category to same as last product variant category to keep compactibility with deleted variant callback to set new variant category | ||
| newVariantId = $scope.nextVariantId(); | ||
| newVariantCategoryId = product.variants[product.variants.length - 1]?.category_id |
There was a problem hiding this comment.
I don't understand why we are doing this, It would make more sense to set the category to blank same as the new screen. I am not sure what you are referring to when you say "to keep compactibility with deleted variant callback to set new variant category"
There was a problem hiding this comment.
I am not sure what you are referring to when you say "to keep compactibility with deleted variant callback to set new variant category"
I think he means to maintain the existing behaviour on the old screen. So there would be no apparent change when using the old screen. But as you say,
It would make more sense to set the category to blank same as the new screen
There was a problem hiding this comment.
In the old screen the supplier is set to blank, I think it would be better to have a consistent behaviour.
There was a problem hiding this comment.
😮 Oh, so this change makes it worse than before? In that case we need to do better.
Wandji would it be hard to make the old screen behave like the new one?
There was a problem hiding this comment.
Ok, that's a good point Wandji. If the Category column is hidden by default, it's not intuitive for the user to choose it.
I just tried it out and can see what you mean.
If your PR ensures the old screen maintains the existing behaviour, I think it's ok to proceed as it is. It adds a bit of technical debt, but that's ok because we're planning to remove the old screen later, once the transition to the new screen is complete.
Is that ok with you Gaetan?
There was a problem hiding this comment.
Is that ok with you Gaetan?
Yeah that's fine, if we get complaint we can always tell them to use the new screen 😉
mkllnk
added
the
user facing changes
|
Hey @wandji20 , Thanks for another PR 🙏 Before this change, we can see that:
After staging the PR:
Attempting to save a variant without this information correctly triggers the warning, for these fields:
Looking great! Merging. |
Use openfoodfoundation/wishlist#520 Flower Farms code in clokify
What? Why?
Closes Creating a new variant is selecting the first producer and first category of the dropdown #12666
Remove callback to set variant category id on create
Add logic to assign a new variant category to the same as the last variant for thesame product from the old products table
Update specs to select variant producer and category
What should we test?
Updating products (variants) from old admin products table should work as before
Updating products (variants) from v3 admin products table should show errors if producer or category is not selected
Visit ... page.
-
Release notes
Require variant category and supplier when creating new product variants
Changelog Category (reviewers may add a label for the release notes):
Require variant category and supplier when creating new product variant
Dependencies
N/A
Documentation updates
N/A