Sort Order "Comments History" tab comments correctly when same timestamp

[PromInc]

Sort comment history on the admin order Comments History tab by entity_id if the timestamps are the exact same second.

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Admin Order "Comments History" tab may be out of order #34304

Manual testing scenarios (*)

1. Create an order with two records in the sales_order_status_history table with the exact same timestamp.
2. View the order in the admin and select the Comments History tab. The comments should be sorted correctly by created_at and entity_id

Questions or comments

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 @PromInc. Thank you for your contribution
Here are some useful tips 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

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[ihor-sviziev]

Could you explain why do we need to have this if block?

[PromInc]

Of course!

Here is an example screenshot of when the bug exists. There are 3 records that occurred at the exact same time. But they are out of order.

We know from the codebase that Magento creates the order first and then the invoice. So it makes sense that the order statuses are listed first and then the invoice record is last.

Because I use the Kount module, it created 2 records about the order status at the exact same time and are not sorted correctly (based on entity_id). The Processing status should be first, then the Review record, and lastly the Invoice record.

The order records (Processing and Review) both live in table sales_order_status_history and thus their entity_id's are sequential and able to be sorted correctly. However the Invoice record doesn't live in that table and will naturally have an entity_id out of sequence.

Thus wehn the above line to sort by comment Invoice created is omitted you get this result.

Notice that invoice if first. The sales_order_status_history will typically contain more than one record per order thus those entity_id's should always be greater than the entity_id coming from table sales_invoice. So this should be a safe assumption/method to detect how to put the invoice AFTER the order information. (not perfect, but given the mis-matched information should suffice)

Once this if block is added, the Invoice record is now sorted after the Order records as it should be and we get the proper depiction of history as it should be.

[ihor-sviziev]

Ok. It was still unclear that order history records are shown from older to newer from top to bottom.

The issue makes sense to me, but I'm afraid I have to disagree with the implementation. It doesn't make any sense to compare the entity_id of order and shipment records, but it makes sense competing for two records of the same type.

As for me, we should sort the records with the same time in the following order, similar as they're added:

• order history

  magento2/app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php

  Line 77 in 6e79dc6

  foreach ($order->getAllStatusHistory() as $orderComment) {

• credit memo history

  magento2/app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php

  Line 87 in 6e79dc6

  foreach ($order->getCreditmemosCollection() as $_memo) {

• shipment history

  magento2/app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php

  Line 106 in 6e79dc6

  foreach ($order->getShipmentsCollection() as $_shipment) {

• invoice history

  magento2/app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php

  Line 125 in 6e79dc6

  foreach ($order->getInvoiceCollection() as $_invoice) {

• tracking numbers history

  magento2/app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php

  Line 145 in 6e79dc6

  foreach ($order->getTracksCollection() as $_track) {

[ihor-sviziev]

Also, the method sortHistoryByTimestamp isn't designed to sort records differently than by timestamps.

So I think a better solution there would be adding a new private method, sortHistory, that will do the following:

1. call sortHistoryByTimestamp to compare timestamps (for backward compatibility)
2. if the timestamp comparison result is the same for two records - try to compare by record type (see list above).
   Note: for easier comparison - I think we might introduce private constants and pass them to the "history" array like this:

private const HISTORY_TYPE_ORDER = 1;
private const HISTORY_TYPE_CREDIT_MEMO = 2;
private const HISTORY_TYPE_SHIPMENT = 3;
private const HISTORY_TYPE_INVOICE = 4;
private const HISTORY_TYPE_TRACKING_NUM = 5;

3. if the record type comparison result is the same for two records - compare by entity_id.

What do you think about it?

[PromInc]

You're right that comparing entity_id for different entity types. That is a flaw in my proposed solution - I'll admit that. Theoretically it shouldn't be an issue, but wise to program to avoid it.

we should sort the records with the same time in the following order, similar as they're added:
Isn't the order of the functions you outlined the same as it currently is? And yet the issue exists?

1. You bring up a good point about the function naming and what it does. I'm open to the final solution using a different function name.
2. I'm not fully following the need for the new constants. I think seeing them in code might help me to better understand your intention.
3. This makes sense to me.

[engcom-Charlie]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

With respect to previous build failure, all the failed tests mentioned in comment are working fine now. There are some static test failures right now. Looking into that.

[engcom-Charlie]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

@magento run Unit Tests, Functional Tests B2B , Functional Tests CE, Functional Tests EE

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

@magento run Unit Tests, Functional Tests B2B , Functional Tests CE, Functional Tests EE

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[engcom-Charlie]

Fixed failing static as well as some functional test cases.

Now the test case which is failing(AdminCheckVATNumberValidationButtonForEUCountryTest) in build is not part of this PR or not failing because of any changes made in this PR. As mentioned in comment requesting for the review, hence moving it to review status.

Thank you!