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

Custom Roles #73

Open
dennisstritzke opened this issue Nov 27, 2024 · 1 comment
Open

Custom Roles #73

dennisstritzke opened this issue Nov 27, 2024 · 1 comment
Assignees

Comments

@dennisstritzke
Copy link

Version 24.8 added a Role equality test, stating ‘[…] preparatory to custom roles.’ As I utilise custom roles already, I want to share my take on them.

How are you imagining custom roles to work?

The Use Case

I want to add additional behaviour to a CRUDView, without having to repeat all the niceties of Neapolitan. Imagine a LeadCRUDView where I would like to add a POST /lead/<uuid:pk>/mark_qualified action.

Currently, we are not able to extend the Neapolitan Roles because they are Enum values. To make it work anyways, I created a CustomRole base class (see below), which replicates to Role Enums behaviour.

Observations and Challenges

  1. Consistency: This approach allows arbitrary functionality to be added in a structured manner.
  2. Complexity in CRUDViews: Adding many methods through roles can make CRUDViews unwieldy. Delegating method definitions to the roles themselves would improve legibility but complicate cross-method interactions.
  3. Fragmentation: Role-related logic is currently scattered across:
    (1) Role handlers
    (2) The role itself
    (3) Context data.
  4. Neapolitan should provide Roles, which are extendable by default.

An Example

The Role

class MarkQualifiedRole(CustomRole):
    permission_name = "leadtool.mark_lead_qualified"
    url_name_component = "markqualified"

    def handlers(self):
        return {"post": "mark_qualified"}

    def url_pattern(self, view_cls):
        url_base = view_cls.url_base
        url_kwarg = view_cls.lookup_url_kwarg or view_cls.lookup_field
        path_converter = view_cls.path_converter

        return f"{url_base}/<{path_converter}:{url_kwarg}>/mark_qualified/"

The view

class LeadBackofficeView(CRUDView):
    def mark_qualified(self, request, *args, **kwargs):
        self.object = self.get_object()
        self.object.status = LeadStatus.PRE_QUALIFIED
        self.object.save()

        return HttpResponseRedirect(reverse("lead-list"))

    @classonlymethod
    def get_urls(cls, roles=None):
        return super().get_urls(roles=[
            Role.LIST,
            Role.CREATE,
            Role.DELETE,
            MarkQualifiedRole(),
        ])

    ...

Base Class and Test

from abc import ABC, abstractmethod

from neapolitan.views import Role


class CustomRole(ABC):
    def __eq__(self, other):
        return self.__class__ == other.__class__

    def __hash__(self):
        return hash(self.__class__)

    @abstractmethod
    def handlers(self):
        raise NotImplementedError

    def extra_initkwargs(self):
        return {}

    @property
    @abstractmethod
    def url_name_component(self):
        raise NotImplementedError

    @abstractmethod
    def url_pattern(self, view_cls):
        raise NotImplementedError


setattr(CustomRole, "get_url", Role.get_url)
setattr(CustomRole, "reverse", Role.reverse)
setattr(CustomRole, "maybe_reverse", Role.maybe_reverse)

Test ensuring the Role doesn't break in obvious ways during Neapolitan upgrades.

from unittest import TestCase

from neapolitan.views import Role

from django_backoffice.role import CustomRole


class TestCustomRole(TestCase):
    def test_has_role_methods(self):
        """
        The CustomRole is a way of providing non-standard behaviour to Neapolitan views. If one of the methods is
        outright missing, things will go most likely wrong somewhere.

        This test is mainly a safeguard during upgrades of Neapolitan.
        """
        role_method_names = [
            method_name
            for method_name in Role.__dict__
            if not method_name.startswith("_")  # No private members and magic methods
            and not method_name.isupper()  # no enum members
        ]

        for method_name in role_method_names:
            with self.subTest(f"Has method {method_name}"):
                self.assertTrue(
                    hasattr(CustomRole, method_name),
                    f"{CustomRole.__name__} is missing method {method_name} of {Role.__name__}",
                )
@carltongibson
Copy link
Owner

Hi @dennisstritzke — thanks for opening the issue here. Quick summary is that my WIP is not dissimilar.

The reason for 80090e5 was that I allow custom roles to proxy for the default roles, and so override those. Using the identity is check didn't allow that.

I'm slowly swinging towards extracting all this from the WORK project, and then I'll be opening a PR here. I'll be sure to mention that here. (But you shouldn't have too much to worry about in terms of breakages I'd think.)

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

No branches or pull requests

2 participants