fix old owner in owner transfer (#638)

projectroles/forms.py

@@ -809,6 +809,28 @@ def clean(self):
 class RoleAssignmentOwnerTransferForm(SODARForm):
     """Form for transferring owner role assignment between users"""
 
+    def _get_old_owner_choices(self, project, old_owner_as):
+        q_kwargs = {}
+        # TODO: Use get_role() once altered
+        inh_role_as = (
+            RoleAssignment.objects.filter(
+                project__in=project.get_parents(), user=old_owner_as.user
+            )
+            .order_by('role__rank')
+            .first()
+        )
+        if (
+            not inh_role_as
+            or inh_role_as.role.rank != ROLE_RANKING[PROJECT_ROLE_OWNER]
+        ):
+            q_kwargs['rank__gt'] = ROLE_RANKING[PROJECT_ROLE_OWNER]
+        if inh_role_as:
+            q_kwargs['rank__lte'] = inh_role_as.role.rank
+        return [
+            get_role_option(role)
+            for role in Role.objects.filter(**q_kwargs).order_by('rank')
+        ]
+
     def __init__(self, project, current_user, current_owner, *args, **kwargs):
         """Override for form initialization"""
         super().__init__(*args, **kwargs)
@@ -828,18 +850,20 @@ def __init__(self, project, current_user, current_owner, *args, **kwargs):
             exclude=[current_owner],
         )
 
-        # Add old_owner_role only if old owner has no inherited role
-        if not RoleAssignment.objects.filter(
-            project__in=project.get_parents(), user=current_owner
-        ):
-            self.selectable_roles = get_role_choices(project, current_user)
-            self.fields['old_owner_role'] = forms.ChoiceField(
-                label='New role for {}'.format(current_owner.username),
-                help_text='New role for the current owner. Select "Remove" in '
-                'the member list to remove the user\'s membership.',
-                choices=self.selectable_roles,
-                initial=Role.objects.get(name=PROJECT_ROLE_CONTRIBUTOR).pk,
-            )
+        old_owner_as = RoleAssignment.objects.get(
+            project=project, user=current_owner, role__name=PROJECT_ROLE_OWNER
+        )
+        self.selectable_roles = self._get_old_owner_choices(
+            project, old_owner_as
+        )
+        self.fields['old_owner_role'] = forms.ChoiceField(
+            label='New role for {}'.format(current_owner.username),
+            help_text='New role for the current owner. Select "Remove" in '
+            'the member list to remove the user\'s membership.',
+            choices=self.selectable_roles,
+            initial=self.selectable_roles[0][0],
+            disabled=True if len(self.selectable_roles) == 1 else False,
+        )
 
         self.fields['project'] = forms.Field(
             widget=forms.HiddenInput(), initial=project.sodar_uuid
@@ -893,17 +917,11 @@ def clean_new_owner(self):
             raise forms.ValidationError(
                 'The new owner shouldn\'t be the current owner.'
             )
-        role_as = RoleAssignment.objects.get_assignment(user, self.project)
-        inh_owners = [
-            a.user for a in self.project.get_owners(inherited_only=True)
-        ]
-        if (role_as and role_as.project != self.project) or (
-            not role_as and user not in inh_owners
-        ):
+        role_as = self.project.get_role(user)
+        if not role_as:
             raise forms.ValidationError(
-                'The new owner has no roles in the {}.'.format(
-                    get_display_name(self.project.type)
-                )
+                'The new owner has no inherited or local roles in the '
+                '{}.'.format(get_display_name(self.project.type))
             )
         return user
 
@@ -1001,8 +1019,8 @@ def clean(self):
             ):
                 self.add_error(
                     'email',
-                    'Local users not allowed, email domain {} not recognized for '
-                    'LDAP users'.format(domain),
+                    'Local users not allowed, email domain {} not recognized '
+                    'for LDAP users'.format(domain),
                 )
         except AttributeError:
             pass
@@ -1151,10 +1169,7 @@ def get_role_choices(
     :param project: Project in which role will be assigned
     :param current_user: User for whom the form is displayed
     :param promote_as: Assignment for promoting inherited user or inactive
-                       assignment overridden by inherited one (RoleAssignment or
-                       None)
-    :param inactive_as: Inactive assignment overridden by inherited one
-    (RoleAssignment or None)
+    assignment overridden by inherited one (RoleAssignment or None)
     :param allow_delegate: Whether delegate should be allowed (bool)
     :return: List
     """
@@ -1168,4 +1183,17 @@ def get_role_choices(
     qs = Role.objects.all()
     if promote_as:
         qs = qs.filter(rank__lt=promote_as.role.rank)
-    return [(role.pk, role.name) for role in qs.exclude(name__in=role_excludes)]
+    return [
+        get_role_option(role) for role in qs.exclude(name__in=role_excludes)
+    ]
+
+
+def get_role_option(role):
+    """
+    Return form option value for a Role object.
+
+    :param role: Role object
+    return: Role key and legend
+    """
+    # TODO: Update for proper display name (see #1027)
+    return role.pk, role.name

projectroles/models.py

@@ -348,6 +348,7 @@ def get_roles(
         :param max_rank: Limit roles to maximum rank (integer or None)
         :return: List of RoleAssignment objects
         """
+        # TODO: Refactor this to order by rank and return the highest
         projects = [self]
         if inherited:
             projects += list(self.get_parents())

projectroles/tests/test_views.py

@@ -3206,10 +3206,10 @@ def test_transfer_as_old_owner(self):
         self.assertEqual(self.app_alert_model.objects.count(), 1)
         self.assertEqual(len(mail.outbox), 1)
 
-    def test_transfer_inherited_owner_old(self):
-        """Test ownership transfer from owner with inherited role"""
-        new_as = self.make_assignment(
-            self.category, self.user_owner, self.role_contributor
+    def test_transfer_old_inherited_member(self):
+        """Test transfer from old owner with inherited non-owner role"""
+        self.make_assignment(
+            self.category, self.user_new, self.role_contributor
         )
         self.assertEqual(self.project.get_role(self.user_owner), self.owner_as)
         with self.login(self.user):
@@ -3220,20 +3220,55 @@ def test_transfer_inherited_owner_old(self):
                 ),
                 data={
                     'project': self.project.sodar_uuid,
-                    'new_owner': self.user_owner_cat.sodar_uuid,
+                    'old_owner_role': self.role_contributor.pk,
+                    'new_owner': self.user_new.sodar_uuid,
                 },
             )
         self.assertEqual(response.status_code, 302)
-        self.assertEqual(self.project.get_owner().user, self.user_owner_cat)
-        self.assertEqual(self.project.get_role(self.user_owner), new_as)
-        # Should only create one alert/mail for new owner
-        # TODO: These are not correctly created, fix!
+        self.assertEqual(self.project.get_owner().user, self.user_new)
+        self.owner_as.refresh_from_db()
+        self.assertEqual(self.project.get_role(self.user_owner), self.owner_as)
+        self.assertEqual(self.owner_as.role, self.role_contributor)
         self.assertEqual(self.app_alert_model.objects.count(), 2)
         self.assertEqual(len(mail.outbox), 2)
 
-    # TODO: Test for old owner with inherited role
-    # TODO: Test for new owner inherited role (create new)
-    # TODO: Test for new owner with overridden local role (update old)
+    def test_transfer_old_inherited_owner(self):
+        """Test transfer from old owner with inherited owner role"""
+        self.assertEqual(
+            self.project.get_role(self.user_owner_cat), self.owner_as_cat
+        )
+        self.assertEqual(self.project.get_role(self.user_owner), self.owner_as)
+        with self.login(self.user):
+            response = self.client.post(
+                reverse(
+                    'projectroles:role_owner_transfer',
+                    kwargs={'project': self.project.sodar_uuid},
+                ),
+                data={
+                    'project': self.project.sodar_uuid,
+                    'old_owner_role': self.role_contributor.pk,
+                    'new_owner': self.user_owner_cat.sodar_uuid,
+                },
+            )
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(self.project.get_owner().user, self.user_owner_cat)
+        self.owner_as.refresh_from_db()
+        self.assertEqual(self.project.get_role(self.user_owner), self.owner_as)
+        self.assertEqual(self.owner_as.role, self.role_contributor)
+        self.assertEqual(
+            self.project.get_role(self.user_owner_cat),
+            RoleAssignment.objects.get(
+                project=self.project,
+                user=self.user_owner_cat,
+                role=self.role_owner,
+            ),
+        )
+        self.assertEqual(self.app_alert_model.objects.count(), 1)
+        self.assertEqual(len(mail.outbox), 1)
+
+    # TODO: Test for new owner inherited member role
+    # TODO: Test for new owner inherited owner role
+    # TODO: Test for new owner with overridden local role
 
 
 class TestProjectInviteCreateView(

projectroles/views.py

@@ -1869,18 +1869,15 @@ def _handle_transfer(
     '''
 
     @transaction.atomic
-    def transfer_owner(
-        self, project, new_owner, old_owner_as, old_owner_role=None
-    ):
+    def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role):
         """
         Transfer project ownership to a new user and assign a new role to the
         previous owner.
 
         :param project: Project object
         :param new_owner: User object
         :param old_owner_as: RoleAssignment object
-        :param old_owner_role: Role object for the previous owner's new role, or
-        None if inherited role exists
+        :param old_owner_role: Role object for the previous owner's new role
         :return:
         """
         app_alerts = get_backend_api('appalerts_backend')
@@ -1894,7 +1891,7 @@ def transfer_owner(
             .order_by('role__rank')
             .first()
         )
-        old_owner_inh = (
+        old_inh_owner = (
             True if old_inh_as and old_inh_as.role == role_owner else False
         )
         # New owner inheritance
@@ -1905,7 +1902,7 @@ def transfer_owner(
             .order_by('role__rank')
             .first()
         )
-        new_owner_inh = (
+        new_inh_owner = (
             True if new_inh_as and new_inh_as.role == role_owner else False
         )
         '''
@@ -1939,21 +1936,13 @@ def transfer_owner(
             raise ex
         '''
 
+        # Inherited owner, delete local role
+        if old_inh_owner:
+            old_owner_as.delete()
         # Update old owner role
-        if old_owner_role:
+        else:
             old_owner_as.role = old_owner_role
             old_owner_as.save()
-        # Inherited user, delete local role
-        elif old_inh_as:
-            old_owner_as.delete()
-        # Handle error (we should not get here)
-        else:
-            raise ValueError(
-                'If old owner is not inherited owner, new role must be set'
-            )
-        # Set the actual old owner role for remaining actions if inherited
-        if old_inh_as:
-            old_owner_role = old_inh_as.role
 
         # Update or create new owner role
         new_role_as = RoleAssignment.objects.filter(
@@ -1984,7 +1973,7 @@ def transfer_owner(
         if tl_event:
             tl_event.set_status('OK')
         # TODO: Update for inherited role
-        if old_owner_role != role_owner and self.request.user != old_owner:
+        if not old_inh_owner and self.request.user != old_owner:
             if app_alerts:
                 app_alerts.add_alert(
                     app_name=APP_NAME,
@@ -2004,7 +1993,7 @@ def transfer_owner(
                     'update', project, old_owner, old_owner_role, self.request
                 )
 
-        if not new_owner_inh and self.request.user != new_owner:
+        if not new_inh_owner and self.request.user != new_owner:
             if app_alerts:
                 app_alerts.add_alert(
                     app_name=APP_NAME,
@@ -2065,7 +2054,7 @@ def form_valid(self, form):
         old_owner = form.current_owner
         old_owner_as = project.get_owner()
         new_owner = form.cleaned_data['new_owner']
-        old_owner_role = form.cleaned_data.get('old_owner_role')
+        old_owner_role = form.cleaned_data['old_owner_role']
         redirect_url = reverse(
             'projectroles:roles', kwargs={'project': project.sodar_uuid}
         )