Unit test variant stock #2638

sauloperez · 2018-09-03T16:12:58Z

What? Why?

As explained in the issue, this adds unit tests, documentation and deprecation warnings to the new VariantStock module. The one that provides backward compatibility for the current stock methods such as #on_hand or #on_demand.

You can read the commits to understand minor details of the decisions taken, like 5c14a8c.

What should we test?

The upgrade is not ready to be tested yet.

Release notes

Changelog Category: Changed

daniellemoorhead · 2018-09-04T07:26:51Z

@sauloperez

Please, please, #assignyourself to PRs. Please 🙏

sauloperez · 2018-09-04T11:46:14Z

lib/open_food_network/variant_stock.rb

      raise_error_if_no_stock_item_available
+      raise_error_if_multiple_stock_items


I'll try to convince you this deserves to become a DB-level constraint although opinions (See https://openfoodnetwork.slack.com/threads/convo/C4NDJT3FY-1535723508.000100/) were against it.

First, at this point, it is too late. The data is already inconsistent, violating data integrity and cleaning it manually is the most we can do. And that is hard and painful.

Second, nothing stops the data to become inconsistent by any other means. Adding this sort of barrier, doesn't prevent this from happening from a race condition, rails console, a controller or even a psql session. It's like poner puertas al campo aka. trying to stop the unstoppable.

As a result, unconfident programming comes up; the lines of code consuming stock_items become paranoid checking there's a single stock item throughout the app.

That is why, I believe this would be better off replaced by adding add_index :spree_stock_items, :variant_id, unique: true from a migration. I know we don't like messing with Spree's schema but this doesn't modify the tables and the data integrity it provides well outweighs it.

I see. I dont like the approach because we should know who's doing what and fix when it's not happening.

"become paranoid checking there's a single stock item throughout the app"
The place from where code is "consuming stock_items" should be one and only one: it's the model and service layer of this part of the app. But I know we don't have that kind of isolation... So, lets do it!

Yes, I'm in favour of a constraint. That does make some of my earlier comments redundant.

luisramos0 · 2018-09-05T22:00:29Z

lib/open_food_network/variant_stock.rb

+# These methods were available in Spree 1, but were removed in Spree 2.  We
+# would still like to use them so that we still give support to the consumers
+# of this methods, making the upgrade backward compatible.
+#


I believe this module should be called Adapter, VariantStockAdapter.
And the documentation could become more simple:

-# This module is an adapter for OFN to work with Spree v2 code.
-#
-# This adapter simplifies the Spree 2 data model by using only a single stock item per variant.
-# Each stock item is associated to a single stock location per instance (default stock location).
-# The stock items are used to track the count_on_hand value (previously a database column on -variants).
-# See https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations
-# for details.

luisramos0 · 2018-09-05T22:02:54Z

lib/open_food_network/variant_stock.rb

+# track the `count_on_hand` value that was previously a database column on
+# variants. See
+# https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations
+# for details.
 #
 # We may decide to deprecate these methods after we designed the Network feature.


We may have the deprecation warning here, for example, "Some methods are or may become deprecated." but I dont think we should mention roadmap/future items in code comments ever. I'd remove the reference to Network feature.

luisramos0 · 2018-09-05T22:04:43Z

lib/open_food_network/variant_stock.rb

+    # Returns the number of items of the variant available in the
+    # stock. When allowing on demand, it returns infinite.
+    #
+    # Note this method relies on Spree's 2.0 #total_on_hand.


I would remove this comment. The code is self explanatory.

luisramos0 · 2018-09-05T22:06:56Z

lib/open_food_network/variant_stock.rb

@@ -22,39 +34,65 @@ def on_hand
      end
    end

+    # Alias for Spree's 2.0 #total_on_hand


Instead of referring to the method used below. It would be more useful to explain what this method does, probably just "Number of items available in the stock for this variant."

luisramos0 · 2018-09-05T22:12:10Z

lib/open_food_network/variant_stock.rb

    def on_hand=(new_level)
      error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
      raise error unless Spree::Config.track_inventory_levels

      self.count_on_hand = new_level
    end

+    # Sets the stock level. As opposed to #on_hand= it does not check `track_inventory_levels`'s value. It does ensure there's a stock item for the variant however raising otherwise. See #raise_error_if_no_stock_item_available for details.


More then explaining what's already in the code, I'd prefer to see here the why it is doing so. Why is it not checking "track_inventory_levels" here? same for on_hand=, why is it not checking for the existence of stock_item?

luisramos0 · 2018-09-05T22:12:33Z

lib/open_food_network/variant_stock.rb

    def count_on_hand=(new_level)
      raise_error_if_no_stock_item_available
      overwrite_stock_levels new_level
    end

    def on_demand
-      stock_items.any?(&:backorderable?)
+      warn_deprecation(__method__)
+      stock_items.first.backorderable?


luisramos0 · 2018-09-05T22:14:51Z

lib/open_food_network/variant_stock.rb

    end

+    # Sets whether the variant can be ordered on demand or not. Note
+    # that although this modifies the stock item, it is not
+    # persisted.


what do you mean by "it is not persisted"? why would you use it then?

luisramos0 · 2018-09-05T22:26:24Z

lib/open_food_network/variant_stock.rb

      raise_error_if_no_stock_item_available
+      raise_error_if_multiple_stock_items


I see. I dont like the approach because we should know who's doing what and fix when it's not happening.

"become paranoid checking there's a single stock item throughout the app"
The place from where code is "consuming stock_items" should be one and only one: it's the model and service layer of this part of the app. But I know we don't have that kind of isolation... So, lets do it!

luisramos0

Awesome work Pau!
I just added some very ignorable comments about comments :-)

sauloperez · 2018-09-06T06:59:13Z

Thanks for your review @luisramos0 ! I tried to share more knowledge and be concise. Let me know if it's all clear. Future devs must have no doubts when reading it.

I move on to add a DB constraint and rename it now.

This also replaces the after_save callback to after_update. We enforce a stock_item exists when modifying the variant and the stock_item gets created on variant's after_create. This means it's not possible to use any of the VariantStock's setters before the variant is persisted so executing the callback on creation is pointless.

This will prevent other devs from relying on this methods and will tell us the exact lines that call this methods, which will come in handy when removing this module.

This means we violated the business rules of having a single stock location for the OFN instance and hence a single stock item per variant. Although it is too late to preserve the data integrity we can know data needs to be cleaned up.

By forbidding more than a row per variant in the spree_stock_items we can ensure all variants have a single stock_item associated.

sauloperez · 2018-09-06T11:23:16Z

db/schema.rb

@@ -961,6 +961,7 @@

  add_index "spree_stock_items", ["stock_location_id", "variant_id"], :name => "stock_item_by_loc_and_var_id"
  add_index "spree_stock_items", ["stock_location_id"], :name => "index_spree_stock_items_on_stock_location_id"
+  add_index "spree_stock_items", ["variant_id"], :name => "index_spree_stock_items_on_variant_id", :unique => true


this now raises

ActiveRecord::RecordNotUnique: PG::Error: ERROR: duplicate key value violates unique constraint "index_spree_stock_items_on_variant_id" DETAIL: Key (variant_id)=(9) already exists. : INSERT INTO "spree_stock_items" ("backorderable", "count_on_hand", "created_at", "stock_location_id", "updated_at", "variant_id") VALUES ($1, $2, $3, $4, $5, $6) RETURNING "id"

when saving a stock_item for a variant that already exists in the table.

luisramos0 · 2018-09-06T11:59:02Z

Awesome! Comments are better now, thanks!
I know it's probably the normal thing to do in Rails but I have to say again, I dont like the fact the name of the module is not stating clearly that it is an Adapter or a Concern. This VariantStock will never be used outside of Variant, it's an add on, it is not a standalone service/module/model, so I'd call it VariantStockConcern.

sauloperez · 2018-09-06T13:53:00Z

Yep, I still need to take a look at that

mkllnk

Really nice pull request. But I would like to discuss the deprecation approach before approving this.

mkllnk · 2018-09-07T07:51:58Z

lib/open_food_network/variant_stock.rb

    def on_hand=(new_level)
      error = 'Cannot set on_hand value when Spree::Config[:track_inventory_levels] is false'
      raise error unless Spree::Config.track_inventory_levels

      self.count_on_hand = new_level
    end

+    # Sets the stock level. As opposed to #on_hand= it does not check
+    # `track_inventory_levels`'s value as it was previously an ActiveModel
+    # setter of te database column of the `spree_variants` table. That is why


Tiny typo:

te database

mkllnk · 2018-09-07T07:52:23Z

lib/open_food_network/variant_stock.rb

+    # #on_hand= is more widely used in Spree's codebase using #count_on_hand=
+    # underneath.
+    #
+    # So, if #count_on_hand= is used, `track_inventory_levels` won't be tacken


mkllnk · 2018-09-07T07:56:18Z

lib/open_food_network/variant_stock.rb

    def save_stock
-      stock_items.each(&:save)
+      stock_items.first.save


In on_demand= we change all stock items, but here we save only the first one. Even though there should be only one item, I kept the each to make it correct in case there is more than one item.

Sure, just take a look at https://github.com/openfoodfoundation/openfoodnetwork/pull/2638/files#diff-1acd2e7e27a227829d5d14a91c863bb6R964. With the DB-level constraint it will only be one item so no need for iterations.

In on_demand= however, I couldn't make the test work and I still don't know why... I can investigate it more in-depth after we merge it.

The Spree config option allow_backorders affects this attribute in various ways, if that helps.

Any idea how? I'll take a look anyway.

that config is gone @Matt-Yorkley from Spree.

mkllnk · 2018-09-07T07:57:25Z

lib/open_food_network/variant_stock.rb

-      end
+      # There shouldn't be any other stock items, because we should
+      # have only one stock location.
+      stock_items.first.__send__(:count_on_hand=, new_level)


Same here. If there would be an accidental stock item, we would see weird bugs.

mkllnk · 2018-09-07T07:59:19Z

lib/open_food_network/variant_stock.rb

    def on_demand
-      stock_items.any?(&:backorderable?)
+      warn_deprecation(__method__)


I think this line should have been in the next commit. The method warn_deprecation is not defined yet.

mkllnk · 2018-09-07T08:04:17Z

lib/open_food_network/variant_stock.rb

@@ -77,7 +84,7 @@ def count_on_hand=(new_level)
    # track_inventory_levels only. It was initially introduced in
    # https://github.com/openfoodfoundation/spree/commit/20b5ad9835dca7f41a40ad16c7b45f987eea6dcc
    def on_demand
-      warn_deprecation(__method__)
+      warn_deprecation(__method__, 'Spree::Config[:track_inventory_levels]')


This is not right. Track inventory levels is a global setting that we don't use at all. Alternatives are in_stock? and stock_items.first.backorderable?.

mkllnk · 2018-09-07T08:06:18Z

lib/open_food_network/variant_stock.rb

@@ -125,5 +134,12 @@ def overwrite_stock_levels(new_level)
      # have only one stock location.
      stock_items.first.__send__(:count_on_hand=, new_level)
    end
+
+    def warn_deprecation(method_name, new_method_name)


I'm feeling a bit uneasy to introduce deprecation warnings without having a clear plan what else to use. We can refer to the stock items, but that may change again when we detailed the network feature. I would only deprecate the methods we are sure how to replace them.

Yes, stock items may change again in upcoming versions, but no network feature can come without us being at a stable version of Spree at least. So we will forsee changes before that feature comes.

I would only deprecate the methods we are sure how to replace them.

It's more or less clear now and we'll get more insights as we move forward with the upgrade. The only thing we know is that using methods that no longer exist in Spree for new features is not an option.

mkllnk · 2018-09-07T08:08:29Z

lib/open_food_network/variant_stock.rb

      raise_error_if_no_stock_item_available
+      raise_error_if_multiple_stock_items


Yes, I'm in favour of a constraint. That does make some of my earlier comments redundant.

sauloperez · 2018-09-10T17:06:29Z

