-
-
Notifications
You must be signed in to change notification settings - Fork 484
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
Pagination and filtering #1220
Pagination and filtering #1220
Conversation
…better performance
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #1220 +/- ##
=======================================
Coverage 96.87% 96.87%
=======================================
Files 23 23
Lines 1278 1282 +4
Branches 211 211
=======================================
+ Hits 1238 1242 +4
Misses 21 21
Partials 19 19
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice having pagination finally implemented! 😊 I have a couple change requests, however; see below.
It would also be great if you could add some tests for this :)
@RumitAP are you going to proceed with this PR? Do you need help? Seems like it just needs a little extra push to be integrated. |
I'm going to try get the adjustments done this week! @RumitAP |
@vitormhenrique @RumitAP is there already some progress? Happy to help. |
Hey sorry, I have this done. Ill push this up tonight |
Co-authored-by: Anders <[email protected]>
Co-authored-by: Anders <[email protected]>
for more information, see https://pre-commit.ci
…/django-simple-history into pagination-and-filtering
for more information, see https://pre-commit.ci
…/django-simple-history into pagination-and-filtering
@jurrian should be ready to merge in now, completed comments and added testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work so far.
I could be mistaken but I checked out your branch locally and the pagination does not show up. I would expect some HTML like this being rendered but I cannot find it anywhere. Perhaps some part has not been committed yet?
@@ -62,6 +68,7 @@ admin class | |||
list_display = ["id", "name", "status"] | |||
history_list_display = ["status"] | |||
search_fields = ['name', 'user__username'] | |||
history__list_per_page = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not history_list_per_page
with one dash as is the pattern with the other naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not exactly sure why you're not seeing the pagination because I just copied in what SimpleHistory as it is in my commits here into our own code and it seems to be working just fine with the pagination. I copied in both the object_history.html and SimpleHistoryAdmin class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I look at the files changed in this PR, I see no html file added or changed. And I guess the change as in the link I showed should be in _object_history_list.html
for it to work.
We need this change too for our project and we are waiting for it. So if you want I can fix the problems so it can get merged. |
I've created a new PR (#1277) based on this one with some fixes. |
Closing in favor of #1277 |
Description
This will add pagination and filtering to the simple history admin view. This is needed because it will allow more performance views on instances with many historical records. Currently it is very likely to timeout depending on the settings .
Related Issue
Closes #1117 and closes #1219.
Motivation and Context
This is needed because it will allow more performance views on instances with many historical records. Currently it is very likely to timeout depending on the settings .
How Has This Been Tested?
We are currently using this in production and would like to have this natively live in this package.
Screenshots (if appropriate):
Types of changes
Checklist:
pre-commit run
command to format and lint.AUTHORS.rst
CHANGES.rst