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

Fix the count in Element/TemplateGetList processor #16160

Merged
merged 2 commits into from Apr 27, 2022
Merged

Fix the count in Element/TemplateGetList processor #16160

merged 2 commits into from Apr 27, 2022

Conversation

Jako
Copy link
Collaborator

@Jako Jako commented Apr 23, 2022

What does it do?

  • Move the sortby queries into the prepareQueryAfterCount.
  • Add a limit: 0 to the baseParams of the TemplatePicker combo in the resource creating window.
  • Some normalizing of the boolean value of the combo property

Why is it needed?

  • The category_name column is not available in prepareQueryBeforeCount. This causes issues during retrieving the count in the Template combo. Because of this, the pagination was not shown with more than 20 templates available.
  • The default limit is 20 in the Model/GetListProcessor and only the first 20 templates are selectable without a search
  • The combo property contains a boolean value and not a string or an integer.

How to test

  • Create more than 20 templates and edit an existing resource. Only the first 20 templates are selectable without a search in the combo.
  • Create more than 20 templates and create a resource. Only the first 20 templates are selectable without a search in the template list.

Related issue(s)/PR(s)

#16135, this PR improves and replaces #16145

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Apr 23, 2022
Copy link
Contributor

@JoshuaLuckers JoshuaLuckers left a comment

Choose a reason for hiding this comment

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

It works fine, however when you select a template from the second "page" it doesn't select this template after you refresh and open the template picker again. This works fine for templates from the first page.

Not a showstopper, just something I've noticed.

@Jako
Copy link
Collaborator Author

Jako commented Apr 24, 2022

This must be an Ext JS issue and has to be investigated later.

@Ibochkarev Ibochkarev added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Apr 24, 2022
@Ibochkarev Ibochkarev added this to the v3.0.1 milestone Apr 24, 2022
@Jako
Copy link
Collaborator Author

Jako commented Apr 26, 2022

During my tests it worked: If the selected Template is on page 2, the right template will be selected, when you go to page 2 in the combo.

It is not possible to have page 2 automatically opened, since the current value is not sent in the request that retrieves the paginated list. This can be somehow done in the combo code, but then the page has to calculated with a minimum of database queries. Is this possible?

@JoshuaLuckers
Copy link
Contributor

During my tests it worked: If the selected Template is on page 2, the right template will be selected, when you go to page 2 in the combo.

Indeed it does! Thanks.

It is not possible to have page 2 automatically opened, since the current value is not sent in the request that retrieves the paginated list. This can be somehow done in the combo code, but then the page has to calculated with a minimum of database queries. Is this possible?

I'm fine if we leave it like it is for now. For me it's not a real problem that needs to be solved.

Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

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

This does the job. I am curious though why it is not necessary to set the limit to 0 on the template combo config (not the picker variation) — somehow the processor's default of 20 is being overridden, right?

@Jako
Copy link
Collaborator Author

Jako commented Apr 26, 2022

The default pagination of a combo is set with the pageSize property of 20 in Ext JS.

@smg6511
Copy link
Collaborator

smg6511 commented Apr 26, 2022

I understand the pagination config. It's the limit of total results (which wasn't working before, but now is). Why would the query only fetch a total of 20 before this fix?

@Jako
Copy link
Collaborator Author

Jako commented Apr 26, 2022

Before, the count query was sorted by a missing column, which returned a total of 0 then. This disabled the pagination. The final query for the results contains the missing column (it is added in prepareQueryAfterCount). The processor uses also an internal default limit of 20.

@opengeek opengeek merged commit 89a0cd2 into modxcms:3.x Apr 27, 2022
@Jako Jako deleted the fix-template-getlist branch April 27, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/ready-for-merging Pull request reviewed and tested and ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants