[BUU] Fixes Products Page ActionView::Template::Error #12710

chahmedejaz · 2024-07-25T18:44:40Z

What? Why?

Closes [BUU] ActionView::Template::Error in admin/products_v3#index #12705

This PR fixes this Error by using the previously implemented logic as per https://github.com/openfoodfoundation/openfoodnetwork/blame/6d6f8735e3eff94f53a142538e158116331175cd/app/views/admin/products_v3/_variant_row.html.haml#L22

What should we test?

Visit the Products page, and make sure that some product variants have nil as their unit_value. i.e. very old products, created at the time when we didn't have validation for unit_value
You should be able to view the page without any errors.

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

rioug

Looks good 👍
However I am wondering if we should show a blank value when unit_value and unit_description are nil ? @openfoodfoundation/core-devs ?

dacook · 2024-07-29T00:56:41Z

That sounds logical to me, I forget if there's a reason we didn't before.
I guess with this current method, any time you save the record, it will get updated to 1 without the user necessarily noticing. Essentially it defaults to 1, which might not be correct for that product.

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?

chahmedejaz · 2024-07-29T06:23:56Z

That sounds logical to me, I forget if there's a reason we didn't before. I guess with this current method, any time you save the record, it will get updated to 1 without the user necessarily noticing. Essentially it defaults to 1, which might not be correct for that product.

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. 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

dacook · 2024-07-30T07:15:24Z

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)

chahmedejaz · 2024-07-30T07:21:13Z

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

chahmedejaz · 2024-08-04T12:49:08Z

app/helpers/admin/products_helper.rb

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


Mimicking old UI behavior, when creating a new variant, then both of these are empty

chahmedejaz · 2024-08-04T13:24:51Z

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

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


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.

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.

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.

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 😄

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.

chahmedejaz · 2024-08-04T13:26:02Z

spec/system/admin/products_v3/update_spec.rb

@@ -58,9 +58,6 @@
      end

      # Unit popout
-      fill_in "Unit value", with: ""


As we don't have required validation as per this, so we can remove its expectation

Can the case of blank unit value now be added here?

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/system/admin/products_v3/update_spec.rb#L251

chahmedejaz · 2024-08-04T13:35:58Z

@dacook @rioug - I've addressed your comments above, please review.
Here are the changes:
1014a50
d4e0b2a

Thanks

rioug

Looks good 👍 Thanks @chahmedejaz

rioug · 2024-08-05T00:53:22Z

app/helpers/admin/products_helper.rb

@@ -11,19 +11,20 @@ def product_image_form_path(product)
    end

    def prepare_new_variant(product)


We could remove the helper has it's not doing much now, but that's fine as it is.

dacook · 2024-08-06T05:21:43Z

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

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


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.

dacook

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:

dacook · 2024-08-06T05:26:08Z

spec/system/admin/products_v3/update_spec.rb

@@ -58,9 +58,6 @@
      end

      # Unit popout
-      fill_in "Unit value", with: ""


Can the case of blank unit value now be added here?

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/system/admin/products_v3/update_spec.rb#L251

chahmedejaz · 2024-08-06T05:54:07Z

Can the case of blank unit value now be added here?

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/system/admin/products_v3/update_spec.rb#L251

Sure, David.
Sorry, I thought the blank error functionality is already tested in the above spec and the removed spec was just validating that if the pop-up remains open if the field is blank 😅
No worries, I'll it now 😄

dacook · 2024-08-06T07:10:06Z

Sure, David. Sorry, I thought the blank error functionality is already tested in the above spec and the removed spec was just validating that if the pop-up remains open if the field is blank 😅 No worries, I'll it now 😄

I think it is covered for "adding variants", but it would be worth covering for "updating">"with invalid data" also. That is, at "shows errors for both product and variant fields". I hope that makes sense.

chahmedejaz · 2024-08-06T07:12:42Z

Sure, David. Sorry, I thought the blank error functionality is already tested in the above spec and the removed spec was just validating that if the pop-up remains open if the field is blank 😅 No worries, I'll it now 😄

I think it is covered for "adding variants", but it would be worth covering for "updating">"with invalid data" also. That is, at "shows errors for both product and variant fields". I hope that makes sense.

Sure, David 👍🏻

chahmedejaz · 2024-08-07T12:20:01Z

Can the case of blank unit value now be added here?
https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/system/admin/products_v3/update_spec.rb#L251

Hi @dacook - It's fixed here: 647a384. Thanks.

dacook

Great, thank you!

filipefurtad0 · 2024-08-08T15:20:15Z

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 unit_value remains empty:

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 /products page does not break - I'd say this is looking good. Merging! Thanks.

12705 - set 1 as default for variant's unit_value

e808c7f

github-actions bot assigned chahmedejaz Jul 25, 2024

chahmedejaz changed the title ~~[BUU] - Fixes Products Page Error~~ [BUU] - Fixes Products Page ActionView::Template::Error Jul 25, 2024

chahmedejaz added the bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. label Jul 25, 2024

chahmedejaz marked this pull request as ready for review July 25, 2024 18:58

chahmedejaz requested review from mkllnk, rioug and dacook July 25, 2024 18:58

rioug approved these changes Jul 29, 2024

View reviewed changes

12705 - incorporate old UI behavior

1014a50

- if unit_value is not present and unit_description then display unit_description only. - if both are not present then display empty fields

chahmedejaz commented Aug 4, 2024

View reviewed changes

12705 - fix specs

d4e0b2a

chahmedejaz commented Aug 4, 2024

View reviewed changes

rioug added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Aug 5, 2024

rioug approved these changes Aug 5, 2024

View reviewed changes

dacook approved these changes Aug 6, 2024

View reviewed changes

dacook requested changes Aug 6, 2024

View reviewed changes

12705 - add specs for updating invalid unit_value

647a384

sigmundpetersen changed the title ~~[BUU] - Fixes Products Page ActionView::Template::Error~~ [BUU] Fixes Products Page ActionView::Template::Error Aug 7, 2024

chahmedejaz requested a review from dacook August 7, 2024 12:18

dacook approved these changes Aug 7, 2024

View reviewed changes

filipefurtad0 self-assigned this Aug 8, 2024

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Aug 8, 2024

filipefurtad0 merged commit 76d874d into openfoodfoundation:master Aug 8, 2024
52 checks passed

filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Aug 8, 2024

dacook added user facing changes Thes pull requests affect the user experience and removed feature toggled These pull requests' changes are invisible by default and are grouped in release notes labels Aug 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Fixes Products Page ActionView::Template::Error #12710

[BUU] Fixes Products Page ActionView::Template::Error #12710

chahmedejaz commented Jul 25, 2024 •

edited by rioug

Loading

rioug left a comment

dacook commented Jul 29, 2024

chahmedejaz commented Jul 29, 2024

dacook commented Jul 30, 2024

chahmedejaz commented Jul 30, 2024 •

edited

Loading

chahmedejaz Aug 4, 2024

chahmedejaz Aug 4, 2024

rioug Aug 5, 2024

rioug Aug 5, 2024

chahmedejaz Aug 5, 2024

dacook Aug 6, 2024

chahmedejaz Aug 4, 2024

dacook Aug 6, 2024

chahmedejaz commented Aug 4, 2024

rioug left a comment

rioug Aug 5, 2024

dacook Aug 6, 2024

dacook left a comment

dacook Aug 6, 2024

chahmedejaz commented Aug 6, 2024

dacook commented Aug 6, 2024

chahmedejaz commented Aug 6, 2024

chahmedejaz commented Aug 7, 2024

dacook left a comment

filipefurtad0 commented Aug 8, 2024

		@@ -11,19 +11,20 @@ def product_image_form_path(product)
		end

		def prepare_new_variant(product)

[BUU] Fixes Products Page ActionView::Template::Error #12710

[BUU] Fixes Products Page ActionView::Template::Error #12710

Conversation

chahmedejaz commented Jul 25, 2024 • edited by rioug Loading

What? Why?

What should we test?

Release notes

rioug left a comment

Choose a reason for hiding this comment

dacook commented Jul 29, 2024

chahmedejaz commented Jul 29, 2024

dacook commented Jul 30, 2024

chahmedejaz commented Jul 30, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahmedejaz commented Aug 4, 2024

rioug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

chahmedejaz commented Aug 6, 2024

dacook commented Aug 6, 2024

chahmedejaz commented Aug 6, 2024

chahmedejaz commented Aug 7, 2024

dacook left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Aug 8, 2024

chahmedejaz commented Jul 25, 2024 •

edited by rioug

Loading

chahmedejaz commented Jul 30, 2024 •

edited

Loading