Users cannot save changes to product list or inventory

tschumilas commented 5 years ago

Description

SELECTED Users on OFN-CAN cannot save changes (stock on hand, prices..) to existing products in either product list or inventory. But it seems (from limited testing) that super admin can do so. Yesterday I posted this on slack - but from my first tests, I thought it was just affecting my own enterprises - but this morning at least one other user reported this. I have canvased users to see how widespread it is. If it helps with narrowing this down, the enterprises I know are affected: Garden Party Flower Farm, La Primavera Flower Farm --- these are both associated with 2 hubs: The Local Flower Collective and Local Flower Hub Preview shop.

Expected Behavior

Users should be able to manage their product list!!!

Actual Behaviour

When users try to save a change to their product list (like updating stock) they get a message: Saving failed. Save failed due to invalid data:500. When users try to save data in their inventory they get a different error: I couldn't get authorisation to save those changes, so they remain unsaved. As super-admin, I am able to save changes. (I think - still testing this)

Steps to Reproduce

To reproduce with sample data created by the Rake task ofn:sample_data:

Update sells to any on Freddy's Farm Shop to make it a producer distributor.
Update Freddy's Farm Shop OC to sell Fredo's products.
Update Fredo's Farm Hub OC to sell Freddy's products.
Attempting to edit Fredo's Farm products in the Bulk Edit Products results in "Saving failed.Save failed due to invalid data:500"

Animated Gif/Screenshot

still testing - will post screenshots and further info - but wanted this up there given its s1

Context

As super admin - users are sending me all their updates by email this morning so I can make their changes. Not very sustainable

Severity

Your Environment

Version used: 2.2
Browser name and version: chrome
Operating System and version (desktop or mobile): windows 10
OFN Platform instance where you discovered the bug, and which version of the software they are using. OFN-CAN

Possible Fix

tschumilas commented 5 years ago

I created a second issue - cannot adjust enterprise permissions - 4139. Trying to find out more about 4138, I tried to manage permissions. As super admin - I am not able to manage permissions for these same grouping of enterprises. They are all associated with the same hubs. Trying to find out more.

kristinalim commented 5 years ago

Here is a link to the related Bugsnag error: https://app.bugsnag.com/open-food-network-canada-1/open-food-network-canada/errors/5d4b6cfdd5b544001a5a9e26

RuntimeErrorspree/admin/products#bulk_update

See image:

20190808203614-runtime_error_product_bulk_update

kristinalim commented 5 years ago

This is the sample params for the above error:

{
  "products": [
    {
      "id": 3077,
      "variants_attributes": [
        {
          "price": "7.00",
          "display_name": "seed pods"
        }
      ]
    }
  ]

This wouldn't create the variant, but I don't understand why it is executing on_hand= here which causes the error.

@sauloperez Any ideas?

kristinalim commented 5 years ago

I can reproduce this issue when there is a supplier-distributor loop, same as in #4139:

Hub A is a distributor of Product B of which Hub B is supplier
Hub B is a distributor of Product A of which Hub A is supplier

This should also happen if the loop is larger, involving more enterprises.

To reproduce with sample data created by the Rake task ofn:sample_data:

Update Fred's Farm to make it a producer distributor.
Set up a shipping method and payment method for Fred's Farm.
Create an OC where Fred's Farm is a distributor of Fredo's Farm products.
Attempting to edit Fredo's Farm products in the Bulk Edit Products results in "Saving failed.Save failed due to invalid data:500"

tschumilas commented 5 years ago

Above I said I was able to save stock as super admin -- and I was, for some. But now for 3 enterprises, (the three involved in this loop I guess), I cannot save stock as super admin either. Is there any work around you can think of for now (this hub opens in 4 hours) that I can do? If I create new enterprises for the ones in the loop, and re-assign the products to the new enterprises, will that work?

tschumilas commented 5 years ago

Just tried the above - as super admin - still won't let me save the product. I created a new enterprise, and tried to transfer a product from one of the problematic enterprises (ie: change the name of the supplier) but - it sill wouldn't save.

kristinalim commented 5 years ago

I'm checking @tschumilas, but I'm not confident that there won't be side-effects when you reassign the products to the new enterprises...

BTW regarding which enterprises are affected, I've confirmed that Garden Party Flower Farm (ID 243) and Garden Party Flower Shop (ID 333) are in a loop. La Primavera Farms (ID 257) is affected because it supplies to Garden Party Flower Farm.

kristinalim commented 5 years ago

For devs, noting also that the Bugsnag error I noted above seems unrelated to this reported issue.

kristinalim commented 5 years ago

The issue is because of Enterprise#touch_distributors which can cause an infinite loop.

tschumilas commented 5 years ago

So - if I could just remove la Primavera (257) as a supplier to Garden Party Flower Farm - would that solve the problem at least for them. (They are the farm I'm trying to get products updated for in the next few hours). But I can't seem to remove their permissions. They are not supplying Garden Party in an active OC or anything - but I think the did at one time, so the permission is there.

tschumilas commented 5 years ago

Actually - just checked and La Prima vera no longer has permission to sell through Garden Party , but removing that didn't solve the problem. Not sure when that got removed. Would removing the permission have prompted the problem? I might have removed it in the last few days - could that have caused this?

kristinalim commented 5 years ago

@tschumilas It has to do with the Garden Party Flower Farm being distributor for any La Primavera Farms products in any OC, whether the OC is still open or not.

tschumilas commented 5 years ago

Frig. can't just change old OC's - eh? I tried to do that - but they won't re-save. For now - what I've done (couldn't think of anything else). I've created a new 'La Primavera Farm 2' enterprise, and am entering new products there that need to be sold in today's OC. Then we'll just pull La Primavera 2 into the OC, and not La Primavera 1. Then I"ll do the same for Garden Party. (Unless a solution is worked out before I get there.). I feel bad because I think I did this somehow. I was changing permissions for Garden Party Farm and Garden Party Flower Shop the other day, and noticed then that I was getting a 'no products in inventory' message in the OC incoming - when I KNEW there were products in inventory. I posted on slack - thought it was just Garden Party, so didn't do a git issue right away. Fiddle sticks. I'm annoyed at myself.

kristinalim commented 5 years ago

@tschumilas There is only one OC where Garden Party Flower Shop (333) is distributor for Garden Party Flower Farm (243): https://openfoodnetwork.ca/admin/order_cycles/416/edit I'm not sure about the complete consequences of updating old OCs... but if the outgoing exchange is gone it would break the loop.

tschumilas commented 5 years ago

Indeed this is exactly where the problems started. So - La Primavera isn't listed in this OC. But maybe they were at first (I may have adjusted their permission after creating this OC) The incoming is Garden Party Flower FARM and the outgoing is Garden Party Flower SHOP (the coordinator). I just tried to delete Garden Party Flower SHOP in outgoing, and add Garden Party Flower Farm as the distributor. But it won't save. maybe I should try to sub a different outgoing enterprise?

tschumilas commented 5 years ago

FYI - what I've done in order to get the affected enterprise into the current order cycle (hundreds of dollars at stake for them otherwise): I created a new enterprise with a slightly modified farm name. I imported a list of products that they have for sale this week (I did not re-assign or copy from the affected/infected enterprise). I took the problem enterprises out of this weeks OC, and put the new one in. So - basically, its a new enterprise in this week's OC. By next week, I'm hopeful we'll figure out how to get us out of this loop. (Like being in Groundhog Day, or a bad Twighlight Zone enterprise where everytime visitors try to leave a town, they find themselves re-entering the town) Thanks to all of you for working on this. Let me know how to help/what to test....

mkllnk commented 5 years ago

@tschumilas Don't blame yourself. The software should not allow you to break it. This is a bug and not your fault. You are doing amazing work in setting it all up. :hugs:

tschumilas commented 5 years ago

thats kind @mkllnk - but I'm pretty sure I did something without realizing it. I'm here to test things if it helps you folks figure out a solution. If not - then I'm also OK to create new enterprises and enter new products.

mkllnk commented 5 years ago

I'm wrote a spec for this bug and one order cycle with two enterprises, both supplying products and selling both their products is enough to trigger this.

    context "in a circular order cycle setup" do
      let(:enterprise1) { create(:distributor_enterprise, is_primary_producer: true) }
      let(:enterprise2) { create(:distributor_enterprise, is_primary_producer: true) }
      let(:variant1) { create(:variant) }
      let(:variant2) { create(:variant) }
      let!(:order_cycle) do
        enterprise1.supplied_products << variant1.product
        enterprise2.supplied_products << variant2.product
        create(
          :simple_order_cycle,
          coordinator: enterprise1,
          suppliers: [enterprise1, enterprise2],
          distributors: [enterprise1, enterprise2],
          variants: [variant1, variant2]
        )
      end

      pending "saves without infinite loop" do
        expect(variant1.update_attributes(cost_price: 1)).to be_truthy
      end
    end
  end

mkllnk commented 5 years ago

Executing the spec, I find many repetitions that look like touching an enterprise in the log:

   (0.4ms)  SELECT COUNT("spree_products"."id") FROM "spree_products" WHERE "spree_products"."supplier_id" = 44 AND ("spree_products".deleted_at IS NULL)
  Spree::Product Load (0.2ms)  SELECT id FROM "spree_products" WHERE "spree_products"."supplier_id" = 44 AND ("spree_products".deleted_at IS NULL)
  Enterprise Load (1.1ms)  SELECT "enterprises".* FROM "enterprises" WHERE "enterprises"."id" IN (SELECT DISTINCT enterprises.id FROM "enterprises" INNER JOIN exchanges
 ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id) INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (38))) AND (enterprises.id != 44) ORDER BY "enterprises"."id" ASC LIMIT 1000
  SQL (0.4ms)  UPDATE "enterprises" SET "updated_at" = '2019-08-09 05:51:24.535924' WHERE "enterprises"."id" = 43

So I looked for that and found this line in the product decorator:

  belongs_to :supplier, class_name: 'Enterprise', touch: true

Removing , touch: true solves the infinite loop. Now we have to find out what we need the touch for and if we can do it differently.

mkllnk commented 5 years ago

For a quick fix, I'm looking at Canada's data and if I can break the loop. This is how to trigger it from the console and the repeating log entries:

Enterprise.find_by_name("Garden Party Flower Farm").supplied_products.first.update_attributes(meta_description: nil)

   (1.3ms)  SELECT COUNT("spree_products"."id") FROM "spree_products" WHERE "spree_products"."supplier_id" = 149 AND ("spree_products".deleted_at IS NULL)
  Enterprise Load (1.5ms)  SELECT "enterprises".* FROM "enterprises" WHERE "enterprises"."id" IN (SELECT DISTINCT enterprises.id FROM "enterprises" INNER JOIN exchanges
 ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id) INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (NULL))) AND (enterprises.id != 149) ORDER BY "enterprises"."id" ASC LIMIT 1000
  SQL (0.6ms)  UPDATE "enterprises" SET "updated_at" = '2019-08-09 06:17:37.921071' WHERE "enterprises"."id" = 249
   (1.3ms)  SELECT COUNT("spree_products"."id") FROM "spree_products" WHERE "spree_products"."supplier_id" = 249 AND ("spree_products".deleted_at IS NULL)
  Enterprise Load (1.5ms)  SELECT "enterprises".* FROM "enterprises" WHERE "enterprises"."id" IN (SELECT DISTINCT enterprises.id FROM "enterprises" INNER JOIN exchanges
 ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id) INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (NULL))) AND (enterprises.id != 249) ORDER BY "enterprises"."id" ASC LIMIT 1000
  SQL (0.8ms)  UPDATE "enterprises" SET "updated_at" = '2019-08-09 06:17:37.927640' WHERE "enterprises"."id" = 318
   (1.4ms)  SELECT COUNT("spree_products"."id") FROM "spree_products" WHERE "spree_products"."supplier_id" = 318 AND ("spree_products".deleted_at IS NULL)
  Enterprise Load (1.6ms)  SELECT "enterprises".* FROM "enterprises" WHERE "enterprises"."id" IN (SELECT DISTINCT enterprises.id FROM "enterprises" INNER JOIN exchanges
 ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id) INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (NULL))) AND (enterprises.id != 318) ORDER BY "enterprises"."id" ASC LIMIT 1000
  SQL (0.6ms)  UPDATE "enterprises" SET "updated_at" = '2019-08-09 06:17:37.934520' WHERE "enterprises"."id" = 333
   (1.2ms)  SELECT COUNT("spree_products"."id") FROM "spree_products" WHERE "spree_products"."supplier_id" = 333 AND ("spree_products".deleted_at IS NULL)
  Spree::Product Load (1.1ms)  SELECT id FROM "spree_products" WHERE "spree_products"."supplier_id" = 333 AND ("spree_products".deleted_at IS NULL)
  Enterprise Load (2.4ms)  SELECT "enterprises".* FROM "enterprises" WHERE "enterprises"."id" IN (SELECT DISTINCT enterprises.id FROM "enterprises" INNER JOIN exchanges
 ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id) INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (2362))) AND (enterprises.id != 333) ORDER BY "enterprises"."id" ASC LIMIT 1000
  SQL (0.8ms)  UPDATE "enterprises" SET "updated_at" = '2019-08-09 06:17:37.944935' WHERE "enterprises"."id" = 243
   (1.3ms)  SELECT COUNT("spree_products"."id") FROM "spree_products" WHERE "spree_products"."supplier_id" = 243 AND ("spree_products".deleted_at IS NULL)
  Spree::Product Load (1.3ms)  SELECT id FROM "spree_products" WHERE "spree_products"."supplier_id" = 243 AND ("spree_products".deleted_at IS NULL)
  Enterprise Load (34.9ms)  SELECT "enterprises".* FROM "enterprises" WHERE "enterprises"."id" IN (SELECT DISTINCT enterprises.id FROM "enterprises" INNER JOIN exchanges
 ON (exchanges.receiver_id = enterprises.id AND exchanges.incoming = 'f') INNER JOIN exchange_variants ON (exchange_variants.exchange_id = exchanges.id) INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (3541,3512,3544,4055,3520,3506,4061,3702,4064,4062,4071,4065,4067,4066,4069,4058,4068,4072,4070,3707,3598,3710,3708,4073,3711,4074,3712,4075,3713,3716,4090,4091,4092,4086,3717,4088,4089,4087,4096,3636,4100,4098,4103,4104,4101,4102,4108,4099,4114,4111,4116,4113,4117,3736,4112,3750,3776,3738,3780,3737,3772,4115,3739,3798,3782,3765,3806,3804,3800,4128,3803,3812,3802,3805,3801,3869,3868,3867,3870,4127,3902,3905,3910,3909,3906,3904,3903,3907,3912,3923,3918,3914,3920,3925,3921,3939,3949,3948,3951,3946,3945,3950,3947,4035,4032,4033,4043,4046,4044,4034,4045,4042,4047,4054,3516,3505,3534,3539,3556,3557,3540,3558,3553,3561,3657,3510,3579,3681,3684,3518,3514,3517,3507,3500,3503,3515,3504,3499,3501,3498,3497,3519,3527,3528,3524,3526,3529,3530,3525,3551,3496,3548,3538,3550,3513,3532,3552,3560,3543,3559,3563,3531,3521,3508,3542,3595,3594,3655,3562,3645,3624,3622,3625,3656,3637,3502,3533))) AND (enterprises.id != 243) ORDER BY "enterprises"."id" ASC LIMIT 1000
  SQL (0.9ms)  UPDATE "enterprises" SET "updated_at" = '2019-08-09 06:17:37.993864' WHERE "enterprises"."id" = 149

From that I can see all the involved enterprises. They are all connected somehow:

Enterprise.where(id: [249, 318, 333, 243, 149]).pluck :name
=> ["Libro Beechwood Food Hub", "Garden Party Flower Farm", "The Local Flower Collective", "Local Flower Hub Preview Shop", "Garden Party Flower Shop"]

Four of these five are producers:

Enterprise.where(id: [249, 318, 333, 243, 149]).is_primary_producer.pluck :name
=> ["Garden Party Flower Farm", "The Local Flower Collective", "Local Flower Hub Preview Shop", "Garden Party Flower Shop"]

mkllnk commented 5 years ago

It's not just one order cycle. I removed all products from OC 416 which is the only one that has "Garden Party Flower Shop" as coordinator.

So Garden Party Flower Farm has many products and is selling them through multiple shops. But Garden Party Flower Shop also has a product that is sold through a shop involving Garden Party Flower Farm. That's how it becomes circular. It's the Bucket of Blooms. Order cycles having that item:

Exchange.with_product(Spree::Product.find(2362)).pluck(:order_cycle_id).uniq
[223, 236, 237, 241, 242, 240, 243, 248, 250, 249, 252, 256, 263, 269, 271, 295, 282, 288, 275, 270, 293, 294]

I find that first order cycle with a ghost product. It probably got deleted and due to a bug it wasn't removed from the order cycle.

Screenshot from 2019-08-09 17-11-33

I'm clearing out some old order cycles. I hope that's okay. So far I've seen only empty ones anyway.

mkllnk commented 5 years ago

@tschumilas I removed the Bucket of Blooms from all order cycles. I can now change variants again. I hope that solves you immediate problem. You will be able to use circular setups again once we fixed the bug.

mkllnk commented 5 years ago

I found this method that causes the infinite loop in the Enterprise model:

  def touch_distributors
    Enterprise.distributing_products(supplied_products.select(:id)).
      where('enterprises.id != ?', id).
      find_each(&:touch)
  end

sauloperez commented 5 years ago

I'm not sure I follow exactly the scenario on which an OC causes a loop. @kristinalim from the steps you listed with sample data, I guess there's some other state we're missing right? because it seems to be the simplest OC possible and I don't think that's broken. What else do Fred's Farm and Fredo's Farm have in common?

What I fail to see is what is actually in the loop you both mention @mkllnk @kristinalim ? What I see in that long SQL log you share @mkllnk is an N+1 and not a loop. See the 318 below

INNER JOIN spree_variants ON (spree_variants.id = exchange_variants.variant_id) WHERE (spree_variants.product_id IN (NULL))) AND (enterprises.id != 318) ORDER BY "enterprises"."id" ASC LIMIT 1000

that's what changes throughout the log. Besides what really freaks me out is the spree_variants.product_id IN (NULL) how does that end up in the query? that's clearly a bug.

sauloperez commented 5 years ago

Really neat investigation guys! :clap: :clap:

tschumilas commented 5 years ago

I'm still not sure I get what caused this - it is not simply having 2 producer hubs sell in each others shops? Certainly that must be happening in lots of places. We would have found this before now. Is it having 2 producer hubs (enterprises that are producers and sell all) AND the same owner login? Or does it have to do with the process of CHANGING from a producer (profile or shop) to a 'sell all' hub? Anyway its working - so understanding what I did might just be academic. THANKS - user and super admin can now save, and I've tried multiple enterprises including those affected here. Permissions are now working too - as super admin and as manager. (issue 4139). AND - not sure if this is related to this fix, but my permissions screen as super admin is WAY faster. It was really slow before - not just slow, it seemed to get stuck - no communication to me.
Should this and 4139 be closed?

mkllnk commented 5 years ago

it is not simply having 2 producer hubs sell in each others shops?

That's exactly it. When you change products of one shop, it will update all the suppliers. If one of the suppliers is also a producer for the first shop, then it will update that shop again which leads to an infinite loop.

luisramos0 commented 5 years ago

Hey guys, nice one!!! As I understand this, there's a workaround to the problem: not having loops. What severity should we have for this bug now? is it a critical feature to support those loops?

