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 imports for builtin django User model #2012

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

rhysyngsun
Copy link
Contributor

@rhysyngsun rhysyngsun commented Feb 5, 2025

What are the relevant tickets?

Part of https://github.com/mitodl/hq/issues/6679

Description (What does it do?)

  • Adds a lint to block direct importing of django.contrib.auth.models.User
  • Replaces all such imports with get_user_model() or settings.AUTH_USER_MODEL

How can this be tested?

You should be able to run the app without issue.

You can also test the lint rule by adding an import to django.contrib.auth.models.User in any file and run pre-commit run and it should error.

You can observe that the initial commit for this PR that added this check failed for all of the existing usages: https://results.pre-commit.ci/run/github/672068771/1738783559.KuMD_PjETSqF69WSAV5Ufg

@rhysyngsun rhysyngsun added the Needs Review An open Pull Request that is ready for review label Feb 5, 2025
@rhysyngsun rhysyngsun force-pushed the nl/fix_user_model_refs branch from ff987f5 to 011c959 Compare February 5, 2025 19:33
@shanbady shanbady self-assigned this Feb 5, 2025
@rhysyngsun rhysyngsun force-pushed the nl/fix_user_model_refs branch from 011c959 to 6a35cb4 Compare February 5, 2025 19:37
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

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

worked in catching the User import. had a question about the if TYPE_CHECKING: condition

@@ -25,6 +25,11 @@
)
from main.models import TimestampedModel, TimestampedModelQuerySet

if TYPE_CHECKING:
from django.contrib.auth import get_user_model

Copy link
Contributor

Choose a reason for hiding this comment

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

what does this code do? It doesnt seem like User is referenced anywhere in this file. Might be worth leaving a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shanbady shanbady added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Feb 5, 2025
@rhysyngsun rhysyngsun mentioned this pull request Feb 5, 2025
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

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

LGTM

@rhysyngsun rhysyngsun force-pushed the nl/fix_user_model_refs branch from 6a35cb4 to 1357f29 Compare February 5, 2025 21:33
@rhysyngsun rhysyngsun merged commit f59e570 into main Feb 5, 2025
11 checks passed
@rhysyngsun rhysyngsun deleted the nl/fix_user_model_refs branch February 5, 2025 21:43
@odlbot odlbot mentioned this pull request Feb 6, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants