From d670dbecd99b86173ec10adac11c64182bbb9f8f Mon Sep 17 00:00:00 2001
From: Nicolas ROCHE <nroche@entrouvert.com>
Date: Fri, 31 Jul 2020 17:08:36 +0200
Subject: [PATCH] manager: display parent roles OU on the role members page
 (#43269)

---
 src/authentic2/manager/role_views.py          |  6 ++-
 .../authentic2/manager/role_members.html      |  4 +-
 tests/test_manager.py                         | 42 +++++++++++++++++++
 3 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/src/authentic2/manager/role_views.py b/src/authentic2/manager/role_views.py
index 22594e1c..306b90d0 100644
--- a/src/authentic2/manager/role_views.py
+++ b/src/authentic2/manager/role_views.py
@@ -19,17 +19,17 @@ import json
 from django.core.exceptions import PermissionDenied
 from django.utils.translation import ugettext_lazy as _
 from django.urls import reverse
 from django.views.generic import FormView, TemplateView
 from django.views.generic.detail import SingleObjectMixin
 from django.contrib import messages
 from django.contrib.contenttypes.models import ContentType
 from django.db.models.query import Q, Prefetch
-from django.db.models import Count
+from django.db.models import Count, F
 from django.contrib.auth import get_user_model
 
 from django_rbac.utils import get_role_model, get_permission_model, get_ou_model
 
 from authentic2.utils import redirect
 from authentic2 import hooks, data_transfer
 
 from . import tables, views, resources, forms, app_settings
@@ -190,17 +190,19 @@ class RoleMembersView(views.HideOUColumnMixin, RoleViewMixin, views.BaseSubTable
         if app_settings.ROLE_MEMBERS_FROM_OU:
             kwargs['ou'] = self.object.ou
         return kwargs
 
     def get_context_data(self, **kwargs):
         ctx = super(RoleMembersView, self).get_context_data(**kwargs)
         ctx['children'] = views.filter_view(self.request,
                                             self.object.children(include_self=False, annotate=True))
-        ctx['parents'] = views.filter_view(self.request, self.object.parents(include_self=False, annotate=True))
+        ctx['parents'] = views.filter_view(self.request, self.object.parents(
+            include_self=False, annotate=True).order_by(F('ou').asc(nulls_first=True)))
+        ctx['has_multiple_ou'] = get_ou_model().objects.count() > 1
         ctx['admin_roles'] = views.filter_view(self.request,
                                                self.object.get_admin_role().children(include_self=False,
                                                                                      annotate=True))
         ctx['from_ldap'] = self._can_manage_members and not self.can_manage_members
         return ctx
 
 members = RoleMembersView.as_view()
 
diff --git a/src/authentic2/manager/templates/authentic2/manager/role_members.html b/src/authentic2/manager/templates/authentic2/manager/role_members.html
index 6ccfe7bf..37177cd0 100644
--- a/src/authentic2/manager/templates/authentic2/manager/role_members.html
+++ b/src/authentic2/manager/templates/authentic2/manager/role_members.html
@@ -112,17 +112,19 @@
     <a rel="popup" href="{% url "a2-manager-role-add-child" pk=object.pk %}" class="role-add icon-add-sign"></a>
   {% else %}
     <a title="{% trans "Permission denied" %}" class="disabled role-add icon-add-sign"></a>
   {% endif %}
  </div>
  <div class="role-inheritance gadjo-folding">
    {% trans "Parent roles:" %}
    {% for parent in parents %}
-     <a class="role" href="{% url "a2-manager-role-members" pk=parent.pk %}">{{ parent }}</a>
+     <a class="role" href="{% url "a2-manager-role-members" pk=parent.pk %}">
+       {% if parent.ou and has_multiple_ou %}{{ parent.ou }} - {% endif %}{{ parent }}
+     </a>
      {% if parent.direct %}
        {% if not object.is_internal %}
          <a rel="popup" href="{% url "a2-manager-role-remove-parent" pk=object.pk parent_pk=parent.pk %}" class="role-remove icon-minus-sign"></a>
        {% else %}
          <a title="{% trans "This role is technical, you cannot modify its permissions." %}" class="disabled role-add icon-minus-sign"></a>
        {% endif %}
      {% else %}
        <a title="{% trans "Indirect parent role" %}" class="disabled role-remove icon-minus-sign"></a>
diff --git a/tests/test_manager.py b/tests/test_manager.py
index 5cd22025..b347300f 100644
--- a/tests/test_manager.py
+++ b/tests/test_manager.py
@@ -1029,8 +1029,50 @@ def test_manager_widgets_field_id_other_user(app, admin, simple_user, simple_rol
     # same request from the page served for admin
     select2_json = request_select2(app, response)
     # simple_user doesn't see all roles
     assert simple_role.pk == select2_json['results'][0]['id']
 
     # anymous user receive 404
     app.session.flush()
     select2_json = request_select2(app, response, get_kwargs={'status': 404})
+
+
+def test_display_parent_roles_on_role_page(app, superuser, settings):
+    ou1 = get_default_ou()
+    ou1.name = ('ou1')
+    ou1.save()
+
+    child = Role.objects.create(name='child', slug='role', ou=ou1)
+    parent1 = Role.objects.create(name='parent1', slug='role1', ou=None)
+    parent2 = Role.objects.create(name='parent2', slug='role2', ou=ou1)
+    child.add_parent(parent1)
+    child.add_parent(parent2)
+    child.save()
+
+    # do not display roles if we have a single OU
+    url = reverse('a2-manager-role-members', kwargs={'pk': child.pk})
+    login(app, superuser)
+    response = app.get(url, status=200)
+    parent_roles_html = response.html.find_all('div', {'class': 'role-inheritance'})[3]
+    assert 'Parent roles:' in parent_roles_html.text
+    assert [x.text.strip() for x in parent_roles_html.find_all('a', {'class': 'role'})] == \
+        ['parent1', 'parent2']
+
+    # display parent roles if we have multiple OUs
+    ou2 = OU.objects.create(name='ou2')
+    response = app.get(url, status=200)
+    parent_roles_html = response.html.find_all('div', {'class': 'role-inheritance'})[3]
+    assert 'Parent roles:' in parent_roles_html.text
+    assert [x.text.strip() for x in parent_roles_html.find_all('a', {'class': 'role'})] == \
+        ['parent1', 'ou1 - parent2']
+
+    # display parent roles sorted by OU
+    parent3 = Role.objects.create(name='parent3', slug='role3', ou=ou2)
+    child.add_parent(parent3)
+    parent4 = Role.objects.create(name='parent4', slug='role4', ou=ou1)
+    child.add_parent(parent4)
+    child.save()
+    response = app.get(url, status=200)
+    parent_roles_html = response.html.find_all('div', {'class': 'role-inheritance'})[3]
+    assert 'Parent roles:' in parent_roles_html.text
+    assert [x.text.strip() for x in parent_roles_html.find_all('a', {'class': 'role'})] == \
+        ['parent1', 'ou1 - parent2', 'ou1 - parent4', 'ou2 - parent3']
-- 
2.27.0