Matt-Yorkley commented 5 years ago

I think it's still S1?

tschumilas commented 5 years ago

How would we 'control' producer shops/hubs not selling to each other? An instance admin would need some way of watching for this. I think its really likley to happen again - I'm surprised we haven't found it before now. Lots of users have their fingers into lots of different enterprises, especially once they see how flexible OFN is at supporting their various business models. So - I"d say its still s1.

But I thought we fixed this - at least these enterprises can save products again on OFN-CAN. But - still can't save to inventory.

kristinalim commented 5 years ago

Sorry I hadn't written back here - I had limited time and saw that @mkllnk already was doing progress. :slightly_smiling_face: Awesome work, @mkllnk!

I agree @tschumilas, it's not your fault. The software should have supported this, or explicitly did not allow this scenario to happen. We devs weren't even aware of this quirk - it was an undiscovered bug.

@kristinalim from the steps you listed with sample data, I guess there's some other state we're missing right? because it seems to be the simplest OC possible and I don't think that's broken. What else do Fred's Farm and Fredo's Farm have in common?

@sauloperez In the default seed data, Fredo's Farm already distributes Fred's Farm products. So from that, setting up Fred's Farm to distribute Fredo's products too establishes the loop.

What severity should we have for this bug now? is it a critical feature to support those loops?

I agree with @Matt-Yorkley's assignment of S1. It's possible to have this setup as a real-world scenario, and once you establish the loop you can't update the OC anymore to break the loop. And even if the OC is not current (an OC long in the past), the loop can be established.

tschumilas commented 5 years ago

Today another user - also in the flower collective - cannot save products. Same error. (Maybe she couldn't last week when I reported this either - but just hadn't tried. She can't recall.) So its 262 (Sweet Gale Gardens) sells in her OWN shop and in 249 (The Local Flower Collective), and 318 (Local Flower Hub Preview Shop). But in this case, her chain is uni-directional. Neither of these hubs sell to her own OC.

BUT - here is the work around I've found - the problem seems to be limited to the enterprise owner's login. I know, @mkllnk you posted on slack that this should have nothing to do with it. BUT it seems to. If we add a second manager login to the enterprise, (not super admin -- just another regular user), that login can save products in a product list and in inventory. I posted this description with regard to the main login I use across multiple enterprises in another git issue: https://github.com/openfoodfoundation/openfoodnetwork/issues/4145 But - its not just me (which is nice to know). I'm canvasing others who sell to these 2 hubs again for problems. (This always comes up on a Thursday because thats when our OC opens,, so today everyone is updating)

luisramos0 commented 5 years ago

is it the same error message? "Saving failed. Save failed due to invalid data:500." on the products list?

luisramos0 commented 5 years ago

For devs: I think this issue is related to this bugsnag entry: https://app.bugsnag.com/open-food-network-canada-1/open-food-network-canada/errors/5d4b6cfdd5b544001a5a9e26?filters[event.since][0]=30d&filters[error.status][0]=open

@tschumilas what was the user trying to do when she saw the error? the error message indicates the user was creating a new variant, is that correct or was the user only updating a variant? does the error occur in all types of changes to products or just when adding a new variant to an existing product?

tschumilas commented 5 years ago

I've checked in with the user - and will report back. But, I am able - using a manager login (not super admin) to both add new variants and save changes to existing variants to her enterprise. AND - what is really messing me up now - just now, I could also save and create variants on my own enterprise - that is associated with the login I thought was 'contaminated" (So - sorry @mkllnk - I questioned you on this.) But I swear to you - when I posted previously - I could not do this. I even tried to create a new enterprise with that login - that was not entangled in any OC - and I could not save products until I added another manager login and used that. What the heck - I now look like a dolt to you guys. Would this bugsnag issue come and go like that? Anyway - checking with other users and will keep you updated.

luisramos0 commented 5 years ago

For devs: one hypothesis is that something is failing on bulk update when creating a variant here and then the setting of the on_hand value fails.

The same when creating a product, there are 21 errors in uk live, see this bugsnag event. Something goes wrong and the product gets created without stock_item for the variants... This happens after @product.valid? is tested for truth... so, the product ends up being valid but the stock_item is not created.

I have checked some live DBs and there are no variants without stock items, except for deleted variants.

luisramos0 commented 5 years ago

@tschumilas I made myself a manager of "Sweet Gale Gardens" and the bulk product update page (although slow...) is working correctly for some tests I did.

The owner of this enterprise only manages this so she will be seeing the same as I am. Must have been something specific the user tried to do in the page...

tschumilas commented 5 years ago

Thanks @luisramos0 - the user has still not gotten back to me, so I'm assuming you are correct and it was something specific she did at that moment. Will post here if I learn anything further.

luisramos0 commented 5 years ago

so PR #4159 is ready for review and will make this error in bulk product update fail silently. The user will not see an error and will be able to continue using the page although some of the data changed may not have been saved correctly. In the backstage, with this PR, when this happens we will be notified and we will have information about what exact product and variant failed to update.

kristinalim commented 5 years ago

@tschumilas The details in the error notification suggest that the user did not enter an item quantity for the variant they added. Could that be the problem? The item quantity is required.

tschumilas commented 5 years ago

I"m trying to work with this user - who says she gets an error every time she creates a new variant. But - to be honest - I'm doubtful - I have added myself as a manager on her shop too, and I can't reproduce. Its frustrating relaying things here from users sometimes - I just can't pin them down on exactly what happens. Sorry. But regarding this issue generally - the 'quick fix' that @mkllnk did seems to work. The user with the original problem, and my enterprise (also had original problem) can save in the bulk edits screen. BUT, my enterprises are still not able to save to inventory. (I"m the only user with inventories in this Flower Mobius Loop - everyone else in this network just uses their product list. So - as it stands, I'm the only one affected, and I can manage. I don't know why - but when I add another login/manager to my enterprise, that manager is able to save in inventory. I know - you are telling me login has nothing to do with it. Anyway - I'm doing what works - so if we have other S1s, do them first. This one is fine to wait. @kristinalim @luisramos0

luisramos0 commented 5 years ago

ok, thanks @tschumilas S1 by definition cannot wait, so based on what you say we can change to S2, ok?

If your inventory is broken, I think we should get you to go to your inventory and have someone check the logs at the same time, or you can share your access with a dev so that a dev can reproduce it and check logs. That will be the easiest. This is for the inventory page issue.

I think we should create a separate issue for the product list problem, I think it's a minor problem, probably S3, I think there's something the user is missing, maybe settign quantity as Kristina suggested. We should investigate and find the cause and improve user error message so that the user knows what she is doing wrong. Can you please create this separate issue for the product list error?

Thank you!

luisramos0 commented 5 years ago

Question for @mkllnk: #4157 is marked to close this issue but @tschumilas is saying the inventory page is still broken after the workaround is done so: is 4157 really fixing the inventory page problem? or is 4157 just fixing the original product list problem for @tschumilas ? as far as I understand, 4157 will fix the original problem of the product list so we should probably create another issue for the current inventory page issue.

I am seeing 3 issues:

original @tschumilas problem in product list due to permissions loop (4138 fixed by 4157)
inventory page broken for @tschumilas (new issue)
problem list broken for user when creating variant (new issue)

Does this make sense?

tschumilas commented 5 years ago

Yes - this all make sense.

Original problem I posted - I noted 2 problems- saving product list and saving to inventory. So - the saving to product list is solved by @mkllnk 's fix.
So - the next step on the saving inventory is to arrange a time when a dev can check the logs. So - I can do that with whoever is available - maybe once the current S1s are fixed?
Product list broken for user - I'm actually going to ask her if I can try her login to sort out what is happening - and will open a new issue for that if necessary.

openfoodfoundation / openfoodnetwork