Openfoodnetwork: Product Import code is tightly coupled to Spree models

[mkllnk]

Description

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

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

Context

This came up during the Spree upgrade when I posted https://github.com/openfoodfoundation/openfoodnetwork/issues/2783#issuecomment-459601530.

Severity

tech-debt

Product import is not a core feature, but it would be good to solve this before the next Spree upgrade. It will make the upgrade much easier.

Possible Fix

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]

this is closely related to this comment here
I think we can include the comment in the scope of this issue: reduce the size of product_import/entry_validator class in some way.

[RachL]

@luisramos0 @mkllnk is this issue concerning something we have to do as part of the v2 roll-out Epic?

[luisramos0]

no, it's a tech debt issue that we consider important but will not block any release.

[kshlyk]

I'd like to implement this improvement. Could you please assign it to me?

[luisramos0]

awesome, it's an interesting one that will allow you to write new code :+1:

Please share early so we can give you feedback ;-)

In terms of the solution, one thing I'd like to mention is that this is not code for app/models but for services. So it would be great to have all this code moved from app/models/product_import to app/services/product_import 💪

[kshlyk]

Thank you for motivation @luisramos0! I'll share my thoughts before implementing. Now I need to investigate a bit :-)

[luisramos0]

Hello @kshlyk happpy new year!
It's a been a month since your last comment. Are you still planning to tackle this issue? Let us know if you have any questions. Thanks!

[kshlyk]

Hi @luisramos0. Happy New Year! I'm kind of overloaded on the current project last days. Yes, I'm going to carry out fixing this issue but I need some extra time. Please let me know in case I should hurry up.

[luisramos0]

Hi @kshlyk thanks for the update. It's great you are planning to tackle this!
I just wanted to make sure the issue was up to date, and it is!
Good luck with your current project!

[kshlyk]

Hi @luisramos0 @mkllnk! I've started working on the issue. I should breakdown huge classes EntryValidator and EntryProcessor. There is a good solution proposed by Maikel to create separate class for each type of entry: NewProduct, NewVariant, ExistingVariant, NewInventoryItem and ExistingInventoryItem and to place all specific validation and processing logic into them. In order to recognise entry type I need to do some validation. As our import process contains three steps: upload, validate and save, I'm not sure which step is more suitable for entry type recognition logic. Should I make it on upload step immediately or keep upload clear and recognise entry type on validate step? What do you recommend?

[luisramos0]

Hello @kshlyk
I was looking at the code now, this is the critical method:
https://github.com/openfoodfoundation/openfoodnetwork/blob/e1eface5f8e8906cc22545cc26e408030a0ac99c/app/models/product_import/product_importer.rb#L173

Some of these classes are very overloaded with logic. My first objective would be to just extract some of this code to new classes....

The tests for new_product or new_inventory_item are mostly in EntryValidator.
I see the types are also at:
app/assets/javascripts/admin/product_import/filters/filter_entries.js.coffee

Anyway, EntryValidator with 450 lines, so much logic and an constructor with 8 parameters is just crazy bad. So, I'd start by focusing only on the EntryValidator class by extracting validation logic to classes of specific types like NewInventoryItemValidator.
If you want to use polymorphism as Maikel suggests please use the FactoryMethod pattern to create those classes.

I hope this helps :+1:

[luisramos0]

Yesterday I moved subscriptions to the Orders domain in #4783. I think this is a great opportunity to move product import to it's place and create the Catalog engine :-)
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Tech-Doc:-How-OFN-is-organized-in-Domains-using-Rails-Engines

It would be something like this to create the new domain #4787

[kshlyk]

OK, I've got the point. Let's start refactoring from the EntryValidator. I'd create entry-specific classes as children of the SpreadsheetEntry and implement valid? method for ones. This way I can isolate validation logic for each type and put it into appropriate class. Do you see any problems with that approach?

As for engines I absolutely agree with you. I'll move the product import logic into the Catalog engine once it appears in the master.

[luisramos0]

yes, you can do that :+1:
I always "prefer composition over inheritance".
in my book inheritance almost always brings unnecessary complexity. this is just my personal opinion. it's your code, enjoy!

[mkllnk]

I always "prefer composition over inheritance".
this is just my personal opinion.

It's not just your opinion. It's a well known design pattern preferred by many experts.

[kshlyk]

Thank you guys. As far as I understand in a composition way it should be something like this:

class Validator
  def self.for(entry)
    case recognise_entry_type(entry)
    when 'new_product'
      NewProductValidator.new
    when 'new_variant'
      NewVariantValidator.new
    when 'existing_variant'
      ExistingVariantValidator.new
    ...
    end
  end
end

class NewProductValidator
  def validate(entry)
    #validation
  end
end

class NewVariantValidator
  def validate(entry)
    #validation
  end
end

class ProductImporter
  ...
  def validate_entries
    @entries.each do |entry|
      Validator.for(entry).validate(entry)
    end
  end
end

[luisramos0]

yes! and the Validator can be the ValidatorFactory :+1:

[kshlyk]

Thank you, great!

[mkllnk]

If all validators have the same interface, you don't need magic strings like 'new_product'. You can return an instance of the right validator straight away.

[kshlyk]

OK. It's just a wireframe of the solution structure. I'm not sure yet if all validators should have the same initialization. I need to pass some extra params which may differ from type to type.

[daniellemoorhead]

Hey @kshlyk, it's been a fair while since there's been any activity on this card. Do you think you'll come back to finish it soon, or should we move it back into Dev Ready to allow someone else to pick it up?

[kshlyk]

Hey @daniellemoorhead, I hope to finish soon. I'll inform you of my progress by next weekend.

[kirstenalarsen]

Think it's time to unassign @kshlyk and move back to dev ready