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

[fix] Invalidate org membership cache when organization is_active flag is changed #357 #379

18 changes: 18 additions & 0 deletions openwisp_users/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

@nemesifier nemesifier May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dispatch_uid='handle_organization_is_active_change',
dispatch_uid='handle_org_is_active_change',

)

for model in [OrganizationUser, OrganizationOwner]:
for signal, name in signal_tuples:
signal.connect(
Expand Down Expand Up @@ -130,6 +137,17 @@ def connect_receivers(self):
dispatch_uid='make_first_org_user_org_owner',
)

@classmethod
def handle_organization_update(cls, instance, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this for consistency: handle_org_is_active_change

try:
nemesifier marked this conversation as resolved.
Show resolved Hide resolved
old_instance = instance._meta.model.objects.get(pk=instance.pk)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
old_instance = instance._meta.model.objects.get(pk=instance.pk)
old_instance = Organization.objects.only('is_active').get(pk=instance.pk)

Calling only should be more efficient.

except instance._meta.model.DoesNotExist:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
except instance._meta.model.DoesNotExist:
except Organization.DoesNotExist:

return
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):
"""
Expand Down
17 changes: 17 additions & 0 deletions openwisp_users/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to invalidate_org_membership_cache

"""
Invalidates cache of users when organization become inactive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can place this on the top of the file, after User.

qs = OrganizationUsers.objects.filter(
organization_id=organization_pk
).select_related('user')
User = get_user_model()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant, User is already imported at the top of the file.

for user in qs.iterator():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for user in qs.iterator():
for org_user in qs.iterator():

if not isinstance(user, User):
user = user.user
user._invalidate_user_organizations_dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all this really necessary? Why do you feel the need of checking if user.user is an instance of User? Could it be anything different? Let me know.

I think this could be simplified as following:

Suggested change
if not isinstance(user, User):
user = user.user
user._invalidate_user_organizations_dict()
user._invalidate_user_organizations_dict()

If I am missing something let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the if condition can be removed but I cannot do this

user._invalidate_user_organizations_dict()

This will cause error as organization user does not have this method.
So we can do this

user.user._invalidate_user_organizations_dict()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable name user is misleading here, this is an OrganizationUser instance, so let's call it org_user.
Then you can call: org_user.user._invalidate_user_organizations_dict().

10 changes: 10 additions & 0 deletions openwisp_users/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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='[email protected]')
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, [])
Expand Down
Loading