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

Pagination and filtering #1220

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

RumitAP
Copy link

@RumitAP RumitAP commented Jul 31, 2023

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):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the pre-commit run command to format and lint.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1220 (7a34825) into master (29108a5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7a34825 differs from pull request most recent head f2f3ba0. Consider uploading reports for the commit f2f3ba0 to get more accurate results

@@           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           
Files Coverage Δ
simple_history/admin.py 98.66% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@ddabble ddabble left a 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 :)

simple_history/admin.py Outdated Show resolved Hide resolved
simple_history/admin.py Outdated Show resolved Hide resolved
simple_history/admin.py Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
docs/admin.rst Outdated Show resolved Hide resolved
docs/admin.rst Outdated Show resolved Hide resolved
@jurrian
Copy link

jurrian commented Sep 4, 2023

@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.

@vitormhenrique
Copy link

I'm going to try get the adjustments done this week! @RumitAP

@jurrian
Copy link

jurrian commented Oct 4, 2023

@vitormhenrique @RumitAP is there already some progress? Happy to help.

@RumitAP
Copy link
Author

RumitAP commented Oct 4, 2023

Hey sorry, I have this done. Ill push this up tonight

@RumitAP RumitAP requested a review from ddabble October 5, 2023 02:56
@RumitAP
Copy link
Author

RumitAP commented Oct 5, 2023

@jurrian should be ready to merge in now, completed comments and added testing

Copy link
Author

@RumitAP RumitAP left a comment

Choose a reason for hiding this comment

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

Completed

Copy link

@jurrian jurrian left a 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
Copy link

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?

Copy link
Author

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

Copy link

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.

@jurrian
Copy link

jurrian commented Oct 20, 2023

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.

@jurrian jurrian mentioned this pull request Nov 9, 2023
11 tasks
@jurrian
Copy link

jurrian commented Nov 9, 2023

I've created a new PR (#1277) based on this one with some fixes.

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.

Add Pagination and Filtering No pagination in admin list (a long history fails or super slow to load)
5 participants