Openfoodnetwork: Hidden "count on hand" of on demand variant should never be relevant

[kristinalim]

The producer "count on hand" of an "on demand" variant should never be relevant. But this is used when the variant override has false "on demand" and blank "count on hand".

Description

When the variant override has false "on demand" and a blank "count on hand", the stock check looks at the producer's "count on hand" setting. But this value is no longer visible in the producer's UI when the producer's setting for "on demand" is checked. This became a problem in #3058.

See the table below. In scenario 4, the producer's "count on hand" setting is used when checking the stock even if it is supposed to be "on demand".

| | VO | VO | Supplier | Supplier | Final | Final |
|-------|-------|-------|-------|-------|-------|-------|
| | On Demand | On Hand | On Demand | On Hand | On Demand | On Hand |
| 1 | false | 10 | false | 20 | false | 10 |
| 2 | false | 10 | true | 20 | false | 10 |
| 3 | false | nil | false | 20 | false | 20 |
| 4 >> | false | nil | true | 20 | false | 20 |
| 5 | nil | 10 | false | 20 | false | 10 |
| 6 | nil | 10 | true | 20 | false | 10 |
| 7 | nil | nil | false | 20 | false | 20 |
| 8 | nil | nil | true | 20 | true | nil |
| 9 | true | 10 | false | 20 | true | nil |
| 10 | true | 10 | true | 20 | true | nil |
| 11 | true | nil | false | 20 | true | nil |
| 12 | true | nil | true | 20 | true | nil |

The code used to generate this table is in this comment.

Expected Behavior

The producer's "count on hand" setting should never be used when the producer's setting for "on demand" is true.

Actual Behavior

This is not the case when the variant override has false "on demand" and a blank "count on hand".

Steps to Reproduce

See same section in #3066.

Context

Related to #3058.

Severity

Some users might not realize that their inventories are using the 0 "count of hand" of the producer, like what happened in #3058. This can result to variants unintentionally being hidden from the shopfront.

bug-s2

Possible Fix

PENDING

[kristinalim]

As @luisramos0 suggested, I outlined intents behind different VO settings.

Intent 1: Use producer stock settings
Currently, achieved by leaving VO "on demand" and "count on hand" untouched.

| | VO | VO | Supplier | Supplier | Final | Final |
|-------|-------|-------|-------|-------|-------|-------|
| | On Demand | On Hand | On Demand | On Hand | On Demand | On Hand |
| 7 | nil | nil | false | 20 | false | 20 |
| 8 | nil | nil | true | 20 | true | nil |

Intent 2: Force the variant to be on demand
Currently, achieved by ticking VO "on demand". Any value in "count on hand" is ignored.

| | VO | VO | Supplier | Supplier | Final | Final |
|-------|-------|-------|-------|-------|-------|-------|
| | On Demand | On Hand | On Demand | On Hand | On Demand | On Hand |
| 10 | true | 10 | true | 20 | true | nil |
| 12 | true | nil | true | 20 | true | nil |
| 9 | true | 10 | false | 20 | true | nil |
| 11 | true | nil | false | 20 | true | nil |

Intent 3: Use hub-specific stock level
Currently, achieved by setting the VO "count on hand", as long as VO "on demand" is unticked.

| | VO | VO | Supplier | Supplier | Final | Final |
|-------|-------|-------|-------|-------|-------|-------|
| | On Demand | On Hand | On Demand | On Hand | On Demand | On Hand |
| 1 | false | 10 | false | 20 | false | 10 |
| 5 | nil | 10 | false | 20 | false | 10 |
| 2 | false | 10 | true | 20 | false | 10 |
| 6 | nil | 10 | true | 20 | false | 10 |

I can't imagine any use case where the shop user would specify in the VO to use limited stock but not actually have any specific stock level in mind. @myriamboure Is this correct? If I'm right, then we don't need to support the following scenarios:

| | VO | VO | Supplier | Supplier | Final | Final |
|-------|-------|-------|-------|-------|-------|-------|
| | On Demand | On Hand | On Demand | On Hand | On Demand | On Hand |
| 3 | false | nil | false | 20 | false | 20 |
| 4 >> | false | nil | true | 20 | false | 20 |

Our immediate concern is that the user does not encounter scenario 4, so I think a quick solution is to:

• Require users to enter specific VO stock level if they force limited stock, with false "on demand".
• Identify per instance which existing VOs are affected by scenario 4, and possibly inform the hub managers.

Then we can further simplify the logic but at lower priority. What do you think, @luisramos0 ?

[kristinalim]

Link to discussion on Slack: https://openfoodnetwork.slack.com/messages/CDLKH9MM0/convo/CDLKH9MM0-1542404863.056000/

[Matt-Yorkley]

Scenario 3 seems fine. I agree with you on scenario 4. So if the variant has on_demand: true, and the VO has on_demand: false, the user has to set a stock level. Seems reasonable.

[kristinalim]

To be clear, my proposal is that we prevent scenario 3 too, so we don't have to deal with the possibility of the producer changing on_demand from false to true which could make existing VOs invalid.

So, if the VO has on_demand: false, the user has to set a stock level.

Thanks, @Matt-Yorkley!

[luisramos0]

Having different behaviour for nil and false is bad, can we make these work in the same way?

We can call it "VO on_demand checkbox checked" (true) or "VO on_demand checkbox unchecked" (both nil and false)

• if VO has on_demand checked, VO will be on_demand (scenario 2)
• if VO has on_demand unchecked, and VO.on_hand filled, VO stock will be used (scenario 3)
• if VO has on_demand unchecked, and VO.on_hand empty, variant on_demand and variant on_hand will be used normally (scenario 1)

What I am proposing is just to make cases 3 and 4 work as 7 and 8. If on_demand unchecked and on_hand is empty, use producer's variant.on_demand and variant.on_hand.

[kristinalim]

Right now, we are able to save any "on demand" and "count on hand", even if they are not compatible.

@myriamboure What do you think about adding validation (not just in the UI level) to require variant overrides to be configured like below. It would significantly reduce complexity, because then we'd no longer have to support the other possible combinations.

| On Demand | Count on Hand |
|-|-|
| Use producer stock settings | should be blank |
| Yes | should be blank |
| No | should be set |

If we do this, we would need to transform existing VOs so they still pass the new validation rules. But it is straightforward:

| On Demand | Count on Hand | |
|-|-|-|
| Use producer stock settings | set | Current: We use this count on hand and consider it as having limited stock. Update records: Set on demand to "No". |
| Yes | set | Current: We disregard this count on hand. Update records: Clear the count on hand of these VOs. |
| No | blank | Current: We use the count on hand of the producer. Update records: Set the count on hand of these VOs to the current producer count. |

I actually already prepared this for #3104, but realized late that it doesn't belong to the same PR. I will open a separate PR if everybody is okay with this.

[luisramos0]

Sounds good, I am not sure about the Update Records part of this very last case.
An alternative would be Update records: Set on demand to "Use producer stock settings"

[myriamboure]

@kristinalim I understand the logic.
I disagree thought to some of the actions you propose. Today when you put on demand for a product, the stock on hand remains, but is just not used. It is disturbing if the behavior is opposite on inventory ;-)
So when we can do is:

If On Demand = Use producer stock settings, AND count on hand = set
Current: We use this count on hand and consider it as having limited stock.
Update records: We need to do the contrary, in UX we need to update count on hand, not on demand setup.

• Case one: the producer in its catalog has set up "on demand", but have left some count on hand value. It is possible today in the product catalog to setup a count on hand value AND have "on demand" set up (then on demand is read and not stock on hand). Today if you create a product, clear the count on hand and setup "on demand", there is an error which prevent to save, you need to put some stock in hand, 0 or something else... but maybe it doesn't concern data and data can be cleared...? Actually in some view afterward I see "infinite" in count on hand when "on demand" is set up so I am confused.
  Anyway, ideally I would change something in the original product catalog setup, if in this product catalog "on demand" is set up, then count on hand should be cleared and "infinite" written in count on hand. In that case, if "use producer stock info" is selected in inventory (which is default when you import a new product), the "count on hand" would show "infinite" (in grey to know it is original info from producer) so the user would know that the producer has setup "on demand". Same when she selects back "use producer stock info" afterwards, today she can't know as far as I remember.
• Case two: the producer in its catalog has set up a count on hand value and not "on demand"
  Then if I select "Use producer stock info" in on demand, if the producer stock info is that count on hand is X, we should write back X in the count on hand box ! In grey, as it is when you see the original producer info before you decide if you use it or set up your own.

If On Demand = Yes, AND count on hand = set
Current: We disregard this count on hand.
Update records: Clear the count on hand of these VOs. AND write "infinite" (like above, but not grey as it's not producer catalog info, it's override).

If On Demand = No, AND count on hand = set
Current: We use the count on hand of the producer.
Update records: Set the count on hand of these VOs to the current producer count, in greyed, so the user can see what is in producer catalog and decide if she source her stock from producer catalog or set up a distinct stock level.

Is that clear ?

[kristinalim]

@myriamboure Sorry, what I said might have been confusing. I think our goal for how this will work is the same, and this is already mostly how #3104 makes the Inventory page work.

When I said "clear the count on hand" and "update records", I was referring to the value stored in the database. While this DB value is blank, the user can still see the COH placeholder (which is just the gray text background for the field, like a hint, not the field value itself) which reflects the producer stock settings.

My intention in this issue is to get rid of the confusing OD and COH combinations for the variant overrides (not the producer settings, which I think we don't need to tackle now).

Because, for example, I don't think we should allow the user to set a COH of 10 but say "Use producer stock settings". The website treats this as limited stock even if the producer setting says "On Demand". In this case, we should require that the record says "No" for OD as well, and we could set all existing VOs like this to have "No" for OD because they behave in the same way anyway.

Same thing when VO OD is "Yes" but the COH was set. This COH is ignored anyway, so we shouldn't allow it to be saved in the first place. The updated UI in #3104 no longer makes this possible, but there are still existing records with these settings. So we can clear the COH of these existing records, and the user will only see "On demand" placeholder in the COH field.

Am I correct that we actually want this to work similarly?

These are the COH placeholders that we show in , which I also posted here:

| Producer | Producer | VO | VO |
|-|-|-|-|
| On Demand | COH | On Demand | COH Placeholder |
| Yes | blank | Use producer settings | "On demand" |
| Yes | 20 | Use producer settings | "On demand" |
| Yes | blank | Yes | "On demand" |
| Yes | 20 | Yes | "On demand" |
| Yes | blank | No | blank |
| Yes | 20 | No | blank |
| No | 20 | Use producer settings | 20 |
| No | 20 | Yes | "On demand" |
| No | 20 | No | blank placeholder, but we pre-fill the field with 20 and the user can edit |

[myriamboure]

Maybe I don't understand clearly what you mean by placeholder, but there is still something a little bit unclear, and we need to distinguish the final values and migration question.

To be sure I understand, let me add one column to the table about "what is the value taken into account by the system in COH field.
What is (black) is override, action written in inventory, what is (greyed) is not override, info taken from producer catalog, is it is today.

| Producer | Producer | VO | VO | VO |
|-|-|-|-|-|
| On Demand | COH | On Demand | COH Placeholder | COH value read |
| Yes | blank | Use producer settings | "On demand" (greyed) | unlimited |
| Yes | 20 | Use producer settings | "On demand" (greyed) | unlimited |
| Yes | blank | Yes | "On demand" (black) | unlimited |
| Yes | 20 | Yes | "On demand" (black) | unlimited |
| Yes | blank | No | blank | 0 |
| Yes | 20 | No | blank | 0 |
| No | 20 | Use producer settings | 20 (greyed) | 20 |
| No | 20 | Yes | "On demand" (black) | unlimited |
| No | 20 | No | blank placeholder, but we pre-fill the field with 20 and the user can edit SO 20 (greyed) | 20 |

Let's take case
| No | 20 | Use producer stock info | 20 (greyed) | 20 |
I'm in the inventory, the set up is like this, and I start typing 40 in the COH field. It appears in black, and automatically it would change the OD list value to "no". Else we would have inconsistent records. I can use producer stock info and have a different stock info at the same time ? Is that how you see it ? BUT the thing that is slightly annoying conceptualy, is that the "use producer stock info" only concerns OD... as OD is "no" in producer info, it's not conceptually unconsistent to have
| No | 20 | Use producer settings | 40 (black) | 40 |
But it is unconsistent on UX side...

You see what I'm a bit puzzled with ?

Because, for example, I don't think we should allow the user to set a COH of 10 but say "Use producer stock settings". The website treats this as limited stock even if the producer setting says "On Demand". In this case, we should require that the record says "No" for OD as well, and we could set all existing VOs like this to have "No" for OD because they behave in the same way anyway.

YES ! I think that's a bit the case I explain above in fact... so we can agree on a convention, to preserve UX consistency, that is COH is overriden, OD is automatically switched to "no" even if conceptually could remain "use producer setting"

Also about migrations:

• all cases where there are VOs will have specific rules, like: if stock info has been overriden, on demand will be setup to "no", etc. We will need to manually take back the products we have forced as on demand to "use producer stock info" as they will automatically be in "yes" when we migrate.
• cases where there there has not been overrides at all, and default value when you import a new product, OD will be set to "user producer setting".

If all what I said makes sense, I think we are on the same page and you can just go ahead :-) Thank you so much @kristinalim for taking so much care in finding precise solutions :-) it's really reasuring to work with you as a PO :-)

