Fix bug in inventory management page #2893

luisramos0 · 2018-10-19T15:04:25Z

What? Why?

Closes #2739
Initially I thought this happened where permissions to change existing variant overrides of deleted products were checked. The initial commit (now removed from this PR) was making the inventory page ignore variant overrides of deleted products.
After further investigation I concluded the problem was when permissions were deleted but variant overrides were not revoked (after discovering the field variant_override.permission_revoked_at :-)).

This PR is:

adding a before_destroy function to enterprise_relationships (the enterprise permission) to revoke variant_overrides related to the permission being deleted
adding a migration to take care of existing broken data

What should we test?

Inventory page and permissions.

Test adapted from a comment in #2739:

I have a producer and hub in my test.
For this test, there are two distinct users, one manages Hub, the other manages Producer ("Hub does this" should be read "User that manages Hub does this")
For a base to test, Hub creates some inventory data for their own products or some Other Producer
Producer creates Product X
Producer permits Hub to sell their stuff: enterprise permission "Producer permits Hub to add products to inventory".
Hub adds product X to inventory and sets the inventory data
Producer removes permission to Hub
- product X disappears from Hub inventory and Hub CAN update other inventory data (this was broken before the fix)
Producer adds Hub permission back
- product X reappears in Hub inventory and Hub CAN update all inventory data including the product X inventory
Producer deletes product X from their products (permission is still in place)
- product X disappears from Hub inventory and Hub CAN update all inventory data (except product X that has been deleted)
At this point, Hub is not selling any Products from Producer and (apparently) there are no inventory items in Hub related to Producer SO:
Producer removes useless permission to Hub sell their products
- Hub CAN update its inventory data (this was broken before the fix) note: inventory entry is still in the database but it's permission is now revoked

After this fix is installed in any live instance, the result of this query should be empty (the migration will do this part of the job) and should remain empty forever (the fix will do this part of the job) ;-)
select distinct hub.id, hub.name, supplier.id, supplier.name from variant_overrides vo, spree_variants v, spree_products p, enterprises hub, enterprises supplier where vo.variant_id = v.id and hub.id = vo.hub_id and supplier.id = p.supplier_id and v.product_id = p.id and vo.hub_id != p.supplier_id and (p.supplier_id, vo.hub_id) not in (select er.parent_id, er.child_id from enterprise_relationships er, enterprise_relationship_permissions erp where er.id = erp.enterprise_relationship_id and erp.name = 'create_variant_overrides') order by hub.id;

Release notes

Changelog Category: Fixed
Fix broken inventory page. When a producer deletes a permission for a hub to add producer's products to the hub's inventory, the hub's inventory items are now being correctly updated with a revoked stamp that will not break the inventory page anymore.

How is this related to the Spree upgrade?

Not related.

luisramos0 · 2018-10-20T09:37:06Z

I see there's a field called variant_override.permission_revoked_at which is a date when the VO permission was revoked. I believe this issue is related to this field and not to deleted products.
When the permission is revoked by the producer to create VOs, the VOs.permission_revoked_at must be updated and then permission_revoked_at must be used when rendering the inventory page AND when validating permissions (it should be the case already because this field is inserted in a default_scope of the VO).

luisramos0 · 2018-10-20T10:18:35Z

so, after testing deletion of permissions I realised VOs permissions were not being revoked on delete. So, I added a new commit to revoke permissions on VOs when permission is deleted. Simple fix to a complex problem. I believe this fixes the original issue completely. We just need to run some migration to revoke all VOs that have no permissions.

luisramos0 · 2018-10-20T10:29:38Z

So, this PR needs:

- migration to fix existing VOs without permissions but still with nil permission_revoked_at
- auto tests for the error case - DONE, the new spec is failing without the fix, as it should be :-)
- validate if the deleted products commit is really needed and auto test, I think we will not need it - DONE, not needed

…tract methods

…es are revoked when permission is deleted

luisramos0 · 2018-10-20T11:36:20Z

All done, this PR is ready for review and test.

Matt-Yorkley

Nice.

mkllnk

Good work! Really nice to see your deep understanding. I just learned how permissions are modelled. :-D

I have some suggestions how to improve the migration, but that shouldn't block an S2 issue. So I'm approving it. 👍

mkllnk · 2018-10-23T02:55:15Z

db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb

+  def up
+    # This process was executed when the permission_revoked_at colum was created (see AddPermissionRevokedAtToVariantOverrides)
+    #   It needs to be repeated due to #2739
+    variant_override_hubs = Enterprise.where(id: VariantOverride.all.map(&:hub_id).uniq)


It's good practice to declare ActiveRecord models used in the migration again so that the migration is independent from changes in the model code. Example: https://coderwall.com/p/zav1dg/using-models-in-rails-migrations

You probably just took it from the previous migration, but this call is a bit clearer and more efficient:

Suggested change

variant_override_hubs = Enterprise.where(id: VariantOverride.all.map(&:hub_id).uniq)

variant_override_hubs = Enterprise.where(id: VariantOverride.uniq.pluck(:hub_id))

It also enables ActiveRecord to nest the VariantOverride query within the Enterprise query instead of making two queries.

yeah, this was copy pasted from the original revoked_at code. I tested it with an example.

It would probably be better writing SQL, correct? Things like this will break in the future if these things are removed from the model :
"hub.relationships_as_child.with_permission"

Hmm. what do I do? Is there an easy fix? Can I ignore this for now? We will have to look at it when/if it breaks in the future.

thanks for the pluck lesson!

Sorry to be late but I totally missed this PR. My concern is that VariantOverride.all can potentially blow up some instance's DB. That turns into a SELECT without WHERE constraints thus, selecting the whole table 💥 I suggest we process them in chunks.

We can check if Canada (the only instance affected?) has just few variant overrides and run it there but we'll need to fix it before we run it in other instances.

ok, do you think this query can blow a database? "select id from variant_overrides;"
Canada has 908 entries.

Anyway, I can re-write, can you please guide me here. What's the proper way of doing this?

mkllnk · 2018-10-23T03:11:33Z

db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb

+
+    variant_override_hubs.each do |hub|
+      permitting_producer_ids = hub.relationships_as_child
+        .with_permission(:create_variant_overrides).map(&:parent_id)


Another good use of pluck:

Suggested change

.with_permission(:create_variant_overrides).map(&:parent_id)

.with_permission(:create_variant_overrides).pluck(:parent_id)

mkllnk · 2018-10-23T05:52:58Z

Staged on https://staging.openfoodnetwork.org.au/.

RachL · 2018-10-23T18:15:50Z

Thank you @luisramos0 for such a detailed test scenario. I'm amazed by the work done, kudos!

Here are my testing notes :
https://docs.google.com/document/d/1IxAyJfbW0EPp8AzcLCnT5L0QJ12ZAVowFH4dLI6akZ0/edit#
Nothing to rise up all good :)

luisramos0 · 2018-10-23T18:49:18Z

Lets ship this one directly to Canada :-D

sauloperez · 2018-10-24T09:49:03Z

db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb

+  def up
+    # This process was executed when the permission_revoked_at colum was created (see AddPermissionRevokedAtToVariantOverrides)
+    #   It needs to be repeated due to #2739
+    variant_override_hubs = Enterprise.where(id: VariantOverride.all.map(&:hub_id).uniq)


Sorry to be late but I totally missed this PR. My concern is that VariantOverride.all can potentially blow up some instance's DB. That turns into a SELECT without WHERE constraints thus, selecting the whole table 💥 I suggest we process them in chunks.

We can check if Canada (the only instance affected?) has just few variant overrides and run it there but we'll need to fix it before we run it in other instances.

This commit makes use of three ActiveRecord features: 1. Using `select` instead of `all.map` enables ActiveRecord to nest one select into the other, resulting in one more efficient query instead of two. 2. Using `find_each` saves memory by loading records in batches. https://api.rubyonrails.org/classes/ActiveRecord/Batches.html#method-i-find_each 3. Using `pluck` creates only an array, avoiding loading all the other columns of the records into objects. Running this on the current Canadian database, fixes the following variant overrides: ``` [] [] [] [] [] [] [925, 924, 966, 965] [] [] [] [] [462, 863, 464, 822, 949, 947, 944, 939, 942, 946, 945, 943, 438, 937, 938, 941, 940, 467, 952, 875, 453, 953, 454, 951, 487, 460, 457, 528, 527, 486, 459, 458, 461, 529, 530, 950, 642, 384, 380, 643, 385, 381, 644, 386, 382, 960, 959, 379, 640, 377, 375, 532, 639, 376, 374, 646, 390, 389, 637, 406, 408, 647, 391, 393, 633, 396, 400, 398, 645, 388, 387, 648, 394, 392, 536, 632, 399, 397, 395, 634, 403, 401, 635, 404, 402, 636, 407, 405, 535, 534, 638, 410, 409, 948, 533, 537, 531, 877, 880, 894, 893, 672, 671, 673, 674, 703, 714, 715, 716, 717, 862, 864, 879, 876, 865, 881, 878, 463, 954, 866, 823, 957, 958, 955, 956, 899, 897] [] [969] ```

mkllnk · 2018-10-25T01:09:00Z

@sauloperez @luisramos0 I added another commit addressing all the potential performance issues. The commit contains explanations for all changes.

Since this is a database migration that doesn't change any staging data, we don't need to stage this again. I'm tempted to deploy it to the Canadian server, but I would like the approval from @sauloperez.

I tested this with the Canadian data and the migration runs without error. But I'm not able to re-produce the issue with the current data. I can save prices without any problem (as super admin). So I'm not sure how to test this.

luisramos0 · 2018-10-25T15:05:47Z

It's not obvious to replicate. You can follow the test case above in a version without the fix.

Another option is to run something like "update variant_override set permission_revoked_at = null where hub_id = X"
This migration will reset all revoked permissions correctly.

mkllnk · 2018-10-25T22:14:27Z

@sauloperez I used the find_each method to load the records in batches. I didn't find the documentation very clear about the underlying SQL used, but this post suggests that it uses limit in SQL queries to load only 1000 rows at a time. Do you know more about it?

sauloperez · 2018-10-29T15:46:09Z

Yes, AFAIK that's what it does. It might not be the most known AR's feature but I find it essential for data migrations. There's also the option to go deeper into AR and use find_in_batches. I've had the need to use it a few times.

sstead · 2018-10-30T10:51:41Z

Hi all, @mkllnk deployed this to Aus production today and I'm wondering if it could be related to a bug that's been reported this evening. A user has said .... "I just went in and checked my inventory and random inventory amounts have been loaded for the 236 products I have loaded on the system."

If you have a chance to look into this that would be most appreciated @sauloperez or @luisramos0 . If it's a bug it could effect more users, so good to catch early. Thanks

luisramos0 · 2018-10-30T11:31:25Z

I dont think it can be related as here we are updating permissions only. We are not changing stock levels.

The only possibility I can see is that in this PR we are changing the permissions and revoked inventory items are excluded from the default query scope. So, any query or update to inventory items will not include revoked inventory items (inventory items = variant overrides). But how can that make "stock levels random"?

I'd create a new issue for that @sstead
Has the user sold much stock in the last few days?
Can we pick a specific example? Hub name, product name, "correct stock levels", new "random stock level"?

luisramos0 · 2018-10-30T21:09:13Z

ok, in order to connect this with stock management I tried some permissions related scenarios. we have a problem.

first, this PR will always be correct. It's just making data consistent in the database.

second, the problem I'll describe will only happen for enterprises that had broken permissions, i.e., they were selling products for which they didn't have permissions to "manage inventory".

Explanation: What this PR is doing is revoking permissions on inventory items. The problem is that these inventory items can be on the hubs shopfront and they can still be bought even after this PR.
Before this PR: when the permission is removed by the producer, the hubs inventory items are not revoked. this breaks the inventory management page (original issue) but customers can still buy and inventory items stock is updated correctly. I have tested this just now.
After this PR: when the permission is removed by the producer, the hubs inventory items are now correctly revoked BUT when a customer buys one of these inventory items that have been revoked, the stock updated is the variant's stock (the producer variant, not the hubs inventory item). I can replicate this problem. This is most probably happening because our "scoping" logic cant see the revoked inventory item and takes the main variant.

Validation Before we move to a solution we need to validate that what I am talking about is what is happening in AUS. I cant access the server. We need to check if this user's enterprise has some variant_overrides revoked or if another enterprise has revoked variant_overrides of its products. If this explanation is correct there must be some of these in the DB for this user.
Also, "random" stock levels would be the "levels the user expected" minus stock sold meanwhile through the related enterprise (producer or hub).

Quick workaround in AUS: lets see how many variant overrides have permission_revoked_at != nil in AUS. I believe making this field nil for all entries is a valid shortcut. We can then rebuild this data from the permissions with the migration in this PR.

Current Release: we have to revert this PR from master and the current release until we have a solution for this problem.

Solution: if a producer revokes permissions for a hub to manage inventory, all the products disappear from the hubs shopfront. is this possible/valid? or do we need to consider mixed cases between permissions to "manage inventory" and "add to order cycle"? what does it mean when a hub can "add to order cycle" but cannot "manage inventory". Is this a valid usage case?

sstead · 2018-10-30T22:06:30Z

@luisramos0 , In the case of our user, I don't think revoked permissions is the problem. This user is a farm shop, selling only her own products- there are no permissions. She is using Inventory to set her stock levels (probably without needing to- she could just as easily not use Inventory).

Has the user sold much stock in the last few days? - She's had 5 orders in the last 24 hours.
Can we pick a specific example? Hub name, product name, "correct stock levels", new "random stock level"? - Terrewah Farm. Rockmelon and Fuji Apples were 2 impacted. The original level was 0 (these aren't in season), and they became non-zero (not sure what the incorrect figure was) . She said most products were impacted- some showed as 0 stock, when the product was available, and vice versa.

myriamboure · 2018-10-30T22:26:23Z

Hum... @sstead is this possible that the user did a mistake? She put some stock for the wrong products? I just don't see how this PR could have had such impact... and it is a very possible human mistake in what you describe...

@luisramos0 about your solution, I'm not sure about the need to separate the "add to order cycle" and "add to inventory" permission and don't see any reason why a supplier would give one and not the other. Weither the hub add a commission through enterprise fee, or override product prices and stock is not really the producer business, I mean if the hub doesn't have a calculated margin why couldn't he just setup the price she wants to sell the products?
@sstead don't you think? @mkllnk any reason why we separated those permissions?
Is it maybe, if a producer manage their own stock for their own shop in their catalog, and allow a hub to sell their products but want the hub to manage their own stock, then they can only allow to add to inventory and not directly to OC? Maybe that could be a case...

But today, technically if you revoke permission to add to inventory, but keep permission to add to order cycle, products can still be listed in OC and appear in shop, it's just that hub can't override info through inventories.
So the behavior you describe is expected behavior, if hub add product to OC it takes directly in producer stock level...

I don't think we need to revert then, right?

Or can there be a problem with the fact that the user uses herself the inventory for her own catalog? Is there any permission involved to manage own inventory that could have been impacted? Could it be a form of conflict? The inventory is linked to her shop, and I guess there should be some default permission between the shop of a producer and her own product catalog... ? Even if they have the same name and are technically the same agent id in the system...?

luisramos0 · 2018-10-30T23:59:29Z

@myriamboure
"if you revoke permission to add to inventory, but keep permission to add to order cycle, products can still be listed in OC and appear in shop, it's just that hub can't override info through inventories.
So the behavior you describe is expected behavior, if hub add product to OC it takes directly in producer stock level..."

This is exactly the problem I have confirmed. You are saying it's a feature. If it's a feature, it's a feature this PR fixed.
Before PR: permissions were revoked and the stock levels would still be updated at inventory level (hub). These were inaccessible stock levels because the management page was broken.
After PR: permissions are revoked and the stock levels are now updated at variant level (producer)

If this is correct, no need for workaround or revert. Unless we find something else.

@sstead as a producer I have tried to give myself permissions to manage my own inventory but the inventory management page didnt show up anything. So, I am not sure what do you mean by managing own stock in inventory... what permissions are in place, can you share a screen shot?

Probably better to open a new issue.

myriamboure · 2018-10-31T00:29:59Z

If this is correct, no need for workaround or revert. Unless we find something else.

I think it is correct and we don't need to do anything else.

@luisramos0 you just need to be a producer with a "own shop" package, normally in inventory you should see your own products... well AFAK... let's see what @sstead says. Agree about opening a new issue.

kirstenalarsen · 2018-10-31T00:31:36Z

hi people - there is absolutely no way I can get my head around this at the moment. What I need to know is what it means for the user whose inventory levels all changed? @luisramos0 @myriamboure @sstead ?

sstead · 2018-10-31T01:39:18Z

I've opened a new issue here - #2960

I don't think it was user error - she thoroughly checked items before opening OC, and products that haven't been in season for months suddenly had >0 on hand. Sorry I don't have time to get my head into this PR right now- but from a quick scan I don't think there are any 'revoked' permissions here. It could just be something funny relating to an enterprise who is a farm shop doing overrides on their own products- no permission in place, the system just lets a shop do overrides on itself.

For now the user has updated the invenotry levels back to what they should be and hasn't reported any more issues today.

…leted_products_break_inventory" This reverts commit 8a3f621, reversing changes made to 7cac463.

luisramos0 · 2018-10-31T10:02:54Z

I have checked the DB. thanks @mkllnk
There are revoked inventory items indeed. And those revoked entries are owned by the user herself.

Lets continue the conversation in #2960

luisramos0 self-assigned this Oct 19, 2018

luisramos0 mentioned this pull request Oct 19, 2018

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

Closed

luisramos0 force-pushed the deleted_products_break_inventory branch 2 times, most recently from b6ad422 to d17e503 Compare October 19, 2018 22:33

luisramos0 force-pushed the deleted_products_break_inventory branch from 1cbef29 to ce4430b Compare October 20, 2018 10:14

Refactor EnterpriseRelationship before save hook: some renames and ex…

5ae0ad8

…tract methods

luisramos0 force-pushed the deleted_products_break_inventory branch from ce4430b to dadc56c Compare October 20, 2018 10:52

luisramos0 added 2 commits October 20, 2018 12:32

Add before_destroy to enterprise_relationship so that variant overrid…

9079437

…es are revoked when permission is deleted

Migration: Revoke variant overrides without permissions

d375bb8

luisramos0 force-pushed the deleted_products_break_inventory branch from dadc56c to d375bb8 Compare October 20, 2018 11:33

Matt-Yorkley approved these changes Oct 20, 2018

View reviewed changes

HugsDaniel approved these changes Oct 22, 2018

View reviewed changes

mkllnk approved these changes Oct 23, 2018

View reviewed changes

mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Oct 23, 2018

kristinalim mentioned this pull request Oct 23, 2018

Add tests for permissions for variant overrides #2833

Merged

RachL self-assigned this Oct 23, 2018

RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Oct 23, 2018

sauloperez suggested changes Oct 24, 2018

View reviewed changes

mkllnk self-assigned this Oct 25, 2018

sauloperez approved these changes Oct 25, 2018

View reviewed changes

sauloperez merged commit 8a3f621 into openfoodfoundation:master Oct 25, 2018

luisramos0 deleted the deleted_products_break_inventory branch October 25, 2018 16:47

sstead mentioned this pull request Oct 31, 2018

Users inventory levels changed for unknown reason #2960

Closed

sauloperez added a commit to coopdevs/openfoodnetwork that referenced this pull request Oct 31, 2018

Revert "Merge pull request openfoodfoundation#2893 from luisramos0/de…

1a7b296

…leted_products_break_inventory" This reverts commit 8a3f621, reversing changes made to 7cac463.

mkllnk mentioned this pull request Oct 31, 2018

Revert "Merge pull request #2893 from luisramos0/deleted_products_bre… #2961

Closed

luisramos0 mentioned this pull request Oct 31, 2018

Fix variant_overrides permissions for overrides that belong to the supplier herself #2963

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in inventory management page #2893

Fix bug in inventory management page #2893

luisramos0 commented Oct 19, 2018 •

edited

Loading

luisramos0 commented Oct 20, 2018

luisramos0 commented Oct 20, 2018

luisramos0 commented Oct 20, 2018 •

edited

Loading

luisramos0 commented Oct 20, 2018

Matt-Yorkley left a comment

mkllnk left a comment

mkllnk Oct 23, 2018

mkllnk Oct 23, 2018

luisramos0 Oct 23, 2018

luisramos0 Oct 23, 2018

sauloperez Oct 24, 2018 •

edited

Loading

luisramos0 Oct 24, 2018

mkllnk Oct 23, 2018

mkllnk commented Oct 23, 2018

RachL commented Oct 23, 2018

luisramos0 commented Oct 23, 2018

sauloperez Oct 24, 2018 •

edited

Loading

mkllnk commented Oct 25, 2018

luisramos0 commented Oct 25, 2018 •

edited

Loading

mkllnk commented Oct 25, 2018

sauloperez commented Oct 29, 2018

sstead commented Oct 30, 2018

luisramos0 commented Oct 30, 2018

luisramos0 commented Oct 30, 2018 •

edited

Loading

sstead commented Oct 30, 2018

myriamboure commented Oct 30, 2018

luisramos0 commented Oct 30, 2018

myriamboure commented Oct 31, 2018

kirstenalarsen commented Oct 31, 2018

sstead commented Oct 31, 2018 •

edited

Loading

luisramos0 commented Oct 31, 2018

	variant_override_hubs = Enterprise.where(id: VariantOverride.all.map(&:hub_id).uniq)
	variant_override_hubs = Enterprise.where(id: VariantOverride.uniq.pluck(:hub_id))

	.with_permission(:create_variant_overrides).map(&:parent_id)
	.with_permission(:create_variant_overrides).pluck(:parent_id)

Fix bug in inventory management page #2893

Fix bug in inventory management page #2893

Conversation

luisramos0 commented Oct 19, 2018 • edited Loading

What? Why?

What should we test?

Release notes

How is this related to the Spree upgrade?

luisramos0 commented Oct 20, 2018

luisramos0 commented Oct 20, 2018

luisramos0 commented Oct 20, 2018 • edited Loading

luisramos0 commented Oct 20, 2018

Matt-Yorkley left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Oct 23, 2018

Choose a reason for hiding this comment

mkllnk Oct 23, 2018

Choose a reason for hiding this comment

luisramos0 Oct 23, 2018

Choose a reason for hiding this comment

luisramos0 Oct 23, 2018

Choose a reason for hiding this comment

sauloperez Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

luisramos0 Oct 24, 2018

Choose a reason for hiding this comment

mkllnk Oct 23, 2018

Choose a reason for hiding this comment

mkllnk commented Oct 23, 2018

RachL commented Oct 23, 2018

luisramos0 commented Oct 23, 2018

sauloperez Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

mkllnk commented Oct 25, 2018

luisramos0 commented Oct 25, 2018 • edited Loading

mkllnk commented Oct 25, 2018

sauloperez commented Oct 29, 2018

sstead commented Oct 30, 2018

luisramos0 commented Oct 30, 2018

luisramos0 commented Oct 30, 2018 • edited Loading

sstead commented Oct 30, 2018

myriamboure commented Oct 30, 2018

luisramos0 commented Oct 30, 2018

myriamboure commented Oct 31, 2018

kirstenalarsen commented Oct 31, 2018

sstead commented Oct 31, 2018 • edited Loading

luisramos0 commented Oct 31, 2018

luisramos0 commented Oct 19, 2018 •

edited

Loading

luisramos0 commented Oct 20, 2018 •

edited

Loading

sauloperez Oct 24, 2018 •

edited

Loading

sauloperez Oct 24, 2018 •

edited

Loading

luisramos0 commented Oct 25, 2018 •

edited

Loading

luisramos0 commented Oct 30, 2018 •

edited

Loading

sstead commented Oct 31, 2018 •

edited

Loading