search

openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 723 forks source link

[BUU] No explanation why cloning failed #12626

Closed drummer83 closed 1 month ago

drummer83 commented 4 months ago

Description

If cloning of a product fails there is an error message. However it does not indicate what's causing the problem, so the user may not be able to fix it.

Expected Behavior

The error message should include information about what went wrong and what could be done.

Actual Behaviour

There is no additional information on the root cause.

Steps to Reproduce

  1. Activate the feature toggle 'admin_style_v3'.
  2. Go to the page /admin/products.
  3. Make cloning of a product fail. It's unclear how it can be done - it happened accidentally. Maybe it had to do with an invalid format of the product image?

Animated Gif/Screenshot

image

Workaround

Severity

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

Your Environment

Possible Fix

chahmedejaz commented 2 months ago

Hi @drummer83 - I've implemented it in such a way that the errors that can be fixed by the user will display which field has an invalid value. Any other errors will still display the default one. Please review the new error message. Thanks. image

CC: @RachL

RachL commented 2 months ago

@chahmedejaz that sounds good! Many thanks. I assume you did it, but in doubt: we need this message to have a translation key.

I'm curious: do you have an example of cloning fail we could use when testing this PR?

drummer83 commented 2 months ago

Thank you, @chahmedejaz! That's fine by me. The text can always be adjusted in Transifex.

Could you please add instructions on how to make cloning fail during testing on our staging server? I have DB access, so I could also tweak or delete some entries on that variant manually, if required.

chahmedejaz commented 2 months ago

@RachL, @drummer83 -

I assume you did it, but in doubt: we need this message to have a translation key

Yes, Rachel, it will have :)

I'm curious: do you have an example of cloning fail we could use when testing this PR?

Could you please add instructions on how to make cloning fail during testing on our staging server?

This happens only for very old products that don't have valid data as per our current product/variant validations. Normally it would be very hard to reproduce unless we have very old products that were created at the time we didn't have new validations.

I have DB access, so I could also tweak or delete some entries on that variant manually, if required.

Sure Konrad, one of the ways you can create invalid products is to set nil to product's variant_unit. When you clone this product, then it will fail. However, doing this will cause some unexpected UI behavior which can be ignored for the sake of its testing. You may refer to this spec which validates this failure case: https://github.com/openfoodfoundation/openfoodnetwork/blob/50ebfe412ceb9fb77308f9f90950759da183ab51/spec/system/admin/products_v3/actions_spec.rb#L266-L278 Please let me know if you have any questions. Thanks