CRUDView does not work with namespaced urlpatterns #16
Comments
|
Yeah... I basically never use namespaces, since they're a bit of a pain, and real life URL name collisions are quite rare. Maybe |
|
Yeah agreed. I like them, but every time I use them I find myself thinking "you know, I could just name all of these urls explicitly with a prefix instead of a namespace". In any case, I had noodled on how to support it - I can see both of these being possibly good ways to handle it: as well as: I suppose though, it does make a little more sense in get_urls(), because generating the urls is when you'd "decide" the url namespace...but the problem with that is since get_urls() is a classmethod, it can't set the namespace for the CRUDView, which needs to have the namespace stored so it knows how to do its reverse() calls correctly. It seems to me like maybe the latter approach with the attribute in the CRUDView is the only way that would work in the current API? |
|
It's the |
|
More or less, I think I'd lean to not namespacing unless a proof-of-concept can demonstrate that adding it is not going to cause issues. Currently we're able to programmatically construct the right view names, like so: And similar. We'll need some complexity of different URL patterns maybe, but everywhere and always needing to handle an optional namespace there seems like quite a lot of grief. Happy to be shown otherwise, but it's not something I'm going to put thought to myself at this stage. |
|
Makes sense to me! |
|
For reference, there was good discussion about this on fosstodon https://fosstodon.org/@carlton/110503156769347222 I'm not adverse to looking at something here if it's clean, but I'm not personally going to work on it, so closing for that reason. |
|
Reopening this, as it might be feasible now that URL name generation AND reversing lives within the view. Still not 100% convinced but it's gone from No to Maybe. |
|
I've had a go at supporting this with a mixin and subclassed UsageThe key is you have to specify the namespace, as follows: # apps/fstp/views.py
class TenderCRUDView(NominoCRUDView):
model = models.Tender
namespace = "fstp" #<----
fields = ["related_customer","name","submission_date","current_version",]The app's # apps/fstp/urls.py
from django.urls import path
from . import views
app_name = "fstp"
urlpatterns = [
path("test-import/", views.test_import_view, name="test_import"),
]
urlpatterns += views.TenderCRUDView.get_urls()OverridesSome of the methods shown below do not change functionality; they are only needed:
MixinThe only change really needed is to the from django import template
from django.utils.safestring import mark_safe
register = template.Library()
from django.urls import reverse, NoReverseMatch
def action_links(view, object):
prefix = view.get_prefix()
actions = [
(url, name)
for url, name in [
(
view.safe_reverse(f"{prefix}-detail", kwargs={"pk": object.pk}),
"View",
),
(
view.safe_reverse(f"{prefix}-update", kwargs={"pk": object.pk}),
"Edit",
),
(
view.safe_reverse(f"{prefix}-delete", kwargs={"pk": object.pk}),
"Delete",
),
]
if url is not None
]
links = [f"<a href='{url}'>{anchor_text}</a>" for url, anchor_text in actions]
return mark_safe(" | ".join(links))
@register.inclusion_tag("nominopolitan/partial/detail.html")
def object_detail(object, fields):
# ---> original code goes here
@register.inclusion_tag("nominopolitan/partial/list.html")
def object_list(objects, view):
# ---> original code goes hereSubclass of CRUDViewI used a mixin from neapolitan.views import CRUDView, Role
from django.urls import NoReverseMatch, path, reverse
from django.utils.decorators import classonlymethod
from django.core.exceptions import ImproperlyConfigured
import logging
log = logging.getLogger(__name__)
class NominopolitanMixin:
def reverse(self, role, view, object=None):
"""Allows for reverse of namespaced URLS.
Args:
role (Role): Needed to work in mixin. If if Role this would be self.
view (CRUDView): The view from CRUDView or its subclass
object (_type_, optional): As per original Role. Defaults to None.
"""
url_name = (
f"{view.namespace}:{view.url_base}-{role.url_name_component}"
if view.namespace
else f"{view.url_base}-{role.url_name_component}"
)
url_kwarg = view.lookup_url_kwarg or view.lookup_field
match role:
case Role.LIST | Role.CREATE:
return reverse(url_name)
case _:
if object is None:
raise ValueError("Object required for detail, update, and delete URLs")
return reverse(
url_name,
kwargs={url_kwarg: getattr(object, view.lookup_field)},
)
# *** No changes in functionality. Only required to work in mixin structure
def maybe_reverse(self, view, object=None):
"""
Required in order to work in mixin.
No change to functionality from Role.
"""
try:
return self.reverse(view, object)
except NoReverseMatch:
return None
@staticmethod
def get_url(role, view_cls):
"""
No change in functionality from Role.get_url but needs to be
a static method to work in the mixin.
"""
return path(
role.url_pattern(view_cls),
view_cls.as_view(role=role),
name=f"{view_cls.url_base}-{role.url_name_component}",
)
class NominoCRUDView(NominopolitanMixin, CRUDView):
"""Base class for all CRUD views with custom functionality."""
class Meta:
abstract = True
# Add new attribute namespace
namespace = None
def get_prefix(self):
"""Helper function to get the prefix for the URL name"""
return f"{self.namespace}:{self.url_base}" if self.namespace else self.url_base
def safe_reverse(self, viewname, kwargs=None):
"""Attempt to reverse a URL, returning None if it fails."""
try:
return reverse(viewname, kwargs=kwargs)
except NoReverseMatch:
return None
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
# Override the create_view_url to use our namespaced reverse
view_name = f"{self.get_prefix()}-create"
context["create_view_url"] = self.safe_reverse(view_name)
return context
def get_success_url(self):
"""
Override to include self.namespace if it exists, using self.get_prefix()
"""
assert self.model is not None, (
"'%s' must define 'model' or override 'get_success_url()'"
% self.__class__.__name__
)
url_name = f"{self.get_prefix()}-list"
if self.role is Role.DELETE:
success_url = reverse(url_name)
else:
detail_url = f"{self.get_prefix()}-detail"
success_url = reverse(detail_url, kwargs={"pk": self.object.pk})
return success_url
# *** Below methods only required in order to work in specific subclassed app structure.
# If modifying neapolitan these would not be necessary.
def get_template_names(self):
"""Only required to work in with subclassed app templates.
Changes `f"neapolitan/object"` to `f"nominopolitan/object"`
"""
if self.template_name is not None:
return [self.template_name]
if self.model is not None and self.template_name_suffix is not None:
return [
f"{self.model._meta.app_label}/"
f"{self.model._meta.object_name.lower()}"
f"{self.template_name_suffix}.html",
# change to use specified app templates
f"nominopolitan/object{self.template_name_suffix}.html",
]
msg = (
"'%s' must either define 'template_name' or 'model' and "
"'template_name_suffix', or override 'get_template_names()'"
)
raise ImproperlyConfigured(msg % self.__class__.__name__)
@classonlymethod
def get_urls(cls, roles=None):
"""Required to specify mixin as class"""
if roles is None:
roles = iter(Role)
return [NominopolitanMixin.get_url(role, cls) for role in roles] |
Perhaps this isn't very high priority for now and could just be documented as a known limitation, and I know you're potentially going to make API changes anyway. But I thought it was worth mentioning.
A simple example here: https://github.com/LucidDan/neapolitan-example - the main branch is a working copy of the tutorial project, slightly altered to use a projects.urls module.
The branch namespaces breaks due to the reverse urls not being valid, because of the addition of
app_name = "projects"in the projects.urls module:I really like url namespaces and find them extremely useful in large projects, but even the Django implementation itself is a bit magical and complicated to use...I'd totally understand if you decided its better to just say "yeah this doesn't work, make sure you do get_urls() from your main project urls.py" 🤷
The text was updated successfully, but these errors were encountered: