From 509bd2ffe86b5ee7b51feaf338985572117d4371 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Thu, 23 May 2024 20:56:15 +0530 Subject: [PATCH 1/7] [openwisp_users/apps.py] fixes issue #357 When the status ( is_active ) of an organization changes it now invalidates the cache of all the organizationsusers. --- openwisp_users/apps.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/openwisp_users/apps.py b/openwisp_users/apps.py index 918786c7..11253121 100644 --- a/openwisp_users/apps.py +++ b/openwisp_users/apps.py @@ -3,7 +3,7 @@ from django.apps import AppConfig from django.conf import settings from django.contrib.auth import get_user_model -from django.core.exceptions import ValidationError +from django.core.exceptions import ValidationError,ObjectDoesNotExist from django.db import IntegrityError, transaction from django.db.models.signals import post_delete, post_save, pre_save from django.utils.translation import gettext_lazy as _ @@ -98,11 +98,18 @@ def set_default_settings(self): def connect_receivers(self): OrganizationUser = load_model('openwisp_users', 'OrganizationUser') OrganizationOwner = load_model('openwisp_users', 'OrganizationOwner') + Organization = load_model("openwisp_users" , "Organization") signal_tuples = [ (post_save, 'post_save'), (post_delete, 'post_delete'), ] + pre_save.connect( + self.handle_organization_update, + sender = Organization, + dispatch_uid='handle_organization_is_active_change' + ) + for model in [OrganizationUser, OrganizationOwner]: for signal, name in signal_tuples: signal.connect( @@ -130,6 +137,22 @@ def connect_receivers(self): dispatch_uid='make_first_org_user_org_owner', ) + @classmethod + def handle_organization_update(cls , instance , sender,**kwargs) : + Organization = load_model("openwisp_users" , "Organization") + OrganizationUsers = load_model("openwisp_users" ,"OrganizationUser") + try: + old_obj = Organization.objects.get(pk=instance.pk) + except ObjectDoesNotExist: + logger.warning(f"Organization with pk {instance.pk} does not exist.") + return + except Exception as e: + logger.exception(f"An error occurred while fetching the organization: {e}") + return + if getattr(old_obj , "is_active") != getattr(instance , "is_active") : + for user in OrganizationUsers.objects.filter(organization = instance): + cls._invalidate_user_cache(user) + @classmethod def pre_save_update_organizations_dict(cls, instance, **kwargs): """ From f8fc46dbb5381aac8498aca4e87ceba97e4399e3 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Tue, 28 May 2024 13:09:45 +0530 Subject: [PATCH 2/7] Added suggested changes but failing some testcases queries running is 1 more than expected --- openwisp_users/apps.py | 27 +++++++++++---------------- openwisp_users/tasks.py | 17 +++++++++++++++++ openwisp_users/tests/test_models.py | 10 ++++++++++ 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/openwisp_users/apps.py b/openwisp_users/apps.py index 11253121..999a525b 100644 --- a/openwisp_users/apps.py +++ b/openwisp_users/apps.py @@ -3,7 +3,7 @@ from django.apps import AppConfig from django.conf import settings from django.contrib.auth import get_user_model -from django.core.exceptions import ValidationError,ObjectDoesNotExist +from django.core.exceptions import ValidationError from django.db import IntegrityError, transaction from django.db.models.signals import post_delete, post_save, pre_save from django.utils.translation import gettext_lazy as _ @@ -98,7 +98,7 @@ def set_default_settings(self): def connect_receivers(self): OrganizationUser = load_model('openwisp_users', 'OrganizationUser') OrganizationOwner = load_model('openwisp_users', 'OrganizationOwner') - Organization = load_model("openwisp_users" , "Organization") + Organization = load_model('openwisp_users', 'Organization') signal_tuples = [ (post_save, 'post_save'), (post_delete, 'post_delete'), @@ -106,8 +106,8 @@ def connect_receivers(self): pre_save.connect( self.handle_organization_update, - sender = Organization, - dispatch_uid='handle_organization_is_active_change' + sender=Organization, + dispatch_uid='handle_organization_is_active_change', ) for model in [OrganizationUser, OrganizationOwner]: @@ -138,20 +138,15 @@ def connect_receivers(self): ) @classmethod - def handle_organization_update(cls , instance , sender,**kwargs) : - Organization = load_model("openwisp_users" , "Organization") - OrganizationUsers = load_model("openwisp_users" ,"OrganizationUser") + def handle_organization_update(cls, instance, **kwargs): try: - old_obj = Organization.objects.get(pk=instance.pk) - except ObjectDoesNotExist: - logger.warning(f"Organization with pk {instance.pk} does not exist.") + old_instance = instance._meta.model.objects.get(pk=instance.pk) + except instance._meta.model.DoesNotExist: return - except Exception as e: - logger.exception(f"An error occurred while fetching the organization: {e}") - return - if getattr(old_obj , "is_active") != getattr(instance , "is_active") : - for user in OrganizationUsers.objects.filter(organization = instance): - cls._invalidate_user_cache(user) + from .tasks import organization_update_task + + if instance.is_active != old_instance.is_active: + organization_update_task.delay(instance.pk) @classmethod def pre_save_update_organizations_dict(cls, instance, **kwargs): diff --git a/openwisp_users/tasks.py b/openwisp_users/tasks.py index e8d792d3..6fb46356 100644 --- a/openwisp_users/tasks.py +++ b/openwisp_users/tasks.py @@ -12,6 +12,7 @@ from django.utils.timezone import now, timedelta from django.utils.translation import gettext_lazy as _ from openwisp_utils.admin_theme.email import send_email +from swapper import load_model from . import settings as app_settings @@ -83,3 +84,19 @@ def password_expiration_email(): sleep(random.randint(1, 2)) else: email_counts += 1 + + +@shared_task +def organization_update_task(organization_pk): + """ + Invalidates cache of users when organization become inactive + """ + OrganizationUsers = load_model('openwisp_users', 'OrganizationUser') + qs = OrganizationUsers.objects.filter( + organization_id=organization_pk + ).select_related('user') + User = get_user_model() + for user in qs.iterator(): + if not isinstance(user, User): + user = user.user + user._invalidate_user_organizations_dict() diff --git a/openwisp_users/tests/test_models.py b/openwisp_users/tests/test_models.py index 75dbef1f..bfb0bc9d 100644 --- a/openwisp_users/tests/test_models.py +++ b/openwisp_users/tests/test_models.py @@ -185,6 +185,16 @@ def test_invalidate_cache_org_user_user_changed(self): self.assertEqual(user1.is_member(org), False) self.assertEqual(user2.is_member(org), True) + def test_invalidate_cache_org_status_changed(self): + org = self._create_org(name="testorg1") + user1 = self._create_user(username='testuser1', email='user1@test.com') + self._create_org_user(user=user1, organization=org) + self.assertEqual(user1.is_member(org), True) + org.is_active = False + org.full_clean() + org.save() + self.assertEqual(user1.is_member(org), False) + def test_organizations_managed(self): user = self._create_user(username='organizations_pk') self.assertEqual(user.organizations_managed, []) From c90fb1a015041166effd68423d391ce88ad293fb Mon Sep 17 00:00:00 2001 From: kaushikaryan04 <73491041+kaushikaryan04@users.noreply.github.com> Date: Tue, 28 May 2024 13:22:29 +0530 Subject: [PATCH 3/7] Update test_models.py --- openwisp_users/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_users/tests/test_models.py b/openwisp_users/tests/test_models.py index bfb0bc9d..05f34b4c 100644 --- a/openwisp_users/tests/test_models.py +++ b/openwisp_users/tests/test_models.py @@ -186,7 +186,7 @@ def test_invalidate_cache_org_user_user_changed(self): self.assertEqual(user2.is_member(org), True) def test_invalidate_cache_org_status_changed(self): - org = self._create_org(name="testorg1") + org = self._create_org(name='testorg1') user1 = self._create_user(username='testuser1', email='user1@test.com') self._create_org_user(user=user1, organization=org) self.assertEqual(user1.is_member(org), True) From dbd110793da31944c707308ad2c77196ada24208 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Thu, 30 May 2024 01:08:43 +0530 Subject: [PATCH 4/7] [fix] Invalidate cache when Organization status is changed #357 I made a pre_save signal that calls a celery task that would invalidate cache of all users in the organization if the status is changed. But it is failing a test case which i have mentioned. Fixes #357 --- openwisp_users/tests/test_api/test_api.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/openwisp_users/tests/test_api/test_api.py b/openwisp_users/tests/test_api/test_api.py index 20520242..1b385bcc 100644 --- a/openwisp_users/tests/test_api/test_api.py +++ b/openwisp_users/tests/test_api/test_api.py @@ -50,7 +50,7 @@ def test_organization_list_nonsuperuser_api(self): def test_organization_post_api(self): path = reverse('users:organization_list') data = {'name': 'test-org', 'slug': 'test-org'} - with self.assertNumQueries(6): + with self.assertNumQueries(7): r = self.client.post(path, data, content_type='application/json') self.assertEqual(r.status_code, 201) self.assertEqual(Organization.objects.count(), 2) @@ -86,7 +86,7 @@ def test_organization_put_api(self): 'email': 'testorg@test.com', 'url': '', } - with self.assertNumQueries(6): + with self.assertNumQueries(8): r = self.client.put(path, data, content_type='application/json') self.assertEqual(r.status_code, 200) self.assertEqual(r.data['name'], 'test org change') @@ -99,7 +99,7 @@ def test_organization_patch_api(self): data = { 'name': 'test org change', } - with self.assertNumQueries(5): + with self.assertNumQueries(6): r = self.client.patch(path, data, content_type='application/json') self.assertEqual(r.status_code, 200) self.assertEqual(r.data['name'], 'test org change') @@ -110,7 +110,7 @@ def test_create_organization_owner_api(self): org1_user1 = self._create_org_user(user=user1, organization=org1) path = reverse('users:organization_detail', args=(org1.pk,)) data = {'owner': {'organization_user': org1_user1.pk}} - with self.assertNumQueries(17): + with self.assertNumQueries(18): r = self.client.patch(path, data, content_type='application/json') self.assertEqual(r.status_code, 200) self.assertEqual(r.data['owner']['organization_user'], org1_user1.pk) @@ -122,7 +122,7 @@ def test_remove_organization_owner_api(self): self._create_org_owner(organization_user=org1_user1, organization=org1) path = reverse('users:organization_detail', args=(org1.pk,)) data = {'owner': {'organization_user': ''}} - with self.assertNumQueries(11): + with self.assertNumQueries(12): r = self.client.patch(path, data, content_type='application/json') self.assertEqual(r.status_code, 200) self.assertEqual(r.data['owner'], None) @@ -166,7 +166,7 @@ def test_change_organizationowner_for_org(self): self.assertEqual(org1.owner.organization_user.id, org1_user1.id) path = reverse('users:organization_detail', args=(org1.pk,)) data = {'owner': {'organization_user': org1_user2.id}} - with self.assertNumQueries(26): + with self.assertNumQueries(27): r = self.client.patch(path, data, content_type='application/json') org1.refresh_from_db() self.assertEqual(org1.owner.organization_user.id, org1_user2.id) @@ -720,7 +720,7 @@ def test_user_list_for_nonsuperuser_api(self): def test_organization_slug_post_custom_validation_api(self): path = reverse('users:organization_list') data = {'name': 'test-org', 'slug': 'test-org'} - with self.assertNumQueries(6): + with self.assertNumQueries(7): r = self.client.post(path, data, content_type='application/json') self.assertEqual(r.status_code, 201) self.assertEqual(Organization.objects.count(), 2) From 2714cddfb9ad14904c9d26a5d60600ae0ca2ccd7 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Fri, 31 May 2024 13:54:44 +0530 Subject: [PATCH 5/7] [fix] Improved previous commit Refactored the code, changed name of a function that describes it better and made a query more efficient. --- openwisp_users/apps.py | 9 +++++---- openwisp_users/tasks.py | 10 ++++++---- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/openwisp_users/apps.py b/openwisp_users/apps.py index 999a525b..75bb4996 100644 --- a/openwisp_users/apps.py +++ b/openwisp_users/apps.py @@ -139,14 +139,15 @@ def connect_receivers(self): @classmethod def handle_organization_update(cls, instance, **kwargs): + Organization = instance._meta.model try: - old_instance = instance._meta.model.objects.get(pk=instance.pk) - except instance._meta.model.DoesNotExist: + old_instance = Organization.objects.only('is_active').get(pk=instance.pk) + except Organization.DoesNotExist: return - from .tasks import organization_update_task + from .tasks import invalidate_org_membership_cache if instance.is_active != old_instance.is_active: - organization_update_task.delay(instance.pk) + invalidate_org_membership_cache.delay(instance.pk) @classmethod def pre_save_update_organizations_dict(cls, instance, **kwargs): diff --git a/openwisp_users/tasks.py b/openwisp_users/tasks.py index 6fb46356..4dd61348 100644 --- a/openwisp_users/tasks.py +++ b/openwisp_users/tasks.py @@ -17,6 +17,7 @@ from . import settings as app_settings User = get_user_model() +OrganizationUser = load_model('openwisp_users', 'OrganizationUser') @shared_task @@ -87,12 +88,13 @@ def password_expiration_email(): @shared_task -def organization_update_task(organization_pk): +def invalidate_org_membership_cache(organization_pk): """ - Invalidates cache of users when organization become inactive + Invalidates organization membership cache of all users of an + organization when organization.is_active changes + (organization is disabled or enabled again). """ - OrganizationUsers = load_model('openwisp_users', 'OrganizationUser') - qs = OrganizationUsers.objects.filter( + qs = OrganizationUser.objects.filter( organization_id=organization_pk ).select_related('user') User = get_user_model() From 2020feaefd2478b0e8399b37443e7a762d4e0f89 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Sat, 1 Jun 2024 11:49:19 +0530 Subject: [PATCH 6/7] [fix] Made suggested changes Added a condition that reduced query count in 2 testcases and imporved naming of variables and funtions. --- openwisp_users/apps.py | 9 ++++++--- openwisp_users/tasks.py | 7 ++----- openwisp_users/tests/test_api/test_api.py | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/openwisp_users/apps.py b/openwisp_users/apps.py index 75bb4996..8072e8cc 100644 --- a/openwisp_users/apps.py +++ b/openwisp_users/apps.py @@ -105,9 +105,9 @@ def connect_receivers(self): ] pre_save.connect( - self.handle_organization_update, + self.handle_org_is_active_change, sender=Organization, - dispatch_uid='handle_organization_is_active_change', + dispatch_uid='handle_org_is_active_change', ) for model in [OrganizationUser, OrganizationOwner]: @@ -138,7 +138,10 @@ def connect_receivers(self): ) @classmethod - def handle_organization_update(cls, instance, **kwargs): + def handle_org_is_active_change(cls, instance, **kwargs): + if instance._state.adding: + # If it's a new organization, we don't need to update any cache + return Organization = instance._meta.model try: old_instance = Organization.objects.only('is_active').get(pk=instance.pk) diff --git a/openwisp_users/tasks.py b/openwisp_users/tasks.py index 4dd61348..7a732902 100644 --- a/openwisp_users/tasks.py +++ b/openwisp_users/tasks.py @@ -97,8 +97,5 @@ def invalidate_org_membership_cache(organization_pk): qs = OrganizationUser.objects.filter( organization_id=organization_pk ).select_related('user') - User = get_user_model() - for user in qs.iterator(): - if not isinstance(user, User): - user = user.user - user._invalidate_user_organizations_dict() + for org_user in qs.iterator(): + org_user.user._invalidate_user_organizations_dict() diff --git a/openwisp_users/tests/test_api/test_api.py b/openwisp_users/tests/test_api/test_api.py index 1b385bcc..a87c4e38 100644 --- a/openwisp_users/tests/test_api/test_api.py +++ b/openwisp_users/tests/test_api/test_api.py @@ -50,7 +50,7 @@ def test_organization_list_nonsuperuser_api(self): def test_organization_post_api(self): path = reverse('users:organization_list') data = {'name': 'test-org', 'slug': 'test-org'} - with self.assertNumQueries(7): + with self.assertNumQueries(6): r = self.client.post(path, data, content_type='application/json') self.assertEqual(r.status_code, 201) self.assertEqual(Organization.objects.count(), 2) @@ -720,7 +720,7 @@ def test_user_list_for_nonsuperuser_api(self): def test_organization_slug_post_custom_validation_api(self): path = reverse('users:organization_list') data = {'name': 'test-org', 'slug': 'test-org'} - with self.assertNumQueries(7): + with self.assertNumQueries(6): r = self.client.post(path, data, content_type='application/json') self.assertEqual(r.status_code, 201) self.assertEqual(Organization.objects.count(), 2) From cc31d24361a6c2aac2d6cec954ee608bc2b936ad Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Wed, 5 Jun 2024 18:32:27 -0400 Subject: [PATCH 7/7] [tests] Fixed test_can_change_inline_org_owner Added is_active parameter in post request to avoid disabling org. --- openwisp_users/tests/test_admin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openwisp_users/tests/test_admin.py b/openwisp_users/tests/test_admin.py index 74b99e49..ce7a584e 100644 --- a/openwisp_users/tests/test_admin.py +++ b/openwisp_users/tests/test_admin.py @@ -1395,6 +1395,7 @@ def test_can_change_inline_org_owner(self): params = { 'name': org.name, 'slug': org.slug, + 'is_active': 'on', 'owner-TOTAL_FORMS': '1', 'owner-INITIAL_FORMS': '1', 'owner-MIN_NUM_FORMS': '0',