Skip to content

Conversation

BlackbitDevs
Copy link
Contributor

Steps to reproduce bug:

  1. Open data object grid for a folder
  2. So some filtering by column filters
  3. Edit "Grid Options", enter name "Filter Saving Test", go to tab "Save & Share", enable "Save filters" and click "Save copy & Share"
  4. Reload grid
  5. Select grid configuration "Filter Saving Test" from "Grid Options" dropdown

Expected behaviours:

  1. Column filters should be set
  2. When going to "Grid Options" again, the "Save filters" checkbox should be checked

Actual behaviour:

  1. Although saved filters get applied, column filters are not visibly set (in UI it looks as if there were no filters active - beside the "Filters active!" hint above the grid)
  2. When clicking "Grid Options" button again, in "Save & Share" tab, the "Save filters" checkbox is disabled.

Reasoning:

  1. Presetting column filter values
    if (this.filter) {
    this.filter.forEach(filt => {
    this.store.setFilters(new Ext.util.Filter(filt))
    this.filterUpdateFunction(this.grid, this.toolbarFilterInfo, this.clearFilterButton);
    });
    }

    This applies the saved filters to the store. But there is no connection to the column filters here. For this reason, the filters get applied but not shown in the column filters. Instead the [ExtJS docs](https://docs.sencha.com/extjs/7.4.0/classic/ Ext.grid.filters.Filters.html) suggest:
columns: [{
        dataIndex: 'show',
        text: 'Show',
        flex: 1,
        filter: {
            type: 'string',
            value: 'star',
        }
    }]

With this approach the preset value gets applied to the column filter and thus as well to the store.

  1. Saving "Save filter" checkbox
    The reason why the checkbox was not saved surprised me very much: Although the Dao object contains setSaveFilters() and getSaveFilters(), the database table gridconfigs is missing a corresponding column. When saving the grid config in
    $gridConfig->setSaveFilters($metadata['saveFilters'] ?? false);

    The value for saveFilters column gets set.
    In
    the grid config is tried to be saved. But as there is no column for this field in
    if (in_array($key, $this->getValidTableColumns('gridconfigs'))) {

    it gets simply ignored.
    As the admin-ui-classic bundle does not have any migrations, I have added this in [Grid Options] Add column for "saveFilters" checkbox in saved grid configs pimcore#18528.

This PR fixes this. Additionally it adds a translation for Save filters checkbox label which was hard-coded before.

- restore filters after loading
- store "save filters" in database
- add translation for "save filters"
Copy link

sonarqubecloud bot commented Jul 3, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
1 New Critical Issues (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@BlackbitDevs BlackbitDevs changed the title [Grid Options] Fixes for saving grid configuration with "save filters": [Grid Options] Fixes for saving grid configuration with "save filters" Jul 3, 2025
@kingjia90 kingjia90 self-assigned this Jul 16, 2025
Comment on lines -341 to -347
if (this.filter) {
this.filter.forEach(filt => {
this.store.setFilters(new Ext.util.Filter(filt))
this.filterUpdateFunction(this.grid, this.toolbarFilterInfo, this.clearFilterButton);
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your PR!

Is this removed part intentional? The "Filter Active!" part would be missing.
I guess only this.store.setFilters(new Ext.util.Filter(filt)) needs to be removed?

Other than that, the rest LGTM

@kingjia90
Copy link
Contributor

@BlackbitDevs a gentle reminder, in case of 🏖️

@kingjia90
Copy link
Contributor

kingjia90 commented Jul 30, 2025

Mhh but I guess that due to the migration pimcore/pimcore#18528 , this fix wouldn't be in the window of next bugfix release of next week as it may required to be in a minor release

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.

2 participants