diff --git a/docs/source/app_projectroles_usage.rst b/docs/source/app_projectroles_usage.rst index 0d5d7253..8c7bc2f2 100644 --- a/docs/source/app_projectroles_usage.rst +++ b/docs/source/app_projectroles_usage.rst @@ -339,6 +339,19 @@ project permissions, or by a site admin using the ``batchupdateroles`` management command. The latter supports multiple projects in one batch. It is also able to send invites to users who have not yet signed up on the site. +Remove All Roles from User +-------------------------- + +To easily remove all roles from a user, use the ``removeroles`` management +command. For owner roles, you can supply the user name of a user for whom to +transfer those roles. If no owner is supplied, each ownership will be +transferred to the parent category owner. Example: + +.. code-block:: console + + $ ./manage.py removeroles --user alice --owner bob + + User Status Checking -------------------- diff --git a/docs/source/major_changes.rst b/docs/source/major_changes.rst index f70d4246..bbdf0ec3 100644 --- a/docs/source/major_changes.rst +++ b/docs/source/major_changes.rst @@ -16,6 +16,7 @@ v1.1.0 (WIP) Release Highlights ================== +- Add removeroles management command - Add app setting type constants - Add app setting definition as objects - Update app settings API diff --git a/projectroles/management/commands/removeroles.py b/projectroles/management/commands/removeroles.py index 809b700e..4d04037b 100644 --- a/projectroles/management/commands/removeroles.py +++ b/projectroles/management/commands/removeroles.py @@ -10,7 +10,10 @@ from projectroles.management.logging import ManagementCommandLogger from projectroles.models import RoleAssignment, SODAR_CONSTANTS -from projectroles.views import RoleAssignmentDeleteMixin +from projectroles.views import ( + RoleAssignmentOwnerTransferMixin, + RoleAssignmentDeleteMixin, +) logger = ManagementCommandLogger(__name__) User = get_user_model() @@ -23,12 +26,25 @@ USER_NOT_FOUND_MSG = 'User not found with username: {}' -class Command(RoleAssignmentDeleteMixin, BaseCommand): +class Command( + RoleAssignmentOwnerTransferMixin, RoleAssignmentDeleteMixin, BaseCommand +): help = ( 'Remove all roles from a user. Replace owner roles with given user or ' 'parent owner.' ) + @classmethod + def _get_parent_owner(cls, project, prev_owner): + """Return assignment for first parent owner who is not previous owner""" + if not project.parent: # Can't traverse further + return None + parent_owner_as = project.parent.get_owner() + if parent_owner_as.user != prev_owner: + return parent_owner_as.user + # If the owner is the same, recurse + return cls._get_parent_owner(project.parent, prev_owner) + def add_arguments(self, parser): parser.add_argument( '-u', @@ -91,25 +107,65 @@ def handle(self, *args, **options): continue # Owner role reassignment if role_as.role.name == PROJECT_ROLE_OWNER: - # TODO: If no set owner, get parent owner - # TODO: If parent owner is not found, fail and continue - # TODO: If parent owner has existing local role, promote that - # TODO: Else add owner role - pass + # Fail top category if no new owner is specified + if not owner and not project.parent: + logger.warning( + 'Failed to transfer ownership for top level {} {}: no ' + 'new owner provided'.format( + project.type.lower(), p_title + ) + ) + fail_count += 1 + continue + # Get parent owner if not set + if not owner: + owner = self._get_parent_owner(project, user) + # owner = old_owner_as.user + # Fail if alternate parent owner is not found + if not owner: + logger.warning( + 'Failed to transfer ownership in {}: no parent owner ' + 'found'.format(p_title) + ) + fail_count += 1 + continue + try: + with transaction.atomic(): + self.transfer_owner( + project=project, + new_owner=owner, + old_owner_as=role_as, + old_owner_role=None, + notify_old=False, + ) + logger.info( + 'Transferred ownership in {} to {}'.format( + p_title, owner.username + ) + ) + role_count += 1 + except Exception as ex: + logger.error( + 'Failed to transfer ownership in {} to {}: {}'.format( + p_title, owner.username, ex + ) + ) + fail_count += 1 # Non-owner role removal else: try: with transaction.atomic(): self.delete_assignment(role_as, None, False) + logger.info( + 'Deleted role "{}" from {}'.format(r_name, p_title) + ) + role_count += 1 except Exception as ex: logger.error( 'Failed to delete assignment "{}" from {}: ' '{}'.format(r_name, p_title, ex) ) fail_count += 1 - continue - logger.info('Deleted role "{}" from {}'.format(r_name, p_title)) - role_count += 1 logger.info( 'Removed roles from user "{}" ({} OK, {} failed)'.format( user.username, role_count, fail_count diff --git a/projectroles/tests/test_commands.py b/projectroles/tests/test_commands.py index 39ef2b59..dccadf12 100644 --- a/projectroles/tests/test_commands.py +++ b/projectroles/tests/test_commands.py @@ -1069,12 +1069,114 @@ def test_command_remote(self): 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""" + def test_command_owner_unset(self): + """Test removing owner with unset new owner""" self.assertEqual(self.category.has_role(self.user_owner_cat), True) + # Inherited owner role + self.assertEqual(self.project.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), True) + self.assertEqual( + TimelineEvent.objects.filter( + event_name='role_owner_transfer' + ).count(), + 0, + ) + self.assertEqual( + AppAlert.objects.filter(alert_name='role_update').count(), 0 + ) + + call_command(self.cmd_name, user=self.user_owner) + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), False) + # Assert local owner roles + self.assertEqual(self.category.get_owner().user, self.user_owner_cat) + self.assertEqual(self.project.get_owner().user, self.user_owner_cat) + self.assertEqual( + TimelineEvent.objects.filter( + event_name='role_owner_transfer' + ).count(), + 1, + ) + # No alerts for inherited owner or old removed user + self.assertEqual( + AppAlert.objects.filter(alert_name='role_update').count(), 0 + ) + + def test_command_owner_unset_top_level(self): + """Test removing owner with unset new owner at top level (should fail)""" + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + call_command(self.cmd_name, user=self.user_owner_cat) + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + + def test_command_owner_unset_same_owner(self): + """Test removing owner with unset new owner and same owner on all levels (should fail)""" + self.owner_as.user = self.user_owner_cat + self.owner_as.save() + self.assertEqual(self.category.get_owner().user, self.user_owner_cat) + self.assertEqual(self.project.get_owner().user, self.user_owner_cat) + call_command(self.cmd_name, user=self.user_owner_cat) + self.assertEqual(self.category.get_owner().user, self.user_owner_cat) + self.assertEqual(self.project.get_owner().user, self.user_owner_cat) + + def test_command_owner_set_no_role(self): + """Test removing owner role with set new owner without existing role""" + user_new = self.make_user('user_new') + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), True) + self.assertEqual(self.project.has_role(user_new), False) + self.assertEqual( + AppAlert.objects.filter(alert_name='role_update').count(), 0 + ) + call_command(self.cmd_name, user=self.user_owner, owner=user_new) + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), False) + self.assertEqual(self.project.get_owner().user, user_new) + # Alert for new owner + self.assertEqual( + AppAlert.objects.filter(alert_name='role_update').count(), 1 + ) + + def test_command_owner_set_no_role_top_level(self): + """Test removing owner role with set new owner without existing role on top level""" + user_new = self.make_user('user_new') + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), True) + self.assertEqual(self.project.has_role(user_new), False) + call_command(self.cmd_name, user=self.user_owner_cat, owner=user_new) + self.assertEqual(self.category.has_role(self.user_owner_cat), False) + self.assertEqual(self.project.has_role(self.user_owner_cat), False) + self.assertEqual(self.project.has_role(self.user_owner), True) + self.assertEqual(self.category.get_owner().user, user_new) + self.assertEqual(self.project.has_role(user_new), True) + + def test_command_owner_set_existing_role(self): + """Test removing owner role with set new owner and existing role""" + user_new = self.make_user('user_new') + self.make_assignment(self.project, user_new, self.role_contributor) + self.assertEqual(self.project.get_owner().user, self.user_owner) + self.assertEqual(self.project.has_role(user_new), True) + self.assertEqual( + RoleAssignment.objects.filter(user=user_new).count(), 1 + ) + call_command(self.cmd_name, user=self.user_owner, owner=user_new) + self.assertEqual(self.project.get_owner().user, user_new) + self.assertEqual(self.project.has_role(self.user_owner), False) + self.assertEqual( + RoleAssignment.objects.filter(user=user_new).count(), 1 + ) + + def test_command_owner_invalid_new_owner_name(self): + """Test removing owner with invalid new owner name (should fail)""" + self.assertEqual(self.project.has_role(self.user_owner), True) + with self.assertRaises(SystemExit): + call_command( + self.cmd_name, user=self.user_owner, owner='INVALID-NAME' + ) self.assertEqual(self.project.has_role(self.user_owner), True) - ''' class TestSyncGroups(TestCase): diff --git a/projectroles/views.py b/projectroles/views.py index 237bb1c4..9ac72a29 100644 --- a/projectroles/views.py +++ b/projectroles/views.py @@ -2401,7 +2401,6 @@ def form_valid(self, form): new_owner, old_owner_as, old_owner_role, - issuer=self.request.user, request=self.request, ) except Exception as ex: