Openfoodnetwork: [Spree Upgrade] Refactor all uses of spree_products.count_on_hand

[oeoeaio]

Several instances of dead code that use these attributes have been identified as per: #2004

We need to work out an approach for finding and dealing with other uses of this method.

Likely to be related to #2014

[sauloperez]

spree_products.count_on_hand is just the aggregation of the count_on_hand of the variants of the product:

The couple following code snippets from core/app/models/spree/product.rb show a clear view of how this changes in Spree 2.0:

has_many :stock_items, through: :variants

def total_on_hand
  if Spree::Config.track_inventory_levels
    self.stock_items.sum(&:count_on_hand)
  else
    Float::INFINITY
  end
end

[sauloperez]

I just spotted a use of spree_products.count_on_hand in:

https://github.com/openfoodfoundation/openfoodnetwork/blob/46d2d6cb8ac9425485cc9635cc5dbc515583151f/app/views/spree/admin/overview/_enterprises_producers_tab.html.haml#L28-L35

That method is defined as follows:
https://github.com/openfoodfoundation/openfoodnetwork/blob/46d2d6cb8ac9425485cc9635cc5dbc515583151f/app/models/enterprise.rb#L206-L208

That partial is used by app/views/spree/admin/overview/multi_enterprise_dashboard.html.haml (the one admins of multiple enterprises see, not only superadmins):

Note you have to actually need to click on the Producers tab to see the data, although it is already loaded. Besides, calling that method from the view incurs into an N+1. Skylight spotted this and more performance issues there

[sauloperez]

We overrode the dashboard's controller Spree::Admin::OverviewController by means of a class_eval in app/controllers/spree/admin/overview_controller_decorator.rb. There we have custom logic to either render single_enterprise_dashboard or multi_enterprise_dashboard.

[sauloperez]

Given all the problems aforementioned I would simply remove that tab. It's something easy to do but I'd like to check somehow (using analytics?) if someone uses it. Thoughts @myriamboure @enricostano @daniellemoorhead @kirstenalarsen ?

[myriamboure]

Hum... @sauloperez I'm wondering one thing, for a producer who only manages their products catalog (as it is used by hubs on OFN) but doesn't manage any hub, that would mean they wouldn't have access to their enterprise from dashboard, am I right? That would be an issue I guess... isn't it possible to just have one tab "my enterprises" and put them all in if the point is to remove the producer tab? Or is the problem to bring info on active products / products in OC? Cause in that case we can also make the dashboard simpler, I'm not sure those info are so relevant also as you don't know which products are in which OC so for me it brings more confusion than useful info.

[sauloperez]

I've found all the uses of spree_products.count_on_hand in https://github.com/openfoodfoundation/openfoodnetwork/pull/2183. Check the commit for the details.

[daniellemoorhead]

@sstead any thoughts on the above ☝️ ?

[sauloperez]

@myriamboure the problem is just with that Producers tab on the right. The problem comes with those counts of active products, which currently depend on a database column that in Spree 2.0 no longer exists.

I feel like just a very small percentage of our users noticed that tab so I would consider removing it rather than reimplementing it, considering the fact that it has a negative impact on performance. Again, we don't have the data to claim no one uses it but I think we can do an educated guess and simplify the product.

[myriamboure]

I have no objection to that, we can see then if people should and reimplement something else simpler later on if there is an obvious need...
Just one clarification @sauloperez : if the "producer" tab disappear, will the "hub" tab also disappeared? I guess it should, that would make no sense having a sub table with one tab. So then we would only have "my enterprises" with all my enterprises in it, hubs and producers mixed. Am I right?
@sstead any objection?

[sstead]

I agree, that tab would be rarely used. I never use it, so i’m comfortable to see it go. I don’t use the dashboard at all, ever, come to think of it.

[sauloperez]

So then we would only have "my enterprises" with all my enterprises in it, hubs and producers mixed. Am I right?

Exactly. Just a list of enterprises.

[sauloperez]

Ok. Moving forward with this one. I created https://github.com/openfoodfoundation/openfoodnetwork/issues/2289 out of this discussion.

[mkllnk]

https://github.com/openfoodfoundation/openfoodnetwork/pull/2183 identified all uses of spree_products.count_on_hand at the time. Here is an updated summary:

• ~app/models/enterprise.rb~
• ~app/views/spree/admin/overview/_enterprises_producers_tab.html.haml~
• ~spec/controllers/cart_controller_spec.rb~
• ~spec/models/enterprise_spec.rb~

[mkllnk]

I just discovered more recent code using count_on_hand:

• https://github.com/openfoodfoundation/openfoodnetwork/blob/eb9064f5be3433ed72f708c32bccbaa635a2988d/app/models/product_import/entry_processor.rb#L78-L83
• https://github.com/openfoodfoundation/openfoodnetwork/blob/eb9064f5be3433ed72f708c32bccbaa635a2988d/app/models/product_import/entry_processor.rb#L196
• https://github.com/openfoodfoundation/openfoodnetwork/blob/eb9064f5be3433ed72f708c32bccbaa635a2988d/app/models/product_import/entry_validator.rb#L269

These require some effort to abstract. The current code operates directly on the database which is the most efficient. If we want to keep it that way, we have to change the SQL queries to work with Spree's new database schema.

[mkllnk]

I have been looking more into the Product Import code and there are a few things where I don't understand the design decisions. It is work in progress and I understand that it needs refactoring. Right now, a Spree upgrade would break it. Before I continue working on this, I would like some input from @Matt-Yorkley about the roadmap for Product Import and how we should tackle it.

[luisramos0]

Hey guys, I found this issue on the code review column in zenhub. It isn't ready for review is it?
I only found @mkllnk 's commit related to active distributors.

[mkllnk]

Thanks @luisramos0. I think this got moved around with a connected PR. It should go back to dev-ready as there is still some work to do, especially around product import.

[mkllnk]

I just found that the ProductSerializer is using on_demand. We need to check if that's needed or replace the implementation.

[luisramos0]

The required change in Product Import is represented in #2782 and was moved out of here into the spree upgrade "phase 2" epic #2953

This epic represents changes in the core parts of OFN only.

So far we have #2688 identified, it depends on the fixing (de-defacing?) of "new product" page in #2982

We still need to close the scope of this epic, what other references to count_on_hand need to be fixed?

[sauloperez]

The only other reference I found (quick check) other than product import and migrations is https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/javascripts/unit/bulk_product_update_spec.js.coffee#L186. I tried locally and removing the line still makes the test pass what I'd be great to understand what's the test actually doing.

Anyway, I'd say we can create an issue for that and close this Epic as soon as #2688 is closed. As you said, this is core only.

[luisramos0]

I thoroughly scanned the code again for product.count_on_hand and only found the reference in bulk_product_update_spec.js.coffee.
I am fixing that reference in PR #3494 so that we can close this issue.