Merge lp:~salgado/django-group-access/delegate-to-parent-relation into lp:django-group-access

[Guilherme Salgado]

This makes it possible to use access_relation (former access_child_relation) to do the access-group checking on a parent as well as a child relation.

I've also removed a .filter() call which seemed redundant in accessible_by_user().

[lp:~salgado/django-group-access/delegate-to-parent-relation]

5. By Guilherme Salgado on 2011-09-19

Revert my removal of the (seemingly redundant) .filter() call and add a comment explaining why it's there

[Nicola Heald (notnownikki)]

review: Approve

=== modified file 'models.py'
--- models.py	2011-09-13 13:50:21 +0000
+++ models.py	2011-09-19 15:07:26 +0000
@@ -11,18 +11,22 @@
     def accessible_by_user(self, user):
         if AccessGroup.objects.filter(members=user, supergroup=True).count():
             return self.all()
-        if hasattr(self.model, 'access_child_relation'):
-            acr = getattr(self.model, 'access_child_relation')
+        if hasattr(self.model, 'access_relation'):
+            acr = getattr(self.model, 'access_relation')
             k = '%s__access_groups__in' % acr
             access_groups_dict = {k: AccessGroup.objects.filter(members=user)}
             k = '%s__isnull' % acr
-            no_child_records = {k: True}
+            no_related_records = {k: True}
             k = '%s__owner' % acr
             direct_owner_dict = {k: user}
             available = self.filter(
                 models.Q(**access_groups_dict) |
                 models.Q(**direct_owner_dict) |
-                models.Q(**no_child_records)).distinct()
+                models.Q(**no_related_records)).distinct()
+            # Although this extra .filter() call seems redundant it turns out
+            # to be a huge performance optimization.  Without it the ORM will
+            # join on the related tables and .distinct() them, which killed
+            # performance in HEXR leading to 30+ seconds to load a page.
             return self.filter(pk__in=available)
         else:
             available = self.filter(

=== modified file 'sandbox/models.py'
--- sandbox/models.py	2011-09-16 18:17:01 +0000
+++ sandbox/models.py	2011-09-19 15:07:26 +0000
@@ -9,7 +9,7 @@
 
 class AccessRestrictedParent(models.Model):
     name = models.CharField(max_length = 64)
-    access_child_relation = 'accessrestrictedmodel'
+    access_relation = 'accessrestrictedmodel'
     
     objects = AccessManager()
     
@@ -33,4 +33,38 @@
     def __unicode__(self):
         return self.name
 
+
+class Project(AccessGroupMixin):
+    owner = models.ForeignKey(User, null=True)
+    name = models.CharField(max_length = 64)
+    objects = AccessManager()
+
+    def __unicode__(self):
+        return self.name
+
+
+class Build(AccessGroupMixin):
+    owner = models.ForeignKey(User, null=True)
+    name = models.CharField(max_length = 64)
+    project = models.ForeignKey(Project)
+    access_relation = 'project'
+    objects = AccessManager()
+
+    def __unicode__(self):
+        return self.name
+
+
+class Release(models.Model):
+    owner = models.ForeignKey(User, null=True)
+    name = models.CharField(max_length = 64)
+    build = models.ForeignKey(Build)
+    # We don't have a direct reference to the parent which is
+    # accessgroup-controlled, so we have to go via Build.
+    access_relation = 'build__project'
+    objects = AccessManager()
+
+    def __unicode__(self):
+        return self.name
+
+
 post_save.connect(process_auto_share_groups, AccessRestrictedModel)

=== modified file 'tests.py'
--- tests.py	2011-09-16 18:17:01 +0000
+++ tests.py	2011-09-19 15:07:26 +0000
@@ -1,3 +1,5 @@
+import itertools
+
 from django.test import TestCase
 from django.conf import settings
 from django.core.management import call_command
@@ -6,6 +8,9 @@
 from django_group_access.sandbox.models import (
     AccessRestrictedModel,
     AccessRestrictedParent,
+    Build,
+    Project,
+    Release,
 )
 from django_group_access.models import AccessGroup, Invitation
 
@@ -31,6 +36,64 @@
         loading.cache.loaded = False
 
 
+class AccessRelationTests(SyncingTestCase):
+
+    def setUp(self):
+        super(AccessRelationTests, self).setUp()
+        self.owner = _create_user()
+        self.project = Project(owner=self.owner, name='project')
+        self.project.save()
+        self.build = Build(
+            owner=self.owner, name='build', project=self.project)
+        self.build.save()
+        self.release = Release(
+            owner=self.owner, name='release', build=self.build)
+        self.release.save()
+        group = self._create_access_group_with_one_member()
+        self.project.access_groups.add(group)
+        self.project.save()
+        self.user_with_access = group.members.all()[0]
+        self.user_without_access = _create_user()
+
+    def _create_access_group_with_one_member(self):
+        g = AccessGroup(name='oem')
+        g.save()
+        g.members.add(_create_user())
+        g.save()
+        return g
+
+    def test_direct_reference(self):
+        # Build has a foreign key to Project so its access_relation just need
+        # to point to project and we'll take advantage of Django's ORM to do
+        # the access group checks on Project.
+        query_method = Build.objects.accessible_by_user
+
+        self.assertEqual('project', Build.access_relation)
+        self.assertEqual(
+            [self.build.name], [b.name for b in query_method(self.owner)])
+        self.assertEqual(
+            [self.build.name],
+            [b.name for b in query_method(self.user_with_access)])
+        self.assertEqual(
+            [], [b for b in query_method(self.user_without_access)])
+
+    def test_indirect_reference(self):
+        # Release has no foreign key to Project, but it has one to Build
+        # and it can use that to tell us to do the access group checks on
+        # Project.
+        query_method = Release.objects.accessible_by_user
+
+        self.assertEqual('build__project', Release.access_relation)
+        self.assertEqual(
+            [self.release.name], [r.name for r in query_method(self.owner)])
+        self.assertEqual(
+            [self.release.name],
+            [r.name for r in query_method(self.user_with_access)])
+        self.assertEqual(
+            [], [r for r in query_method(self.user_without_access)])
+
+
+
 class InvitationTest(TestCase):
 
     def setUp(self):
@@ -238,3 +301,11 @@
         self.assertEqual(available[6].name, 'the cabal record 2')
         self.assertEqual(available[7].name, 'the cabal record extra')
         self.assertEqual(available[8].name, 'the stonecutters record 1')
+
+
+counter = itertools.count()
+def _create_user():
+    random_string = 'asdfg%d' % counter.next()
+    user = User.objects.create_user(
+        random_string, '[e-mail]' % random_string)
+    return user