Keep stock selection when error on saving #12621

cyrillefr · 2024-06-27T14:56:55Z

What? Why?

Closes [BUU] Stock settings selection is lost, if there are errors on saving #12569

What should we test?

Feature Admin V3 toggled on
Visit /admin/products
Add a variant ( + sign) to a product
Do not fill anything, except the On Hand part
- click on checkbox On demand
- or choose a number
Click Save changes
Page should return in error mode with a cannot be blank in the Unit column
But the status for the On 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 variant

So 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):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

mkllnk · 2024-06-27T23:17:45Z

app/services/sets/product_set.rb

+      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?


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.

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.

mkllnk · 2024-06-27T23:19:34Z

app/views/admin/products_v3/_variant_row.html.haml

@@ -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)


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.

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.

mkllnk · 2024-06-27T23:21:14Z

spec/system/admin/products_v3/products_spec.rb

+      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


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.

I have refactored the spec in this way :)

sigmundpetersen · 2025-01-06T10:52:10Z

Hi all.
Why was this one closed? I can't find a mention of that in the issue or pr.
Seems the changes were ready for another round of review, but that the pr was left hanging in the 'In progress' column.

@openfoodfoundation/reviewers should we reopen and have another look at it?
Or do you think it should stay closed?

mkllnk

Much better!

It's still a little bit cumbersome but that's worth fixing the bug. Thank you!

I'm sorry that I missed your updates. Can you please re-open? I would approve this as is.

dacook

I'm guessing the new, separate "desired" attributes are to reduce risk. It's a shame it makes it pretty complicated, but it protects against creating new stock issues. I probably would have chosen a different name for these temporary variables, but don't have any better ideas.

I have a few other thoughts on how this could be organised, because as Maikel says it's pretty cumbersome.

But it would be great to fix the bug 👍

dacook · 2025-01-07T04:54:31Z

app/views/admin/products_v3/_variant_row.html.haml

-      = f.label :on_demand do
-        = f.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked'
+      = f.label method_on_demand do
+        = f.check_box method_on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked'


I wish there was a better way to manage these two cases. If we need to add this complexity, the on_hand/on_demand field might deserve to have its own partial.

dacook · 2025-01-07T04:55:50Z

app/models/concerns/variant_stock.rb

+
+    on_hand
+  end
+


These are arguably view-specific logic, so perhaps these could be moved to view helpers. Also method_on_demand.

cyrillefr · 2025-01-13T12:50:37Z

Hello @mkllnk , @dacook , @sigmundpetersen

Sorry for the PR closing. I did some local project cleaning a fee weeks ago and I guess by deleting a local branch, it automatically closed this PR. It was not my intention. I must have made some bad filtering and did not see the corresponding issue was still open ....

Do you want me to re open this PR ? (I guess i can do it by restoring the deleted branch).

sigmundpetersen · 2025-01-13T13:10:48Z

Do you want me to re open this PR ? (I guess i can do it by restoring the deleted branch).

Yes, thank you @cyrillefr . Maybe we could get the fix merged, that would be nice 👍

rioug · 2025-02-03T22:20:23Z

@cyrillefr If you are not on it already, could you address the conflict ?

- 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

- 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 · 2025-02-05T15:37:20Z

Conflict is solved, but I need to fix the specs since behaviour seemed to have a bit changed since last time.

github-actions bot assigned cyrillefr Jun 27, 2024

mkllnk added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Jun 27, 2024

mkllnk requested changes Jun 27, 2024

View reviewed changes

cyrillefr force-pushed the KeepStockSelectionWhenErrorOnSaving branch from 6bc72f9 to 32b386e Compare June 28, 2024 15:46

cyrillefr closed this Dec 20, 2024

cyrillefr deleted the KeepStockSelectionWhenErrorOnSaving branch December 20, 2024 19:17

dacook requested review from mkllnk and dacook January 7, 2025 04:24

mkllnk reviewed Jan 7, 2025

View reviewed changes

dacook reviewed Jan 7, 2025

View reviewed changes

cyrillefr restored the KeepStockSelectionWhenErrorOnSaving branch February 3, 2025 14:41

cyrillefr reopened this Feb 3, 2025

cyrillefr added 3 commits February 5, 2025 14:42

Fixes linter error

4d73660

cyrillefr force-pushed the KeepStockSelectionWhenErrorOnSaving branch from 32b386e to 75dc20c Compare February 5, 2025 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep stock selection when error on saving #12621

Keep stock selection when error on saving #12621

cyrillefr commented Jun 27, 2024

mkllnk Jun 27, 2024

cyrillefr Jun 28, 2024

mkllnk Jun 27, 2024

cyrillefr Jun 28, 2024

mkllnk Jun 27, 2024

cyrillefr Jun 28, 2024

sigmundpetersen commented Jan 6, 2025

mkllnk left a comment

dacook left a comment

dacook Jan 7, 2025

dacook Jan 7, 2025

cyrillefr commented Jan 13, 2025

sigmundpetersen commented Jan 13, 2025

rioug commented Feb 3, 2025

cyrillefr commented Feb 5, 2025

Keep stock selection when error on saving #12621

Are you sure you want to change the base?

Keep stock selection when error on saving #12621

Conversation

cyrillefr commented Jun 27, 2024

What? Why?

What should we test?

How I did it

Release notes

Dependencies

Documentation updates

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigmundpetersen commented Jan 6, 2025

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrillefr commented Jan 13, 2025

sigmundpetersen commented Jan 13, 2025

rioug commented Feb 3, 2025

cyrillefr commented Feb 5, 2025