-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Keep stock selection when error on saving #12621
base: master
Are you sure you want to change the base?
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6bc72f9
to
32b386e
Compare
What? Why?
What should we test?
/admin/products
On demand
Save changes
cannot be blank
in theUnit
columnOn Hand
part 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_hand
when variant exists (current only case)on_hand_desired
for 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