Skip to content

Conversation

@pavelshulgabi
Copy link

@pavelshulgabi pavelshulgabi commented Aug 11, 2018

added component status based filtering

Description

New functionality filters out all disabled UI components (based on internal logic some components are not applicable when the store has single store mode enabled) as it affects children length comparison and different useful UX behaviours.

Fixed Issues

  1. M2.2.0 Admin Grid column ordering/positioning not working when single store mode set On #12070 M2.2.0 Admin Grid column ordering/positioning not working when single store mode set On

Manual testing scenarios

  1. Install Magento
  2. Ensure single store mode on
  3. View sales order grid
  4. Launch Browser development tools and monitor network traffic.
  5. Move a column. If there is an AJAX call after each column move, then the fix has worked. Without the fix, there will be no AJAX calls for each column move.

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)
  • All automated tests passed successfully (all builds on Travis CI are green)

…single store mode set On

* added component status based filtering
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 11, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team magento-engcom-team added partners-contribution Pull Request is created by Magento Partner Partner: Balance Internet labels Aug 11, 2018
@magento-engcom-team
Copy link
Contributor

Hi @pavelshulga. Thank you for your contribution
Here is 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

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

@gwharton
Copy link
Contributor

Looking at the code, this would appear to fix the original problem, but column ordering is still not working for me, even though I have applied this fix. There is obviously something else going on. Will investigate.

Column ordering seems to no longer work regardless of whether single store mode is on or not, which suggests there is a new unrelated issue affecting column ordering.

@gwharton
Copy link
Contributor

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @gwharton. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @gwharton, here is your new Magento instance.
Admin access: https://pr-17512.engcom.dev.magento.com/admin
Login: admin Password: 123123q

@gwharton
Copy link
Contributor

Here is a video of column ordering not working on the test instance.

https://www.youtube.com/watch?v=d_pH8U9NayE

@pavelshulgabi
Copy link
Author

pavelshulgabi commented Aug 11, 2018 via email

@pavelshulgabi
Copy link
Author

@gwharton , thanks for sharing credentials to the instance. I was able to log in and check the functionality and the solution.

I used the sales order grid and was able to successfully reorganise columns. After each change, Magento sends AJAX call to save all the changes (URL /admin/mui/bookmark/save/?isAjax=true).

@gwharton
Copy link
Contributor

If I move a column, i immediately get an ajax call to save the position. In my case I see the following data in the Ajax call.

"positions":{"ids":0,"billing_name":1,"signifyd_guarantee_status":2,"created_at":3,"shipping_name":4,"grand_total":5,"base_grand_total":6,"increment_id":7,"billing_address":8,"shipping_address":9,"shipping_information":10,"customer_email":11,"customer_group":12,"customer_name":13,"subtotal":14,"status":15,"payment_method":16,"total_refunded":17,"shipping_and_handling":18,"actions":19}
The column positions in the AJAX call match whats on screen.

If I then just click refresh, the page reloads, and somewhere amongst the loading is another ajax call to save the bookmark positions again.

"positions":{"ids":0,"billing_name":1,"signifyd_guarantee_status":2,"created_at":3,"shipping_name":4,"grand_total":5,"base_grand_total":6,"increment_id":7,"billing_address":8,"shipping_address":9,"shipping_information":10,"customer_email":11,"customer_group":12,"subtotal":13,"customer_name":14,"status":15,"payment_method":16,"total_refunded":17,"shipping_and_handling":18,"actions":19}

The order has changed.

Please try yourself, log in to the test instance, go to sales order grid, move customer name column one position to the left, so it is customer email, group, name and then subtotal. Then refresh the page and check again.

@pavelshulgabi
Copy link
Author

@gwharton

Thanks for the detailed information regarding your testing. Could you please review the video of my test attempt and advise what should be adjusted as the outcomes seem perfectly fine for me?
https://drive.google.com/open?id=1dkfUS52IppEMUoxJsm-0Vh0e2Wq4_9s8

Possibly the issue with the testing is due to racing between two concurrent AJAX requests during your test run which overwrite changes of each other (case when earlier started request is being process after later one).

@gwharton
Copy link
Contributor

Hi there @pavelshulga

I've done a bit more debug on my own freshly deployed 2.2.5 test instance with your PR applied.

