Skip to content

Commit

Permalink
Add/remove user from organization synchronously (#2715)
Browse files Browse the repository at this point in the history 
* add/remove user from organization synchronously

Since an organization is a Group, adding/removing user from an organization does not take long and can be done synchronously. Sharing organization projects will take longer and remains asynchronous

* fix lint error cyclic-import

* add docstring

* ignore items order in test

* add  comment

* fix failing tests
  • Loading branch information
@kelvin-muchiri
kelvin-muchiri authored Oct 9, 2024
1 parent 0169c90 commit 19a52eb
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 367 deletions.
67 changes: 1 addition & 66 deletions onadata/apps/api/tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,13 @@
from django.conf import settings
from django.core.files.uploadedfile import TemporaryUploadedFile
from django.core.files.storage import default_storage
from django.core.mail import send_mail
from django.contrib.auth import get_user_model
from django.db import DatabaseError
from django.utils import timezone
from django.utils.datastructures import MultiValueDict

from onadata.apps.api import tools
from onadata.apps.api.models.organization_profile import OrganizationProfile
from onadata.apps.logger.models import Instance, ProjectInvitation, XForm, Project
from onadata.apps.api.tools import invalidate_organization_cache
from onadata.celeryapp import app
from onadata.libs.utils.email import send_generic_email
from onadata.libs.utils.model_tools import queryset_iterator
Expand Down Expand Up @@ -191,69 +188,7 @@ def regenerate_form_instance_json(xform_id: int):
safe_delete(cache_key)


class ShareProjectBaseTask(app.Task): # pylint: disable=too-few-public-methods
"""A Task base class for sharing a project."""

autoretry_for = (
DatabaseError,
ConnectionError,
)
retry_backoff = 3


@app.task(base=ShareProjectBaseTask)
def add_org_user_and_share_projects_async(
org_id: int,
user_id: int,
role: str = None,
email_subject: str = None,
email_msg: str = None,
): # pylint: disable=invalid-name
"""Add user to organization and share projects asynchronously"""
try:
organization = OrganizationProfile.objects.get(pk=org_id)
user = User.objects.get(pk=user_id)

except OrganizationProfile.DoesNotExist as err:
logger.exception(err)

except User.DoesNotExist as err:
logger.exception(err)

else:
tools.add_org_user_and_share_projects(organization, user, role)

invalidate_organization_cache(organization.user.username)

if email_msg and email_subject and user.email:
send_mail(
email_subject,
email_msg,
settings.DEFAULT_FROM_EMAIL,
(user.email,),
)


@app.task(base=ShareProjectBaseTask)
def remove_org_user_async(org_id, user_id):
"""Remove user from organization asynchronously"""
try:
organization = OrganizationProfile.objects.get(pk=org_id)
user = User.objects.get(pk=user_id)

except OrganizationProfile.DoesNotExist as err:
logger.exception(err)

except User.DoesNotExist as err:
logger.exception(err)

else:
tools.remove_user_from_organization(organization, user)

invalidate_organization_cache(organization.user.username)


@app.task(base=ShareProjectBaseTask)
@app.task(retry_backoff=3, autoretry_for=(DatabaseError, ConnectionError))
def share_project_async(project_id, username, role, remove=False):
"""Share project asynchronously"""
try:
Expand Down
176 changes: 2 additions & 174 deletions onadata/apps/api/tests/test_tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,13 @@
from django.core.cache import cache
from django.contrib.auth import get_user_model
from django.db import DatabaseError, OperationalError
from django.test import override_settings

from onadata.apps.api.tasks import (
send_project_invitation_email_async,
regenerate_form_instance_json,
add_org_user_and_share_projects_async,
remove_org_user_async,
share_project_async,
ShareProject,
)
from onadata.apps.api.models.organization_profile import OrganizationProfile
from onadata.apps.logger.models import ProjectInvitation, Instance
from onadata.apps.main.tests.test_base import TestBase
from onadata.libs.permissions import ManagerRole
Expand Down Expand Up @@ -122,180 +118,12 @@ def mock_get_full_dict(
instance.refresh_from_db()
self.assertFalse(instance.json)


def set_cache_for_org(org, request):
"""Utility to set org cache"""
org_profile_json = OrganizationSerializer(
org, context={"request": request}
).data
org_profile_json = OrganizationSerializer(org, context={"request": request}).data
cache.set(f"{ORG_PROFILE_CACHE}{org.user.username}-owner", org_profile_json)

@patch("onadata.apps.api.tasks.tools.add_org_user_and_share_projects")
class AddOrgUserAndShareProjectsAsyncTestCase(TestBase):
"""Tests for add_org_user_and_share_projects_async"""

def setUp(self):
super().setUp()

self.org_user = User.objects.create(username="onaorg")
alice = self._create_user("alice", "1234&&")
self.org = OrganizationProfile.objects.create(
user=self.org_user, name="Ona Org", creator=alice
)
self.extra = {"HTTP_AUTHORIZATION": f"Token {self.user.auth_token}"}

def test_user_added_to_org(self, mock_add):
"""User is added to organization"""
request = self.factory.get("/", **self.extra)
request.user = self.user
set_cache_for_org(self.org, request)
cache_key = f"{ORG_PROFILE_CACHE}{self.org.user.username}-owner"
self.assertIsNotNone(cache.get(cache_key))
add_org_user_and_share_projects_async.delay(
self.org.pk, self.user.pk, "manager"
)
mock_add.assert_called_once_with(self.org, self.user, "manager")
self.assertEqual(cache.get(cache_key), None)

def test_role_optional(self, mock_add):
"""role param is optional"""
add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk)
mock_add.assert_called_once_with(self.org, self.user, None)

@patch("onadata.apps.api.tasks.logger.exception")
def test_invalid_org_id(self, mock_log, mock_add):
"""Invalid org_id is handled"""
add_org_user_and_share_projects_async.delay(sys.maxsize, self.user.pk)
mock_add.assert_not_called()
mock_log.assert_called_once()

@patch("onadata.apps.api.tasks.logger.exception")
def test_invalid_user_id(self, mock_log, mock_add):
"""Invalid org_id is handled"""
add_org_user_and_share_projects_async.delay(self.org.pk, sys.maxsize)
mock_add.assert_not_called()
mock_log.assert_called_once()

@patch("onadata.apps.api.tasks.add_org_user_and_share_projects_async.retry")
def test_database_error(self, mock_retry, mock_add):
"""We retry calls if DatabaseError is raised"""
mock_add.side_effect = DatabaseError()
add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk)
self.assertTrue(mock_retry.called)
_, kwargs = mock_retry.call_args_list[0]
self.assertTrue(isinstance(kwargs["exc"], DatabaseError))

@patch("onadata.apps.api.tasks.add_org_user_and_share_projects_async.retry")
def test_connection_error(self, mock_retry, mock_add):
"""We retry calls if ConnectionError is raised"""
mock_add.side_effect = ConnectionError()
add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk)
self.assertTrue(mock_retry.called)
_, kwargs = mock_retry.call_args_list[0]
self.assertTrue(isinstance(kwargs["exc"], ConnectionError))

@patch("onadata.apps.api.tasks.add_org_user_and_share_projects_async.retry")
def test_operation_error(self, mock_retry, mock_add):
"""We retry calls if OperationError is raised"""
mock_add.side_effect = OperationalError()
add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk)
self.assertTrue(mock_retry.called)
_, kwargs = mock_retry.call_args_list[0]
self.assertTrue(isinstance(kwargs["exc"], OperationalError))

@override_settings(DEFAULT_FROM_EMAIL="[email protected]")
@patch("onadata.apps.api.tasks.send_mail")
def test_send_mail(self, mock_email, mock_add):
"""Send mail works"""
self.user.email = "[email protected]"
self.user.save()
add_org_user_and_share_projects_async.delay(
self.org.pk, self.user.pk, "manager", "Subject", "Body"
)
mock_email.assert_called_with(
"Subject",
"Body",
"[email protected]",
("[email protected]",),
)
mock_add.assert_called_once_with(self.org, self.user, "manager")

@override_settings(DEFAULT_FROM_EMAIL="[email protected]")
@patch("onadata.apps.api.tasks.send_mail")
def test_user_email_none(self, mock_email, mock_add):
"""Email not sent if user email is None"""
add_org_user_and_share_projects_async.delay(
self.org.pk, self.user.pk, "manager", "Subject", "Body"
)
mock_email.assert_not_called()
mock_add.assert_called_once_with(self.org, self.user, "manager")


@patch("onadata.apps.api.tasks.tools.remove_user_from_organization")
class RemoveOrgUserAsyncTestCase(TestBase):
"""Tests for remove_org_user_async"""

def setUp(self):
super().setUp()

self.org_user = User.objects.create(username="onaorg")
alice = self._create_user("alice", "1234&&")
self.org = OrganizationProfile.objects.create(
user=self.org_user, name="Ona Org", creator=alice
)
self.extra = {"HTTP_AUTHORIZATION": f"Token {self.user.auth_token}"}

def test_user_removed_from_org(self, mock_remove):
"""User is removed from organization"""
request = self.factory.get("/", **self.extra)
request.user = self.user
set_cache_for_org(self.org, request)
cache_key = f"{ORG_PROFILE_CACHE}{self.org.user.username}-owner"
self.assertIsNotNone(cache.get(cache_key))
remove_org_user_async.delay(self.org.pk, self.user.pk)
mock_remove.assert_called_once_with(self.org, self.user)
self.assertEqual(cache.get(cache_key), None)

@patch("onadata.apps.api.tasks.logger.exception")
def test_invalid_org_id(self, mock_log, mock_remove):
"""Invalid org_id is handled"""
remove_org_user_async.delay(sys.maxsize, self.user.pk)
mock_remove.assert_not_called()
mock_log.assert_called_once()

@patch("onadata.apps.api.tasks.logger.exception")
def test_invalid_user_id(self, mock_log, mock_remove):
"""Invalid user_id is handled"""
remove_org_user_async.delay(self.org.pk, sys.maxsize)
mock_remove.assert_not_called()
mock_log.assert_called_once()

@patch("onadata.apps.api.tasks.remove_org_user_async.retry")
def test_database_error(self, mock_retry, mock_remove):
"""We retry calls if DatabaseError is raised"""
mock_remove.side_effect = DatabaseError()
remove_org_user_async.delay(self.org.pk, self.user.pk)
self.assertTrue(mock_retry.called)
_, kwargs = mock_retry.call_args_list[0]
self.assertTrue(isinstance(kwargs["exc"], DatabaseError))

@patch("onadata.apps.api.tasks.remove_org_user_async.retry")
def test_connection_error(self, mock_retry, mock_remove):
"""We retry calls if ConnectionError is raised"""
mock_remove.side_effect = ConnectionError()
remove_org_user_async.delay(self.org.pk, self.user.pk)
self.assertTrue(mock_retry.called)
_, kwargs = mock_retry.call_args_list[0]
self.assertTrue(isinstance(kwargs["exc"], ConnectionError))

@patch("onadata.apps.api.tasks.remove_org_user_async.retry")
def test_operation_error(self, mock_retry, mock_remove):
"""We retry calls if OperationError is raised"""
mock_remove.side_effect = OperationalError()
remove_org_user_async.delay(self.org.pk, self.user.pk)
self.assertTrue(mock_retry.called)
_, kwargs = mock_retry.call_args_list[0]
self.assertTrue(isinstance(kwargs["exc"], OperationalError))


class ShareProjectAsyncTestCase(TestBase):
"""Tests for share_project_async"""
Expand Down
Loading

0 comments on commit 19a52eb

Please sign in to comment.