Saving failed notification when saving product changes in spree - UK

[sineadfenton]

Description

In UK production site, in spree. Click save on the product page and you see an error message when infact the request has been processed. This is not the intended flow, instead you should see a saved/complete notification.

Expected Behavior

You should seed a "saved" notification when you click save and not 'Saving failed.Save failed due to invalid data:422'. This should only show when there is an error.

Actual Behavior

You click save and Saving failed.Save failed due to invalid data:422 comes up

Steps to Reproduce

1. Go to product page
2. Make a change - I changed the weight of an item.
3. Click save
4. Saving processes and then you get Saving failed with "Save failed due to invalid data:422" at the bottom of the screen
5. Refresh the page. You'll see your changes have actually gone through

Animated Gif/Screenshot

Context

Users will be confused and this there is an error when there is not. this will result in more support requests

Severity

Your Environment

• Version used:
• Browser name and version: Chrome
• Operating System and version (desktop or mobile): Desktop, windows UK production was upgraded to 1.20.0 today (24/09) and this issue is new as of today.

Possible Fix

Have the correct message when a task is successfully completed, and error show up only when there is an error.

[sineadfenton]

Saving failed.Save failed due to invalid data:422

[myriamboure]

@sineadfenton I logged in today on UK production with a hub manager user to reproduce the bug and see how to move it forward. I couldn't reproduct it, I tested on both price and weight and had the correct "product saved" message. Can you please describe more precisely which user was concerned, were you logged in as super admin? What product exactly did you try changing, for which type of user, etc. ? Is the user trying to change the product info the owner of the producer profile or a hub manager that has right to edit products on that producer? Without those info I don't see how we can move forward, if we can't reproduce the bug...

[OliverUK]

@myriamboure It happens when I'm logged in and try and save a change in BEP. It doesn't seem to matter what the change is.

http://recordit.co/PDpnjTo1pt

[myriamboure]

@OliverUK as I could do it on UK production using Nick account, it seems to happen under certain specific conditions. Your profile is regular hub owner and manager. Not super admin. You are the owner of Global Organic Markets, in your video the product you try to upload info for, so there shouldn't be any permission bug.... Global organic markets sell the products through multiple hubs, so catalog seems to be shared by multiple distributors, but only one (stroudco) can manage products. All those are exactly same conditions as when I did the test yesterday on UK production. Can it be linked to a loading time error @kristinalim ? @OliverUK that could help Kristina if you can before reproducing the error click on your browser menu "developers tool / console" and look at consol tab and share screenshot of the error that appears when you try to modify a product. Then you can click on the error to see details and share them. You can click on "network" and share details. Unless @kristinalim manages to reproduce the bug... it might be hard to find where the issue comes from :-(

[kristinalim]

@myriamboure No, it's not related to a server timeout.

It's unlikely related to permissions too. My investigation so far is hinting that the bulk update is attempting to save invalid data somewhere, but unfortunately I don't know where yet.

Finding what caused the 422

The error code (HTTP status) above is 422.

I have tested that:

• Normal authorization issue - 401
• Invalid products and variants - 400
• Some other scenarios - 500

But when we save products and variants, there are other updates that happen outside of these records (e.g. associated data, possible caches). I was able to get a 422 when I faked a validation error (ActiveRecord::RecordInvalid), but I haven't been able to find areas where this could have happened yet.

I will search a bit more... I'm not familiar with the procedure for debugging with live data yet. I might request server access if needed/possible, or ask another dev to run code that might give more information.

Some (not all) changes saved in spite of error

You'll see your changes have actually gone through

It seems that submitting even with an obvious invalid input saves changes for some (not all) valid records. Ideally, no changes are saved if there is a problem with any record. I created a separate issue for this: openfoodfoundation/wishlist#336

[OliverUK]

@myriamboure I'm afraid the only thing I get in the console is that google analytics was blocked.

[OliverUK]

@kristinalim I hope this isn't just confusing the issue but I've got another problem and wondered if it might be related. If I got to the inventory for Stroudco and try and filter for Producer:Wharf Farm, it doesn't come up. They also don't come up if I leave Producer unfiltered and search for their produce. This happens for me logged in as me and it happens when I log in as super admin but it seems to work fine for Nick. Edit: I should mention I've checked enterprise permissions and it's something that's been fine before. http://recordit.co/k6dAumaPgP

[myriamboure]

@OliverUK please to avoid mixing up conversation please open a new issue for the new bug. It is likely all those bug might have a common cause, but let's keep discussion separate so that we can investigate for each one. Can you open a new issue please and then reference it here to ask if that can be related?

[myriamboure]

Thanks @OliverUK ! Here is the issue as a record: https://github.com/openfoodfoundation/openfoodnetwork/issues/2799

[kristinalim]

@OliverUK If it is from a permission problem, it could be related. Thanks for creating the new issue!

I have been able to confirm that openfoodfoundation/openfoodnetwork#2776 is not related to this issue. For Sacred Lotus:

• All products are valid
• Their master variants are valid
• Their non-master variants are valid

[kristinalim]

Sorry, I haven't been able to figure this out yet.

I've already checked nested associated records of products under Sacred Lotus and Global Organic Markets, and haven't been able to find anything which the software considers invalid. It appears that this isn't related to openfoodfoundation/openfoodnetwork#2776.

I've looked into possible permission related issues too, but no promising hint so far.

I'll be back to continue investigating in a couple of hours.

[Matt-Yorkley]

I've added myself as a manager for that enterprise and can update the products on that page successfully...? (logged in as manager, not superadmin)

[myriamboure]

@Matt-Yorkley maybe you can "sit" (virtually) with @OliverUK or even hack his account with his permission to reproduce the bug? It seems @OliverUK has the issue with any item he tries to change in BEP... but no one managed to reproduce the error so far.

[myriamboure]

@OliverUK is it only for sacred lotus that you have an issue? Or all products you have access to?

[OliverUK]

@Matt-Yorkley @myriamboure fine by me. Can change my password to a temporary one and share with you, Matt.

[myriamboure]

@Matt-Yorkley I'm wondering if there is not some common problem also below this issue https://github.com/openfoodfoundation/openfoodnetwork/issues/2739#issuecomment-425881286, I found that behavior super strange, seems like a big breach in our app... and also https://github.com/openfoodfoundation/openfoodnetwork/issues/2756#issuecomment-423355684. I don't know it feels to me like if all those issues could be connected... @OliverUK did this start just after you were added as a manager to a new enterprise? Or created a new enterprise? Do you have any clue of things you've done just before it starts bugging?

[OliverUK]

@Matt-Yorkley and maybe also this: https://github.com/openfoodfoundation/openfoodnetwork/issues/2799

Nick made me manager of a new enterprise on 13th September but there is a good week between that and the above. It's closer to the v1.20 release.

I'm glad no one else has reported this. I'm happy to let you log in as me and see what happens.

[OliverUK]

@Matt-Yorkley have sent you my password on Slack

[Matt-Yorkley]

Okay, I've tried with Oliver's login, and I can reproduce the bug with that. I get the 422 error. So a permissions issue makes the most sense, if I can save on one account and not on another.

Bugsnag is giving me nothing, and the production log is also giving me nothing...

[Matt-Yorkley]

@kristinalim maybe you can have a look as well. I'm looking in the #bulk_update method of products_controller_decorator.rb and trying to match up what's shown in the logs.

Production log from failed update "422" error:

It looks like the products_set is being saved, as the redirect to to the api route is definitely happening. And I guess the authorize! in line 44 is also passing, otherwise the response would be different...?

[lin-d-hop]

Was anything that touches permissions recently merged to master? This appears in v1.20.

[myriamboure]

@HugsDaniel just did some tests and found strange permission bugs on can instance, seems all the s2 and some s3 bugs could be linked to a same permission issue.

[lin-d-hop]

@myriamboure I definitely think they are all related!

[Matt-Yorkley]

Update: on oliver's account, I try to update a product and the error message shows, but if I reload the page the product has actually been updated successfully.

So the message is misleading.

[HugsDaniel]

Update: on oliver's account, I update a product and the error message shows, but if I reload the page the product has actually been updated successfully.

This doesn't appear to be the case in Can instance

[kristinalim]

Update: on oliver's account, I update a product and the error message shows, but if I reload the page the product has actually been updated successfully.

@Matt-Yorkley This could be misleading because an incomplete save continues to save other changes: https://github.com/openfoodfoundation/openfoodnetwork/issues/2776

[Matt-Yorkley]

But if I can save with one account with no errors, but on another account I get the 422, we can assume the product data that's being saved is ok, right?

[kristinalim]

@Matt-Yorkley Yes, you're right.

[Matt-Yorkley]

The redirect to '/api/products/bulk_products?page=1;per_page=500;' after saving seems a bit odd... we fetch 500 products every time we hit save on one or more products?

[Matt-Yorkley]

Also, the angular controller looks like it should be passing $scope.currentFilters as part of the request, but this always seems to be blank, and it doesn't seem to be set anywhere?

[Matt-Yorkley]

@kristinalim Looks like the initial POST request is fine, but the subsequent redirect to the API route is what returns the 422, with this:

[kristinalim]

@Matt-Yorkley I'm on it now too. Getting the same error in the console when running this:

u = Spree::User.find_by_email("olivermuller@gmx.com")
products = OpenFoodNetwork::Permissions.new(u).editable_products.merge(Spree::Product.not_deleted).order('created_at DESC').ransack({}).result.page(1).per(500)
json_object = ActiveModel::ArraySerializer.new(products, each_serializer: Api::Admin::ProductSerializer)
json_object.to_json

[kristinalim]

Nice detective work, @Matt-Yorkley. So it's #import_date that's breaking. It's a simple change - I'll submit the PR for this.

[kristinalim]

I submitted PR openfoodfoundation/openfoodnetwork#2831 which should address this.

Sorry I was looking at the wrong places. :slightly_frowning_face: HTTP 422 is normally used when submitted data could not be processed, and the error was happening in a separate read-only request that did not indicate the error in the logs.

[lin-d-hop]

UK have deployed this patch to prod and can confirm that the issues we've been experiencing have been fixed. Thanks humble superheros!

[OliverUK]

Thank you guys!! Well done indeed.