The following happened

  • Login to admin
  • Sales order grid
  • Enable all columns
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Move a column
  • Check AJAX call with browser column order -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Refresh Browser
  • Check browser column order unchanged -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Move a column
  • Check AJAX call with browser column order -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Refresh Browser
  • Check browser column order unchanged -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Move a column
  • Check AJAX call with browser column order -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Refresh Browser
  • Check browser column order unchanged -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Move a column
  • Check AJAX call with browser column order -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Move a column
  • Check AJAX call with browser column order -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Hide a column
  • Check AJAX call with browser column order -> OK (still includes hidden column)
  • Check browser column order with current bookmark in database -> OK (still includes hidden column)
  • ACTION : Move a column
  • Check AJAX call with browser column order -> OK
  • Check browser column order with current bookmark in database -> OK
  • ACTION : Refresh Browser
  • During the page refresh an AJAX Call is made with incorrect column ordering
  • Check browser column order unchanged -> FAIL - new column ordering matches the above AJAX call
  • Check browser column order with current bookmark in database -> OK database matches above AJAX call
  • ACTION : Refresh Browser
  • During the page refresh an AJAX Call is made with incorrect column ordering
  • Check browser column order unchanged -> FAIL - new column ordering matches the above AJAX call
  • Check browser column order with current bookmark in database -> OK database matches above AJAX call
  • ACTION : Refresh Browser
  • During the page refresh an AJAX Call is made with incorrect column ordering
  • Check browser column order unchanged -> FAIL - new column ordering matches the above AJAX call
  • Check browser column order with current bookmark in database -> OK database matches above AJAX call

Log of ajax calls and database content.txt

You can see that during a normal refresh when the column ordering is correct, there is no AJAX call during the page refresh. Once it starts going wrong, an AJAX call is made during the page refresh which is what is incorrectly adjusting the column ordering.

I notice in your video, that you also saw an AJAX call during your page refresh, although the resulting column ordering did still appear correct. Perhaps it was moving a column that wasnt visible in your example. Try again with all columns visible and keep watching for AJAX calls made during the page refresh.

@gwharton
Copy link
Contributor

gwharton commented Aug 13, 2018

@pavelshulga. Im pretty confident there are two independent issues.

When this issue was raised column ordering only failed when single store mode was on. This is the first bug and is the one you fixed with this PR. Sometime after raising the issue column ordering also stopped working when single store was set to off. I think this is bug no.2 and is what we are seeing here as you have fixed bug no.1. Bug no.2 appears in all modes and seems related to the extra erroneous ajax call during page load/refresh.

@pavelshulgabi
Copy link
Author

@gwharton
Thanks for the detailed feedback.

Could you please add steps to reproduce the bug 2 (for future reference)? In meantime, I'll work on investigation/resolution.

@gwharton
Copy link
Contributor

I'm struggling to get a definitive set of steps to 100% reproduce the bug, apart from "move columns around at random, and it normally occurs within 5 or 6 attempts.

I have done some digging though,

In file Magento_Ui/js/grid/listing.js initElement() gets called for each column as it is created on page load. Once all columns are loaded a call is made to initPositions() to set the positions.

If I run require('uiRegistry').get("sales_order_grid.sales_order_grid.sales_order_columns") just prior to the initPositions call, the order of the columns in the _elems array that is returned is in the default order.

If you re-run require('uiRegistry').get("sales_order_grid.sales_order_grid.sales_order_columns") just after the initPositions call, the order of the columns in the _elems array is correct and should match what should be rendered to the screen (usually, aslong as the bug is not occurring)

I don't fully understand whats going on in the initPositions call, but it looks like it retrieves the column ordering from the local storage/uiRegistry.

For some reason, when this bug is occurring, on a page refresh, if I break execution just after the initPositions call and if I run require('uiRegistry').get("sales_order_grid.sales_order_grid.sales_order_columns") ,the order of the columns in the _elems array is incorrect and is not what was expected. For some reason, the incorrect column ordering is obtained from local storage/uiRegistry.

The code knows that the column ordering it has just retrieved is not the same (somehow) and it triggers off the change event which ultimately results in the firing off, of the AJAX call with the wrong ordering in it.

Might give some clues as to where to look.......

@gwharton
Copy link
Contributor

gwharton commented Aug 19, 2018

I did some testing on all versions from 2.2.0 to 2.2.5

