Place backorders for linked products via DFC integration #12856

mkllnk · 2024-09-11T04:26:55Z

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

What? Why?

Closes [DFC Orders] Create/update order when order is placed within OFN #12178
Closes [DFC Orders] On order cycle close finalise order in another platform #12179

What should we test?

Use a server with access to the OIDC provider Les Communs (UK & FR staging).
Go to Admin, Enterprises, OIDC settings to set up your account. Be aware that only one OFN user can be connected to one DFC user. So maybe just use the testdfc user.
Go to Admin, Product Import and import DFC products from https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts.
Add retail variants from this catalog to an order cycle.
Your user must be the owner of the distributor.
Order from that order cycle.

In Shopify (https://admin.shopify.com/store/test-hodmedod):

A new draft order should be placed after a few seconds.
The ordered items should be the wholesale variants of the ordered items.
Ordering more in OFN should add to the draft order with a few seconds delay.
Four hours after the order cycle finishes (this time needs to be set before the first order is done), the draft order should complete.
Cancelled orders should be taken into account and the final order should have reduced quantities.

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

Documentation updates

rioug

What a journey ! I am glad I have some background knowledge on the project 🫘

I think it looks good ! 👏
I left a few comments that should help other reviewer and some suggestion to clarify a few things.

engines/dfc_provider/app/services/dfc_request.rb

app/jobs/backorder_job.rb

rioug · 2024-09-11T05:46:05Z

spec/services/fdc_backorderer_spec.rb

+    it "completes an order", vcr: true do
+      backorder = subject.find_or_build_order(order)
+
+      expect(backorder.semanticId).to match %r{^https.*/[0-9]+$}
+      expect(backorder.lines.count).to eq 1
+
+      subject.complete_order(order, backorder)
+
+      remaining_open_order = subject.find_or_build_order(order)
+      expect(remaining_open_order.semanticId).not_to eq backorder.semanticId
+    end


~~Shouldn't that be under it's own describe block ? You are testingcomplete_order aren't new ?~~
It will be addressed later on

app/jobs/backorder_job.rb

rioug · 2024-09-11T06:21:10Z

app/services/fdc_offer_broker.rb

@@ -2,13 +2,23 @@

 # Finds wholesale offers for retail products.
 class FdcOfferBroker
+  Solution = Struct.new(:product, :factor, :offer)


If we were on ruby >=3.2 I would have suggested to use Data instead of Struct here : https://ruby-doc.org/3.2.5/Data.html

Interesting, I didn't know about that thanks!

dacook

Well done! It was a long journey with a lot to work through, but it looks like it's near to completion now. I love how it brings together a complex standard and proves that it's possible to use in the real world.

I see three TODOs; perhaps you're planning to work on those still.
Still, I think this looks good to me. My brain was mush for the last few commits but it mostly seems to make sense 👍 (apart from the few questions I have!)

engines/dfc_provider/spec/services/dfc_request_spec.rb

app/jobs/backorder_job.rb

spec/services/fdc_backorderer_spec.rb

spec/jobs/backorder_job_spec.rb

app/jobs/backorder_job.rb

dacook · 2024-09-13T05:18:31Z

app/services/fdc_offer_broker.rb

@@ -2,13 +2,23 @@

 # Finds wholesale offers for retail products.
 class FdcOfferBroker
+  Solution = Struct.new(:product, :factor, :offer)


Interesting, I didn't know about that thanks!

app/services/fdc_offer_broker.rb

app/models/spree/variant.rb

dacook · 2024-09-13T06:03:29Z

app/jobs/complete_backorder_job.rb

+      line.quantity = line.quantity.to_i - wholesale_items_contained_in_stock
+
+      retail_stock_changes = wholesale_items_contained_in_stock * transformation.factor
+      linked_variant.on_hand -= retail_stock_changes


I have to confess my brain didn't quite make it to the end of the PR before turning into mush.
It seems that we're adjusting the backorders relative to what they were before. Would it not be more robust to look at all current retail orders and re-calculate the backorder requirement from scratch?
Hmm.. no we can't because we don't know how much of our "on_hand" stock was from a backorder, unless we look at the backorder.

I can't work out if that's a problem or not 🤯

Yeah, it's a tricky one. I would also favour recalculating from scratch, but the use of OFN stock is so much more helpful/flexible for this one (we've already encountered an edge case for one of our pilot hubs that will need to utilise that)...

..so long as everything always works, I think we're good! 🫣

RaggedStaff · 2024-09-15T19:23:29Z

app/jobs/backorder_job.rb

+    user = order.distributor.owner
+
+    # We are assuming that all variants are linked to the same wholesale
+    # shop and its catalog:


I'm happy with that assumption (for now 😉 ), and it will be true for the pilots we're running.

RaggedStaff · 2024-09-15T19:29:52Z

app/services/fdc_backorderer.rb

+    return if open_orders.blank?
+
+    # If there are multiple open orders, we don't know which one to choose.
+    # We want the order we placed for the same distributor in the same order


Ok. I'm pretty sure the Shopify Orders endpoint will display old (completed orders), we should only have 1 Draft order though.

