From b652896c2ad3edde20d6e6cc8ed78013456df397 Mon Sep 17 00:00:00 2001
From: Valentin Deniaud <vdeniaud@entrouvert.com>
Date: Tue, 3 Aug 2021 16:08:09 +0200
Subject: [PATCH 4/4] manager: display inheritance info excerpt in table on
 role page (#53481)

---
 src/authentic2/manager/role_views.py          | 21 +++--
 .../static/authentic2/manager/css/style.css   |  4 +
 .../authentic2/manager/role_members.html      | 84 +++++++++++++------
 tests/test_ldap.py                            |  2 +-
 tests/test_manager.py                         | 50 -----------
 tests/test_role_manager.py                    | 64 ++++++++++++++
 6 files changed, 141 insertions(+), 84 deletions(-)

diff --git a/src/authentic2/manager/role_views.py b/src/authentic2/manager/role_views.py
index e1b6cd74..ab8d5b13 100644
--- a/src/authentic2/manager/role_views.py
+++ b/src/authentic2/manager/role_views.py
@@ -242,14 +242,21 @@ class RoleMembersView(views.HideOUColumnMixin, RoleViewMixin, views.BaseSubTable
 
     def get_context_data(self, **kwargs):
         ctx = super().get_context_data(**kwargs)
-        ctx['children'] = views.filter_view(
-            self.request, self.object.children(include_self=False, annotate=True)
+        ctx['children'] = list(
+            views.filter_view(
+                self.request,
+                self.object.children(include_self=False, annotate=True).order_by(
+                    F('ou').asc(nulls_first=True), 'name'
+                ),
+            )[:11]
         )
-        ctx['parents'] = views.filter_view(
-            self.request,
-            self.object.parents(include_self=False, annotate=True).order_by(
-                F('ou').asc(nulls_first=True), 'name'
-            ),
+        ctx['parents'] = list(
+            views.filter_view(
+                self.request,
+                self.object.parents(include_self=False, annotate=True).order_by(
+                    F('ou').asc(nulls_first=True), 'name'
+                ),
+            )[:11]
         )
         ctx['has_multiple_ou'] = OU.objects.count() > 1
         ctx['admin_roles'] = views.filter_view(
diff --git a/src/authentic2/manager/static/authentic2/manager/css/style.css b/src/authentic2/manager/static/authentic2/manager/css/style.css
index 54eb8264..a9e6441a 100644
--- a/src/authentic2/manager/static/authentic2/manager/css/style.css
+++ b/src/authentic2/manager/static/authentic2/manager/css/style.css
@@ -273,3 +273,7 @@ span.activity {
 table.main.plaintable tr:hover td {
 	cursor: default;
 }
+
+a.role-inheritance-view-all {
+    font-style: italic;
+}
diff --git a/src/authentic2/manager/templates/authentic2/manager/role_members.html b/src/authentic2/manager/templates/authentic2/manager/role_members.html
index 481e7f8c..b63c3009 100644
--- a/src/authentic2/manager/templates/authentic2/manager/role_members.html
+++ b/src/authentic2/manager/templates/authentic2/manager/role_members.html
@@ -71,6 +71,64 @@
            <button>{% trans "Add" %}</button>
    </form>
  {% endif %}
+
+ <div class="section">
+   <h3>{% trans "Child roles" %}
+     {% if view.can_manage_members %}
+     <a href="{% url "a2-manager-role-children" pk=object.pk %}" class="button">{% trans "Edit" %}</a>
+     {% else %}
+     <a title="{% trans "Permission denied" %}" class="button disabled">{% trans "Edit" %}</a>
+     {% endif %}
+   </h3>
+   <div>
+     {% if children %}
+     <ul class="objects-list single-links">
+       {% for child in children|slice:":10" %}
+       <li>
+         <a  class="role-inheritance-child" href="{% url "a2-manager-role-members" pk=child.pk %}">{% if child.ou and has_multiple_ou %}{{ child.ou }} - {% endif %}{{ child }}</a>
+         {% if not child.direct %}
+         <span class="badge">{% trans "Indirect" %}</span>
+         {% endif %}
+       </li>
+       {% endfor %}
+       {% if children|length > 10 %}
+       <li><a class="role-inheritance-view-all" href="{% url "a2-manager-role-children" pk=object.pk %}">({% trans "view all roles" %})</a></li>
+       {% endif %}
+     </ul>
+     {% else %}
+     <p>{% trans "This role has no children." %}</p>
+     {% endif %}
+   </div>
+ </div>
+ <div class="section">
+   <h3>{% trans "Parent roles" %}
+     {% if not object.is_internal %}
+     <a href="{% url "a2-manager-role-parents" pk=object.pk %}" class="button">{% trans "Edit" %}</a>
+     {% else %}
+     <a title="{% trans "This role is technical, you cannot modify its permissions." %}" class="button disabled">{% trans "Edit" %}</a>
+     {% endif %}
+   </h3>
+   <div>
+     {% if parents %}
+     <ul class="objects-list single-links">
+       {% for parent in parents|slice:":10" %}
+       <li>
+         <a class="role-inheritance-parent" href="{% url "a2-manager-role-members" pk=parent.pk %}">{% if parent.ou and has_multiple_ou %}{{ parent.ou }} - {% endif %}{{ parent }}</a>
+         {% if not parent.direct %}
+         <span class="badge">{% trans "Indirect" %}</span>
+         {% endif %}
+       </li>
+       {% endfor %}
+       {% if parents|length > 10 %}
+       <li><a class="role-inheritance-view-all" href="{% url "a2-manager-role-parents" pk=object.pk %}">({% trans "view all roles" %})</a></li>
+       {% endif %}
+     </ul>
+     {% else %}
+     <p>{% trans "This role has no parents." %}</p>
+     {% endif %}
+   </div>
+ </div>
+
  <fieldset class="gadjo-foldable gadjo-folded" id="other-properties">
  <legend class="gadjo-foldable-widget">{% trans "Advanced parameters" %}</legend>
  <div class="role-inheritance gadjo-folding">
@@ -105,31 +163,5 @@
     <a title="{% trans "Permission denied" %}" class="disabled role-add icon-add-sign"></a>
   {% endif %}
  </div>
- <div class="role-inheritance gadjo-folding">
-   {% trans "Child roles:" %}
-   {% for child in children %}
-     <a href="{% url "a2-manager-role-members" pk=child.pk %}">{{ child }}</a>
-     {% if not forloop.last %} − {% endif %}
-   {% endfor %}
-  {% if view.can_manage_members %}
-    <a href="{% url "a2-manager-role-children" 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 %}">
-       {% if parent.ou and has_multiple_ou %}{{ parent.ou }} - {% endif %}{{ parent }}
-     </a>
-    {% if not forloop.last %} − {% endif %}
-   {% endfor %}
-   {% if not object.is_internal %}
-     <a href="{% url "a2-manager-role-parents" pk=object.pk %}" class="role-add icon-add-sign"></a>
-   {% else %}
-     <a title="{% trans "This role is technical, you cannot modify its permissions." %}" class="disabled role-add icon-add-sign"></a>
-   {% endif %}
- </div>
  </fieldset>
 {% endblock %}
diff --git a/tests/test_ldap.py b/tests/test_ldap.py
index 6bd2d5d4..e86a88e0 100644
--- a/tests/test_ldap.py
+++ b/tests/test_ldap.py
@@ -652,7 +652,7 @@ def test_group_to_role_mapping_modify_disabled(slapd, settings, db, app, admin,
     assert 'synchronised from LDAP' in response.text
     q = response.pyquery.remove_namespaces()
     assert not q('form.manager-m2m-add-form')
-    assert q('div.role-inheritance .role-add.disabled')
+    assert q('div.section h3 a.button.disabled')
     assert not q('table tbody td a.icon-remove-sign js-remove-object')
 
 
diff --git a/tests/test_manager.py b/tests/test_manager.py
index 6432d7d5..6ecc92db 100644
--- a/tests/test_manager.py
+++ b/tests/test_manager.py
@@ -1161,56 +1161,6 @@ def test_manager_role_inheritance_list_search_permission(app, admin, simple_user
     assert {e.text_content() for e in q('table tbody td.name')} == {simple_role.name}
 
 
-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',
-    ]
-
-
 def test_manager_service_search(app, admin, ou1):
     Service.objects.create(ou=ou1, slug='test', name='Test Service')
     Service.objects.create(ou=get_default_ou(), slug='example', name='Example Service')
diff --git a/tests/test_role_manager.py b/tests/test_role_manager.py
index b2795b0f..11aa222e 100644
--- a/tests/test_role_manager.py
+++ b/tests/test_role_manager.py
@@ -16,15 +16,21 @@
 
 import json
 
+import pytest
+from django.urls import reverse
 from django.utils.encoding import force_bytes, force_text
 from webtest import Upload
 
 from authentic2.a2_rbac.models import OrganizationalUnit, Role
 from authentic2.a2_rbac.utils import get_default_ou
 from authentic2.custom_user.models import User
+from django_rbac.utils import get_ou_model, get_role_model
 
 from .utils import login, text_content
 
+OU = get_ou_model()
+Role = get_role_model()
+
 
 def test_manager_role_export(app, admin, ou1, role_ou1, ou2, role_ou2):
     import csv
@@ -336,3 +342,61 @@ def test_manager_role_csv_import(app, admin, ou1, ou2):
     resp = app.get('/manage/roles/csv-import/')
     resp = resp.click('Download sample')
     assert 'name,slug,ou' in resp.text
+
+
+@pytest.mark.parametrize('relation', ['child', 'parent'])
+def test_role_members_display_inheritance_info(app, superuser, settings, simple_role, relation):
+    url = reverse('a2-manager-role-members', kwargs={'pk': simple_role.pk})
+
+    resp = login(app, superuser, url)
+    assert 'This role has no children.' in resp.text
+    assert 'This role has no parents.' in resp.text
+
+    for i in range(10):
+        role = Role.objects.create(name=f'Role {i}', ou=get_default_ou())
+        getattr(simple_role, 'add_%s' % relation)(role)
+
+    resp = app.get(url)
+    if relation == 'child':
+        assert 'This role has no children.' not in resp.text
+        assert 'This role has no parents.' in resp.text
+    elif relation == 'parent':
+        assert 'This role has no children.' in resp.text
+        assert 'This role has no parents.' not in resp.text
+
+    for i, el in enumerate(resp.pyquery.find('a.role-inheritance-%s' % relation)):
+        assert el.text == f'Role {i}'
+    assert '(view all roles)' not in resp.text
+
+    role = Role.objects.create(name=f'Role a', ou=get_default_ou())
+    getattr(simple_role, 'add_%s' % relation)(role)
+    resp = app.get(url)
+    assert 'Role a' not in resp.text
+    assert '(view all roles)' in resp.text
+
+    resp = resp.click('(view all roles)', href=relation)
+    assert 'Role a' in resp.text
+
+    # display OU if there are more than one
+    ou1 = OU.objects.create(name='ou1')
+    resp = app.get(url)
+    for i, el in enumerate(resp.pyquery.find('a.role-inheritance-%s' % relation)):
+        assert el.text == f'Default organizational unit - Role {i}'
+
+    # sort by OU, then name
+    Role.objects.filter(name__in=['Role 2', 'Role 3', 'Role 4']).update(ou=ou1)
+    Role.objects.filter(name__in=['Role 5', 'Role 6']).update(ou=None)
+
+    resp = app.get(url)
+    assert [el.text for el in resp.pyquery.find('a.role-inheritance-%s' % relation)] == [
+        'Role 5',
+        'Role 6',
+        'Default organizational unit - Role 0',
+        'Default organizational unit - Role 1',
+        'Default organizational unit - Role 7',
+        'Default organizational unit - Role 8',
+        'Default organizational unit - Role 9',
+        'Default organizational unit - Role a',
+        'ou1 - Role 2',
+        'ou1 - Role 3',
+    ]
-- 
2.20.1

