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

ENH Reports list filtering and pagination. #197

Conversation

mfendeksilverstripe
Copy link
Contributor

@mfendeksilverstripe mfendeksilverstripe commented Oct 11, 2024

ENH Reports list filtering and pagination.

Screenshot 2024-10-11 at 2 42 57 PM

Description

Minor quality of life improvements to the reports list in the Reports admin.

  • filtering capability added
  • pagination capability added
  • description expose in the list as a new column
  • minor code cleanup

Manual testing steps

  • just need a sample vanilla install as it should have some out of the box reports
  • you may want to change the pagination limit to force the pagination to show even with small number of reports for testing purposes

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks! A few changes requested below.

code/ReportAdmin.php Outdated Show resolved Hide resolved
code/ReportAdmin.php Outdated Show resolved Hide resolved
code/ReportAdmin.php Outdated Show resolved Hide resolved
code/ReportAdmin.php Outdated Show resolved Hide resolved
code/ReportAdmin.php Outdated Show resolved Hide resolved
@GuySartorelli GuySartorelli self-assigned this Oct 11, 2024
@GuySartorelli
Copy link
Member

Please also bump the framework dependency to ^5.2 in composer.json

@mfendeksilverstripe
Copy link
Contributor Author

Have pushed up the requested changes @GuySartorelli , please review.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, works well locally.
Given there's no behat testing for this module, I won't ask for tests to be added for this change.

Thanks for adding this! It's a nice quality of life feature.

All the built-in reports look silly for not having descriptions now lol but we can deal with that separately if we ever get around to it.

@GuySartorelli GuySartorelli merged commit 57abc23 into silverstripe:5 Oct 14, 2024
8 checks passed
@mfendeksilverstripe mfendeksilverstripe deleted the feature/better-reports-ui branch October 14, 2024 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants