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

add decorator which forces django template admin #565

Merged
merged 3 commits into from
Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@
# https://docs.djangoproject.com/en/dev/ref/settings/#std:setting-TEMPLATES-BACKEND
"BACKEND": "django.template.backends.django.DjangoTemplates",
# https://docs.djangoproject.com/en/dev/ref/settings/#template-dirs
"DIRS": [APP_DIR / "templates"],
"DIRS": [APP_DIR / "dtl_templates"],
Copy link
Contributor

Choose a reason for hiding this comment

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

The only problem I see with this is that now we need to check every time there is a django update if these files have changed. I can't think of a better way to divorce them though given the constraints

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to include the dtl_tempaltes dir in this PR as well?

Copy link
Contributor Author

@alligatortower alligatortower Jul 11, 2023

Choose a reason for hiding this comment

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

In all cases being adequately captured by this PR (not all but hopefully enough):

  • The DTL will be used instead of Jinja2
  • DTL will look for templates defined locally
  • By default (unless we add one), it will never find a locally defined template, because even the base dir it's looking in does not exist
  • It will default back to the base django templates

specifically
re django update collisions: I don't think this makes us any more vulnerable to collisions than we would otherwise be
re adding the dir: It'll fail gracefully if the file isn't found: https://github.com/django/django/blob/d1855c4847215f3afe3708736be13388bb6437eb/django/template/loaders/filesystem.py#L20 (I did not include the dir in my test project)
edit: to be more exact, it'll fail gracefully assuming it does eventually find a template. and since the admin does not expect you to have defined any templates locally, we can be confident it'll find a default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I may have misinterpreted because of the line the comment was attached to -- If you're referring to the methods the decorator is overriding. yes absolutely that is vulnerable to the underlying django updating. I tried to minimize the surface area but it's a fundamentally hacky approach. EXTREMELY HAPPY to throw all of this away in favor of a less fragile approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

No you had it right the first comment, I meant the templates. That's awesome that'll just fail and go back to the default in django so I'm good with that. The methods you mentioned don't seem to scary about being updated too often and us needing to fix

"OPTIONS": {
# https://docs.djangoproject.com/en/dev/ref/settings/#template-loaders
# https://docs.djangoproject.com/en/dev/ref/templates/api/#loader-types
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from allauth.account.models import EmailAddress
from django.contrib.auth.admin import admin
from django.contrib.auth.models import Group
from django.contrib.sites.models import Site
from robots.admin import RuleAdmin
from robots.models import Rule, Url

from {{cookiecutter.repo_name}}.util.admin import force_django_template

admin.site.unregister(Rule)
admin.site.unregister(Url)
admin.site.unregister(Site)
admin.site.unregister(Group)
admin.site.unregister(EmailAddress)


@admin.register(Rule)
@force_django_template
class CustomRuleAdmin(RuleAdmin):
pass


@admin.register(Url)
@force_django_template
class UrlAdmin(admin.ModelAdmin):
pass


@admin.register(Site)
@force_django_template
class SiteAdmin(admin.ModelAdmin):
pass


@admin.register(Group)
@force_django_template
class GroupAdmin(admin.ModelAdmin):
pass


@admin.register(EmailAddress)
@force_django_template
class EmailAdmin(admin.ModelAdmin):
pass
Original file line number Diff line number Diff line change
@@ -1,5 +1,69 @@
# from django.contrib import admin # noqa
from django.contrib.admin.decorators import display
from django.contrib.admin.options import helpers
from django.contrib.auth.admin import admin
from django.forms.renderers import DjangoTemplates
from django.template.base import mark_safe

# from .models import TestFileModel # noqa

# admin.site.register(TestFileModel) # noqa
def force_django_template(AdminClass: type[admin.ModelAdmin]):
""" Replaces your admin class with one which forces the use of the DjangoTemplates renderer
Noted exceptions:
- sub-widgets rendered by the RelatedFieldWidgetWrapper
Usage:
@admin.register(MyModel)
@force_django_template
class MyModelAdmin(admin.ModelAdmin):
...
"""
action_form_class = None
if ActionFormClass := getattr(AdminClass, "action_form", None):

class ForcedDjangoActionForm(ActionFormClass):
default_renderer = DjangoTemplates()

action_form_class = ForcedDjangoActionForm

def get_form(self, request, obj=None, change=False, **kwargs):
form = AdminClass.get_form(self, request, obj=obj, change=change, **kwargs)
form.default_renderer = DjangoTemplates()
return form

def changelist_view(self, request, extra_context=None):
template_response = AdminClass.changelist_view(
self, request, extra_context=extra_context
)
template_response.using = "django"
return template_response

def get_changelist_form(self, request, **kwargs):
Form = AdminClass.get_changelist_form(self, request, **kwargs)
Form.renderer = DjangoTemplates()
return Form

def get_changelist_formset(self, request, **kwargs):
FormSet = AdminClass.get_changelist_formset(self, request, **kwargs)
FormSet.renderer = DjangoTemplates()
return FormSet

@display(description=mark_safe('<input type="checkbox" id="action-toggle">'))
def action_checkbox(self, obj):
"""
A list_display column containing a checkbox widget.
"""
del self
Copy link
Owner

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh
del [attr] is to just my LSP up from complaining about unused attrs. del just removes the variable from local (function) context. When you're defining an actual method in a class its smart enough to ignore self, but not here.

I was annoyed at having to do it at first, but grew to appreciate having a list of unused attrs at the top

return helpers.checkbox.render(
helpers.ACTION_CHECKBOX_NAME, str(obj.pk), renderer=DjangoTemplates()
)

return type(
f"{AdminClass.__name__}_forceddjangotemplates",
(AdminClass,),
{
"get_form": get_form,
"action_form": action_form_class,
"changelist_view": changelist_view,
"get_changelist_form": get_changelist_form,
"get_changelist_formset": get_changelist_formset,
"action_checkbox": action_checkbox,
},
)