Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin updates can trigger new backorders until the order cycle is closed #13065

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jan 9, 2025

What? Why?

We had a case when an order was updated after the order cycle closed. The backorder amendment failed because the backorder was completed already. We now handle this gracefully by skipping the amendment. But if the order cycle is still open or will open then we can actually create a new backorder. We can assume that the order cycle hasn't had a backorder yet.

What should we test?

  • Have an upcoming or open order cycle with FDC products.
  • Place an order as admin for that order cycle with FDC products.
  • A backorder in Shopify should be created.
  • After the order cycle closed, update one of the orders in OFN.
  • No new backorder should be created.

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

mkllnk added 3 commits January 9, 2025 12:10
Admins may want to pre-process some orders manually for going public.
And it's good to reserve stock for these.

At some point in the future, the supplier may have an order cycle with
its own times but the current FDC implementation allows orders at any
time.
@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Jan 9, 2025
@mkllnk mkllnk self-assigned this Jan 9, 2025
@mkllnk mkllnk marked this pull request as ready for review January 9, 2025 02:57
@RaggedStaff
Copy link
Collaborator

I'm away at orfc, so won't ba able to test this till next week.

I'm also a bit confused @mkllnk - I thought you said (in the issue) we couldn't create a new back order when the order cycle is closed. 😕 did you figure out a way to track the order number?

@mkllnk
Copy link
Member Author

mkllnk commented Jan 9, 2025

I'm away at orfc, so won't ba able to test this till next week.

Okay, maybe we can test it then.

I'm also a bit confused @mkllnk - I thought you said (in the issue) we couldn't create a new back order when the order cycle is closed. 😕 did you figure out a way to track the order number?

There's no technical limitation in creating backorders. And this pull request only creates backorders for order cycles before they close. After they close, we can assume that a backorder was placed and completed and then we shouldn't create a second one. But before the close time, it's okay.

@mkllnk mkllnk requested a review from dacook January 9, 2025 21:17
@mkllnk
Copy link
Member Author

mkllnk commented Jan 9, 2025

I'm away at orfc, so won't ba able to test this till next week.

Actually, next week is fine. We won't have complete code review until next week either.

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great fix

@@ -28,7 +28,7 @@ def amend_backorder(order)

backorder = orderer.find_open_order(order)

update(backorder, user, distributor, order_cycle)
update(backorder, user, distributor, order_cycle) if backorder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it amounts to the same thing, but I thought that we usually avoid implied booleans and use the more explicit present?

But i'm sure it amounts to the same thing, and this looks cleaner so I like it :)


expect(backorder.lines[0].quantity).to eq 1 # beans
expect(backorder.lines[1].quantity).to eq 5 # chia
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, that's a tricky one to test. I guess the only other way would be to use VCR.

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one !

@RaggedStaff RaggedStaff added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 14, 2025
@RaggedStaff
Copy link
Collaborator

So I tested this & got an error email (Back Order failed) when I submitted an Order (via the admin interface) after OC close.

I thought that wasn't supposed to happen now? 😕

@rioug
Copy link
Collaborator

rioug commented Jan 14, 2025

@mkllnk
Copy link
Member Author

mkllnk commented Jan 15, 2025

I tested this & got an error email (Back Order failed) when I submitted an Order (via the admin interface) after OC close.

Argh! I was so focused on the amending part that I forgot that the old initial backorder method is called whenever an order is finalised. I can add a check there or maybe make sure that it's only called during customer-facing checkout.

When an admin creates an order, then AmendBackorderJob is called which
can also trigger a new backorder if needed.

This means that we are not creating backorders via subscriptions any
more. It has never been requested and we can bring that back if needed.
@mkllnk mkllnk force-pushed the dfc-amend-nothing branch from 44b797b to c9eed4f Compare January 15, 2025 04:53
@mkllnk mkllnk added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jan 15, 2025
@RaggedStaff
Copy link
Collaborator

@mkllnk - I started testing this... got a NoMethodError on the BackOrder job (which I started manually through sidekiq).

I think it's related to removing a Product (Dulse flakes) from the Shopify DFC app before OC completion (long story, wasn't intentional on my part 🙄 ). It would be good if we could handle that more gracefully (users being what they are 🤨 ). Could we include in the error handling issue? Also happy to put it in a separate issue & prioritise (medium/low)

Anyway, The admin interface still shows Test OC 24 as Open, so not sure I can validly test this. 😕

@mkllnk mkllnk added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk feedback-needed labels Jan 17, 2025
@mkllnk
Copy link
Member Author

mkllnk commented Jan 17, 2025

Hm, I fixed that Bugsnag issue but now I just get a server error 500 from the Shopify API. Going through the OFN orders I think that the Shopify order would be adjusted to just one case of Balsamic which should be a valid order. I don't know why it fails.

@mkllnk
Copy link
Member Author

mkllnk commented Jan 17, 2025

Digging into the stack trace it looks like I get the error when trying to retrieve the catalog. Maybe the token is expired? Oh, David was testing yesterday...

I tried reconnecting but I wonder if the Shopify app is missing a valid token. Is that something I can check myself? A 500 error is really not great there either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-staged-uk staging.openfoodnetwork.org.uk user facing changes Thes pull requests affect the user experience
Projects
Status: In review
Status: Code review 🔎
Development

Successfully merging this pull request may close these issues.

NoMethodError in AmendBackorderJob@default
5 participants