Sales Order Grid column ordering not saved properly

gwharton commented 5 years ago

Preconditions (*)

magento 2.3-develop vanilla

Steps to reproduce (*)

deploy 2.3-develop

Sales -> Orders
    Enable all columns
    Open Browser Development Tools
    Filter network requests for the url string "?isAjax=true"
    Make a note of column orders
    Move a column and note new position
    Verify that there is an "?isAjax=true" call made to the server after column move
    Refresh Page
    Verify column ordering on page after refresh
    Note whether an "?isAjax=true" call is made during page reload
    Repeat Refreshing Page noting column ordering and presence of "?isAjax=true" call

Expected result (*)

Column ordering is maintained across page refreshes
"?isAjax=true" calls should only be made immediately after column move, not page refresh

Actual result (*)

Seemingly randomly chosen columns move on successive page refreshes (not every refresh).
"?isAjax=true" calls are made back to the server during page refresh (not every refresh) with incorrect column order
The ajax call incorrectly changes column ordering stored on the server. The page then renders the wrong order in accordance with what is set on the server.

Old Issue

magento/magento2 #17691 - Admin Grid column ordering/positioning not working

m2-assistant[bot] commented 5 years ago

Hi @gwharton. Thank you for your report. To help us process this issue please make sure that you provided the following information:

[x] Summary of the issue
[x] Information on your environment
[x] Steps to reproduce
[x] Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

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

@gwharton do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

[x] yes
[ ] no

Join Magento Community Engineering Slack and ask your questions in #github channel.

m2-assistant[bot] commented 5 years ago

Hi @engcom-Charlie. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[x] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
Details
If the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.
[x] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.
[x] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[x] 4. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[x] 6. Add label Issue: Confirmed once verification is complete.
[x] 7. Make sure that automatic system confirms that report has been added to the backlog.

magento-engcom-team commented 5 years ago

:white_check_mark: Confirmed by @engcom-Charlie Thank you for verifying the issue. Based on the provided information internal tickets MC-18662 were created

Issue Available: @engcom-Charlie, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

sagarwebkul commented 5 years ago

@magento-engcom-team give me 2.3-develop instance

magento-engcom-team commented 5 years ago

Hi @sagarwebkul. Thank you for your request. I'm working on Magento 2.3-develop instance for you

magento-engcom-team commented 5 years ago

Hi @sagarwebkul, here is your Magento instance. Admin access: https://i-23889-2-3-develop.instances.magento-community.engineering/admin Login: admin Password: 123123q Instance will be terminated in up to 3 hours.

m2-assistant[bot] commented 5 years ago

Hi @mahsn. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

[ ] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 2. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 3. If the issue is not relevant or is not reproducible any more, feel free to close it.

Join Magento Community Engineering Slack and ask your questions in #github channel.

gwharton commented 4 years ago

OK, I've finally managed to get to the bottom of this one. Turns out to be a bit of a gnarly bug in some core javascript.

When the Sales Order Grid (or infact any ui_listing) is loaded, it initialises the columns calling initPositions() when all of the columns are loaded.

https://github.com/magento/magento2/blob/5977749781ebdbb5e28a03db73207a7fb08ae744/app/code/Magento/Ui/view/base/web/js/grid/listing.js#L148-L164

applyPositions is called with an array of positions for the current set of columns. It is transposed into an array of elem/position pairs, elem being the column, and position being the sortOrder for each column.

https://github.com/magento/magento2/blob/5977749781ebdbb5e28a03db73207a7fb08ae744/app/code/Magento/Ui/view/base/web/js/grid/listing.js#L196-L216

This array is then passed to the insertChild function, which is responsible for reordering the columns, in accordance with the array it is passed.

https://github.com/magento/magento2/blob/5977749781ebdbb5e28a03db73207a7fb08ae744/app/code/Magento/Ui/view/base/web/js/lib/core/collection.js#L74-L110

Inside this function, container is the grid containing the columns, and the incoming positions array is iterated over, calling utils.insert(item.elem, container, item.position) to place each "elem" (column) into the correct position.

Now if utils.insert, just took the element, and placed it into the position indicated at item.position overwriting the existing element at that position, then all would be well, but it doesn't. It inserts it into the array at the new position, removes itself from the old position and shifts all lower entries down the array. Thus as we iterate through the array, previous columns that had been placed into the correct position, have now been moved out of position by the insert.

I implemented a dirty hack locally to test the theory, by sorting the incoming positions array in ascending order of position, so the replacements are done in order from top to bottom of the array, thus you never run into the scenareo that a correctly positioned item is moved out of place by accident. With this workaround in place, the listing column ordering bug has gone.

This workaround is a hack, and someone from core JS/UI team need to pick this up.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

gwharton commented 3 years ago

Not stale

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

magento-engcom-team commented 3 years ago

Hi @gwharton.

Thank you for your report and collaboration!

The related internal Jira ticket MC-23895 was closed as Fixed.

The fix will be available with the upcoming 2.4.3 release.

sdzhepa commented 3 years ago

Issue was closed due to a mistake. Reopened

m2-assistant[bot] commented 3 years ago

Hi @engcom-Alfa. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
Details
If the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.
[ ] 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

engcom-Alfa commented 3 years ago

Hi @gwharton

As I have re tested in the 2.4-develop version, it is not reproducible. Below is the working screen record that I have tried today.

https://user-images.githubusercontent.com/51679138/130938033-b056b98a-8a03-4935-ac5b-6dc2b23d1fbe.mp4

Kindly upgrade to the latest version possible and give a try. Let me know if you still face the same issue.

gwharton commented 3 years ago

rNJnrBP0eM

I just deployed this version of 2.4.2-p1 as a test. It doesnt do it every time you move a column, but when it does do it, it is consistent. See the above video. I first move the Braintree Transaction Source column and then refresh. That one works fine. I then move the Allocated Sources column next to the Braintree Transaction Source column and then refresh. That one fails and is positioned one space to the right after refresh. Notice that there is an extra isAjax=true call on page refresh, indicating it has adjusted the column ordering in error.

I traced it to a bug in the collection.js code when it reorders columns. See above notes.

gwharton commented 3 years ago

kYzimzyXrW

And this is on 2.4-develop without MSI.

Notice I move the Grand Total Purchased to the right of the ID column. Refresh page and it works fine. Now move the status column to the right of the ID column and refresh page. The Grand Total Purchased has shifted right by one column and there is an extra ajax call. Refresh the page again and the column moves to the right 1. Refresh page again, the column moves again.

engcom-Alfa commented 3 years ago

HI tried to reproduce in the 2.4-develop version and able to successfully reproduce it. Below is the screen record in which it shows that able to reproduce. (I was able to reproduce, after a few attempts. It is not consistently reproducible)

https://user-images.githubusercontent.com/51679138/130938665-e61aea00-ed23-43b0-a04c-c447153bcc33.mp4

Hence confirming the issue.

magento-engcom-team commented 3 years ago

:white_check_mark: Confirmed by @engcom-Alfa Thank you for verifying the issue. Based on the provided information internal tickets MC-23895 were created

Issue Available: @engcom-Alfa, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

CanBeer commented 2 years ago

I reproduced this issue on Magento 2.4.3-p2 EE.

Based on gwharton analysis, here are a workaround patch, who solve this issue :

Index: vendor/magento/module-ui/view/base/web/js/grid/listing.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/magento/module-ui/view/base/web/js/grid/listing.js b/vendor/magento/module-ui/view/base/web/js/grid/listing.js
--- a/vendor/magento/module-ui/view/base/web/js/grid/listing.js 
+++ b/vendor/magento/module-ui/view/base/web/js/grid/listing.js (date 1668501997851)
@@ -209,7 +209,11 @@
                     position: positions[elem.index]
                 };
             });
-
+            
+            sorting.sort(function(a, b) {
+                return a['position'] - b['position'];
+            });
+            
             this.insertChild(sorting);

             return this;

m2-assistant[bot] commented 6 months ago

Hi @engcom-Bravo. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

1. Add/Edit Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
1. Verify that the issue is reproducible on 2.4-develop branch
  Details
  - Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
1. If the issue is not relevant or is not reproducible any more, feel free to close it.

Join Magento Community Engineering Slack and ask your questions in #github channel.

engcom-Bravo commented 6 months ago

Hi @gwharton,

Thanks for your reporting and collaboration.

Verified the issue in Latest 2.4-develop instance and the issue is reproducible.Kindly refer the attached video.

https://user-images.githubusercontent.com/51679138/130938665-e61aea00-ed23-43b0-a04c-c447153bcc33.mp4

Hence Confirming the issue.

Thanks.

github-jira-sync-bot commented 6 months ago

:white_check_mark: Jira issue https://jira.corp.adobe.com/browse/AC-11935 is successfully created for this GitHub issue.

m2-assistant[bot] commented 6 months ago

:white_check_mark: Confirmed by @engcom-Bravo. Thank you for verifying the issue.
Issue Available: @engcom-Bravo, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

magento / magento2