Add more docs - addressing feedback from code review

apps/accounts/migrations/0056_populate_existing_permissions.py

@@ -67,7 +67,10 @@ def _manage_permission(apps, schema_editor, manage_func):
     Datacenter_historical_model = apps.get_model("accounts", "Datacenter")
     Hostingprovider_historical_model = apps.get_model("accounts", "Hostingprovider")
 
-    # retrieve data based on queryset using historical model to access "created_by" field
+    # retrieve (object, user) ID pairs eligible for assigning permissions
+    # based on queryset of historical model - so that we can access "created_by" field
+    # that is present at the time of this migration.
+    # See more details: https://docs.djangoproject.com/en/4.2/topics/migrations/#historical-models
     eligible_pairs_dc = [
         (dc.id, dc.created_by.id)
         for dc in Datacenter_historical_model.objects.using(db_alias).filter(
@@ -80,13 +83,21 @@ def _manage_permission(apps, schema_editor, manage_func):
             created_by__isnull=False
         )
     ]
-    # Get current models to retrieve the objects for assigning permissions.
+
+    # GOTHA: django-guardian shortcut methods for assigning/removing perms
+    # *do not* support historical models! See details: https://github.com/django-guardian/django-guardian/issues/281#issuecomment-156264129
+    #
+    # As a workaround, we get *current* models to retrieve the objects for assigning permissions
+    # based on the ID pairs we retrieved in the previous step.
+    # Notice that `django_apps.get_model` is used instead of `apps.get_model`.
+    #
     # This is dangerous! Future versions of django-guardian might change the schema
     # and this migration might fail when ran from scratch. If this happens:
     # - pin down the version of django-guardian to the latest working version
     #   so that migrations work for tests
     # - for production / staging it's a non-issue since we always keep snapshots around
     #   and don't run older migrations
+
     Datacenter = django_apps.get_model("accounts", "Datacenter")
     Hostingprovider = django_apps.get_model("accounts", "Hostingprovider")
     User = get_user_model()

apps/accounts/models/user.py

@@ -98,6 +98,10 @@ def hosting_providers_explicit_perms(self) -> models.QuerySet[Hostingprovider]:
             - superuser status
 
         This method is useful to fetch hosting providers for users that belong to the admin group.
+        Because the admin group has global permissions to manage all objects, the admin page
+        or provider portal page (where we show all objects that the users can manage)
+        for the admin group members would have to fetch all available objects. To avoid this,
+        we settled on displaying objects with explicit user-object permissions only.
         """
         return get_objects_for_user(
             self,
@@ -127,6 +131,10 @@ def data_centers_explicit_perms(self) -> models.QuerySet[Datacenter]:
             - superuser status
 
         This method is useful to fetch data centers for users that belong to the admin group.
+        Because the admin group has global permissions to manage all objects, the admin page
+        or provider portal page (where we show all objects that the users can manage)
+        for the admin group members would have to fetch all available objects. To avoid this,
+        we settled on displaying objects with explicit user-object permissions only.
         """
         return get_objects_for_user(
             self,

apps/accounts/permissions.py

@@ -1,13 +1,27 @@
 from dataclasses import dataclass
 
 
-"""
-Definitions for object-level permissions for the `accounts` app.
-"""
-
-
 @dataclass
 class Permission:
+    """
+    This data class provides attributes and methods
+    for the expected representations of object-level permissions in the "accounts" app.
+
+    It is entirely independent of django.contrib.auth.models.Permission.
+
+    The purpose of this data class is to be used in various places in the code
+    (migrations, views, tests) that expect a a permission in various representations.
+    As an example:
+    - guardian.shortcuts methods assign_perm and remove_perm expect the codename of the permission as an argument
+    if the content type of the object can be determined, or full permission name otherwise.
+    We can pass `permissions.manage_provider.codename` or `permissions.manage_provider.full_name`
+    instead of passing strings: "manage_provider" or "accounts.manage_provider"
+
+    The actual object-level permission (based on django.contrib.auth.models.Permission)
+    is created by django-guardian when it picks up the definition of the `Meta` class
+    of a given model.
+    """
+
     codename: str
     full_name: str
     description: str
@@ -19,9 +33,13 @@ def __str__(self):
         return self.full_name
 
 
+"""
+Definitions for object-level permissions for the `accounts` app
+"""
+
 manage_provider = Permission(
     "manage_provider", "accounts.manage_provider", "Manage provider"
 )
 manage_datacenter = Permission(
     "manage_datacenter", "accounts.manage_datacenter", "Manage datacenter"
-)
+)

apps/greencheck/tests/test_api_asn.py

@@ -238,7 +238,7 @@ def test_can_only_destroy_asns_for_own_hosting_provider(
 
         rf = APIRequestFactory()
         url_path = reverse("asn-detail", kwargs={"pk": green_asn.id})
-        # hosting_provider_with_sample_user.users.first()
+
         request = rf.delete(url_path)
         request.user = another_host_user