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

Remove first_name / last_name from admin if missing from model #512

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

Conversation

silviogutierrez
Copy link

Closes #509

This is a "static" approach, which I think is better than overriding get_search_fields. I believe the get_* methods are for truly dynamic behavior (based on the request, the current user, etc). With this, admin checks will still apply (though not particularly useful).

@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #512 (01ac6ec) into master (fcb6c4c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
+ Coverage   94.22%   94.24%   +0.01%     
==========================================
  Files          30       30              
  Lines         900      903       +3     
==========================================
+ Hits          848      851       +3     
  Misses         52       52              
Impacted Files Coverage Δ
auditlog/admin.py 100.00% <100.00%> (ø)

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

@hramezani
Copy link
Member

@silviogutierrez Thanks for this path.

As I mention in the issue, overriding the get_search_fields would be a better approach. please implement it like this.
Also, you need to add some tests and a changelog entry in https://github.com/jazzband/django-auditlog/blob/master/CHANGELOG.md

f"actor__{get_user_model().USERNAME_FIELD}",
]
f"actor__{user_model.USERNAME_FIELD}",
] + (
Copy link

Choose a reason for hiding this comment

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

How about something like the following, which adds whichever field is available, handling the case where either fist_name or last_name do actually exist

    search_fields = [
        "timestamp",
        "object_repr",
        "changes",
        f"actor__{get_user_model().USERNAME_FIELD}",
    ] + [f"actor__{f.name}" for f in get_user_model()._meta.get_fields() if f.name in ["first_name", "last_name"]]

Copy link
Member

Choose a reason for hiding this comment

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

This is better because includes whatever fields that are available.
Feel free to create a new PR with tests and changelog

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.

Document or remove first_name / last_name as a search in the admin for custom users
3 participants