Take sku overrides into account in customer totals report #5186

oeoeaio · 2020-04-07T14:16:16Z

What? Why?

Closes #5185

What should we test?

SKUs of variants overridden in the inventory are visible in the Customer Totals orders and fulfillments report.

Release notes

SKUs set in the product inventory view will now be displayed correctly in the Order Cycle Customer Totals report.

Changelog Category: Fixed

sauloperez · 2020-04-07T16:18:07Z

I might be seeing ghosts. @oeoeaio is that you?? https://giphy.com/gifs/reaction-2wSaulb0fsDydh0IoB

Matt-Yorkley · 2020-04-07T18:24:43Z

I had to reload the page to make sure Github wasn't broken! 😅

Matt-Yorkley · 2020-04-07T18:51:55Z

lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb

+
+      def scoper_for(distributor_id)
+        @scopers_by_distributor_id[distributor_id] ||=
+          OpenFoodNetwork::ScopeVariantToHub.new(distributor_id)


I'm slightly worried about the potential impact of this in the context of large hubs with a lot of overrides (France has a few) that are already struggling to load reports, given that ScopeVariantToHub fetches all of the hub's overrides in it's initializer...

Fair enough. I'm pretty rusty and lots has changed (nice work!), any suggestions or examples of a better approach to this kind of problem?

It might be OK. I guess we can just test it in a staging server with lots of data?

I don't think there's an easy alternative for displaying the alternate SKUs.

I'm thinking this change might also need some product team input...?

Righto, who do I ping? Some acceptance criteria that I can work towards would be really helpful.

Maybe @RachL and @lin-d-hop could weigh in on it...

We're changing the SKUs displayed in the customer totals report to show the SKUs from variant overrides instead of the original variant SKU. Could be a surprise for some users? Maybe it would just need an announcement note in the release?

I do agree that it is the technically correct behaviour. But also concerned of the consequences.
SKU is rarely used in the UK so my preference would be to merge this fix.

Would like to hear @RachL's thoughts.

The biggest concern would be performance implications. @Matt-Yorkley can you help us to be as certain as possible this won't increase any server explosion rates?

hmmmm I have no idea if people use SKU on my instance... but if they do I think the change is logical. So I guess we are good to go 👍

luisramos0 · 2020-04-10T09:00:44Z

Hello @oeoeaio 👋
Moving to In Dev as it is a draft PR, we can move to Code Review once it's not a draft PR.

oeoeaio · 2020-04-10T12:20:22Z

Hey @luisramos0 👋, and thanks, I've marked it as ready for review.

luisramos0

Hey Rob, it's great to see you around 👍

This will make the report correct.
This will make the report slower. Can be considerably slower, we will load all VariantOverrides of all hubs involved.
The alternative is to do the scoping in the report SQL, it's very difficult. I think @Matt-Yorkley tried to do it at some point for the shopfront endpoints, I cant remember if that code was merged at some point.

I think we should test how the report behaves with lots of data before and after this PR.

Matt-Yorkley · 2020-04-10T14:09:33Z

I think @Matt-Yorkley tried to do it at some point for the shopfront endpoints

I added some "only fetch the objects within the daterange" gymnastics to the line items returned in permissions checks (for reports) a while back in #5030. Is that the one? Maybe we could try to do something similar for the various variant-scoping processes? I'm not sure how easy it would be.

luisramos0 · 2020-04-10T14:23:42Z

ah, no, I meant something completely different (older), anyway, you suggestion is awesome!
Implementing a variation of VariantOverride#indexed that uses the report date range to filter overrides used in line_items in that range and then inject it as the 2nd param when creating the scoper.

oeoeaio · 2020-04-11T00:14:42Z

Implementing a variation of VariantOverride#indexed that uses the report date range to filter overrides used in line_items in that range and then inject it as the 2nd param when creating the scoper.

So the theory is that the shear number of overrides being loaded into memory will be the problem, rather than the time taken to query for relevant overrides? I am happy to have a crack and building a list of overrides (based on the report parameters) to inject into the scoper if pulling up a smaller number of overrides is desirable.

oeoeaio · 2020-04-11T07:39:09Z

lib/open_food_network/orders_and_fulfillments_report.rb

+      @report_variant_overrides ||=
+        Reports::VariantOverrides.new(
+          line_items: order_permissions.visible_line_items,
+          distributor_ids: report_line_items.orders.result.pluck('DISTINCT distributor_id'),


Not super happy with this approach, but I couldn't think of a better one. Any ideas?

I had an alternative implementation where the Reports::VariantOverrides class was only responsible for loading variant overrides for a single distributor at a time, which made it easier to just inject the relevant distributor each time a new one was encountered, but it obviously resulted in more queries. See 2a843bf. Let me know which you think is preferable.

Lets go with this version 👍

oeoeaio · 2020-04-11T07:42:25Z

Hey @luisramos0, @Matt-Yorkley, I've had another go at this. The new implementation should only load variant overrides that are relevant to the line items in the report.

Matt-Yorkley · 2020-04-12T09:19:48Z

lib/open_food_network/orders_and_fulfillments_report.rb

-        Reports::VariantOverrides.new(order_permissions.visible_line_items).indexed
+        Reports::VariantOverrides.new(
+          line_items: order_permissions.visible_line_items,
+          distributor_ids: report_line_items.orders.result.pluck('DISTINCT distributor_id'),


report_line_items.orders.result is an ActiveRecord::Relation here, so I think in this case #select will be more efficient than #pluck for building the distributor_ids part of the subquery, e.g:

distributor_ids: report_line_items.orders.result.select(:distributor_id),

luisramos0

Fantastic! It's great to see you working with this code as if it's easy!
This is adding some complexity to an already complex issue in OFN (variant_overrides) but it's a good implementation.

Can you please move the service to app/services? I dont think it needs the reports namespace there, I can see it being re-used in other parts of the app!

lib/open_food_network/reports/variant_overrides.rb

lib/open_food_network/orders_and_fulfillments_report.rb

lib/open_food_network/reports/variant_overrides.rb

Stop using keyword args and accept variant_ids instead of line_items

luisramos0

awesome! thanks @oeoeaio

kirstenalarsen · 2020-04-16T06:54:52Z

Hi people - I did a quick test of this to try and get it through in this release. As I'm not really a tester I don't have a set-up with lots of data to check performance so still best if someone else does that . .

But I am not sure if this implementation is correct and need another couple of brains. This is what I did:

put an sku on two variants of a product ('var a sku' and 'var b sku')

Placed an order, looked at Customer Totals report

Correctly showing product skus

Put both variants into inventory and put a different sku on them
Changed my inventory setting to Coord Inv only (unnecessary as these will override in my shop anyway but just to be sure)
Placed another order - so it's using the inventory variants this time
Ran the Customer Totals report.

It is showing the inventory skus - yay. But it is showing them for all items in the report, including those from before I created the inventory and set those new skus

So the question is - is this correct? Once something is placed into inventory for the Shop and there is a new SKU - should this SKU be retrospectively applied to all orders in the past, including those before there was an inventory override and before the SKU existed? Overriding information that would have been attached to orders created with the product SKU?

I am a bit agnostic on this - think it's likely a rare enough case that we don't need to worry too much and it does make sense that once someone puts it in inventory and attaches a SKU they DO mean that to refer to all previous sales of it . . so maybe it's ok?

oeoeaio · 2020-04-16T10:03:30Z

Hi @kirstenalarsen! Thanks for testing this out.

Once something is placed into inventory for the Shop and there is a new SKU - should this SKU be retrospectively applied to all orders in the past, including those before there was an inventory override and before the SKU existed?

That is the intended behaviour from my perspective. It is far easier to implement this way (I'm actually not even sure I can think of a way to make it not apply retrospectively, as far as I can recall SKU is a property of variants / overrides, not of line items). I have a use-case implementing it this way too: at BBFH, new variants are often added to the shop on an ad-hoc basis while an order cycle is open (as we get new information about products that are available). Often this is done in a rush to get them onto the shop before we send our weekly newsletter. We may not have time to fill in all of the details required to link products to external systems (i.e. SKU for our POS). We would usually reconcile our orders with other systems after an order cycle has closed, and it is at this point that we would probably go back and look at any new products and add them to our POS or match them to an existing POS product. It is only at this point that an SKU would be set most of the time, so it would be useful for us to have any SKUs added to the inventory apply retrospectively.

filipefurtad0 · 2020-04-16T12:12:37Z

Hi @oeoeaio,
And thanks @kirstenalarsen for picking this up! - I'd say that's great testing :-)

Verified all the findings. Also checked whether this is reversible: Checking the "Inherit?" works, and sets back the SKU settings from the shop. The general functionality of reports does not seem affected. All good.

In terms of testing I'd say this is good to go.
Thanks Rob.

Matt-Yorkley reviewed Apr 7, 2020

View reviewed changes

oeoeaio self-assigned this Apr 8, 2020

oeoeaio marked this pull request as ready for review April 10, 2020 12:17

luisramos0 approved these changes Apr 10, 2020

View reviewed changes

oeoeaio force-pushed the customer-totals-inventory-sku branch from fb29d02 to f9ab3a1 Compare April 11, 2020 06:00

Take sku overrides into account in customer totals report

14cf168

oeoeaio force-pushed the customer-totals-inventory-sku branch from f9ab3a1 to 14cf168 Compare April 11, 2020 06:05

oeoeaio commented Apr 11, 2020

View reviewed changes

Matt-Yorkley reviewed Apr 12, 2020

View reviewed changes

Only load up variant overrides for relevant hubs

9a7e782

oeoeaio force-pushed the customer-totals-inventory-sku branch from 403d7f4 to 9a7e782 Compare April 13, 2020 02:07

Matt-Yorkley approved these changes Apr 13, 2020

View reviewed changes

luisramos0 suggested changes Apr 13, 2020

View reviewed changes

oeoeaio added 4 commits April 13, 2020 21:54

Move VariantOverrides class to app/services

4a4173b

Rename VariantOverrides class to VariantOverridesIndexed

6820919

Modify interface of VariantOverridesIndexed#indexed

dcdd3f2

Stop using keyword args and accept variant_ids instead of line_items

Improve naming of variables in VariantOverridesIndexed for readability

f71045b

luisramos0 approved these changes Apr 13, 2020

View reviewed changes

filipefurtad0 added the easy-test label Apr 14, 2020

daniellemoorhead assigned kirstenalarsen Apr 16, 2020

daniellemoorhead added the pr-staged-au staging.openfoodnetwork.org.au label Apr 16, 2020

filipefurtad0 removed the pr-staged-au staging.openfoodnetwork.org.au label Apr 16, 2020

mkllnk merged commit e29a81c into openfoodfoundation:master Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take sku overrides into account in customer totals report #5186

Take sku overrides into account in customer totals report #5186

oeoeaio commented Apr 7, 2020

sauloperez commented Apr 7, 2020 •

edited

Matt-Yorkley commented Apr 7, 2020

Matt-Yorkley Apr 7, 2020 •

edited

oeoeaio Apr 8, 2020

Matt-Yorkley Apr 8, 2020

oeoeaio Apr 8, 2020

Matt-Yorkley Apr 8, 2020 •

edited

lin-d-hop Apr 8, 2020 •

edited

RachL Apr 8, 2020

luisramos0 commented Apr 10, 2020

oeoeaio commented Apr 10, 2020

luisramos0 left a comment

Matt-Yorkley commented Apr 10, 2020 •

edited

luisramos0 commented Apr 10, 2020

oeoeaio commented Apr 11, 2020

oeoeaio Apr 11, 2020

Matt-Yorkley Apr 12, 2020

oeoeaio commented Apr 11, 2020

Matt-Yorkley Apr 12, 2020

oeoeaio Apr 13, 2020

luisramos0 left a comment •

edited

luisramos0 left a comment

kirstenalarsen commented Apr 16, 2020

oeoeaio commented Apr 16, 2020

filipefurtad0 commented Apr 16, 2020 •

edited

Take sku overrides into account in customer totals report #5186

Take sku overrides into account in customer totals report #5186

Conversation

oeoeaio commented Apr 7, 2020

What? Why?

What should we test?

Release notes

sauloperez commented Apr 7, 2020 • edited

Matt-Yorkley commented Apr 7, 2020

Matt-Yorkley Apr 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Apr 8, 2020 • edited

Choose a reason for hiding this comment

lin-d-hop Apr 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 commented Apr 10, 2020

oeoeaio commented Apr 10, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

Matt-Yorkley commented Apr 10, 2020 • edited

luisramos0 commented Apr 10, 2020

oeoeaio commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oeoeaio commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment • edited

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

kirstenalarsen commented Apr 16, 2020

oeoeaio commented Apr 16, 2020

filipefurtad0 commented Apr 16, 2020 • edited

sauloperez commented Apr 7, 2020 •

edited

Matt-Yorkley Apr 7, 2020 •

edited

Matt-Yorkley Apr 8, 2020 •

edited

lin-d-hop Apr 8, 2020 •

edited

Matt-Yorkley commented Apr 10, 2020 •

edited

luisramos0 left a comment •

edited

filipefurtad0 commented Apr 16, 2020 •

edited