Saving failed notification when saving product changes in spree - UK #2769

sineadfenton · 2018-09-24T18:56:50Z

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

Go to product page
Make a change - I changed the weight of an item.
Click save
Saving processes and then you get Saving failed with "Save failed due to invalid data:422" at the bottom of the screen
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 · 2018-09-24T18:57:01Z

Saving failed.Save failed due to invalid data:422

myriamboure · 2018-09-25T15:44:48Z

@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 · 2018-09-26T10:37:19Z

@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 · 2018-09-26T15:19:45Z

@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 · 2018-09-26T19:24:24Z

@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 · 2018-09-27T09:42:24Z

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

OliverUK · 2018-09-27T10:06:29Z

@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 · 2018-10-01T09:55:35Z

@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 · 2018-10-01T12:17:36Z

Thanks @OliverUK ! Here is the issue as a record: #2799

kristinalim · 2018-10-04T04:18:16Z

@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 #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 · 2018-10-04T12:42:44Z

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 #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 · 2018-10-05T14:48:28Z

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 · 2018-10-05T15:00:28Z

@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 · 2018-10-05T15:01:42Z

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

OliverUK · 2018-10-05T15:02:59Z

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

myriamboure · 2018-10-05T15:05:34Z

@Matt-Yorkley I'm wondering if there is not some common problem also below this issue #2739 (comment), I found that behavior super strange, seems like a big breach in our app... and also #2756 (comment). 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 · 2018-10-05T15:24:45Z

@Matt-Yorkley and maybe also this: #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 · 2018-10-05T15:28:26Z

@Matt-Yorkley have sent you my password on Slack

Matt-Yorkley · 2018-10-05T15:58:30Z

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 · 2018-10-05T16:05:27Z

@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 · 2018-10-05T16:06:27Z

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

myriamboure · 2018-10-05T16:10:27Z

@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 · 2018-10-05T16:14:17Z

@myriamboure I definitely think they are all related!

Matt-Yorkley · 2018-10-05T16:16:27Z

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 · 2018-10-05T16:19:30Z

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 · 2018-10-05T16:19:48Z

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: #2776

Matt-Yorkley · 2018-10-05T16:29:26Z

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 · 2018-10-05T16:32:43Z

@Matt-Yorkley Yes, you're right.

Matt-Yorkley · 2018-10-05T16:37:43Z

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 · 2018-10-05T17:13:29Z

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 · 2018-10-05T17:59:15Z

@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 · 2018-10-05T18:02:24Z

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

u = Spree::User.find_by_email("[email protected]")
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 · 2018-10-05T18:06:06Z

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 · 2018-10-05T19:31:42Z

I submitted PR #2831 which should address this.

Sorry I was looking at the wrong places. 🙁 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 · 2018-10-06T11:40:41Z

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

OliverUK · 2018-10-07T13:42:27Z

Thank you guys!! Well done indeed.

sineadfenton added the bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. label Sep 24, 2018

kristinalim self-assigned this Sep 26, 2018

kristinalim mentioned this issue Nov 18, 2022

Invalid input in the "Bulk Edit Products" should not update any record openfoodfoundation/wishlist#336

Open

This was referenced Sep 27, 2018

Cannot Duplicate Order Cycle #2764

Closed

Able to end up with an invalid variant through web UI #2776

Closed

myriamboure mentioned this issue Oct 1, 2018

Inventories broken: changes to inventory don't save #2739

Closed

kristinalim mentioned this issue Oct 5, 2018

Fix product import date error when some but not all variants have import date #2831

Merged

OliverUK mentioned this issue Oct 7, 2018

Producer does not appear in inventory #2799

Closed

sigmundpetersen closed this as completed Oct 10, 2018

sineadfenton mentioned this issue Oct 24, 2018

Saving Failed, 502 error when changing product prices in Spree #2919

Closed

Saving failed notification when saving product changes in spree - UK #2769

Saving failed notification when saving product changes in spree - UK #2769

Comments

sineadfenton commented Sep 24, 2018 • edited by sigmundpetersen Loading

Description

Expected Behavior

Actual Behavior

Steps to Reproduce

Animated Gif/Screenshot

Context

Severity

Your Environment

Possible Fix

sineadfenton commented Sep 24, 2018

myriamboure commented Sep 25, 2018

OliverUK commented Sep 26, 2018

myriamboure commented Sep 26, 2018

kristinalim commented Sep 26, 2018

Finding what caused the 422

Some (not all) changes saved in spite of error

OliverUK commented Sep 27, 2018

OliverUK commented Sep 27, 2018 • edited Loading

myriamboure commented Oct 1, 2018

myriamboure commented Oct 1, 2018

kristinalim commented Oct 4, 2018

kristinalim commented Oct 4, 2018

Matt-Yorkley commented Oct 5, 2018 • edited Loading

myriamboure commented Oct 5, 2018

myriamboure commented Oct 5, 2018

OliverUK commented Oct 5, 2018

myriamboure commented Oct 5, 2018 • edited Loading

OliverUK commented Oct 5, 2018

OliverUK commented Oct 5, 2018

Matt-Yorkley commented Oct 5, 2018

Matt-Yorkley commented Oct 5, 2018 • edited Loading

lin-d-hop commented Oct 5, 2018

myriamboure commented Oct 5, 2018

lin-d-hop commented Oct 5, 2018

Matt-Yorkley commented Oct 5, 2018 • edited Loading

HugsDaniel commented Oct 5, 2018

kristinalim commented Oct 5, 2018

Matt-Yorkley commented Oct 5, 2018

kristinalim commented Oct 5, 2018 • edited Loading

Matt-Yorkley commented Oct 5, 2018 • edited Loading

Matt-Yorkley commented Oct 5, 2018 • edited Loading

Matt-Yorkley commented Oct 5, 2018

kristinalim commented Oct 5, 2018

kristinalim commented Oct 5, 2018

kristinalim commented Oct 5, 2018

lin-d-hop commented Oct 6, 2018 • edited Loading

OliverUK commented Oct 7, 2018

sineadfenton commented Sep 24, 2018 •

edited by sigmundpetersen

Loading

OliverUK commented Sep 27, 2018 •

edited

Loading

Matt-Yorkley commented Oct 5, 2018 •

edited

Loading

myriamboure commented Oct 5, 2018 •

edited

Loading

Matt-Yorkley commented Oct 5, 2018 •

edited

Loading

Matt-Yorkley commented Oct 5, 2018 •

edited

Loading

kristinalim commented Oct 5, 2018 •

edited

Loading

Matt-Yorkley commented Oct 5, 2018 •

edited

Loading

Matt-Yorkley commented Oct 5, 2018 •

edited

Loading

lin-d-hop commented Oct 6, 2018 •

edited

Loading