[Spree Upgrade][PI] Make Product Import create stock items (1 broken spec)

[sauloperez]

Description

In Product Import spreadsheet entries map directly to product, variant and variant override records. That is, an entry can become any of these entities in our DB.

With Spree 2.0 this gets more complex and variant.count_on_hand becomes variant.stock_items.first.count_on_hand. It can't be longer assumed that entry.attributes == variant.attributes (or product or variant override).

[sauloperez]

spec/models/product_importer_spec.rb:312 fails due to the same root cause. Product Import assumes variants have an on_demand column and that is no longer the case. See the test's output below:

104) ProductImport::ProductImporter updating various fields saves and updates
       Failure/Error: expect(beetroot.on_demand).to_not eq true
       
         expected: value != true
              got: true
       
         (compared using ==)
       
         Diff:
         
       # ./spec/models/product_importer_spec.rb:312:in `block (3 levels) in <top (required)>'

[luisramos0]

the description is a bit outdated now because we have since then agreed that we will not play with stock items but instead use the variant_stock facade to handle these cases. done in #3355

[luisramos0]

the only case where this is still a problem is the only broken spec after #3355:
spec/models/product_importer_spec.rb:684

In this spec, we test entry_processor.assign_defaults where we set the defaults.
The challenge here is that we set the defaults before we save the "object", but the object can be product or variant, SO, it's not straight forward to do the same as in #3355 where, for variants we save the variant and then set on_hand, for product we set on_hand in the variant.

Lets use this issue to represent this broken spec, ok?

[luisramos0]

the build error for this broken spec:

 2) ProductImport::ProductImporter applying settings and defaults on import can overwrite fields with selected defaults when importing to product list
     Failure/Error: expect(carrots.on_hand).to eq 9000
     
       expected: 9000
            got: 5
     
       (compared using ==)
     # ./spec/models/product_importer_spec.rb:735:in `block (3 levels) in <top (required)>'

[mkllnk]

I think we are hitting a design flaw within product import here and may need to refactor some parts first.

Wrong assumption

Product import assumes that columns in the spreadsheet correlate to attributes on products, variants or inventory items. This is not the case in Spree 2. For example, products and variants don't have on_hand or on_demand. One line in the spreadsheet affects more than one object in our data model. We end up with code like this:

          if object.public_send(attribute).blank? ||
             ((attribute == 'on_hand' || attribute == 'count_on_hand') &&
             entry.on_hand_nil)

            object.assign_attributes(attribute => setting['value'])
          end

    def check_on_hand_nil(entry, object)
      return if entry.on_hand.present?

      object.on_hand = 0 if object.respond_to?(:on_hand)
      object.count_on_hand = 0 if object.respond_to?(:count_on_hand)
      entry.on_hand_nil = true
    end

Other code smell

Product import uses a class Entry to represent one line in the spreadsheet. It can either be a product, a variant or an inventory item. The code has to handle these cases differently and there are a lot of branches for this. For example:

    def save_to_product_list(entry)
      save_new_product entry if entry.validates_as? 'new_product'

      if entry.validates_as? 'new_variant'
        save_variant entry
        @variants_created += 1
      end

      return unless entry.validates_as? 'existing_variant'

      # ...
    end

Proposal

We create new classes for the five different entry types.

• new_inventory_item
• existing_inventory_item
• new_product
• new_variant
• existing_variant

They could be named like NewProduct and ExistingVariant. They contain the processing logic for these types which should simplify the EntryProcessor. That processor has some code duplication in the methods save_new_inventory_item, save_existing_inventory_item, save_new_product etc. The new classes would need the following methods, for example:

• initialize(entry)
• assign_defaults(settings)
• valid?
• save
• errors

Most of those may be private and the only methods called by the processor are new and save.

[luisramos0]

Hey @mkllnk yes, I totally agree with the problem and I like the solution.
Interfaces would come handy here where you would have an Importable Interface and a Factory that takes an Entry and returns an Importable. You would have 5 implementations of Importable: new_product, new_variant, etc. What you describe is similar to this but without interfaces...

Having said that, I dont think this is strictly needed now, I think it's tech debt that we don't necessarily need to pay now (my opinion). We can add one more branch now and get the upgrade done.

The main reason I say this about this task is that ProductImport (although it has a few OO flaws, this is just one) is far from being core to OFN and we should invest quality refactoring time on core parts of the system, that's where it will pay off really quickly. For this reason I'd create a tech debt issue that is lower in priority than for example both openfoodfoundation/wishlist#334 and #2763
It's obviously your call, just wanted to share my opinion.

[mkllnk]

@luisramos0

Interfaces would come handy here where you would have an Importable Interface and a Factory that takes an Entry and returns an Importable.

I totally agree. This is where I miss Java, too. In Ruby we don't have typed variables and therefore no support of interfaces. We only have unit tests to verify it something adheres to an interface.

I also share your opinion about the priority since I found a quick hacky solution in #3425. I created a new tech-debt issue: openfoodfoundation/wishlist#368.

[sauloperez]

I had that same concern when I first looked at PI's code being familiar with v2 data model. I do agree that if we can fix PI in v2 with too much hassle we can focus on other more core things.

However, I'm of the opinion that when implementing new requirements to any existing code devs should take a first refactoring step to adapt the existing design to fit the new needs. This makes refactorings well scoped and quick and make subsequent requirements easy to implement.

In this case, I'd have done that the first time we touched PI in v2. Just a first small refactor. But that's just my experience and opinion.

[luisramos0]

Agree @sauloperez we could have done the refactor and this refactor move is very important as it will clarify PI a lot. I am just not sure about the size. I think it's a M and that's why I think we should skip it for now.

In the last 6 months for the spree upgrade, I think I have 20 examples (PRs) where I could have done a lot more refactoring. Choosing the refactoring path means delaying delivery, in the spree upgrade case, I could easily take another 6 months just for refactoring! Taking this path of not refactoring is true tech debt in my opinion, true tech debt is deliberate. "We must ship now I deal with the consequences" https://martinfowler.com/bliki/TechnicalDebtQuadrant.html

[sauloperez]

Yes, I also agree. It's been reeeeeaaaally hard for me to be pragmatic and refrain myself from refactoring and cleaning things up within the upgrade. I'm sure you've noticed

[luisramos0]

yes, it's very hard but the bright side is that we do see how to improve it and we are slowly improving the code base.