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

upgrades to admin: search_fields, list_filters and raw_id_field #1041

Merged
merged 4 commits into from
Dec 16, 2021

Conversation

PetrDlouhy
Copy link
Contributor

@PetrDlouhy PetrDlouhy commented Dec 16, 2021

Fixes:

  • long loading times of admin edit pages when many User/Token models

Description of the Change

I defined search_fields, list_filters and raw_id_fields for easier management of the tokens in admin.

Not sure what to test in this.
Also I think, this doesn't need any documentation, it is pretty self-explanatory to users.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@PetrDlouhy
Copy link
Contributor Author

This is my second take on this, I fixed the User without e-mail problem, that was in #723

@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #1041 (ddeb758) into master (78feec8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1041      +/-   ##
==========================================
+ Coverage   96.62%   96.64%   +0.01%     
==========================================
  Files          31       31              
  Lines        1748     1756       +8     
==========================================
+ Hits         1689     1697       +8     
  Misses         59       59              
Impacted Files Coverage Δ
oauth2_provider/admin.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78feec8...ddeb758. Read the comment docs.

@auvipy auvipy merged commit 2980117 into jazzband:master Dec 16, 2021
@auvipy
Copy link
Contributor

auvipy commented Dec 16, 2021

thank you

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

In hindsight, it would probably be a good thing for the Admin UI to be documented somewhere....

@PetrDlouhy
Copy link
Contributor Author

@n2ygk Which part of the Admin UI do you think should be documented. Do you mean the UI as general? Or just the search by e-mail functionality (which don't work if there is no e-mail)?

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2021

@PetrDlouhy not here, but as an update to the Tutorial it would probably be helpful to show users around the admin UI. I'll make an issue.

@@ -38,11 +44,15 @@ class GrantAdmin(admin.ModelAdmin):
class IDTokenAdmin(admin.ModelAdmin):
list_display = ("jti", "user", "application", "expires")
raw_id_fields = ("user",)
search_fields = ("token",) + (("user__email",) if has_email else ())

Choose a reason for hiding this comment

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

Is this valid? Searching from the admin now gives me django.core.exceptions.FieldError: Cannot resolve keyword 'token' into field. Choices are: access_token, application, application_id, created, expires, id, jti, scope, updated, user, user_id

@PetrDlouhy
Copy link
Contributor Author

@danlamanna You are right. Fixed in PR #1085

@charettes
Copy link

charettes commented May 17, 2024

FWIW it might have been better to search by USERNAME_FIELD of the user model instead as nothing guarantees that the email field will be indexed while USERNAME_FIELD must be to support the unique constraint defined on it.

@n2ygk
Copy link
Member

n2ygk commented May 21, 2024

FWIW it might have been better to search by USERNAME_FIELD of the user model instead as nothing guarantees that the email field will be indexed while USERNAME_FIELD must be to support the unique constraint defined on it.

Yes, that makes sense @charettes. A PR to change this would be appreciated.

@charettes
Copy link

@n2ygk done in #1428.

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.

5 participants