[myriamboure]

I hesitate about the last line on the table, should it be 0 or 20 which is read ?
If I say "I don't want to use the producer stock info" (implict = for on demand, but slightly unclear in user mind), shoulnd't we just have "blank" and not pre-filled with 20, and read 0 ? It mlight be more logical actually... do you agree @kristinalim ?
i think we need either to:

• leave it blank and read 0
• pre-fill with 20 and read 20

Else it will be unconsistent with other behavior for other prefilled SOH on the page...

[kristinalim]

@myriamboure Yes, we I agree with what you said above, so we are on the same page. :slightly_smiling_face:

@luisramos0 and @myriamboure I also got quite confused about the third case after @luisramos0 brought it up.

The stock tracker decrements the COH in the producer's counter. So yes, I was wrong, we could not simply copy the counter of the producer, because then this distributor will not share the counter with other distributors in the producer's network.

@myriamboure I like the suggestion of @luisramos0 which seems to preserve existing behaviour and intent. For VO with "No" for OD but with blank COH, if we set this to "Use producer's stock settings":

• When producer OD is currently "No" - This makes sense because we currently use the producer's stock counter anyway. And as mentioned in #3066, both "No" and "Use producer's stock settings" were represented by unticked checkbox, so the latter could have even been the user's intent.
• When current producer OD is currently "Yes" - This currently uses the producer's stock counter, which the producer does not see in all UIs but can change when toggling the OD. After update the existing data, the VO with "Use producer's stock settings" would now be considered OD, which behaves like a VO with limited stock but with a very high producer COH, which was the intent of the producer when setting to OD.

@myriamboure Are you okay with this?

[myriamboure]

Hum... not sure I understand, so I'm tempted to trust your judgment @kristinalim ...
When you say "when producer OD is currenty "No" it could have been "user producer stock setting", what I see is that today in inventory, any entry that has not been forced to "on demand" (override = yes) is:

• "user producer stock info" if no COH have been overriden
• "no" if a COH has been overriden
  So I agree that if there is no COH overriden, we shouldn't consider uncheck case as a "no", but as a "user producer stock info".
  I think that's what you said, so I think we are on the same page.

[kristinalim]

Adding the code I used to confidently generate the table in the issue description here, for the reference of those doing code review.

variant = Spree::Variant.new
variant_override = VariantOverride.new

def set_stock(object, on_demand, count_on_hand)
  object.on_demand = on_demand
  object.count_on_hand = count_on_hand
end

variant.extend(OpenFoodNetwork::ScopeVariantToHub::ScopeVariantToHub)
variant.instance_variable_set :@variant_override, variant_override

variant_configurations = [
  [true, 20],
  [false, 20]
]
variant_override_configurations = [
  [nil, nil],
  [true, nil],
  [false, nil],
  [nil, 10],
  [true, 10],
  [false, 10]
]

results = [["VO OD", "VO OH", "V OD", "V OH", "OD", "OH"]]

variant_configurations.each do |variant_conf|
  set_stock(variant, *variant_conf)

  variant_override_configurations.each do |variant_override_conf|
    set_stock(variant_override, *variant_override_conf)

    results << [
      *variant_override_conf,
      *variant_conf,
      variant.on_demand,
      (variant.count_on_hand unless variant.on_demand)
    ]
  end
end

results.sort_by(&:to_s).each_with_index do |result, i|
  puts "|#{([i] + result).map { |value| " #{value.inspect} |".rjust(8) }.join}"
end

[kristinalim]

Posted by @mkllnk in this comment:

Yes, that is one of the two scenarios I mentioned. I'm not 100% sure, but I think the effect is a bit different than what you describe. We are talking about this scenario:

variant.on_demand = true
variant.count_on_hand = x
variant_override.on_demand = false
variant_override.count_on_hand = nil

Changes to the variant override don't affect the producer view at all. But the shop would see a variant with on_demand: false and count_on_hand: x. The shop would be using the producer's invisible stock level. Your solution above is:

variant_override.on_demand = nil

That results in the shop seeing on_demand: true and count_on_hand: x resulting in on demand being active. That is a change in stock levels for the shop. Instead, I would set the stock level:

variant_override.count_on_hand = variant.count_on_hand # = x

Aside from this scenario, do you see any other situation when the behaviour would change?

Yes, there is the second scenario I mentioned before:

variant.on_demand = true
variant.count_on_hand = x
variant_override.on_demand = nil
variant_override.count_on_hand = y

The shop sees on_demand: true and count_on_hand: y resulting in on demand. Your solution was:

variant_override.on_demand = false

The result for the shop would be on_demand: false and count_on_hand: y. Suddenly there would be a stock level for variants that were on demand. I would propose:

variant_override.on_demand = variant.on_demand

Does this make sense to you? Am I missing something?

[kristinalim]

With this scenario:

variant.on_demand = true
variant.count_on_hand = x # filled
variant_override.on_demand = false
variant_override.count_on_hand = nil

The Inventory page actually shows "On demand":

But the shop uses the producer's invisible COH.

As have been discussed, there are two ways to address this bug:

1. Set override's COH to producer's invisible COH
   
   
   
   • Pro/Con: Use the current stock count that is being used. If producer's COH is currently 0, that means the product hasn't been available in the shop. Doing this would mean it still won't be available. But the distributor can change this in the Inventory page.
   
   
   • Con: The stock count used to be shared among all distributors of the same product, now the stock count is only in the distributor level. It's not technically the same, but the producer doesn't see this value anyway so I think it's okay.
   
   
2. Change to "Use producer stock settings"
   
   
   
   • Pro: This could be what the distributor really intended, there was just no way to set "On Demand" back to its original state nil before.
   
   
   • Pro: The field was showing "On demand", so in the view of the distributor from the Inventory page, this product really has been available.
   
   
   • Con: If there is a product that used to be missing from the shop because producer COH has been 0, this would suddenly be available in the shop now. What if the product is truly no longer available? The distributor could be surprised with orders of the product. But the distributor should have changed the stock level in their Inventory page anyway, so maybe it's okay.
   
   

Because Option 1 seems safer, I'm switching back my vote to this, with the addition of sending a notice to enterprise managers to check their inventory settings.

@myriamboure @luisramos0 @mkllnk Kindly share your thoughts.

[mkllnk]

Thank you for this detailed description. I hadn't considered the 0 stock case yet. I would still vote for option 1, because it keeps the shop display the same. No surprise orders we had trouble with in the past. It also emphasises how important the spread sheets will be. This will be a good way to fix a bug and communicate with our shops how it works. They can then set it up as they wanted to set it up originally.

[kristinalim]

I forgot to talk about the second scenario:

variant.on_demand = true
variant.count_on_hand = x
variant_override.on_demand = nil
variant_override.count_on_hand = y

@mkllnk This is wrong:

The shop sees on_demand: true and count_on_hand: y resulting in on demand.

What happens is the shop treats the variant as having limited stock. Testing with VO COH of 0 removes the variant from the shop, and VO COH of 1 lists it.

So I would proceed with setting variant_override.on_demand = false.

[myriamboure]

Hum... in term of UX I don't think option 1 is acceptable, as most shops actually don't manage any inventory and this case apply to all of them. It means we would force them to start managing an inventory... which is not acceptable IMO.

I would rather go for option 2, which is what was the intent of the distributor with this setup. If the don't manage inventory (have not changed anything from producer catalog on this item basically) they don't even know today there is an error in a product not being displayed when it should be... so I would definitely go for option 2.

@Kristinalim @mkllnk

[mkllnk]

@kristinalim

What happens is the shop treats the variant as having limited stock. Testing with VO COH of 0 removes the variant from the shop, and VO COH of 1 lists it.

So I would proceed with setting variant_override.on_demand = false.

Thank you for verifying that. It's a very confusing behaviour, probably to the bad implementation. Your solution sounds right. For affected enterprises we should probably create two files:

1. The special case discussed where the admin view or the shop view (option 1 or 2) changes. All variants should be listed so that the shop can adjust if needed.
2. All other cases that should be automatically fixed without any change of behaviour. All variants listed in this file are just in case something is not working as expected or the shop would like to verify that everything worked.

[kristinalim]

@myriamboure I'm inclined to follow your opinion because you have a better relationship with the users, but just throwing this question before I make changes again.

About Option 1, it's not clear to me what you mean with "start managing". Were you saying this would be one-time or continuous? You said:

It means we would force them to start managing an inventory... which is not acceptable IMO.

The user could just change the setting back to "Use producer stock settings", as a one-time action after the release, if this is actually what they want. And whatever their intent, they are already aware of the Inventory page. They even clicked the checkbox before - if they didn't, "On Demand" would have been nil ("Use producer stock settings") not false ("No").

So because this follow-up step seems simple enough to me, I like Option 1 because it's safer and IMO easy enough to update to follow the users' intent.

Let me know if Option 2 is still more convincing and I will remove just one change from the PR.

Thanks for the patience! Sorry for so much back and forth.

[myriamboure]

I didn't understand that the user had already clicked at least once on the on demand check box and then unchecked. I'm not sure it changes my opinion, as if they unchecked it and have not setup a COH they do want to follow producer stock, don't they ?

• if they manage themselves the producer catalog they would have put the COH there, eventually they would have left on demand and manage in OC if they put the product in the OC or not
• if they don't then they definitely want the real value from the producer catalog to be used !

So in term of UX @kristinalim I would still go for option 2. Safer doesn't make the most sense in the case IMO. If you think it deserve a second look we can ask some other PO's view ;-) It took me some time to get into the problem and understand fully though so not sure the potential impact deserves it... ;-)

What could be the worse impact of option 2 ? As you said, it would that some product reappear in the shop of a distributor unexpectedly, but I don't think it will happen because most of the time the producer is on demand and distributor manage if stock or not in the shop at OC level. And if it does happen that's what the shop wants as it means they want the real quantity from farmer to be the source... so your con of option 2 is for me a pro in fact...

[kristinalim]

Okay, understood. :slightly_smiling_face: I will update the PR. Thanks for your quick reply @myriamboure!