-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[BUU] Fixes Products Page ActionView::Template::Error #12710
[BUU] Fixes Products Page ActionView::Template::Error #12710
Conversation
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.
Looks good 👍
However I am wondering if we should show a blank value when unit_value
and unit_description
are nil ? @openfoodfoundation/core-devs ?
That sounds logical to me, I forget if there's a reason we didn't before. So if we let it be blank instead, then when you save you should get an error message. Then the user can choose the appropriate number. I think that would be better, if the interface can support it. @chahmedejaz would you be able to test it out and see what works best? |
Sure, David. Let me do a spike on it and get back to you. Thanks |
Hi @chahmedejaz , sorry I didn't have an answer before. I think it's helpful to remember this is a rare case and considered corrupt data. The error above ☝️ means the whole page fails to load which is very disruptive. But if cloning a corrupt product fails, I think that's much less disruptive and easy to work around, so we don't need to worry about solving it now. (the user can just manually create a new product with the same details) |
Thanks @dacook - Sounds good 👍 |
- if unit_value is not present and unit_description then display unit_description only. - if both are not present then display empty fields
@@ -11,19 +11,20 @@ def product_image_form_path(product) | |||
end | |||
|
|||
def prepare_new_variant(product) | |||
product.variants.build do |variant| | |||
variant.unit_value = 1.0 * (product.variant_unit_scale || 1) |
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.
Mimicking old UI behavior, when creating a new variant, then both of these are empty
@@ -18,7 +18,7 @@ | |||
= f.hidden_field :unit_value | |||
= f.hidden_field :unit_description | |||
= f.text_field :unit_value_with_description, | |||
value: unit_value_with_description(variant), 'aria-label': t('admin.products_page.columns.unit_value'), required: true | |||
value: unit_value_with_description(variant), 'aria-label': t('admin.products_page.columns.unit_value') |
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.
Having required
here is causing an error upon saving in the following scenario:
- New varaint button is clicked to create a new variant
- As per Old UI behavior, unit_value is empty for this product.
- However, in the new UI, if this variant is changed then the saving form doesn't work because this field is hidden on the UI until its pop-up is opened
- JS tries to find it to show the required validation error but gets broken because it's not able to find it on the UI
- Hence neither the required error gets displayed nor the form gets submitted.
- This PR removes the required validation which won't cause the failure of the above use-case however, the backend is handling this blank field error, so it will return the erorr "can't be blank" which will be displayed below it.
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.
JS tries to find it to show the required validation error but gets broken because it's not able to find it on the UI
This validation is handled by the browser, probably in Javascript. I think that's a fair trade off, I am wondering if we could show the pop up with error on form submit so we can still benefit from the browser validation but it's probably not worth the effort.
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.
FYI: I ran into the same issue while working on the Product Refactor and I managed to implement the solution I was talking about above. It ended up pretty simple in the 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.
Thanks for implementing this one.
I tried to implement the above, maybe I was looking at it from a different POV and was facing issues. Looking forward to your solution to this in the refactor 😄
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.
The required
JS validation was added after discussing with Mario. It ensures that the popout can't be closed if it's empty, which helps avoid the problem being hidden.
But as you've found, we have a problem if the field is empty and the popout never gets opened.
This change seems like the most practical best solution 👍
I'm not sure if the red border will show on the popout, until you open it. But if there error text is there, I think that's ok.
@@ -58,9 +58,6 @@ | |||
end | |||
|
|||
# Unit popout | |||
fill_in "Unit value", with: "" |
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.
As we don't have required validation as per this, so we can remove its expectation
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.
Can the case of blank unit value now be added here?
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.
Looks good 👍 Thanks @chahmedejaz
@@ -11,19 +11,20 @@ def product_image_form_path(product) | |||
end | |||
|
|||
def prepare_new_variant(product) |
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.
We could remove the helper has it's not doing much now, but that's fine as it is.
@@ -18,7 +18,7 @@ | |||
= f.hidden_field :unit_value | |||
= f.hidden_field :unit_description | |||
= f.text_field :unit_value_with_description, | |||
value: unit_value_with_description(variant), 'aria-label': t('admin.products_page.columns.unit_value'), required: true | |||
value: unit_value_with_description(variant), 'aria-label': t('admin.products_page.columns.unit_value') |
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.
The required
JS validation was added after discussing with Mario. It ensures that the popout can't be closed if it's empty, which helps avoid the problem being hidden.
But as you've found, we have a problem if the field is empty and the popout never gets opened.
This change seems like the most practical best solution 👍
I'm not sure if the red border will show on the popout, until you open it. But if there error text is there, I think that's ok.
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.
Actually sorry I just remembered that we are losing test coverage here. I think the removed test can still be covered in a different way:
@@ -58,9 +58,6 @@ | |||
end | |||
|
|||
# Unit popout | |||
fill_in "Unit value", with: "" |
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.
Can the case of blank unit value now be added here?
Sure, David. |
I think it is covered for |
Sure, David 👍🏻 |
|
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, thank you!
Hey @chahmedejaz , I've edited the DB to trigger the error, and could observer the error 500 when rendering the /products page. After staging the PR, I can confirm the page renders, and that We should keep in mind that this PR only affects the products page and that the shopfront breaks in this case: But at least the user has the possibility to access the products page and fix it - provided they can find the variant which is responsible. I believe this is the intended scenario - the |
What? Why?
What should we test?
nil
as theirunit_value
. i.e. very old products, created at the time when we didn't have validation forunit_value
Release notes
Changelog Category (reviewers may add a label for the release notes):