Magento Version Single Store Mode PR#17512 Applied Column Ordering Test Notes
2.2.0-2.2.5 OFF NO FAILED We see BUG 2, extra AJAX calls on refresh
2.2.0-2.2.5 ON NO FAILED No Ajax call on column move - BUG 1
2.2.0-2.2.5 OFF YES FAILED We see BUG 2, extra AJAX calls on refresh
2.2.0-2.2.5 ON YES FAILED Confirm BUG 1 fixed by PR#17512. We see BUG 2, extra AJAX calls on refresh

It shows that PR #17512 does indeed fix BUG 1, which is the original issue #12070

It also shows that the other bug (BUG 2) happens regardless of single store mode.

I am going to create a new issue to track BUG 2, as they are independent issues. This way PR 17512 can go forward as the fix for issue #12070 while the resolution to BUG 2 is worked on.

@gwharton
Copy link
Contributor

@rogyar

I have created a new issue to capture the second bug mentioned above (#17691).

The testing that I have done on this issue shows that this PR does indeed fix the first bug (#12070) and can therefore proceed accordingly.

It can easily be tested as without this fix in single store mode, there are no AJAX calls made from the browser to the backend when you move any columns. With this PR applied, the AJAX calls are made every time and can easily be inspected to contain the correct information.

@gwharton
Copy link
Contributor

gwharton commented Oct 5, 2018

Manual Testing Scenareo.

  1. Install Magento
  2. Ensure single store mode on
  3. View sales order grid
  4. Launch Browser development tools and monitor network traffic.
  5. Move a column. If there is an AJAX call after each column move, then the fix has worked. Without the fix, there will be no AJAX calls for each column move.

@gwharton gwharton changed the title #12070 M2.2.0 Admin Grid column ordering/positioning not working when single store mode set On [2.2] Admin Grid column ordering/positioning not working when single store mode set On Oct 5, 2018
@gwharton
Copy link
Contributor

gwharton commented Oct 5, 2018

2.3-develop PR is here #18405

@gwharton gwharton changed the title [2.2] Admin Grid column ordering/positioning not working when single store mode set On [Backport][2.2] Admin Grid column ordering/positioning not working when single store mode set On Oct 5, 2018
@gwharton
Copy link
Contributor

gwharton commented Oct 5, 2018

Note, if during testing you see that column positioning is not working properly, you may also be running into issue #17691 which remains unfixed. This PR addresses only the issue of the missing AJAX calls.

@magento-engcom-team
Copy link
Contributor

Hi @josefbehr, thank you for the review.
ENGCOM-2745 has been created to process this Pull Request

@gwharton
Copy link
Contributor

Duplicate of #18561

@gwharton gwharton marked this as a duplicate of #18561 Oct 17, 2018
@gwharton gwharton closed this Oct 17, 2018
@amitmaurya191
Copy link

I did some testing on all versions from 2.2.0 to 2.2.5
Magento Version Single Store Mode PR#17512 Applied Column Ordering Test Notes
2.2.0-2.2.5 OFF NO FAILED We see BUG 2, extra AJAX calls on refresh
2.2.0-2.2.5 ON NO FAILED No Ajax call on column move - BUG 1
2.2.0-2.2.5 OFF YES FAILED We see BUG 2, extra AJAX calls on refresh
2.2.0-2.2.5 ON YES FAILED Confirm BUG 1 fixed by PR#17512. We see BUG 2, extra AJAX calls on refresh

It shows that PR #17512 does indeed fix BUG 1, which is the original issue #12070

It also shows that the other bug (BUG 2) happens regardless of single store mode.

I am going to create a new issue to track BUG 2, as they are independent issues. This way PR 17512 can go forward as the fix for issue #12070 while the resolution to BUG 2 is worked on.

As I am using magento 2.2, I am also facing the ajax call issue on sales order grid issue. Just wanted to know,did you get any solution for this issue. If yes, please share with us also. Thanks

@gwharton
Copy link
Contributor

gwharton commented Mar 28, 2020

Bug 2 still present in 2.4-develop. #23889

@amitmaurya191
Copy link

Bug 2 still presebt in 2.4-develop. #23889

As I can see, This issue is not present in 2.3.4.

@gwharton
Copy link
Contributor

I certainly see it in 2.3.4. I can reproduce with stock 2.3.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants