Invalid input in the "Bulk Edit Products" should not update any record

[kristinalim]

Submitting with some invalid input in the "Bulk Edit Products" page may still save changes for some (not all) valid records.

Description

Let's say you are updating products and variants, and one of them is left invalid.

The bulk product update goes through the products and variants and attempts to save them one by one, not necessarily in the order that they appear in the UI. When it encounters the record that is invalid, it stops attempting to save subsequent records. But changes to previous records have already been saved.

This error message appears: "Saving failed.Save failed due to invalid data:400"

See a demonstration of this behaviour:

Expected Behavior

Ideally, no changes are saved if there is invalid input.

Actual Behavior

Some, not all, valid records are saved.

Steps to Reproduce

As manager of enterprise with few products:

1. Go to "Bulk Edit Products" page.
2. Change the names of all of the products, but leave a product name in the middle of the list blank.
3. Click on "Save Changes". Notice the error.
4. Reload the page. Notice that some of the product names have been saved.

The order in which products are updated is arbitrary, so there is a chance that this will not be replicated in the first try.

Context

I am investigating openfoodfoundation/openfoodnetwork#2769.

Severity

bug-s3

Possible Fix

Wrap updates in a DB transaction.

[sigmundpetersen]

Is the observation in openfoodfoundation/openfoodnetwork#2764 the same issue?

[kristinalim]

Thanks for pointing to that issue, @sigmundpetersen. Yes, this part is the same issue: "when reloading the page the OC has been duplicated but the OC has no variants" In that scenario, there seems to be a failure duplicating the variant associations but the OC itself was created anyway.

The 422 error in openfoodfoundation/openfoodnetwork#2764 is likely caused by the same 422 error in openfoodfoundation/openfoodnetwork#2769. I was already able to successfully create invalid variants through the web UI, but I'm still figuring out the exact scenario that causes openfoodfoundation/openfoodnetwork#2769. 🕵️‍♀️ Will update.

[myriamboure]

I pudated to s3 as it can be pretty confusing for users and lead to catalog management annoying mistakes.

[sauloperez]

This is tightly related to the changes done in openfoodfoundation/openfoodnetwork#2845 to fix openfoodfoundation/openfoodnetwork#2776 .

Agree on the transaction solution but I'm starting to think having bulk actions in the API is not a good idea. The longer it takes to finish the transaction the more chances of causing performance problems due to lock contention (and our app is not particularly performant in terms of DB).

[Matt-Yorkley]

I think we've previously discussed in private the idea of basically remaking the Bulk Edit Products page so that it no longer loads every available product into the DOM. It creates terrible performance issues for superadmins or anyone with a lot of enterprises and the problems get progressively worse as our instances scale, creating issues like this one: openfoodfoundation/openfoodnetwork#2919

There are 18 issues referencing Bulk Edit Product: https://github.com/openfoodfoundation/openfoodnetwork/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+bulk+edit+product

It was an interesting experiment in Angular live-search style filtering, but now it's a monster. Could we de-prioritise BEP bugs and prioritise replacing the BEP page with a functionally similar but technically simpler implementation that uses pagination?

@mkllnk @luisramos0 @kristinalim @sauloperez

[Matt-Yorkley]

In this issue for example, wrapping the transaction is ok with a test of 4 products, but on a live instance there could be 1000 products loaded on that page at once, and often Angular is silently loading more and more products in the background. So if you hit save at one point and then hit save again in 2 seconds time the data being posted will not even be the same (even though the UI may not reflect it).

If you leave that page open on UK production as a superadmin it will spawn new asynchronous requests to the server and pull more products into (client-side) memory every couple of seconds for at least half an hour...

[luisramos0]

yes, it needs rebuilding.
it should load paged results from the server... a wonderful excuse to build pagination on the api!

[mkllnk]

I agree. The data needs to be loaded in a different way. It's broken for admins. But I'm wondering if we need some kind of inception first. It will change the UX.

[luisramos0]

hello @RachL I see you put this in the top 5 bugs.
Was this issue discussed in the gathering? Are we building pagination in this page as indicated in the discussion here? That can take a few dev days.

A quick fix to make it save only when there are no errors could create problems. It would be a tricky change in this method.

[RachL]

@luisramos0 no we didn't discussed this at the gathering. We just agree that I would make a proposition out of the top of the s3 backlog. We can of course put something else and come back to this bug later.

[luisramos0]

ok 👍
I'd fix this one after we change pagination on this page...

[RachL]

@luisramos0 alright so we remove it from the top s3?

[luisramos0]

we can hear other people or decide ourselves.
I'd say, maybe for now, we move it out of top 5 as it's a big task.