"On hand" is not a mandatory field, when creating a product

[filipefurtad0]

Description

The "On Hand" field is not mandatory. The value zero is the default value and is assumed even if this no value is inserted.

Expected Behavior

The behavior for the Price field (as below) is expected to be observed if the On hand field is left blank:

Actual Behaviour

It is not necessary to introduce a value on the "On Hand" field to create a product; when this is left blank then the value 0 is assumed (which is ok, zero is a valid value) but no error message/underlining of these missing fields is seen.

Steps to Reproduce

1. Log in as admin
2. Go to Products, and click +New Product
3. Fill in all the fields leaving "On Hand" empty: you'll need to delete the pre-filled zero (0).
4. Click "Create" -> creation is successful, and no warning is shown to the user.

Animated Gif/Screenshot

See the pic under Expected Behavior showing the underlining and error message regarding the missing field.

Workaround

Adding/updating the price in the product list.

Severity

bug-s4: it's annoying, but you can use it

Your Environment

• Version used: introduced between v.3.2.9 and v.3.2.10
• Browser name and version: Firefox 81
• Operating System and version (desktop or mobile): Desktop Ubuntu 20.04

Possible Fix

[RachL]

@filipefurtad0 we are not tagging regressions so far... For example #5947 is a regression as well.

We track them during major upgrade to be able to prioritize them (that's why there was also a v2-regression label). I'm not sure there is a need to do it more regularly, except maybe knowing where automated tests are not strong enough?

[filipefurtad0]

Thanks for the feedback here @RachL .
That's a good question - I'm not sure either. I figured it could be useful as it adds some context to the findings. Maybe we can chat about it on the Dev-train meeting? I'll remove the label and change the title.

[Tshamp7]

Hello! I saw this on codetriage and I'm interested in working on this if that's okay.

[filipefurtad0]

Hi @Tshamp7 ,
Welcome, please go ahead 🎉
I've assigned you to this issue - please let us know if you have any questions.

[Tshamp7]

Thanks will do!

[RachL]

Hi @Tshamp7 ! I hope this message finds you well. Are you still planning to work on this? No pressure, I just want to make sure this is still something that is currently being worked on :)

[Tshamp7]

Hello! I was going to reach out to you guys, I had some stuff come up at work I've been needing to work on and haven't had a chance. You can unassign it from me. Sorry about that!

…

On Wed, Nov 11, 2020, 1:52 AM Rachel Arnould ***@***.***> wrote: Hi @Tshamp7 <https://github.com/Tshamp7> ! I hope this message finds you well. Are you still planning to work on this? No pressure, I just want to make sure this is still something that is currently being worked on :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6159 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AM63U4226RFCLD4LSL2WLU3SPJNHBANCNFSM4SL7CMMA> .

[RachL]

No worries @Tshamp7 thanks for the quick answer :)

[filipefurtad0]

This seems reproducible in staging-UK, when the chosen language is English. When using the app in French, for example, and creating a new product:

Clicking Create, leads to:

Which is the correct behavior.

I've checked in Production in Katuma and Coopcircuits, and this is OK there as well, in different languages.

Also, I looked at Bugsnag: these errors in staging-UK appear grouped with 11 occurrences in production, in UK, since the v3.4.1-beta was deployed:
https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/5fb380c47c7acc0018a558e1?filters[event.since][0][type]=eq&filters[event.since][0][value]=30d&filters[error.status][0][type]=eq&filters[error.status][0][value]=open&filters[app.release_stage][0][value]=production&filters[app.release_stage][0][type]=eq&pivot_tab=event

Should the severity be increased?

[filipefurtad0]

This seems only to affect instances/sessions using en_GB locale. It should be solved by adding the missing translation on Transifex. Ping @lin-d-hop .

See also discussion, for reference:
https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1605692638182900

[filipefurtad0]

This translation was just pushed to the transifex branch 🎉

So no further action needed for now 👍

[filipefurtad0]

"Price" seems to be a mandatory field in both staging and production again 👍 Removing the regression label.

"Display as" and "On hand" are marked by the (*) as mandatory fields but are not. This was the case before, so no regression here.

[ccozkan]

Hey 👋 , I would like to pick up this issue

[filipefurtad0]

Hi @ccozkan , welcome 🎉
I've assigned you to the issue. Feel free to drop us a line if you have any questions 👍

[Matt-Yorkley]

I have a feeling there might be some additional hidden complexity here, like if you select "weight", then certain fields are required, and if you select "items" then other fields are required... 😬

[ccozkan]

Thanks @filipefurtad0 😄 I've been busy so I had chance to look at and surf around to project recently. I have a question
though. 🤔 I did not quite understand the role of with Spree::StockItem, Spree::Variant, and VariantOverride. Can you please explain them in one-or-two sentences?

[filipefurtad0]

Hey @ccozkan ,

I can add a very brief description sentences on what these concepts do in the app:

Products/Variants need to have a value for stock items set, from which the availability for a customer to purchase it depends on. For example, if a given variant (ex., eggs) has zero stock items, then it will be not available in the shopfront.

Hub managers and producers, can set the stock for the variants when they create a new variant or by editing an existing one, under the /admin/products page. However, the stock values set on this page can be overiden by the settings in the Inventory page - /admin/inventory.

For details on this, see:
https://guide.openfoodnetwork.org/basic-features/products-1/product-variants
https://guide.openfoodnetwork.org/basic-features/products-1/inventory-tool#managing-your-inventory-products

I'm pointing out some files I think are relevant to understand how it works under the hood:
Spree::StockItem - /app/models/spree/stock_item.rb
Spree::Variant - /app/models/spree/variant.rb
VariantOverride - /app/models/variant_override.rb

Does this help? If you need some specific dev insight feel free to drop a line in the #dev channel on Slack.

I'm not sure this is a good first issue as there might be some other things happening - see @Matt-Yorkley 's comment above. If it gets to tricky feel free to look for issues with the good first issue label.

[filipefurtad0]

like if you select "weight", then certain fields are required, and if you select "items" then other fields are required

I see that now @Matt-Yorkley : the "Display as" field is only mandatory if you choose Unit Size = Items. This is working, and I believe this is the expected behaviour. In the user guide for example this field is not set as mandatory (see video).

So maybe this is ok as is, and not a bug? In that case, I'd just add a spec to cover the case in which a items require "Display as" #6447?

[Matt-Yorkley]

the "Display as" field is only mandatory if you choose Unit Size = Items

Do you think the UX is clear enough on this, or could it be improved?

[filipefurtad0]

Yes, I think ideally the * should only appear, when the user chooses "Items". I guess it should not appear if user chooses any other unit (as shown in the user guide video), as the "Display as" is not mandatory.

[filipefurtad0]

Some additional thoughts:

Currently, we can create a new product in two ways:
a) directly from the products page by clicking "New Product" -> in this case, the user stays in the product page /admin/products
b) when editing a product, and clicking "New Product" -> in this case, the user is redirected to /admin/products/new

We have observed some differences in these pages and how these fields react, for example here. So, it looks like we need to test them separately. That might mean need to add double specs for routes a) and b). At the moment, we test this manually still.

Maybe we should remove one of these paths? This would simplify the page, for example, removing the product creation path a), so that when the user clicks "New product" it gets redirected to path b).

The only loss in functionality would be to not be able to scroll down, while creating a new product. I'm not sure this is worth, for maintaining these two paths. Am I missing any other advantage? ping @openfoodfoundation/train-drivers-product-owners

Would this make things simpler codewise and specwise?

[filipefurtad0]

As agreed here, tech debt #6650 was opened to address the removal or product creation through path a), as described above.

[filipefurtad0]

Adjusted the name of the issue, as Display as is not mandatory and this is the expected behaviour. Opened issue #6159 to remove the * symbol.

[RachL]

Hello @ccozkan ! I hope you are doing fine :-) Are you still planning to work on this issue? No pressure, I'm just checking 👍

[ccozkan]

Yes, sorry I've been busy, But I still want to work on it, I will try to tackle this issue this weekend:)

[ccozkan]

I have a suggestion about this issue 🤔 . Wouldn't just changing on_hand's text_field type to number_field, min: 0 on the form, be a simpler solution to the issue?

Otherwise, as far as I understand, I would need to add migration for removing default on_hand value on StockItem model, and add custom associated validation to Product model for checking validity of the StockItem.

[filipefurtad0]

Hey @ccozkan ,

This issue has gone through some changes, since it was opened. I've updated the description, to make it more clear. Right now, it's really only about informing the user, that the on_hand field is mandatory, with the red banner, as it happens for all other mandatory fields marked with the star sign *, as described in the Expected Behavior section.

However, those are interesting thoughts - but I'm not the best in the team to answer those questions.

Maybe an approach would be to address the expected behavior, and then, in additional commits, add the changes you propose? This could then be discussed with the devs reviewing your PR.