Repository: rdmorganiser/rdmo (language: python)

Issue:
Differentiate the appearance of unavailable catalogs in the project create form
### Rationale / Begründung

Catalogs that are unavailable look the same as available ones in the Create Project form. This can be confusing for editors/managers who want to see what the users normally see as catalog choices.

### Affected 

Managers, Admins
- user should not see unavailable catalogs anyway

### Minimal functionality

have some distinction in appearance and sorted by availability


### References / Verweise

* `ProjectCreateView` and `CatalogChoiceField`



Discussion / hints:

Fix patch:
diff --git a/rdmo/projects/forms.py b/rdmo/projects/forms.py
index 9bf14d2421..7ef1761f3a 100644
--- a/rdmo/projects/forms.py
+++ b/rdmo/projects/forms.py
@@ -18,7 +18,12 @@
 
 class CatalogChoiceField(forms.ModelChoiceField):
 
+    _unavailable_icon = ' (<span class="fa fa-eye-slash" aria-hidden="true"></span>)'
+
     def label_from_instance(self, obj):
+        if obj.available is False:
+            return mark_safe('<div class="text-muted">%s%s</br>%s</div>' % (obj.title, self._unavailable_icon, markdown2html(obj.help)))
+
         return mark_safe('<b>%s</b></br>%s' % (obj.title, markdown2html(obj.help)))
 
 
diff --git a/rdmo/projects/views/project_create.py b/rdmo/projects/views/project_create.py
index aadc44cdb7..9e2f6e8137 100644
--- a/rdmo/projects/views/project_create.py
+++ b/rdmo/projects/views/project_create.py
@@ -24,7 +24,8 @@ class ProjectCreateView(LoginRequiredMixin, RedirectViewMixin, CreateView):
     def get_form_kwargs(self):
         catalogs = Catalog.objects.filter_current_site() \
                                   .filter_group(self.request.user) \
-                                  .filter_availability(self.request.user)
+                                  .filter_availability(self.request.user) \
+                                  .order_by('-available', 'order')
         projects = Project.objects.filter_user(self.request.user)
 
         form_kwargs = super().get_form_kwargs()


Test patch:
diff --git a/rdmo/projects/tests/test_view_project.py b/rdmo/projects/tests/test_view_project.py
index 1c48b5d985..9c70f30d96 100644
--- a/rdmo/projects/tests/test_view_project.py
+++ b/rdmo/projects/tests/test_view_project.py
@@ -4,8 +4,11 @@
 from django.urls import reverse
 from pytest_django.asserts import assertTemplateUsed
 
+from rdmo.questions.models import Catalog
+from rdmo.questions.tests.test_models import unavailable_catalogs_kwargs
 from rdmo.views.models import View
 
+from ..forms import CatalogChoiceField
 from ..models import Project
 
 users = (
@@ -16,6 +19,9 @@
     ('user', 'user'),
     ('site', 'site'),
     ('anonymous', None),
+    ('editor', 'editor'),
+    ('reviewer', 'reviewer'),
+    ('api', 'api'),
 )
 
 view_project_permission_map = {
@@ -23,28 +29,25 @@
     'manager': [1, 3, 5, 7],
     'author': [1, 3, 5, 8],
     'guest': [1, 3, 5, 9],
-    'api': [1, 2, 3, 4, 5],
-    'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
+    'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
+    'api': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
 }
 
 change_project_permission_map = {
     'owner': [1, 2, 3, 4, 5, 10],
     'manager': [1, 3, 5, 7],
-    'api': [1, 2, 3, 4, 5],
     'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
 }
 
 delete_project_permission_map = {
     'owner': [1, 2, 3, 4, 5, 10],
-    'api': [1, 2, 3, 4, 5],
-    'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
+    'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
 }
 
 export_project_permission_map = {
     'owner': [1, 2, 3, 4, 5, 10],
     'manager': [1, 3, 5, 7],
-    'api': [1, 2, 3, 4, 5],
-    'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
+    'site': [1, 2, 3, 4, 5, 6, 7, 8, 9, 10],
 }
 
 projects = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
@@ -69,11 +72,10 @@ def test_list(db, client, username, password):
         assert response.status_code == 200
         assertTemplateUsed(response, 'projects/projects.html')
 
-        if username == 'site':
+        if username in ('site', 'api'):
             assert projects == []
             assert response.context['number_of_projects'] == len([])
         else:
-            # breakpoint()
             user_projects_map = view_project_permission_map.get(username, [])
             assert sorted(list(set(map(int, projects)))) == user_projects_map
             assert response.context['number_of_projects'] == len(user_projects_map)
@@ -113,7 +115,10 @@ def test_detail(db, client, username, password, project_id):
     response = client.get(url)
 
     if project_id in view_project_permission_map.get(username, []):
-        assert response.status_code == 200
+        if username == 'api':
+            assert response.status_code == 403
+        else:
+            assert response.status_code == 200
     else:
         if password:
             assert response.status_code == 403
@@ -138,6 +143,38 @@ def test_project_create_get(db, client, username, password):
         assert response.status_code == 302
 
 
+@pytest.mark.parametrize('username,password', users)
+def test_project_create_get_for_extra_users_and_unavailable_catalogs(db, client, username, password):
+
+    for catalog_kwargs in unavailable_catalogs_kwargs:
+        Catalog.objects.create(**catalog_kwargs)
+
+    client.login(username=username, password=password)
+
+    url = reverse('project_create')
+    response = client.get(url)
+
+    if password:
+        assert response.status_code == 200
+        # check the catalogs that are displayed in the form
+        find_catalog_ids = re.findall(r'id="id_catalog_(\d+)', response.content.decode())
+        catalogs_in_form = response.context_data['form'].fields['catalog'].queryset
+        # Catalog.objects.filter_availability(response.context['user'])
+        assert find_catalog_ids == list(map(str, range(catalogs_in_form.count())))
+
+        # check the unavailable catalogs that are displayed in the form
+        # should happen only for users that can see unavailable catalogs (editors, api,..)
+        for unavailable_catalog in catalogs_in_form.filter(available=False):
+            _label = unavailable_catalog.title + CatalogChoiceField._unavailable_icon
+            assert _label in response.content.decode()
+
+        # check the parent select dropdown
+        for project_id in re.findall(r'<option value="(\d+)"', response.content.decode()):
+            assert int(project_id) in view_project_permission_map.get(username, [])
+    else:
+        assert response.status_code == 302
+
+
 @pytest.mark.parametrize('username,password', users)
 def test_project_create_post(db, client, username, password):
     client.login(username=username, password=password)
@@ -173,7 +210,7 @@ def test_project_create_parent_post(db, client, username, password):
     }
     response = client.post(url, data)
 
-    if username == 'user':
+    if username in ('user', 'editor', 'reviewer'):
         assert response.status_code == 200
         assert Project.objects.count() == project_count
     elif password:
@@ -565,7 +602,10 @@ def test_project_answers(db, client, username, password, project_id):
     response = client.get(url)
 
     if project_id in view_project_permission_map.get(username, []):
-        assert response.status_code == 200
+        if username == 'api':
+            assert response.status_code == 403
+        else:
+            assert response.status_code == 200
     else:
         if password:
             assert response.status_code == 403
@@ -583,7 +623,10 @@ def test_project_answers_export(db, client, username, password, project_id, expo
     response = client.get(url)
 
     if project_id in view_project_permission_map.get(username, []):
-        assert response.status_code == 200
+        if username == 'api':
+            assert response.status_code == 403
+        else:
+            assert response.status_code == 200
     else:
         if password:
             assert response.status_code == 403
@@ -602,6 +645,9 @@ def test_project_view(db, client, username, password, project_id):
         response = client.get(url)
 
         if project_id in view_project_permission_map.get(username, []):
+            if username == 'api':
+                assert response.status_code == 403
+                return
             if view in project_views:
                 assert response.status_code == 200
             else:
@@ -625,6 +671,9 @@ def test_project_view_export(db, client, username, password, project_id, export_
         response = client.get(url)
 
         if project_id in view_project_permission_map.get(username, []):
+            if username == 'api':
+                assert response.status_code == 403
+                return
             if view in project_views:
                 assert response.status_code == 200
             else:
@@ -645,6 +694,9 @@ def test_project_questions(db, client, username, password, project_id):
     response = client.get(url)
 
     if project_id in view_project_permission_map.get(username, []):
+        if username == 'api':
+            assert response.status_code == 403
+            return
         assert response.status_code == 200
     elif password:
         assert response.status_code == 403
@@ -661,7 +713,10 @@ def test_project_error(db, client, username, password, project_id):
     response = client.get(url)
 
     if project_id in view_project_permission_map.get(username, []):
-        assert response.status_code == 200
+        if username == 'api':
+            assert response.status_code == 403
+        else:
+            assert response.status_code == 200
     elif password:
         assert response.status_code == 403
     else:
diff --git a/rdmo/questions/tests/test_models.py b/rdmo/questions/tests/test_models.py
index 0a15480ea1..2259541623 100644
--- a/rdmo/questions/tests/test_models.py
+++ b/rdmo/questions/tests/test_models.py
@@ -1,6 +1,32 @@
 from ..models import Catalog, Question, QuestionSet, Section
 
 
+unavailable_catalogs_kwargs = [
+    {
+        'uri': 'https://experimental.com/terms/questions/unavailable-1',
+        'uri_prefix': 'https://experimental.com/terms',
+        'key': 'unavailable-1',
+        'order': 1,
+        'title_lang1': 'New Catalog 1',
+        'title_lang2': 'Neuer Katalog 1',
+        'help_lang1': 'Help text',
+        'help_lang2': 'Hilfe Text',
+        'available': False
+    },
+    {
+        'uri': 'https://experimental.com/terms/questions/unavailable-2',
+        'uri_prefix': 'https://experimental.com/terms',
+        'key': 'unavailable-2',
+        'order': 2,
+        'title_lang1': 'New Catalog 2',
+        'title_lang2': 'Neuer Katalog 2',
+        'help_lang1': 'Help text',
+        'help_lang2': 'Hilfe Text',
+        'available': False
+    },
+]
+
+
 def test_catalog_str(db):
     instances = Catalog.objects.all()
     for instance in instances: