refactor ownership transfer (#836), add tests (#1391)

CHANGELOG.rst

@@ -31,6 +31,8 @@ Changed
     - Deprecate declaring app setting definitions as dict (#1456)
     - Allow ``scope=None`` in ``AppSettingAPI.get_definitions()`` (#1535)
     - Deprecate ``AppSettingAPI.get_all()`` (#1534)
+    - Allow no role for old owner in ``RoleAssignmentOwnerTransferMixin`` (#836, #1391)
+    - Allow no role for old owner in ``perform_owner_transfer()`` (#836, #1391)
 
 Removed
 -------

docs/source/major_changes.rst

@@ -34,6 +34,14 @@ instead of dict, the return data of ``AppSettingAPI.get_definition()`` and
 class. The return data is the same even if definitions have been provided in the
 deprecated dictionary format.
 
+Old Owner Role in Project Modify API Ownership Transfer
+-------------------------------------------------------
+
+In ``ProjectModifyPluginMixin.perform_role_modify()``, the ``old_owner_role``
+argument may be ``None``. This is used in cases where project role for the
+previous owner is removed. Implementations of ``perform_role_modify()`` must be
+changed accordingly. The same also applies to ``revert_role_modify()``.
+
 Deprecated Features
 -------------------
 

projectroles/plugins.py

@@ -140,29 +140,29 @@ def revert_role_delete(self, role_as, request=None):
         pass
 
     def perform_owner_transfer(
-        self, project, new_owner, old_owner, old_owner_role, request=None
+        self, project, new_owner, old_owner, old_owner_role=None, request=None
     ):
         """
         Perform additional actions to finalize project ownership transfer.
 
         :param project: Project object
         :param new_owner: SODARUser object for new owner
         :param old_owner: SODARUser object for previous owner
-        :param old_owner_role: Role object for new role of previous owner
+        :param old_owner_role: Role object for new role of old owner or None
         :param request: Request object or None
         """
         pass
 
     def revert_owner_transfer(
-        self, project, new_owner, old_owner, old_owner_role, request=None
+        self, project, new_owner, old_owner, old_owner_role=None, request=None
     ):
         """
         Revert project ownership transfer if errors have occurred in other apps.
 
         :param project: Project object
         :param new_owner: SODARUser object for new owner
         :param old_owner: SODARUser object for previous owner
-        :param old_owner_role: Role object for new role of previous owner
+        :param old_owner_role: Role object for new role of old owner or None
         :param request: Request object or None
         """
         pass

projectroles/tests/test_commands.py

@@ -13,6 +13,9 @@
 
 from test_plus.test import TestCase
 
+# Appalerts dependency
+from appalerts.models import AppAlert
+
 # Timeline dependency
 from timeline.models import TimelineEvent
 
@@ -54,6 +57,7 @@
     ProjectInviteMixin,
     AppSettingMixin,
     RemoteSiteMixin,
+    RemoteProjectMixin,
 )
 from projectroles.utils import build_secret
 
@@ -71,6 +75,7 @@
 SITE_MODE_SOURCE = SODAR_CONSTANTS['SITE_MODE_SOURCE']
 SITE_MODE_PEER = SODAR_CONSTANTS['SITE_MODE_PEER']
 SITE_MODE_TARGET = SODAR_CONSTANTS['SITE_MODE_TARGET']
+REMOTE_LEVEL_READ_ROLES = SODAR_CONSTANTS['REMOTE_LEVEL_READ_ROLES']
 SYSTEM_USER_GROUP = SODAR_CONSTANTS['SYSTEM_USER_GROUP']
 APP_SETTING_TYPE_BOOLEAN = SODAR_CONSTANTS['APP_SETTING_TYPE_BOOLEAN']
 APP_SETTING_TYPE_INTEGER = SODAR_CONSTANTS['APP_SETTING_TYPE_INTEGER']
@@ -84,6 +89,7 @@
 CUSTOM_PASSWORD = 'custompass'
 REMOTE_SITE_NAME = 'Test Site'
 REMOTE_SITE_URL = 'https://example.com'
+REMOTE_SITE_URL2 = 'https://another.site.org'
 REMOTE_SITE_SECRET = build_secret(32)
 LDAP_DOMAIN = 'EXAMPLE'
 
@@ -211,14 +217,14 @@ def test_add_peer_on_target(self):
 
     def test_add_second_target(self):
         """Test adding second target site"""
-        self.make_site('Existing Site', 'https://another.site.org')
+        self.make_site('Existing Site', REMOTE_SITE_URL2)
         self.assertEqual(RemoteSite.objects.count(), 1)
         self.command.handle(**self.options)
         self.assertEqual(RemoteSite.objects.count(), 2)
 
     def test_add_second_target_existing_name(self):
         """Test adding second target site with existing name (should fail)"""
-        self.make_site(REMOTE_SITE_NAME, 'https://another.site.org')
+        self.make_site(REMOTE_SITE_NAME, REMOTE_SITE_URL2)
         self.assertEqual(RemoteSite.objects.count(), 1)
         with self.assertRaises(SystemExit) as cm:
             self.command.handle(**self.options)
@@ -247,9 +253,7 @@ def test_add_second_target_existing_url_suppress(self):
     @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET)
     def test_add_second_source(self):
         """Test adding second source site (should fail)"""
-        self.make_site(
-            'Existing Site', 'https://another.site.org', mode=SITE_MODE_SOURCE
-        )
+        self.make_site('Existing Site', REMOTE_SITE_URL2, mode=SITE_MODE_SOURCE)
         self.assertEqual(RemoteSite.objects.count(), 1)
         with self.assertRaises(SystemExit) as cm:
             self.command.handle(**self.options)
@@ -685,7 +689,6 @@ class TestCleanAppSettings(
     """Tests for cleanappsettings command and associated functions"""
 
     def setUp(self):
-        super().setUp()
         # Init roles
         self.init_roles()
         # Init users
@@ -695,13 +698,13 @@ def setUp(self):
 
         # Init projects
         self.category = self.make_project(
-            'top_category', PROJECT_TYPE_CATEGORY, None
+            'TestCategory', PROJECT_TYPE_CATEGORY, None
         )
         self.cat_owner_as = self.make_assignment(
             self.category, self.user_owner, self.role_owner
         )
         self.project = self.make_project(
-            'sub_project', PROJECT_TYPE_PROJECT, self.category
+            'TestProject', PROJECT_TYPE_PROJECT, self.category
         )
         self.owner_as = self.make_assignment(
             self.project, self.user_owner, self.role_owner
@@ -962,7 +965,116 @@ def test_command_check(self):
         self.assertEqual(AppSetting.objects.count(), 8)
 
 
-# TODO: Add removeroles tests
+class TestRemoveRoles(
+    ProjectMixin,
+    RoleMixin,
+    RoleAssignmentMixin,
+    RemoteSiteMixin,
+    RemoteProjectMixin,
+    TestCase,
+):
+    """Tests for removeroles command"""
+
+    def setUp(self):
+        # Init roles
+        self.init_roles()
+        # Init users
+        self.user_owner_cat = self.make_user('user_owner_cat')
+        self.user_owner = self.make_user('user_owner')
+        self.user = self.make_user('user')
+        # Init projects
+        self.category = self.make_project(
+            'TestCategory', PROJECT_TYPE_CATEGORY, None
+        )
+        self.cat_owner_as = self.make_assignment(
+            self.category, self.user_owner_cat, self.role_owner
+        )
+        self.project = self.make_project(
+            'TestProject', PROJECT_TYPE_PROJECT, self.category
+        )
+        self.owner_as = self.make_assignment(
+            self.project, self.user_owner, self.role_owner
+        )
+        # Set other variables
+        self.cmd_name = 'removeroles'
+
+    def test_command(self):
+        """Test removing non-owner roles from user"""
+        # Set up non-owner roles for user in category and project
+        self.make_assignment(self.category, self.user, self.role_contributor)
+        self.make_assignment(self.project, self.user, self.role_guest)
+        # Set up another user to ensure we keep their roles
+        user_keep = self.make_user('user_keep')
+        self.make_assignment(self.category, user_keep, self.role_contributor)
+        self.make_assignment(self.project, user_keep, self.role_guest)
+
+        self.assertEqual(self.category.has_role(self.user_owner_cat), True)
+        self.assertEqual(self.project.has_role(self.user_owner), True)
+        self.assertEqual(self.category.has_role(user_keep), True)
+        self.assertEqual(self.project.has_role(user_keep), True)
+        self.assertEqual(self.category.has_role(self.user), True)
+        self.assertEqual(self.project.has_role(self.user), True)
+        self.assertEqual(
+            TimelineEvent.objects.filter(event_name='role_delete').count(), 0
+        )
+        self.assertEqual(
+            AppAlert.objects.filter(alert_name='role_delete').count(), 0
+        )
+
+        call_command(self.cmd_name, user=self.user)
+        self.assertEqual(self.category.has_role(self.user_owner_cat), True)
+        self.assertEqual(self.project.has_role(self.user_owner), True)
+        self.assertEqual(self.category.has_role(user_keep), True)
+        self.assertEqual(self.project.has_role(user_keep), True)
+        self.assertEqual(self.category.has_role(self.user), False)
+        self.assertEqual(self.project.has_role(self.user), False)
+        self.assertEqual(
+            TimelineEvent.objects.filter(event_name='role_delete').count(), 2
+        )
+        # App alerts should not be created
+        self.assertEqual(
+            AppAlert.objects.filter(alert_name='role_delete').count(), 0
+        )
+
+    @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET)
+    def test_command_remote(self):
+        """Test removing roles from remote project"""
+        self.make_assignment(self.category, self.user, self.role_contributor)
+        self.make_assignment(self.project, self.user, self.role_guest)
+        remote_site = self.make_site(
+            name=REMOTE_SITE_NAME,
+            url=REMOTE_SITE_URL,
+            mode=SITE_MODE_SOURCE,
+        )
+        self.make_remote_project(
+            project_uuid=self.category.sodar_uuid,
+            site=remote_site,
+            level=REMOTE_LEVEL_READ_ROLES,
+            project=self.category,
+        )
+        self.make_remote_project(
+            project_uuid=self.project.sodar_uuid,
+            site=remote_site,
+            level=REMOTE_LEVEL_READ_ROLES,
+            project=self.project,
+        )
+
+        self.assertEqual(self.category.has_role(self.user), True)
+        self.assertEqual(self.project.has_role(self.user), True)
+        self.assertEqual(self.category.is_remote(), True)
+        self.assertEqual(self.project.is_remote(), True)
+
+        call_command(self.cmd_name, user=self.user)
+        # Roles should not be removed
+        self.assertEqual(self.category.has_role(self.user), True)
+        self.assertEqual(self.project.has_role(self.user), True)
+
+    '''
+    def test_command_owner(self):
+        """Test removing owner role without defined owner"""
+        self.assertEqual(self.category.has_role(self.user_owner_cat), True)
+        self.assertEqual(self.project.has_role(self.user_owner), True)
+    '''
 
 
 class TestSyncGroups(TestCase):