The Shopify Hub app is storing the ID of the Order returned from the initial POST request, to utilise in future PUT requests. Could we do that ?

We could. And maybe we need to. I tried to get around it to have minimal local data. Local data structures increase maintenance and can become out of sync. And I was worried about longer dev times as well. But I agree that the current solution isn't great.

RaggedStaff · 2024-09-15T19:31:40Z

app/services/fdc_backorderer.rb

+    #
+    # For now, we just guess:
+    open_orders.last.tap do |order|
+      # The DFC Connector doesn't recognise status values properly yet.


@mkllnk could you elaborate on what you mean by "DFC Connector doesn't recognise status values properly"? 😟

I thought this was sorted & we haven't had any issues with the TS Connector. @lecoqlibre do we need to update the Ruby Connector?

Well, we can load the vocabulary with order states. The Connector doesn't have a built-in method for that but I did make it work in a separate branch. I could work on a pull request to make this part work with the importer. I was just shying away from the effort to meet the deadline. The current version is also not recognising the transformation links. That may just be the outdated context though. We are still on 1.8, I think. And I wasn't keen to follow the updates closely because you discovered bugs here and there. Happy to give it another go though.

The simplified API was only returning the response body, not allowing us to inspect if an error occurred. Since an error should be an exception when communicating with a standardised protocol, we raise an error and keep our simple API.

Apparently, the FDC implementation uses those dates to finalise orders.

The job class is getting too big.

We were already importing stock levels from offers but now we are looking at catalog items as well.

This was abandoned when the checkout was re-designed. But I want to refactor the order locking mechanism and it would be good to know that I don't break anything.

From Sidekiq's view, the job is successful when we rescue an error and it will discard it. But we want the option to inspect the job and retry it. Failing jobs are also reported to Bugsnag automatically. I didn't specify `retry: false` because that discards the job as well. But `retry: 0` should sent it straight to the dead set. No automatic retries but it's treated like a failed job.

dacook

Fantastic! I have to admit I don't fully follow the web of semantics, but everything has been put together with clear and comprehensive steps, well done.

As you said, it shouldn't affect any existing functionality (from memory, any changes in the app code are very minor and should be covered by specs anyway).

So I think this is good to merge, ready to test in real life 👍

dacook · 2024-09-26T11:35:04Z

engines/dfc_provider/app/services/supplied_product_builder.rb

+    filename = File.basename(supplied_product.image)
+
+    Spree::Image.new.tap do |image|
+      PrivateAddressCheck.only_public_connections do


So glad There's A Gem For That.

dacook · 2024-09-26T11:37:48Z

app/models/spree/variant.rb

@@ -240,6 +240,8 @@ def set_cost_currency
    end

    def create_stock_items
+      return unless stock_items.empty?


Not covered directly by spec, but not a big deal.

dacook · 2024-09-26T11:40:39Z

engines/dfc_provider/app/services/catalog_item_builder.rb

+      variant.stock_items << Spree::StockItem.new(
+        stock_location: DefaultStockLocation.find_or_create,
+        variant:,
+      )


I wonder if the variant model should be doing this part.

dacook · 2024-09-26T11:43:46Z

engines/dfc_provider/spec/services/supplied_product_builder_spec.rb


    it "creates a new Spree::Product and variant" do
+      # We need this to save stock:
+      DefaultStockLocation.find_or_create


I'm probably missing something, but it looks like this is already performed in CatalogItemBuilder. but maybe it is needed before that.

dacook · 2024-09-26T11:49:25Z

spec/requests/checkout/concurrency_spec.rb

+    order.next # => address
+    order.next # => delivery
+    order.next # => payment
+    order.next # => confirmation


It might not hurt to expect order.state to be confirmation, if perhaps the states change in the future.

Maybe we should be using Orders::WorkflowService#next to better replicate the checkout process ?

dacook · 2024-09-26T11:51:20Z

spec/requests/checkout/concurrency_spec.rb

+    expect(order.completed?).to be true
+    expect(order.payments.count).to eq 1
+  end
+end


dacook · 2024-09-26T11:59:49Z

spec/mailers/previews/backorder_mailer_preview.rb

+      order.line_items.map(&:variant),
+    )
+  end
+end


💯
So much to build for one seemingly small task, well done, and thanks for adding a preview.

dacook · 2024-09-26T12:03:56Z

spec/jobs/complete_backorder_job_spec.rb

-
-      # Combined example for performance
-      expect(Bugsnag).to receive(:notify).and_call_original
-


😢 It's a shame we can't spec this behaviour of sidekiq's

filipefurtad0 · 2024-09-27T21:40:09Z

Oops, apologies @RaggedStaff - I did not notice the staging-FR label on this PR... I'll be sure to re-stage this PR, once I'm done with testing.

rioug

I had a quick look as well, looks good 👍

mkllnk added the user facing changes Thes pull requests affect the user experience label Sep 11, 2024

mkllnk self-assigned this Sep 11, 2024

rioug approved these changes Sep 11, 2024

View reviewed changes

mkllnk force-pushed the dfc-order branch 3 times, most recently from 2877d66 to 3baa845 Compare September 12, 2024 06:43

dacook self-requested a review September 12, 2024 07:27

dacook approved these changes Sep 13, 2024

View reviewed changes

RaggedStaff added the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 13, 2024

RaggedStaff reviewed Sep 15, 2024

View reviewed changes

mkllnk force-pushed the dfc-order branch 3 times, most recently from c82527d to 585bd88 Compare September 19, 2024 07:07

RaggedStaff added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Sep 19, 2024

mkllnk force-pushed the dfc-order branch from 585bd88 to 035f196 Compare September 20, 2024 01:26

dacook added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Sep 24, 2024

mkllnk added 10 commits September 25, 2024 10:55

Create BackorderJob to place wholesale orders

260e4f7

Place backorders for DFC products

98966f6

Raise errors on DFC requests

439f0ca

The simplified API was only returning the response body, not allowing us to inspect if an error occurred. Since an error should be an exception when communicating with a standardised protocol, we raise an error and keep our simple API.

Add SaleSession with correct OrderCycle times

6c6927a

Apparently, the FDC implementation uses those dates to finalise orders.

Start moving backorder logic to service

827e37c

The job class is getting too big.

Find and update existing open order

caa6d28

Add needed quantities to existing line items

a7a3889

Complete an open order

f839452

Simplify order update logic

c7fa3ff

Simplify service with ivar

3e0eb87

mkllnk added 4 commits September 25, 2024 10:55

Fall back to givin product id w/o retail variant

070b93c

Move catalog loading to where it's needed

7f62b49

Import DFC product images

66f0802

Import on-demand stock setting in DFC import

9f43244

mkllnk force-pushed the dfc-order branch from 035f196 to 9f43244 Compare September 25, 2024 00:56

mkllnk mentioned this pull request Sep 25, 2024

Review and migrate legacy checkout specs #10958

Open

8 tasks

RaggedStaff added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Sep 25, 2024

mkllnk force-pushed the dfc-order branch from 2f5767a to 16ff220 Compare September 25, 2024 07:02

RaggedStaff mentioned this pull request Sep 25, 2024

[DFC Orders] Cancelled Order does not reduce DFC Order quantities immediately #12876

Open

RaggedStaff linked an issue Sep 25, 2024 that may be closed by this pull request

[DFC Orders] Cancelled Order does not reduce DFC Order quantities immediately #12876

Open

mkllnk force-pushed the dfc-order branch from 29242c5 to ddab92d Compare September 26, 2024 03:36

mkllnk added 8 commits September 26, 2024 14:32

Import prices and stock levels from DFC catalog

2465780

We were already importing stock levels from offers but now we are looking at catalog items as well.

Restore concurrency spec for the checkout

eece738

This was abandoned when the checkout was re-designed. But I want to refactor the order locking mechanism and it would be good to know that I don't break anything.

Abstract OrderLocker for re-use

61fec65

Place backorders in the background

e31e45b

Report backorder errors instead of failing checkout

49fd1dc

Send error notification to owner

495634b

Notify user of failed backorder completion

989a6d5

mkllnk force-pushed the dfc-order branch from ddab92d to 51b3770 Compare September 26, 2024 04:33

mkllnk marked this pull request as ready for review September 26, 2024 05:08

dacook approved these changes Sep 26, 2024

View reviewed changes

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Sep 30, 2024

rioug approved these changes Sep 30, 2024

View reviewed changes

mkllnk merged commit 1969561 into openfoodfoundation:master Oct 1, 2024
52 of 53 checks passed

mkllnk deleted the dfc-order branch October 1, 2024 00:51

mkllnk removed the pr-staged-fr staging.coopcircuits.fr label Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Place backorders for linked products via DFC integration #12856

Place backorders for linked products via DFC integration #12856

mkllnk commented Sep 11, 2024 •

edited

Loading

rioug left a comment

rioug Sep 11, 2024

rioug Sep 11, 2024

dacook Sep 13, 2024

dacook left a comment

dacook Sep 13, 2024

dacook Sep 13, 2024

RaggedStaff Sep 15, 2024

RaggedStaff Sep 15, 2024

RaggedStaff Sep 15, 2024

mkllnk Sep 17, 2024

RaggedStaff Sep 15, 2024

mkllnk Sep 17, 2024

dacook left a comment

dacook Sep 26, 2024

dacook Sep 26, 2024

dacook Sep 26, 2024

dacook Sep 26, 2024

dacook Sep 26, 2024

rioug Sep 30, 2024

dacook Sep 26, 2024

dacook Sep 26, 2024

dacook Sep 26, 2024

filipefurtad0 commented Sep 27, 2024

rioug left a comment


		# Combined example for performance
		expect(Bugsnag).to receive(:notify).and_call_original

Place backorders for linked products via DFC integration #12856

Place backorders for linked products via DFC integration #12856

Conversation

mkllnk commented Sep 11, 2024 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

rioug 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

dacook 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Sep 27, 2024

rioug left a comment

Choose a reason for hiding this comment

mkllnk commented Sep 11, 2024 •

edited

Loading