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

Fix invalid unit value (rebase of #6250) #6431

Merged

Conversation

andrewpbrett
Copy link
Contributor

@andrewpbrett andrewpbrett commented Nov 30, 2020

What? Why?

Closes #6117
Closes #3912
Closes #4959

See the original PR (#6250) for details; I tried to rebase it after the spree updates and couldn't untangle things... sorry @tsara27 !

There is still a lot left to be desired in this UI; as @Erioldoesdesign noted, it isn't very clear what this field is for. Moreover the error now says "unit value can't be blank" but the label on the field itself is "value"; and even moreover, if you do fill in the field but with something that's not a number, because it's confusing what should go in there, you get the same "unit value can't be blank" error.

So yes, lots of potential for further improvement here...but this gets rid of the 500 slug. And closes three issues for the price of one!

What should we test?

Login as an admin.
Go to products.
Create a new product.
Fill other required details, leave the unit value empty.
Should see an error and not a slug.

Release notes

Fixed a bug where entering an empty or invalid value on the product creation page would show a slug instead of an error message.
Changelog Category: User facing changes

Dependencies

Documentation updates

@andrewpbrett andrewpbrett self-assigned this Dec 1, 2020
@andrewpbrett
Copy link
Contributor Author

@sauloperez I noticed that you'd written some of the specs related to this, so I thought you'd be a good person to review it :)

fill_in 'product_on_hand', with: 0
check 'product_on_demand'
select 'Test Tax Category', from: 'product_tax_category_id'
page.find("div[id^='taTextElement']").native.send_keys('In demand, and on_demand! The hottest cakes in town.')
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't actually need to specify page. find is globally available just like the other Capybara form in put methods

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Dec 2, 2020

Hey @andrewpbrett ,

Great - this seems to work fine now. I tested the Units field with both numeric and non-numeric values, in both pages:

  • /admin/products/new
  • /admin/products/

In all cases, the error "Unit value can't be blank" is displayed now:

image

This is good to go.

Would it be a good idea to extend this test case in the specs:
spec/features/admin/products_spec.rb:134 expect(page).to have_content "Unit value can't be blank"

to all other mandatory fields? We recently had an S2 related to this #6393 - Opened #6447 on this.

Something else noticed while testing: "Display as" and "on hand" don't seem to be mandatory fields - it's possible to create products without filling in anything. Updated #6159 to reflect this.

@filipefurtad0 filipefurtad0 self-assigned this Dec 2, 2020
@andrewpbrett andrewpbrett merged commit 936f5c1 into openfoodfoundation:master Dec 2, 2020
@andrewpbrett andrewpbrett deleted the fix-invalid-unit-value branch December 2, 2020 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants