Ensure order state handles with partial shipment

[digitalrisedorset]

The order flow will set the order to complete only once the order items are dealt with shipment, refund, cancellation. Yet, some edge case with virtual items and with partial shipment, partial refund, partial cancellation can make this flow faulty.

Description (*)

The change in this PR makes the qty to ship calculation to remove the qty refunded, qty cancelled and qty shipped already

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Order is not complete when mixing virtual, refunded and shipped items #38547

Manual testing scenarios (*)

Test 1: place an order with only physical items
validate the order gets complete when the invoice and shipment are processed and all the items are shipped
Test 2: place an order with only physical items and partial refund
validate the order gets complete when the invoice, refund and shipment are processed and all the items are shipped or refund or cancelled
Test 3: place an order with a mix of configurable, physical items and virtual items
validate the order gets complete when the invoice, refund and shipment are processed and all the items are shipped or refund or cancelled

Questions or comments

This change is not backward compliant. However, like @digitalpianism mentions, the current behaviour is not consistent when the order has only physical items. The latest change in the model \Magento\Sales\Model\ResourceModel\Order\Handler\State is what makes the experience consistent as per the requirements of the issue we are dealing with.

Currently, it is possible to refund an item that has not been shipped or just as well refund an item that has already been shipped without being able to trace either scenarios. Yet, the logic in the method \Magento\Sales\Model\ResourceModel\Order\Handler\State::isPartiallyRefundedOrderShipped assumes the refund and the shipping are separate information that can be cumulated

I have added some changes in the method \Magento\Sales\Model\Order\Item::getSimpleQtyToShip. These turn out to do the same as before. However, I would like to encourage to keep these changes (I am not precious but think they improve the readability of the logic in this method)

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @digitalrisedorset. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[digitalrisedorset]

@magento run all tests

[Den4ik]

@digitalrisedorset Thanks for contribution.
As I see suggested logic provide non expected logic.

Refund works only with shipped items.
Cancelation works only with non shipped items.

For example,
ordered = 5
shipped = 3
refunded = 2 (refunded from shipped 3)
canceled = 1
I expect to see 1 to ship.

Suggested logic will cause negative value and max(round($qty, 8), 0) return 0.

[digitalrisedorset]

excellent feedback, I will investigate this further. I will try to bring all the scenarios we may have so that we can verify we have all the possible situations in mind to resolve this issue.

Many thanks @Den4ik

[digitalrisedorset]

@Den4ik following your inputs, I was able to identify more use cases but unfortunately, it seems I have been focusing at the wrong end of the stick.

To make sure we understand what we are trying to do, I may recall here the aim of the PR:

• when an order has its physical items shipped, it should ignore the status of virtual items as these do not need to be shipped.
• when an order is refunded, the refund is intuitively thought to be complementary to the shipping data: shipped data + refund data = all data handled
  --> this was something I had in mind until now: currently, we can refund an item independently to its shipping status.

I am yet to ascertain how the refund data should be considered at this time:

• should the refund data and shipment data be considered separate? my suspicion is not and in this case, the method \Magento\Sales\Model\ResourceModel\Order\Handler\State::isPartiallyRefundedOrderShipped may need being revisited

I suggest not to review any further until I have more information with this part of the refund aspect

[digitalrisedorset]

@Den4ik I have now found out a way to fix the problem and I have committed some changes today. However, I see my change has a conflict with the method \Magento\Sales\Model\ResourceModel\Order\Handler\State::isPartiallyRefundedOrderShipped

The 2 lines change that have been made and conflict with my change are sufficient for this issue to be called close. I am more than happy to go with the change that have been provided and discard all my changes.

In short, if we close my PR and the issue and ensure the lines that conflict with my changes get pushed further, then the issue is resolved

[engcom-Hotel]

Hello @digitalrisedorset,

Please resolve the conflicts of this PR, so that we can pick this for further processing. Meanwhile moving this to On Hold.

Thanks

[engcom-Hotel]

Hello @digitalrisedorset,

Have you had a chance to look into this #38679 (comment)?

Thanks

[digitalrisedorset]

Hello @digitalrisedorset,

Have you had a chance to look into this #38679 (comment)?

Thanks

Hi, I have not looked into it. My PRs are 6 months old and I had no feedback for all this time. I have now moved on. Apologies.

[engcom-Hotel]

Hello @digitalrisedorset,

Apologies for the delay. Currently, we are focusing on PRs from the Community Picked dashboard. Since your PR has been voted as Community Picked, we have selected it for further processing.

We kindly request you to resolve the conflicts so we can proceed with this PR.

Thank you for your contribution!

[engcom-Hotel]

Hello @digitalrisedorset,

A gentle reminder to resolve the conflicts on this PR.

Thanks

[engcom-Hotel]

Hello @digitalrisedorset,

Have you got a chance to fix the conflicts on this PR?

Thanks

[digitalrisedorset]

Hi Abhinav, I plan to take a look when I am back from the christmas break. Apologies, the end of year has been challenging and I have not been able to take a look at the conflict. Kind regards, Herve

On 23/12/2024 13:49, Abhinav Pathak wrote: Hello @digitalrisedorset [1], Have you got a chance to fix the conflicts on this PR? Thanks -- Reply to this email directly, view it on GitHub [2], or unsubscribe [3]. You are receiving this because you were mentioned.Message ID: ***@***.***>

Links: ------ [1] https://github.com/digitalrisedorset [2] #38679 (comment) [3] https://github.com/notifications/unsubscribe-auth/BHVLMTZADRDQHPS4RKO5X3T2HAIFHAVCNFSM6AAAAABRWKJWJWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJZG42DGOBWGE

[engcom-Hotel]

Sure @digitalrisedoreset,

Please resolve the conflicts whenever you are back.

Thanks

[digitalrisedorset]

Sure @digitalrisedoreset,

Please resolve the conflicts whenever you are back.

Thanks

I have made some changes and submitted a PR now. Apologies, I was hoping to do better than this. The steps I have taken are as follows:

• I pulled the latests develop branch
• I created a new branch and integrated my changes for the issue we are working onto.
• The caveats with my change is that the tests side is not changed. I have not been working on Magento projects for a while and I know the tests have changed a lot in the last years so I thought you will be doing this part better than me.

[engcom-Hotel]

Hello @digitalrisedorset,

We are still seeing the conflicts on this PR. Have you created a new PR for the same? If yes can you please provide the details of that PR.

Thanks

[digitalrisedorset]

The new branch that I have created is https://github.com/digitalrisedorset/magento2/tree/feature/order-state-faulty-with-partial-shipment-with-conflict-resolved

[engcom-Hotel]

Thanks @digitalrisedorset, but we are asking about the PR, if you have created any. Because on this PR we can see the conflicts are still there.

[digitalrisedorset]

Apologies team, I think it might be worth to organise a meeting. I have created most of my PRs 10 months ago, I have to say it was disappointing not to have more follow-up earlier. Today, I am torn as to whether my inputs will be taken into account. My suggestion is to work closely to ensure my time is not wasted and rather we set good foundations to work on a good roadmap to help each other.

[engcom-Hotel]

Hello @digitalrisedorset,

Pinged you over slack. Lets discuss there.

Thanks

[engcom-Hotel]

Hello @digitalrisedorset,

It was great discussing with you on Slack yesterday. As per our discussion, we have a new PR #39548, created by @digitalrisedorset, with resolved conflicts. Therefore, we can proceed with this PR.

As a result, we are closing this PR.

Thank you

[Den4ik]

@engcom-Hotel Thanks for update

[engcom-Hotel]

As per the further discussion internally, because this PR has been marked as Community Picked we need to reopen and work on this PR and pull all the commits from this #39548 to here.

Hence reopening this PR.

[m2-assistant]

Hi @digitalrisedorset. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[engcom-Hotel]

Hello @digitalrisedorset,

As per our discussion over slack, we will continue with this PR for resolving the conflicts. But it seems we dont have necessary permission to push the code on this branch. Please allow us the same.

We are getting the below error while pushing the code:

Please do the needful. Meanwhile moving this PR On Hold.

Thanks

[engcom-Hotel]

Hello @digitalrisedorset,

Have you got a chance to look into this #38679 (comment)? We are unable to push code due to access issue. Please provide us the required permission.

Thanks

[Den4ik]

@digitalrisedorset Will you have a chance move updates from #39548 to this PR?

[digitalrisedorset]

HI, I am happy to give you the permissions. Shall we meet to validate the permissions you need. If you ping me a date and time when we can meet. I'd like as well to clear this for you to benefit of the PR.

[Den4ik]

@digitalrisedorset Please follow this instruction https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork