Merge pull request #165 from somewes/develop

fix entity group is_active param logic

Author: somewes

entity/models.py

@@ -321,8 +321,14 @@ def get_membership_cache(self, group_ids=None, is_active=True):
         :rtype: dict
         """
         membership_queryset = EntityGroupMembership.objects.filter(
-            Q(entity__isnull=True) | (Q(entity__isnull=False) & Q(entity__is_active=is_active))
+            # Select all memberships that are defined by a sub entity kind only
+            Q(entity__isnull=True) |
+            # Select memberships that define a single entity (null kind) and respect active flag
+            (Q(entity__isnull=False) & Q(sub_entity_kind__isnull=True) & Q(entity__is_active=is_active)) |
+            # Select memberships that are all of a kind under an entity and only query active supers
+            (Q(entity__isnull=False) & Q(sub_entity_kind__isnull=False) & Q(entity__is_active=True))
         )
+
         if is_active is None:
             membership_queryset = EntityGroupMembership.objects.all()
 
@@ -387,7 +393,10 @@ def get_all_entities(self, membership_cache=None, entities_by_kind=None, return_
             membership_cache = EntityGroup.objects.get_membership_cache([self.id], is_active=is_active)
 
         if entities_by_kind is None:
-            entities_by_kind = entities_by_kind or get_entities_by_kind(membership_cache=membership_cache)
+            entities_by_kind = entities_by_kind or get_entities_by_kind(
+                membership_cache=membership_cache,
+                is_active=is_active,
+            )
 
         # Build set of all entity ids for this group
         entity_ids = set()
@@ -584,19 +593,29 @@ def get_entities_by_kind(membership_cache=None, is_active=True):
                     kinds_with_all.add(entity_kind_id)
 
     # Get entities for 'all'
-    all_entities_for_types = Entity.objects.filter(
-        entity_kind_id__in=kinds_with_all
-    ).values_list('id', 'entity_kind_id')
+    all_entities_for_types = Entity.all_objects.filter(
+        entity_kind_id__in=kinds_with_all,
+    )
+    if is_active is not None:
+        all_entities_for_types = all_entities_for_types.filter(is_active=is_active)
+
+    all_entities_for_types = all_entities_for_types.values_list('id', 'entity_kind_id')
 
     # Add entity ids to entity kind's all list
     for id, entity_kind_id in all_entities_for_types:
         entities_by_kind[entity_kind_id]['all'].append(id)
 
-    # Get relationships
+    # Get relationships for memberships defined by all of a kind under a super
     relationships = EntityRelationship.objects.filter(
         super_entity_id__in=super_ids,
-        sub_entity__entity_kind_id__in=kinds_with_supers
-    ).values_list(
+        sub_entity__entity_kind_id__in=kinds_with_supers,
+    )
+
+    # Make sure to respect the active flag for the sub entities under the supers
+    if is_active is not None:
+        relationships = relationships.filter(sub_entity__is_active=is_active)
+
+    relationships = relationships.values_list(
         'super_entity_id', 'sub_entity_id', 'sub_entity__entity_kind_id'
     )
 

entity/tests/model_tests.py

@@ -1030,6 +1030,7 @@ def test_get_all_entities_active(self):
         turn_off_syncing()
 
         account_type = ContentType.objects.get_for_model(Account)
+        team_type = ContentType.objects.get_for_model(Team)
 
         # Set up teams
         teams = Team.objects.bulk_create([
@@ -1069,6 +1070,43 @@ def test_get_all_entities_active(self):
             [account_entities[3], None],
         ])
 
+        # Group 0 consists of an inactive account (0) and 3 actives (1-3)
         self.assertEqual(len(entity_groups[0].get_all_entities()), 3)
         self.assertEqual(len(entity_groups[0].get_all_entities(is_active=False)), 1)
         self.assertEqual(len(entity_groups[0].get_all_entities(is_active=None)), 4)
+
+        # Check the same thing for an entity group defined by a super entity and sub entity kind
+        team_entities = list(Entity.all_objects.filter(entity_type=team_type).order_by('entity_id'))
+        entity_groups[1].bulk_add_entities([
+            [team_entities[0], account_entities[0].entity_kind]
+        ])
+
+        # Team 0 has accounts 0, 3, 6, 9, 12, 15, 18
+        # account 0 is inactive
+        self.assertEqual(len(entity_groups[1].get_all_entities()), 6)
+        self.assertEqual(len(entity_groups[1].get_all_entities(is_active=False)), 1)
+        self.assertEqual(len(entity_groups[1].get_all_entities(is_active=None)), 7)
+
+        entity_ids = list(entity_groups[1].get_all_entities())
+
+        # Make 3 more of them inactive
+        entity_ids = [
+            entity_id
+            for entity_id in entity_ids[0:3]
+        ]
+        Entity.objects.filter(id__in=entity_ids).update(is_active=False)
+        self.assertEqual(len(entity_groups[1].get_all_entities()), 3)
+        self.assertEqual(len(entity_groups[1].get_all_entities(is_active=False)), 4)
+        self.assertEqual(len(entity_groups[1].get_all_entities(is_active=None)), 7)
+
+        # Set a super to inactive and make sure the active subs are ignored
+        team_entities = list(Entity.all_objects.filter(entity_type=team_type).order_by('entity_id'))
+        entity_groups[2].bulk_add_entities([
+            [team_entities[1], account_entities[0].entity_kind]
+        ])
+        Entity.objects.filter(id=team_entities[1].id).update(is_active=False)
+
+        # Nothing should be returned because the super is inactive which makes the group invalid
+        self.assertEqual(len(entity_groups[2].get_all_entities()), 0)
+        self.assertEqual(len(entity_groups[2].get_all_entities(is_active=False)), 0)
+        self.assertEqual(len(entity_groups[2].get_all_entities(is_active=None)), 7)

entity/version.py

@@ -1 +1 @@
-__version__ = '5.1.0'
+__version__ = '5.1.1'

release_notes.md

@@ -1,5 +1,7 @@
 ## Release Notes
 
+- 5.1.1:
+    - Fix logic holes related to the active flag when generating entity group cache
 - 5.1.0:
     - Add decorator to suppress entity syncing
 - 5.0.2:

setup.cfg

@@ -2,7 +2,7 @@
 max-line-length = 120
 exclude = build,docs,venv,env,*.egg,migrations,south_migrations
 max-complexity = 15
-ignore = E402,E722
+ignore = E402,E722,W504
 
 [bdist_wheel]
 universal = 1