Reset stock for absent products in DFC catalog

[mkllnk]

ℹ️ Funded Feature. Please track ALL ASSOCIATED WORK under the associated tracking code #11678 DFC Orders

What? Why?

• Closes [DFC Orders] Reset stock of absent products during catalog import #13190

During DFC product import, we now reset stock for absent products. The exact conditions are:

• The product belongs to the enterprise we are importing to.
• The product is linked to the catalog we are importing.
• The product does not appear in the catalog anymore.

Review notes

The pull request includes:

• Rescue from invalid DFC catalog URLs #13182

New commits start at: Reset stock for absent products in DFC catalog

What should we test?

1. Product import
   • Import from a DFC catalog, with stock or on demand.
   • Remove a product from the DFC catalog.
   • Import the catalog again.
   • The removed product should be reset locally so that it disappears from the shop.
     • If the product was on-demand, it should become stock-controlled
2. Order Cycle Open - Product sync
   • Import from a DFC catalog, with stock or on demand.
     • Note: the products must be imported to an enterprise that you are the owner of.
   • Remove the product from the DFC catalog.
   • Add imported product to a new order cycle and schedule it to open at the current time.
   • The removed product should be reset locally so that it disappears from the shop.
     • If the product was on-demand, it should become stock-controlled

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Follow-up

• Create a new issue to apply this logic when syncing at order cycle open (ref: [DFC Orders] Sync remote products when order cycle opens #13167)

[dacook]

Great, looks good to me.
I have to admit it took me a while to get my head around the logic, it's complicated! Well done.

I suggest a comment to explain the intent (mostly copied from your issue description)

Also what do you think about conditionally showing the reset_products message only when count.positive?

[mkllnk]

I suggest a comment to explain the intent (mostly copied from your issue description)

Thank you, done.

what do you think about conditionally showing the reset_products message only when count.positive?

Yes, that had crossed my mind and I almost did it yesterday. Now I added that condition.

I also added a preview of the products that will be reset. It's not perfect because I'm not passing on the variant ids of products to reset but that should be okay for now. This uncertainty would also occur during automatic import.

I think I have a new idea about how to go forward with this. During manual import, we could give the choice between resetting stock (default behaviour) or unlinking the variant. Once the semantic link is removed, the automatic import won't reset it anymore. But let's wait for someone to have a problem before we suggest a solution. 😉

[dacook]

Good idea, that makes things much more transparent. And it's really well explained too 💯

[rioug]

Looks good 👍

[RaggedStaff]

Further update following slack conversation...

I don't think we can just remove a product. If the product is back ordered, the OFN hub could be holding stock (for a mapped variant product). In that scenario, we want the hub to be able to sell the rest of the stock, but not back order more.

In other scenarios (for non-mapped products), we need to prevent any further sales... as the hub won't be holding any stock.

@mkllnk - Will your suggestion (to just amend the on demand flag) work for both of those scenarios? Do we need different logic, depending on whether the product has variant mappings or not?

[mkllnk]

Will your suggestion (to just amend the on demand flag) work for both of those scenarios?

Let's play it through.

Mapped variants, stock control

The variant becomes unavailable. The stock had been calculated from the wholesale stock but should now be reduced to zero.

Mapped variants, on-demand

The distributor has left-over stock from the last order cycle and is happy to sell that until sold out. The solution is to set the variant to stock-controlled but not touch the stock level.

Non-mapped products, stock control

The product becomes unavailable. We need to set the stock to zero.

Non-mapped products, on-demand

Since there was no mapping, we were able to backorder the exact amount we needed. The stock level should be 0 already. A positive stock level is only achieved if orders were cancelled and the backorder couldn't be adjusted (already complete). In that case we have left-over stock and the distributor may be happy to sell that?

General solution

When we have a product locally that is missing in the remote catalog, we need to distinguish between two cases:

• Stock-controlled: Our stock level is a cache of the remote stock level. Since the product vanished, we need to set the stock level to zero.
• On-demand: We track local stock and our local stock level is independent of the remote catalog. But we can't order any more. So we need to change the product from on-demand to stock-controlled (backorderable = false) and don't touch the stock level.

[RaggedStaff]

Mapped variants, stock control

The variant becomes unavailable. The stock had been calculated from the wholesale stock but should now be reduced to zero.

Not sure I agree with this case. If it's a mapped variant, I'm ordering cases, so the OFN stock reflects locally held stock right? 😕 Does it make a difference whether it's limited or not? Or is OFN tracking when to reorder wholesale variants somewhere else?

Mapped variants, on-demand

✔️

Non-mapped products, stock control

✔️

Non-mapped products, on-demand

✔️
I think I agree on those 3.

General solution

When we have a product locally that is missing in the remote catalog, we need to distinguish between two cases:

* Stock-controlled: Our stock level is a cache of the remote stock level. Since the product vanished, we need to set the stock level to zero.

* On-demand: We track local stock and our local stock level is independent of the remote catalog. But we can't order any more. So we need to change the product from on-demand to stock-controlled (`backorderable = false`) and don't touch the stock level.

I think those are the 2 scenarios, I'm not sure we're aligned on when to apply them. I think we need to determine when stock reflects locally held stock (for split wholesale cases) and leave that in place (but remove the on demand flag to prevent more back orders).

[mkllnk]

Mapped variants, stock control

The variant becomes unavailable. The stock had been calculated from the wholesale stock but should now be reduced to zero.

Not sure I agree with this case. If it's a mapped variant, I'm ordering cases, so the OFN stock reflects locally held stock right? 😕 Does it make a difference whether it's limited or not? Or is OFN tracking when to reorder wholesale variants somewhere else?

That's not how it works at the moment. If the variant in Shopify has limited stock then we just cache the stock level in OFN. We don't remember any local stock. We only remember local stock when the variant is set to on-demand, unlimited stock.

We don't have a way to track the remote stock level and remember left-overs from the last order cycle. We would need two stock levels for that while OFN has only one per variant.

This discussion is now getting out of the scope of this issue about resetting stock. I think it would be a bigger piece of work to introduce two stock levels.

[RaggedStaff]

That's not how it works at the moment. If the variant in Shopify has limited stock then we just cache the stock level in OFN. We don't remember any local stock. We only remember local stock when the variant is set to on-demand, unlimited stock.

Ok. So if I have a Product with mapped variants & variants have limited stock, OFN doesn't set it to On Demand ? 😕 I'm really confused - how does OFN handle mapped variant stock levels, if they're not unlimited ? Or are we not handling that yet ? (I checked & Hod's don't have any on the FDC export atm 😅 )

We don't have a way to track the remote stock level and remember left-overs from the last order cycle. We would need two stock levels for that while OFN has only one per variant.

So, we can't track the remote stock level, but if it's unlimited we are tracking local stock levels, is that correct ?

This discussion is now getting out of the scope of this issue about resetting stock. I think it would be a bigger piece of work to introduce two stock levels.

Agreed, and I hope we don't need to.

[mkllnk]

how does OFN handle mapped variant stock levels, if they're not unlimited ? Or are we not handling that yet ?

We don't handle that yet.

[mkllnk]

if it's unlimited we are tracking local stock levels, is that correct ?

Correct.

[RaggedStaff]

Thanks @mkllnk.

I think this makes sense now & it's the most sensible course for now.

1 final follow up question - if OFN was to handle limited stock of mapped variants, how would you go about it (understanding that will be a separate issue) ?

[dacook]

Great explanation 👍

[filipefurtad0]

Garethe provided feedback via Slack, I should be able to proceed now 👍

[filipefurtad0]

Hey @mkllnk @dacook @RaggedStaff ,

On the proposed test cases:

Product Import

i) If a product is set to on demand (with on_hand = 0) and another one is stock controlled (on_hand = 2), then:

Removing these two products from the catalog on Shopify, and re-importing the catalog:

This removes the products from the shopfront, in both cases, the available stock is zero.

ii) If a product is set to on demand (but has available stock which is non-zero, in the example below, on_hand = 222) and another one is stock controlled (on_hand = 2):

then, repeating the steps above (removing the products from the DFC catalog, and re-importing them):

• for the product set to on demand, but with on_hand = 222 -> the product gets stock controlled, and since it has non-zero available stock, it is not removed from the shopfront. It becomes stock controlled.
• for the product set to on_hand = 2 -> it is removed from the shopfront, as the on_hand is now zero.

Order Cycle Opening (Product Sync)

The outcome is the same, on product sync, by opening of an order cycle, i.e.,:

• for the product set to on demand, but with on_hand = 222 -> the product gets stock controlled, and since it has non-zero available stock, it is not removed from the shopfront. It becomes stock controlled.
• for the product set to on_hand = 2 -> it is removed from the shopfront, as the on_hand is now zero.

So, this looks good to merge?

I've noticed a minor regression though, but it is only affecting the UI, and not the import functionality itself:

I've noticed that on product import, the number of successfully imported products is not displayed, as it used to.

After this PR, importing 10 products, displays:

Before this PR:

[dacook]

Great news, thanks Filipe. I'll have a look at that regression tomorrow.

[dacook]

It took me an embarrassingly long time to figure out what was happening there. I discovered that the count variable has been removed in the translations. I'm not sure why, will ask in #translations

[dacook]

As this is an existing translation, the old translation (without count variable) is already present in about 11 locale files. This results in the @count variable no longer printing:

I've noticed that on product import, the number of successfully imported products is not displayed, as it used to.

#13191 (comment)

Ideally, I think we would announce to translators to re-translate (maybe through transifex if there's a feature for that). Or create a different key.
But given that it's going to display exactly as it was before, I will go ahead and update the locale files to resolve this issue.

[filipefurtad0]

Thinking about this now, I'm not sure why the spec ./spec/system/admin/dfc_product_import_spec.rb:20 did not catch this regression... It fails on my local system (before you've fixed it @dacook ).

  imports from given catalog (FAILED - 1)

Failures:

  1) DFC Product Import imports from given catalog
     Failure/Error: expect(page).to have_content "Imported products: 1"
       expected to find text "Imported products: 1" in "Logged in as : [email protected] Account Logout Store Dashboard Products Reports Enterprises Products Inventory Import DFC product catalog import Imported products:"

[dacook]

Gaetan are you ok with the final commit? I think it should be ok but it would be good to have another sense check.

[rioug]

Gaetan are you ok with the final commit? I think it should be ok but it would be good to have another sense check.

Yeah it should be good.

[filipefurtad0]

Hey @dacook ,

I see your latest commit only changes the locales, so I did not perform additional tests on the functionality introduced on this PR, just on how the import page is rendered.

I think changing the en.yml file would have triggered change requests on Transifex, so I think this would have been an option, instead of changing all locales yourself (right? Am I missing something?). But I've tested two of them on staging-FR, and I think we're good:

• en_FR.yml

• fr.yml

Merging, thanks for addressing this.