Pre-fill new variant fields when adding a variant#14392
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
sigmundpetersen
moved this from All the things 💤
to Code review 🔎
in OFN Delivery board
emilythericky
force-pushed
the
wishlist-560-duplicate-variant-info
branch
from
a032f50 to
08d3048
Compare
When a hub manager adds a variant to a product, copy the producer, category, tax category, unit, unit value and price from the most recently created variant, and default the quantity to 0. Previously these fields were blank, which caused confusing save errors when the required producer, category and unit fields were left empty — behaviour that regressed when those fields moved from the product to the variant. Defaulting the quantity to 0 (and not copying the on-demand flag) leaves new variants out of stock until the user sets a quantity, preventing accidental sales.
emilythericky
force-pushed
the
wishlist-560-duplicate-variant-info
branch
from
08d3048 to
444b667
Compare
rioug
added
the
user facing changes
rioug
left a comment
rioug
left a comment
There was a problem hiding this comment.
Hi @emilythericky, the changes look good, there are just a couple of things I would like cleaned up.
I also noticed, this changes the behaviour from copying data from the product's first variant to copying data from the product's last variant. @RachL @mariocarabotta are you ok with this change ?
|
|
||
| def copy_template_fields(template, new_variant) | ||
| NEW_VARIANT_TEMPLATE_FIELDS.each do |field| | ||
| next unless template.respond_to?(field) && new_variant.respond_to?(:"#{field}=") |
There was a problem hiding this comment.
This is unnecessary we control both the template and the new variant, using a non existent field would be a bug that we should fix. Could you remove this guard clause ?
| '<input id="new_variant_id" type="hidden" name="form[products][0][variants_attributes][1][id]">', | ||
| ); | ||
|
|
||
| form.dispatchEvent(new Event("custom-event")); |
There was a problem hiding this comment.
Could we use the event type used in production rails-nested-form:add here ? You'll also need to update the html to use rails-nested-form:add->bulk-form#toggleFormChanged
| form.dispatchEvent(new Event("custom-event")); | |
| form.dispatchEvent(new Event("rails-nested-form:add")); |
| '<input id="existing_variant_id" type="hidden" name="form[products][0][variants_attributes][0][id]" value="42">', | ||
| ); | ||
|
|
||
| form.dispatchEvent(new Event("custom-event")); |
There was a problem hiding this comment.
same as above, use rails-nested-form:add event type.
github-project-automation
Bot
moved this from Code review 🔎
to In Progress ⚙
in OFN Delivery board
Remove the respond_to? guard in copy_template_fields: both the template and the new variant are Spree::Variant instances we control, so a missing field would be a bug to fix, not a case to skip. Use the production rails-nested-form:add event in the bulk form controller tests instead of a custom event name.
That was intentional -- the most recently added variant is usually the closest match for the next one. Discussed this with @mkllnk in the issue here . Happy to change that if folks prefer it the other way around. |
changes are in! |
rioug
left a comment
rioug
left a comment
There was a problem hiding this comment.
Great ! thanks @emilythericky 🙏
All good then, I had missed the discussion in the ticket. The change makes sense to me, I just wanted to flag it because it's a different behaviour that what we currently have. |
3 participants
When a hub manager adds a variant to a product, copy the producer, category, tax category, unit, unit value and price from the most recently created variant, and default the quantity to 0.
Previously these fields were blank, which caused confusing save errors when the required producer, category and unit fields were left empty — behaviour that regressed when those fields moved from the product to the variant. Defaulting the quantity to 0 (and not copying the on-demand flag) leaves new variants out of stock until the user sets a quantity, preventing accidental sales.
What? Why?
When adding a variant on the bulk products page, the new row was blank. The producer and category fields are required, so leaving them empty caused save errors — and because those columns are often hidden, users couldn't see why the save failed. Many resorted to creating new products instead of adding variants.
This previously worked: the old products page auto-filled the category when adding a variant. The behaviour was lost when producer and category moved from the product to the variant during the data model refactor, and the new bulk products page never reinstated it.
This PR copies the key fields from the most recently created variant onto a new variant row: producer, category, tax category, unit, unit value and price. Quantity (on hand) defaults to 0 and the variant is left not on-demand, so a new variant starts out of stock and can't be sold accidentally before stock is set. All fields remain editable.
Before - Save button doesn't appear until a field is edited + Producer/Category not copied + confusing error message when trying to save
After
What should we test?
unit value and price from the last variant.
error.
previous one.
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.