openfoodfoundation / openfoodnetwork

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

[BUU] Edits on new product page are leaving another product with an empty price #11633

Closed RachL closed 5 months ago

RachL commented 1 year ago

Description

Better check the step to reproduce to understand

Steps to Reproduce

Under admin_style_v3 feature toggle

  1. Go to staging FR and set your user as a manager of Release Farm
  2. Go to Products v3 page
  3. Try changing something like removing a "y" in Fryys
  4. Hit the save button
  5. See an error message gets triggered on the large pizza about an empty price, also you didn't change the large pizza during your setting

Animated Gif/Screenshot

Before saving

image

After saving

image

Severity

bug-s3: a feature is broken but there is a workaround

dacook commented 8 months ago

This is due to the variant being invalid, a problem which would appear if you had updated that row, or edit it in the variant screen. I wasn't able to replicate this on staging because I couldn't find any variants with a blank price anymore, but I think it would still be an issue.

But you're right that it's strange to be updating a row that hasn't been changed. I had hoped to filter and ignore unchanged rows on the client side (like we do with angular), but I can't find a way to do it with StimulusReflex, because its form serialisation method isn't customisable . It's a real shame because the user interface makes it look like it's doing that (with the orange borders). Sometimes I'd like to switch to using a rails standard form submit (probably with ujs), but I'm not sure if it's possible to customise serialisation in that case either. One way to achieve it would be to alter the inputs, eg make them all disabled upon submit.

Estimate

  1. Add beforeBulkUpdate JS to disable any un-changed elements. Ensure disabled elements look the same when being submitted. And they should be re-enabled if the submit is cancelled/failed for any reason.
  2. Hmm, it should be easy to do on the server side in Rails, we can just ignore each record unless product/variant.changed? (see product_set.rb)

Each are approx 0.5 days For efficiency, we could do both, but opt 2 would be quickest and solve this problem on its own.

RachL commented 6 months ago

@dacook for this one, I guess I need to retest once SR is removed from the page, correct?

dacook commented 6 months ago

Actually that probably won't make a difference, so I wouldn't bother retesting.

I've updated the estimate section, to make the solution clearer.

RachL commented 5 months ago

I can't reproduce this one anymore. I'm closing it, let's reopen if we find out how to reproduce :+1: