Keep stock selection when error on saving #12621
Conversation
- added 2 not to be persisted attributes aimed at dealing with the UI
- added them to the permitted list
- updated view to switch mode about on_hand/on_demand
that is: from an already persisted variant or not
- Not persisted deals with on_*_desired not to be persisted fields
- Persisted mode deals with regular on_* fields
- the corresponding spec for both on_hand/on_demand
mkllnk
added
the
feature toggled
app/services/sets/product_set.rb
Outdated
| variant.on_demand = on_demand if on_demand.present? | ||
| variant.on_demand = variant.on_demand_desired if variant.on_demand_desired.present? | ||
| variant.on_hand = on_hand.to_i if on_hand.present? | ||
| variant.on_hand = variant.on_hand_desired.to_i if variant.on_hand_desired.present? |
There was a problem hiding this comment.
It's not clear to me which value is used in which case here. Looking only at this logic, the stock could even be set twice when updated. So first it would set the old on_hand value when it's present and then set the new desired value. This is a bit inefficient but also confusing to follow.
I think that it would be clearer to have one branch of logic:
- Form always sets the desired value.
- We always use the desired value to update stock.
There was a problem hiding this comment.
Yes, it was a bit cluttered. After a few trials (and errors), I think this is better now.
The conditional I added is because in legacy mode, you enter the create variant with on Hand/on demand whereas in v3 you do not.
| @@ -30,14 +31,14 @@ | |||
| = error_message_on variant, :price | |||
| %td.col-on_hand.field.popout{'data-controller': "popout"} | |||
| %button.popout__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')} | |||
| = variant.on_demand ? t(:on_demand) : variant.on_hand | |||
| = variant.send(method_on_demand) ? t(:on_demand) : variant.send(method_on_hand) | |||
There was a problem hiding this comment.
Using send is a sign that there's an issue with the code design. I think that it would be better to implement the reader on the model to read the value from the database if the desired value isn't set.
There was a problem hiding this comment.
I have added 2 methods in variant_stock to choose from which field to read. I have applied the same logic as in the view.
| it 'displays the correct value afterwards for On Hand' do | ||
| within new_variant_row do | ||
| fill_in "Name", with: "Large box" | ||
| click_on "On Hand" | ||
| fill_in "On Hand", with: "19" | ||
| end | ||
|
|
||
| click_button "Save changes" | ||
|
|
||
| expect(page).to have_content "Please review the errors and try again" | ||
| within row_containing_name("Large box") do | ||
| expect(page).to have_content "19" | ||
| end | ||
| end | ||
|
|
||
| it 'displays the correct value afterwards for On demand' do | ||
| within new_variant_row do | ||
| fill_in "Name", with: "Large box" | ||
| click_on "On Hand" | ||
| check "On demand" | ||
| end | ||
|
|
||
| click_button "Save changes" | ||
|
|
||
| expect(page).to have_content "Please review the errors and try again" | ||
| within row_containing_name("Large box") do | ||
| expect(page).to have_content "On demand" | ||
| end | ||
| end |
There was a problem hiding this comment.
Great to have this tested! 💯
Since system specs are slow, I would combine these two examples. You can fill both values, hit save once and then assert on both values.
There was a problem hiding this comment.
I have refactored the spec in this way :)
- 2 new methods for reading either current/desired on hand/on demand depending on variant state. Goal is to get rid of send method in View - referring in on_hand/on_demand is in fact irrelevant. In the piece of code, only desired on_hand/on_demand can be called as we are only in new variant (non persisted) mode - View does not use send method anymore, replaced by current_or_desired - refactor of the spec -> 2 examples in one to get more speed.
cyrillefr
force-pushed
the
KeepStockSelectionWhenErrorOnSaving
branch
from
6bc72f9
to
32b386e
Compare
What? Why?
What should we test?
/admin/productsOn demandSave changescannot be blankin theUnitcolumnOn Handpart should keep what was entered at first.How I did it
I added 2 not to be persisted attributes aimed at dealing with the UI. Their purpose is to be passed and passed by in case of validation errors from and to the UI.
Therefore, there is no interference with the current fields and logic.
In the view: I added some kind of "mode": when variant exists on DB or not.
So instead of on_hand/on_demand methods passed in form helpers, it will be now either:
on_handwhen variant exists (current only case)on_hand_desiredfor a new variantSo that, in
variant, one can get values from the ui whether or not the variant exists or not.In case validation fails, the value for on_hand/on_demand are stored and are passed by to the ui in the variant object.
If everything fine, then variant is persisted in DB.
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