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

Allow dynamic model registration #140

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

Conversation

Etenil
Copy link
Contributor

@Etenil Etenil commented May 18, 2020

The built-in User model can't be fetched by a apps.get_model() nor given as class as they would go in a recursive import.

# settings.py
from django.contrib.auth import get_user_model() # <= recursive import of settings!

This allows to do the following instead:

# settings.py
def lazy_user_model():
    from django.contrib.auth import get_user_model()
    return get_user_model()

DJANGO_EASY_AUDIT_REGISTERED_CLASSES = [
    lazy_user_model
]

@jheld
Copy link
Collaborator

jheld commented May 19, 2020

thank you for this. Can you please add at least one test that utilizes this?

@jheld jheld self-requested a review May 19, 2020 01:15
@Etenil
Copy link
Contributor Author

Etenil commented May 19, 2020

@jheld I had trouble testing this, as the lazy invocation for the dynamic model happens when the settings are loaded. So override_settings won't actually re-run the settings loading logic, it only overrides the pre-loaded settings.

Instead I've opted to pre-register the models in settings.py using both the dynamic and string registrations. Commenting any of them out causes the tests to fail.

Let me know if this is good enough or if you have a better idea on how to proceed.

@jheld
Copy link
Collaborator

jheld commented May 19, 2020

@Etenil I appreciate the effort you've put in.

We might merge as is.

I had a feeling this might happen. I think I got around it with the delta/changed fields setting by instead of importing the setting variable directly, doing a getattr on settings when doing the logic. Would that work for you? I think override_settings still works in that case -- and I think/hope I created a test case/altered a test case for it. If that doesn't work/taking too much time to implement, we'll merge as is.

@Etenil
Copy link
Contributor Author

Etenil commented May 27, 2020

@jheld I'm not really sure how to do what you mentioned, would you have an example?

@jheld
Copy link
Collaborator

jheld commented May 27, 2020

Yes. I think I have both a test case (and supporting code in the library which is being tested) for the delta/changed field on/off flag. The setting that's being tested is in the README. It was I think one of the last betas of the current official release.

If you have trouble finding it let me know I can add more detail to here.

@jheld
Copy link
Collaborator

jheld commented Aug 7, 2020

@Etenil can you rebase? We may not require an additional test, but want to make sure things are still passing.

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.

2 participants