add remove role support in owner transfer form (#836)

CHANGELOG.rst

@@ -28,6 +28,7 @@ Added
     - ``siteappsettings`` site app plugin (#1304)
     - ``SiteAppSettingsFormView`` view (#1304)
     - ``SODARAppSettingFormMixin`` form helper mixin (#1545)
+    - Old owner "remove role" option in ``RoleAssignmentOwnerTransferForm`` (#836)
 
 Changed
 -------

docs/source/major_changes.rst

@@ -21,6 +21,7 @@ Release Highlights
 - Add removeroles management command
 - Add app setting type constants
 - Add app setting definition as objects
+- Update owner transfer form to allow setting no role for old owner
 - Update app settings API
 - Remove support for features deprecated in v1.0
 - Remove squashed migrations

projectroles/forms.py

@@ -73,6 +73,7 @@
     '{exception}'
 )
 APP_SETTING_SOURCE_ONLY_MSG = '[Only editable on source site]'
+REMOVE_ROLE_LABEL = '--- Remove role from {project_title} ---'
 
 
 # Base Classes and Mixins ------------------------------------------------------
@@ -982,10 +983,19 @@ def _get_old_owner_choices(cls, project, old_owner_as):
             q_kwargs['rank__gt'] = ROLE_RANKING[PROJECT_ROLE_OWNER]
         if inh_role_as:
             q_kwargs['rank__lte'] = inh_role_as.role.rank
-        return [
+        ret = [
             get_role_option(project, role)
             for role in Role.objects.filter(**q_kwargs).order_by('rank')
         ]
+        if (
+            not inh_role_as
+            or inh_role_as.role.rank != ROLE_RANKING[PROJECT_ROLE_OWNER]
+        ):
+            remove_label = REMOVE_ROLE_LABEL.format(
+                project_title=get_display_name(project.type)
+            )
+            ret.append((0, remove_label))
+        return ret
 
     def __init__(self, project, current_user, current_owner, *args, **kwargs):
         """Override for form initialization"""
@@ -1026,9 +1036,10 @@ def __init__(self, project, current_user, current_owner, *args, **kwargs):
         )
 
     def clean_old_owner_role(self):
-        role = Role.objects.filter(
-            pk=self.cleaned_data.get('old_owner_role')
-        ).first()
+        field_val = int(self.cleaned_data.get('old_owner_role'))
+        if not field_val:
+            return None  # Remove old owner role
+        role = Role.objects.filter(pk=field_val).first()
         if not role:
             raise forms.ValidationError('Selected role not found')
 

projectroles/tests/test_views.py

@@ -3892,7 +3892,7 @@ def test_get(self):
         self.assertEqual(response.status_code, 200)
         form = response.context['form']
         self.assertIsNotNone(form.fields.get('old_owner_role'))
-        self.assertEqual(len(form.fields['old_owner_role'].choices), 3)
+        self.assertEqual(len(form.fields['old_owner_role'].choices), 4)
         # Assert finder role is not selectable
         self.assertNotIn(
             self.role_finder.pk,
@@ -3912,7 +3912,7 @@ def test_get_old_inherited_member(self):
         # Only delegate and contributor roles allowed
         self.assertEqual(
             [c[0] for c in form.fields['old_owner_role'].choices],
-            [self.role_delegate.pk, self.role_contributor.pk],
+            [self.role_delegate.pk, self.role_contributor.pk] + [0],
         )
         self.assertEqual(form.fields['old_owner_role'].disabled, False)
 
@@ -3945,7 +3945,7 @@ def test_get_category(self):
         self.assertEqual(response.status_code, 200)
         form = response.context['form']
         self.assertIsNotNone(form.fields.get('old_owner_role'))
-        self.assertEqual(len(form.fields['old_owner_role'].choices), 4)
+        self.assertEqual(len(form.fields['old_owner_role'].choices), 5)
         # Assert finder role is selectable
         self.assertIn(
             self.role_finder.pk,
@@ -4174,6 +4174,79 @@ def test_post_new_local_role(self):
         self.assertEqual(self.app_alert_model.objects.count(), 2)
         self.assertEqual(len(mail.outbox), 2)
 
+    def test_post_no_old_role(self):
+        """Test POST with no old owner role"""
+        self.assertEqual(self.app_alert_model.objects.count(), 0)
+        self.assertEqual(len(mail.outbox), 0)
+        with self.login(self.user):
+            response = self.client.post(
+                self.url,
+                data={
+                    'project': self.project.sodar_uuid,
+                    'new_owner': self.user_guest.sodar_uuid,
+                    'old_owner_role': 0,
+                },
+            )
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(self.project.get_owner().user, self.user_guest)
+        self.assertIsNone(
+            RoleAssignment.objects.filter(
+                project=self.project, user=self.user_owner
+            ).first()
+        )
+        self.assertEqual(self.app_alert_model.objects.count(), 2)
+        self.assertEqual(
+            self.app_alert_model.objects.filter(
+                alert_name='role_delete', user=self.user_owner
+            ).count(),
+            1,
+        )
+        self.assertEqual(
+            self.app_alert_model.objects.filter(
+                alert_name='role_update', user=self.user_guest
+            ).count(),
+            1,
+        )
+        self.assertEqual(len(mail.outbox), 2)
+        self.assertIn(
+            SUBJECT_ROLE_DELETE.format(
+                project_label='project', project=self.project.title
+            ),
+            mail.outbox[0].subject,
+        )
+        self.assertIn(
+            SUBJECT_ROLE_UPDATE.format(
+                project_label='project', project=self.project.title
+            ),
+            mail.outbox[1].subject,
+        )
+
+    def test_post_old_inherited_member_no_old_role(self):
+        """Test POST with old inherited member and no old owner role"""
+        inh_as = self.make_assignment(
+            self.category, self.user_owner, self.role_contributor
+        )
+        self.assertEqual(self.project.get_role(self.user_owner), self.owner_as)
+        with self.login(self.user):
+            response = self.client.post(
+                self.url,
+                data={
+                    'project': self.project.sodar_uuid,
+                    'new_owner': self.user_guest.sodar_uuid,
+                    'old_owner_role': 0,
+                },
+            )
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(self.project.get_owner().user, self.user_guest)
+        self.assertIsNone(
+            RoleAssignment.objects.filter(
+                project=self.project, user=self.user_owner
+            ).first()
+        )
+        self.assertEqual(self.project.get_role(self.user_owner), inh_as)
+        self.assertEqual(self.app_alert_model.objects.count(), 2)
+        self.assertEqual(len(mail.outbox), 2)
+
 
 class TestProjectInviteCreateView(
     ProjectMixin, RoleAssignmentMixin, ProjectInviteMixin, ViewTestBase

projectroles/views.py

@@ -125,6 +125,7 @@
 )
 ROLE_CREATE_MSG = 'Membership granted with the role of "{role}".'
 ROLE_UPDATE_MSG = 'Member role changed to "{role}".'
+ROLE_DELETE_MSG = 'Your membership in this {project_type} has been removed.'
 ROLE_FINDER_INFO = (
     'User can see nested {categories} and {projects}, but can not access them '
     'without having a role explicitly assigned.'
@@ -1869,8 +1870,8 @@ def _update_app_alerts(self, app_alerts, project, user, inh_as):
                 project=project.title, role=inh_as.role.name
             )
         else:
-            message = 'Your membership in this {} has been removed.'.format(
-                get_display_name(project.type)
+            message = ROLE_DELETE_MSG.format(
+                project_type=get_display_name(project.type)
             )
             # Dismiss existing alerts
             AppAlert = app_alerts.get_model()
@@ -2298,18 +2299,27 @@ def transfer_owner(
         # Notify old owner
         if notify_old and not old_inh_owner and issuer != old_owner:
             if app_alerts:
+                if old_owner_role:
+                    alert_name = 'role_update'
+                    message = ROLE_UPDATE_MSG.format(
+                        project=project.title, role=old_owner_role.name
+                    )
+                    alert_url = reverse(
+                        'projectroles:detail',
+                        kwargs={'project': project.sodar_uuid},
+                    )
+                else:
+                    alert_name = 'role_delete'
+                    message = ROLE_DELETE_MSG.format(
+                        project_type=get_display_name(project.type)
+                    )
+                    alert_url = None
                 app_alerts.add_alert(
                     app_name=APP_NAME,
-                    alert_name='role_update',
+                    alert_name=alert_name,
                     user=old_owner,
-                    message=ROLE_UPDATE_MSG.format(
-                        project=project.title,
-                        role=old_owner_role.name if old_owner_role else 'None',
-                    ),
-                    url=reverse(
-                        'projectroles:detail',
-                        kwargs={'project': project.sodar_uuid},
-                    ),
+                    message=message,
+                    url=alert_url,
                     project=project,
                 )
             if (
@@ -2321,7 +2331,11 @@ def transfer_owner(
             ):
                 # NOTE: Request is needed here
                 email.send_role_change_mail(
-                    'update', project, old_owner, old_owner_role, request
+                    'update' if old_owner_role else 'delete',
+                    project,
+                    old_owner,
+                    old_owner_role,
+                    request,
                 )
         # Notify new owner
         if not new_inh_owner and issuer != new_owner: