Skip to content
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

Remove required field asterisk from on_hand's field on new products form #6861

Merged
merged 3 commits into from
Feb 18, 2021
Merged

Remove required field asterisk from on_hand's field on new products form #6861

merged 3 commits into from
Feb 18, 2021

Conversation

ccozkan
Copy link
Contributor

@ccozkan ccozkan commented Feb 10, 2021

What? Why?

Closes #6159

Even though original issue suggests showing "on hand is a mandatory field" errors using model validations, I believe this would add more complexity, when same trouble can be solved with more simple way.

Default value of count_on_hand is defined as 0 in database level, therefore StockItem with nil count_on_hand will be build as 0. In order to detect the violence of presence validation, we would need to add another migration(which removes default value of count_on_hand on StockItem), and add custom associated validation to Product model for checking validity of the StockItem. And also maybe some changes would be needed regarding concerns/variant_stock#save_stock.

This PR suggests adding html required attribute to 'on hand' field, and changing it is type from text_field to number_field, since it is an integer column.

What should we test?

  • Adding new product on admin/products/new with deleting 'on hand' field value. HTML required attribute should point that box can not be left blank.
  • On hand value can be increased and decreased by clicking its up/down arrows on the field box.

Release notes

Change on_hand's field to number_field and add required attribute on new products form
Remove required field asterisk from on_hand's field on new products form

User facing changes

Copy link
Contributor

@andrewpbrett andrewpbrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution here @ccozkan. Sorry if I'm misunderstanding something here; I read through the issue and it looks like it's changed quite a bit over time. In my opinion we should actually just remove the * that indicates this field is required, since that matches the backend behavior most closely.

The required attribute isn't 100% reliable (e.g. if we change the form in the future to rely on javascript instead of a regular post request) and making it a number field will have different effects in different browsers. So I think removing the * is probably the simplest thing to do here.

@ccozkan ccozkan changed the title Change on_hand's field to number_field and add required attribute on new products form Remove required field asterisk from on_hand's field on new products form Feb 15, 2021
Copy link
Contributor

@andrewpbrett andrewpbrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ccozkan !

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Feb 17, 2021

Hey @ccozkan ,

Before staging this PR we had this situation, when creating a product and leaving mandatory fields blank:

image

And after this PR we can check the asterisk is gone:

image

We're good to go here. Thanks again for your contribution! 🎉

@ccozkan
Copy link
Contributor Author

ccozkan commented Feb 17, 2021

Thank you too! It's been a good experience for me🙌

@sauloperez sauloperez merged commit a21a8b1 into openfoodfoundation:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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