Ok, so I renamed and moved it @luisramos0 . Aligned with what you suggested I moved it to concerns/ to follow Rails 4 convention (In Rails 4 there's a models/concern/ by default).

It's an old misconception that all stuff in models/ needs to be AR's models and all other abstractions we come up with should go into lib/. Any of these abstractions are also part of our domain logic and as such, it makes sense they also live under app/. This CodeClimate's blog post is a nice and short explanation of the topic.

Thoughts?

luisramos0 · 2018-09-10T22:50:32Z

Awesome! I will do the same on OrderShippingMethod.
For me it makes perfect sense, /lib for generic not-ofn specific stuff and /app for ofn stuff.

What I am concerned about is the difficulty of creating a service culture: business logic like this should not be in models or concerns (models are for ORM and data), it should be in services. I believe the best place for this would be /app/services, but it would have to be a service, not a concern.

sauloperez · 2018-09-11T06:20:56Z

To me, it is also less than ideal to have a concern (or name it mixin) rather than a service but this is here to 🔥 asap, so it feels good enough. Don't worry, the service culture is going to come sooner or later. I also have to convince myself sometimes 😅 .

sauloperez · 2018-09-19T13:10:43Z

Track inventory levels is a global setting that we don't use at all

is that explained/documented somewhere @mkllnk ?

Also, #in_stock?'s definition uses exactly that config.

openfoodnetwork/lib/open_food_network/scope_variant_to_hub.rb

Lines 25 to 29 in c4d4b52

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

luisramos0 · 2018-09-23T11:06:09Z

2 approvals. moving to test ready.
I think @Matt-Yorkley just offered some advise at one point, he was not planning to review the PR.
correct me if I am wrong.

luisramos0 · 2018-09-23T11:06:53Z

moving to ready to go actually, no tests here.

sauloperez · 2018-09-24T07:28:54Z

I followed @Matt-Yorkley 's advice but nothing relevant came out of it, at least for now.

sauloperez changed the base branch from master to 2-0-stable September 3, 2018 16:13

daniellemoorhead assigned sauloperez Sep 4, 2018

sauloperez added the pr-wip label Sep 4, 2018

sauloperez commented Sep 4, 2018

View reviewed changes

sauloperez removed the pr-wip label Sep 4, 2018

sauloperez force-pushed the unit-test-variant-stock branch 2 times, most recently from 0402200 to 8edff73 Compare September 4, 2018 14:32

luisramos0 reviewed Sep 5, 2018

View reviewed changes

luisramos0 approved these changes Sep 5, 2018

View reviewed changes

sauloperez force-pushed the unit-test-variant-stock branch from 8edff73 to 2cdcd22 Compare September 6, 2018 06:56

sauloperez force-pushed the unit-test-variant-stock branch from 2cdcd22 to 3066913 Compare September 6, 2018 09:28

sauloperez added 4 commits September 6, 2018 13:20

Add deprecation warnings for VariantStock methods

59883f9

This will prevent other devs from relying on this methods and will tell us the exact lines that call this methods, which will come in handy when removing this module.

Raise when a variant has more than a stock item

8848af1

This means we violated the business rules of having a single stock location for the OFN instance and hence a single stock item per variant. Although it is too late to preserve the data integrity we can know data needs to be cleaned up.

Replace exception with DB uniqueness constraint

cd53ec1

By forbidding more than a row per variant in the spree_stock_items we can ensure all variants have a single stock_item associated.

sauloperez force-pushed the unit-test-variant-stock branch from d4cfd51 to cd53ec1 Compare September 6, 2018 11:21

sauloperez commented Sep 6, 2018

View reviewed changes

mkllnk requested changes Sep 7, 2018

View reviewed changes

luisramos0 mentioned this pull request Sep 9, 2018

[Spree 2 Upgrade] - Added adapted order.shipping_method #2654

Merged

Fix typos in comments

8d8670b

sauloperez force-pushed the unit-test-variant-stock branch from cd648b0 to 26a988d Compare September 18, 2018 09:48

Move variant_stock.rb to concerns/

314ad54

sauloperez force-pushed the unit-test-variant-stock branch from 26a988d to 314ad54 Compare September 18, 2018 10:19

sauloperez added 2 commits September 19, 2018 15:12

Ask to use backorderable? instead of on_demand

0491a96

Merge branch '2-0-stable' into unit-test-variant-stock

99ba9d7

mkllnk approved these changes Sep 23, 2018

View reviewed changes

sauloperez merged commit 52a412a into openfoodfoundation:2-0-stable Sep 24, 2018

sauloperez deleted the unit-test-variant-stock branch September 24, 2018 09:14

		raise_error_if_no_stock_item_available
		raise_error_if_multiple_stock_items

Unit test variant stock #2638

Unit test variant stock #2638

Conversation

sauloperez commented Sep 3, 2018 • edited Loading

What? Why?

What should we test?

Release notes

daniellemoorhead commented Sep 4, 2018

sauloperez Sep 4, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

sauloperez commented Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

luisramos0 commented Sep 6, 2018

sauloperez commented Sep 6, 2018

mkllnk left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez Sep 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

sauloperez Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Sep 10, 2018 • edited Loading

luisramos0 commented Sep 10, 2018

sauloperez commented Sep 11, 2018

sauloperez commented Sep 19, 2018

luisramos0 commented Sep 23, 2018

luisramos0 commented Sep 23, 2018

sauloperez commented Sep 24, 2018 • edited Loading

sauloperez commented Sep 3, 2018 •

edited

Loading

sauloperez Sep 4, 2018 •

edited

Loading

sauloperez commented Sep 6, 2018 •

edited

Loading

sauloperez Sep 19, 2018 •

edited

Loading

sauloperez Sep 19, 2018 •

edited

Loading

sauloperez commented Sep 10, 2018 •

edited

Loading

sauloperez commented Sep 24, 2018 •

edited

Loading