carltongibson / neapolitan

Quick CRUD views for Django
https://noumenal.es/neapolitan/
MIT License
456 stars 33 forks source link

Add role specific `get_context_data` methods #57

Open joshuadavidthomas opened 1 month ago

joshuadavidthomas commented 1 month ago

Recently I have found myself working on a CRUDView for a complex model with relationships to a bunch of ancillary models. The get_context_data method has started to become a bit overstuffed with variables I only ever need for one role or another.

It would be nice if the view offered something for role specific contexts, similar to get_FOO_display for model choices.

After playing around a bit, I quite like where I landed in my CRUDView subclass:

def get_context_data(self, **kwargs: object) -> dict[str, object]:
    context = super().get_context_data(**kwargs)
    role_context = self.get_role_context_data(context, **kwargs)
    if role_context is not None:
        context.update(role_context)
    return context

def get_role_context_data(
    self, context: dict[str, object], **kwargs: object
  ) -> dict[str, object] | None:
    if not hasattr(self, "role"):
        return None

    func_name = f"get_{self.role.value}_context_data"
    func = getattr(self, func_name, None)

    if func is None or not callable(func):
        return None

    role_context = func(context=context, **kwargs)

    if not isinstance(role_context, dict):
        msg = f"`{func_name}` must return a dictionary"
        raise ValueError(msg)

    return role_context

This allows for defining role specific context data functions, e.g. get_create_context_data - key line being role_context = func(context=context, **kwargs). They need to be defined with **kwargs and return a dict as I have it now, but you could probably even do away with the kwargs requirement somehow if desired.

carltongibson commented 1 month ago

Hi @joshuadavidthomas. Thanks for the report.

Let me first acknowledge the issue. Yes, something here would be good.

Then your method should probably not return an Optional, but rather an empty dictionary where it currently returns None. That would simplify the call sites it seems.

I'm going to ask you to stick with this for the moment.

I'm working through several related thoughts to do with roles, and customisation, and I won't have a better answer for you until that falls out. Until then adding the hook as you've done is perfectly fine.