Skip to content
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.

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

Fixed cannot sort by column in backend's grids #3985

Merged
merged 5 commits into from
May 9, 2024
Merged

Fixed cannot sort by column in backend's grids #3985

merged 5 commits into from
May 9, 2024

Conversation

fballiano
Copy link
Contributor

#3927 broke grid sorting functionality in the backend because the js was looking for the "name" attribute.

This PR rewrites the doSort methods fixing the bug

@github-actions github-actions bot added the JavaScript Relates to js/* label May 9, 2024
@empiricompany
Copy link
Contributor

Sorry, but the patch solves the sorting issues in the default grid,
but the module for advanced grids, widely used, mage-enhanced-admin-grids continues to not work.

The problem is here: 91b1944
where is being removed the attribute name

I have also try to change attribute name to data-name and update it here
https://github.com/empiricompany/mage-enhanced-admin-grids/blob/7aa9a82bc86d1ea87c07dd00db7251ff1ea96162/js/bl/customgrid/config.js#L226

but not resolve the issues, the sorting and filters are not working.

@fballiano
Copy link
Contributor Author

did you try element.parentNode.dataSet.columnId?

@empiricompany
Copy link
Contributor

did you try element.parentNode.dataSet.columnId?

Tried now, but it doesn't work.

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label May 9, 2024
@fballiano
Copy link
Contributor Author

I've re-added the "name" attribute to the "a", which btw is also reported by phpstorm

Screenshot 2024-05-09 alle 11 35 42

@empiricompany
Copy link
Contributor

True, the tag ID should be used instead.
For now, thank you for restoring "name".

We should find a solution for these old, widely used extensions. For example, I was thinking of creating a dedicated clone repository of OpenMage and keep it updated then store all patches in the PRs then keep synchronized with the main one.
But i'm not sure i have so much time...

@empiricompany
Copy link
Contributor

Sorry, for it to work, we also need to revert " js/mage/adminhtml/grid.js" to version 20.6.0

https://raw.githubusercontent.com/OpenMage/magento-lts/v20.6.0/js/mage/adminhtml/grid.js

---
 js/mage/adminhtml/grid.js | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/js/mage/adminhtml/grid.js b/js/mage/adminhtml/grid.js
index 55eb3b2ef05..9e2a168e5d4 100644
--- a/js/mage/adminhtml/grid.js
+++ b/js/mage/adminhtml/grid.js
@@ -141,15 +141,15 @@ varienGrid.prototype = {
 
     },
     doSort : function(event){
-        var element = Event.findElement(event, 'a');
+        event.preventDefault();
+        const element = event.target.closest('a');
+        const parentElement = element.parentNode;
 
-        if(element.name && element.title){
-            this.addVarToUrl(this.sortVar, element.name);
+        if (element && parentElement && parentElement.dataset.columnId && element.title) {
+            this.addVarToUrl(this.sortVar, parentElement.dataset.columnId);
             this.addVarToUrl(this.dirVar, element.title);
             this.reload(this.url);
         }
-        Event.stop(event);
-        return false;
     },
     loadByElement : function(element){
         if(element && element.name){

@fballiano
Copy link
Contributor Author

done

@empiricompany
Copy link
Contributor

For those who want to fix it immediately 20.0.7 without downgrade, you can apply the patch like this:

{
    [...]
    "require": {
        "aydin-hassan/magento-core-composer-installer": "~2.1.0",
        "openmage/magento-lts": "^20.7.0",
    [...]
    "extra": {
        "enable-patching": true,
        [...]
        "patches": {
            "openmage/magento-lts": {
                "Fixed cannot sort by column in backend's grids #3985": "https://patch-diff.githubusercontent.com/raw/OpenMage/magento-lts/pull/3985.patch"
            }
        }
    }

@fballiano
Copy link
Contributor Author

I don't understand how the js thing can happen but whatever :-\

@fballiano fballiano merged commit f8f2f44 into OpenMage:main May 9, 2024
17 checks passed
@fballiano fballiano deleted the regressiongrid branch May 9, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml JavaScript Relates to js/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants