Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 51 commits into
base: 2.4-develop
Choose a base branch
from
Open

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

Changes from 22 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
b7af8a5
Correctly label option for Protection Type
PromInc Jul 8, 2021
242c12f
Correctly label option for Protection Type
PromInc Jul 8, 2021
08d20be
Correctly label option for Protection Type
PromInc Jul 8, 2021
6c87486
Correctly label option for Protection Type
PromInc Jul 8, 2021
692f96d
Correctly label option for Protection Type
PromInc Jul 8, 2021
4febd7c
Correctly label option for Protection Type
PromInc Jul 8, 2021
41188db
Correctly label option for Protection Type
PromInc Jul 8, 2021
bf1c340
Correctly label option for Protection Type
PromInc Jul 8, 2021
ba24475
Sort by timestamp and id
PromInc Oct 11, 2021
836a325
Merge branch 'magento:2.4-develop' into 2.4-develop
PromInc Oct 11, 2021
eba21cf
Update ResetMethod.php
PromInc Oct 11, 2021
7947440
Update ResetMethod.php
PromInc Oct 11, 2021
36a2245
Update CollectionFactory.php
PromInc Oct 11, 2021
e46991f
Update ResetMethodTest.php
PromInc Oct 11, 2021
81e3cbc
Update ResetMethodTest.php
PromInc Oct 11, 2021
c1fb73c
Update CollectionFactoryTest.php
PromInc Oct 11, 2021
5174e50
Update FrequencyTest.php
PromInc Oct 11, 2021
c81212c
Update QuantityTest.php
PromInc Oct 11, 2021
75999e0
Update SecurityManagerTest.php
PromInc Oct 11, 2021
47e17a5
Update en_US.csv
PromInc Oct 11, 2021
1875102
Update ResetMethodTest.php
PromInc Oct 11, 2021
f597b7a
Force invoices to be after other records
PromInc Oct 11, 2021
6e79dc6
Update app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php
PromInc Oct 12, 2021
a5b81c2
Add type for sorting
PromInc Nov 8, 2021
a23b463
Update app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php
PromInc Nov 9, 2021
70cf0a4
Update app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php
PromInc Nov 9, 2021
4109ff2
Update app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php
PromInc Nov 9, 2021
d4c0ba5
define arguments
PromInc Nov 9, 2021
1e8d5c6
Update app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php
PromInc Nov 9, 2021
e65dea4
Merge branch '2.4-develop' of https://github.com/magento/magento2 int…
Nov 16, 2021
451831a
Merge branch '2.4-develop' of https://github.com/magento/magento2 int…
Dec 8, 2021
5ce6523
Merge branch '2.4-develop' of https://github.com/magento/magento2 int…
Dec 10, 2021
84cd2f4
Merge branch '2.4-develop' of https://github.com/magento/magento2 int…
engcom-Lima Dec 10, 2021
16ecc87
Merge branch '2.4-develop' of https://github.com/PromInc/magento2 int…
engcom-Lima Dec 10, 2021
30a9d66
Merge branch '2.4-develop' of https://github.com/magento/magento2 int…
engcom-Lima Dec 14, 2021
284dad8
[BUGFIX] - Fixed static test failures
engcom-Lima Dec 14, 2021
d4d00a3
Merge branch 'magento:2.4-develop' into 2.4-develop
PromInc Dec 22, 2021
f7a746f
Use configuration for mage-messages cookie
PromInc Dec 22, 2021
30dd1a4
Create Config.php
PromInc Dec 22, 2021
0a9223d
Pass the configured the cookie domain to Javascript
PromInc Dec 22, 2021
496302d
set the cookieDomain via the config value
PromInc Dec 22, 2021
fc46a6b
Delete Config.php
PromInc Dec 22, 2021
78c55d6
Revert "set the cookieDomain via the config value"
PromInc Dec 28, 2021
ceede4b
Revert "Pass the configured the cookie domain to Javascript"
PromInc Dec 28, 2021
2ade2a7
Revert "Use configuration for mage-messages cookie"
PromInc Dec 28, 2021
54a7be0
Update function name
PromInc Dec 28, 2021
170e2fb
Merge remote-tracking branch 'magento2/2.4-develop' into 2.4-develop
engcom-Charlie Feb 24, 2022
caf892c
Issue-34304 Fixed multiple tests failurs.
engcom-Charlie Mar 1, 2022
7130231
Merge remote-tracking branch 'magento2/2.4-develop' into 2.4-develop
engcom-Charlie Mar 1, 2022
76e55e0
Issue-34304 Fixed tests failurs.
engcom-Charlie Mar 2, 2022
259be51
Issue-34304 Fixed tests failurs.
engcom-Charlie Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion app/code/Magento/Sales/Block/Adminhtml/Order/View/Tab/History.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public function getFullHistory()
$history = [];
foreach ($order->getAllStatusHistory() as $orderComment) {
$history[] = $this->_prepareHistoryItem(
$orderComment->getId(),
$orderComment->getStatusLabel(),
$orderComment->getIsCustomerNotified(),
$this->getOrderAdminDate($orderComment->getCreatedAt()),
Expand All @@ -85,13 +86,15 @@ public function getFullHistory()

foreach ($order->getCreditmemosCollection() as $_memo) {
$history[] = $this->_prepareHistoryItem(
$_memo->getId(),
__('Credit memo #%1 created', $_memo->getIncrementId()),
$_memo->getEmailSent(),
$this->getOrderAdminDate($_memo->getCreatedAt())
);

foreach ($_memo->getCommentsCollection() as $_comment) {
$history[] = $this->_prepareHistoryItem(
$_comment->getId(),
__('Credit memo #%1 comment added', $_memo->getIncrementId()),
$_comment->getIsCustomerNotified(),
$this->getOrderAdminDate($_comment->getCreatedAt()),
Expand All @@ -102,13 +105,15 @@ public function getFullHistory()

foreach ($order->getShipmentsCollection() as $_shipment) {
$history[] = $this->_prepareHistoryItem(
$_shipment->getId(),
__('Shipment #%1 created', $_shipment->getIncrementId()),
$_shipment->getEmailSent(),
$this->getOrderAdminDate($_shipment->getCreatedAt())
);

foreach ($_shipment->getCommentsCollection() as $_comment) {
$history[] = $this->_prepareHistoryItem(
$_comment->getId(),
__('Shipment #%1 comment added', $_shipment->getIncrementId()),
$_comment->getIsCustomerNotified(),
$this->getOrderAdminDate($_comment->getCreatedAt()),
Expand All @@ -119,13 +124,16 @@ public function getFullHistory()

foreach ($order->getInvoiceCollection() as $_invoice) {
$history[] = $this->_prepareHistoryItem(
$_invoice->getId(),
__('Invoice #%1 created', $_invoice->getIncrementId()),
$_invoice->getEmailSent(),
$this->getOrderAdminDate($_invoice->getCreatedAt())
$this->getOrderAdminDate($_invoice->getCreatedAt()),
'Invoice created'
);

foreach ($_invoice->getCommentsCollection() as $_comment) {
$history[] = $this->_prepareHistoryItem(
$_comment->getId(),
__('Invoice #%1 comment added', $_invoice->getIncrementId()),
$_comment->getIsCustomerNotified(),
$this->getOrderAdminDate($_comment->getCreatedAt()),
Expand All @@ -136,6 +144,7 @@ public function getFullHistory()

foreach ($order->getTracksCollection() as $_track) {
$history[] = $this->_prepareHistoryItem(
$_track->getId(),
__('Tracking number %1 for %2 assigned', $_track->getNumber(), $_track->getTitle()),
false,
$this->getOrderAdminDate($_track->getCreatedAt())
Expand Down Expand Up @@ -303,6 +312,13 @@ public static function sortHistoryByTimestamp($a, $b)
{
$createdAtA = $a['created_at'];
$createdAtB = $b['created_at'];

if( $createdAtA->getTimestamp() === $createdAtB->getTimestamp() ) {
if( $a['comment'] == 'Invoice created' || $b['comment'] == 'Invoice created' ) {
return ( $a['comment'] == 'Invoice created' ? 1 : -1 );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.
image

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.
image

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.
image

Copy link
Contributor

@ihor-sviziev ihor-sviziev Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Copy link
Contributor

Choose a reason for hiding this comment

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

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;
  1. if the record type comparison result is the same for two records - compare by entity_id.

What do you think about it?

Copy link
Author

Choose a reason for hiding this comment

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

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.

return $a['entity_id'] <=> $b['entity_id'];
}

return $createdAtA->getTimestamp() <=> $createdAtB->getTimestamp();
}
Expand Down