2014 refactor variant count on hand #2413

mkllnk · 2018-06-28T06:38:25Z

What? Why?

Part of #2014.

This prepares the inventory report and maybe more for the Spree upgrade. It also fixes a bug in filtering stocked variants and removes some unused code.

What should we test?

Check that the inventory report displays the right list of variants.
Also check shops for correct display of item availability.
And sanity check stock management.

Release notes

Fixed bug in inventory report which ignored the distributor or on-demand setting for stock calculation. This may slow down the inventory report generation.

How is this related to the Spree upgrade?

This refactors some code to be compatible with Spree 2.0.

mkllnk · 2018-06-28T08:05:05Z

@sauloperez I just noticed that I repeated your commit 0cbd967. @Matt-Yorkley added that line in a503b5e. I guess that was a merge conflict mistake.

sauloperez · 2018-07-09T23:03:56Z

lib/open_food_network/scope_variant_to_hub.rb

+      def in_stock?
+        return true unless Spree::Config[:track_inventory_levels]
+
+        on_demand || (count_on_hand > 0)


I'm not familiar with variant overrides but I guess count_on_hand is a column on a database table added by OFN, right?

It's a spree database column on both spree_product and spree_variant.

Just a heads-up @mkllnk: on_demand is being removed as well in 2.0.

@Matt-Yorkley Thanks for the heads up. I wasn't sure if I considered that. But this method is only active if the variant has a VariantOverride. The VariantOverride does have on_demand and count_on_hand. The vanilla Spree::Variant still has an in_stock? method in both Spree versions. So we should be safe to call it.

sauloperez · 2018-07-16T15:28:45Z

@mkllnk have you tried rendering any report that uses products_and_inventory_report_base.rb? or is it still too early?

mkllnk · 2018-07-19T01:22:49Z

@sauloperez Yes, I did test that just now. But it should also be covered by our specs.

What do you mean with "too early"? This is production code, going into master.

mkllnk · 2018-08-07T08:36:02Z

Staged on https://staging1.openfood.com.au/.

RachL · 2018-08-07T14:20:52Z

@myriamboure or @sstead I'm afraid I don't know very well the inventory feature to test this PR. Can one of you have a look? I will be interested in ready the testing notes afterwards.
I've started some notes here, but let's be honest you can just use the document to save yourself the time of creating it, other than that nothing much interesting:
https://docs.google.com/document/d/1-2soDLPWbvUQNpubrDmUE6B1lrK1VlNJWNHrrpt8UM8/edit

sstead · 2018-08-08T04:40:24Z

@mkllnk can you clarify, are we testing all 3 reports within the Products and Inventory report?

I don't have anymore time to test today but here's where I'm up to so far...

Issues:

In some cases, when I filter by distributor only, I'm seeing products from suppliers who have not granted that shop P-OC E2E. E.g. Sally2's Hub and Producer of Wine.
In some cases, products are not showing for unknown reasons
When filtering by OC, the report is filtering by the 'incoming' section of the OC, not outgoing.
Some inventory settings aren't applying in the report - e.g. I would expect 'hidden' inventry items to be hidden on the report - not the case.
There seems to be a bug with the Advanced settings for OCs, which makes it hard to test the impact of this setting on the OC filter in the report.

I think there's some design flaws with these reports:

We shouldn't use the work 'inventory' unless we're specifically refering to the Inventory page. In the case of the Inventory (on hand) report, if you're filtering by a supplier only, it's got nothing to do with the Inventory page, it's referring to On Hand only. I suggest we take the word Inventory out of this report.
It's confusing the way this report switches between looking at the BEP page (in the case of filtering by supplier only) and then switching to look at both the BEP page and the Inventory page (in the case of filtering by a distributor and a supplier).
In the case where I select a distributor filter, I would prefer if the other filters were then restricted to allowable options - e.g. only OCs which that distributor is in. Or only suppliers who have granted P-OC to.

https://docs.google.com/document/d/1-2soDLPWbvUQNpubrDmUE6B1lrK1VlNJWNHrrpt8UM8/edit#

kirstenalarsen · 2018-08-08T04:51:15Z

@sstead how sure are you that these problems are all new / due to this change? I am not at all across this, just wondering if your excellent issue detection skills have ever been applied to these reports before?

sstead · 2018-08-08T05:48:14Z

Yes there’s a very good chance these are prexisting, this isn’t a report I test often. Should we log the other issues for later PRs? I can test more narrowly if that’s what’s needed.

…

________________________________ From: Kirsten Larsen <[email protected]> Sent: Wednesday, August 8, 2018 2:51:16 PM To: openfoodfoundation/openfoodnetwork Cc: Sally; Mention Subject: Re: [openfoodfoundation/openfoodnetwork] 2014 refactor variant count on hand (#2413) @sstead<https://github.com/sstead> how sure are you that these problems are all new / due to this change? I am not at all across this, just wondering if your excellent issue detection skills have ever been applied to these reports before? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2413 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKVzsJjqGZjJKS82RVo2aoaKv9qbnT93ks5uOm5EgaJpZM4U60-M>.

daniellemoorhead · 2018-08-08T06:14:20Z

@sstead best way to tell if these are existing is to test on production, yes?

And if you find them then log them as separate bugs 😃

mkllnk · 2018-08-08T08:02:59Z

I would be very surprised if those issues are caused by my code change. I only changed the on_hand filter. That already fixed a bug that the on_demand property was ignored.

This report is much older than the new inventory feature. It probably does make sense to rename it now. Maybe stock report?

Anyway, to proceed with the Spree upgrade, I just want the report to perform as well as before. Changing the report and fixing it are other issues.

Our added scope is not used anywhere and conflicts with the Spree upgrade. #2014

This change could impact the performance of the report. But therefore it takes VariantOverrides into consideration. The old code ignored the distributor for this filter. It also ignored the `on_demand` flag.

sstead · 2018-08-09T07:01:24Z

Testing notes

Yes, the Inventory on hand report and the lettuceshare report no longer show items with <1 on hand value in Inventory.
The shop seems to be behaving as normal
The stock management is working as normal (pre-existing bug only)

Broader issues spotted while testing are logged in this issue - https://github.com/openfoodfoundation/openfoodnetwork/issues/2529

https://docs.google.com/document/d/1-2soDLPWbvUQNpubrDmUE6B1lrK1VlNJWNHrrpt8UM8/edit#

mkllnk mentioned this pull request Jun 28, 2018

Refactor uses of spree_variants.count_on_hand #2014

Closed

mkllnk self-assigned this Jul 3, 2018

sauloperez approved these changes Jul 9, 2018

View reviewed changes

HugsDaniel approved these changes Aug 3, 2018

View reviewed changes

mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Aug 7, 2018

sstead self-assigned this Aug 8, 2018

mkllnk removed the pr-staged-au staging.openfoodnetwork.org.au label Aug 8, 2018

luisramos0 self-requested a review August 8, 2018 15:24

luisramos0 approved these changes Aug 8, 2018

View reviewed changes

mkllnk added 4 commits August 9, 2018 14:22

Remove unused scope Spree::Variant.in_stock

27ddb71

Our added scope is not used anywhere and conflicts with the Spree upgrade. #2014

Style ScopeVariantToHub

289d0b9

Prepare inventory report for Spree upgrade

b84e524

This change could impact the performance of the report. But therefore it takes VariantOverrides into consideration. The old code ignored the distributor for this filter. It also ignored the `on_demand` flag.

Override in_stock? to prepare for Spree 2.0

9dcbd90

oeoeaio merged commit 9dcbd90 into openfoodfoundation:master Aug 9, 2018

sstead mentioned this pull request Aug 9, 2018

Malfunctions with the OC advanced settings - restricting variants to coordinator's inventory #1632

Closed

mkllnk deleted the 2014-refactor-variant-count-on-hand branch September 13, 2018 08:09

sstead mentioned this pull request Mar 24, 2022

Bugs in the Products and Inventory reports openfoodfoundation/wishlist#198

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2014 refactor variant count on hand #2413

2014 refactor variant count on hand #2413

mkllnk commented Jun 28, 2018

mkllnk commented Jun 28, 2018

sauloperez Jul 9, 2018

Matt-Yorkley Jul 9, 2018

Matt-Yorkley Jul 9, 2018

mkllnk Jul 10, 2018

sauloperez commented Jul 16, 2018

mkllnk commented Jul 19, 2018

mkllnk commented Aug 7, 2018

RachL commented Aug 7, 2018

sstead commented Aug 8, 2018

kirstenalarsen commented Aug 8, 2018

sstead commented Aug 8, 2018 via email

daniellemoorhead commented Aug 8, 2018 •

edited

Loading

mkllnk commented Aug 8, 2018

sstead commented Aug 9, 2018

2014 refactor variant count on hand #2413

2014 refactor variant count on hand #2413

Conversation

mkllnk commented Jun 28, 2018

What? Why?

What should we test?

Release notes

How is this related to the Spree upgrade?

mkllnk commented Jun 28, 2018

sauloperez Jul 9, 2018

Choose a reason for hiding this comment

Matt-Yorkley Jul 9, 2018

Choose a reason for hiding this comment

Matt-Yorkley Jul 9, 2018

Choose a reason for hiding this comment

mkllnk Jul 10, 2018

Choose a reason for hiding this comment

sauloperez commented Jul 16, 2018

mkllnk commented Jul 19, 2018

mkllnk commented Aug 7, 2018

RachL commented Aug 7, 2018

sstead commented Aug 8, 2018

kirstenalarsen commented Aug 8, 2018

sstead commented Aug 8, 2018 via email

daniellemoorhead commented Aug 8, 2018 • edited Loading

mkllnk commented Aug 8, 2018

sstead commented Aug 9, 2018

daniellemoorhead commented Aug 8, 2018 •

edited

Loading