From ba70721ea58b81e7d444b5f01886825dff29bb53 Mon Sep 17 00:00:00 2001 From: Mikko Nieminen Date: Tue, 21 Jan 2025 17:12:10 +0100 Subject: [PATCH] add ui logic and tests (#1090) --- .../templates/projectroles/project_form.html | 26 +- projectroles/tests/test_ui.py | 79 +++++ projectroles/tests/test_views.py | 50 ++- projectroles/views.py | 284 ++++++++++-------- 4 files changed, 296 insertions(+), 143 deletions(-) diff --git a/projectroles/templates/projectroles/project_form.html b/projectroles/templates/projectroles/project_form.html index 88db9e7f..e8cd1dbd 100644 --- a/projectroles/templates/projectroles/project_form.html +++ b/projectroles/templates/projectroles/project_form.html @@ -41,14 +41,10 @@

Update {% get_display_name object.type title=True %}

- {% if object.type == 'PROJECT' %} -
- {# TODO: Add logic to disable button with tooltip #} - - Delete - + + {# TODO: Convert to btn-group, how to enable tooltip for disabled button? #} +
+ {% if object.type == 'PROJECT' %} @@ -58,8 +54,18 @@

Unarchive {% endif %} -

- {% endif %} + {% endif %} + + + Delete + + +
{% elif parent.pk %}

Create {{ project_display }} or {{ category_display }} Under {{ parent.title }}

{% elif disable_categories %} diff --git a/projectroles/tests/test_ui.py b/projectroles/tests/test_ui.py index 2650cd92..ee013ca3 100644 --- a/projectroles/tests/test_ui.py +++ b/projectroles/tests/test_ui.py @@ -1773,6 +1773,12 @@ def test_archive_button(self): [self.superuser], self.url_top, 'sodar-pr-btn-archive', False ) + def test_delete_button(self): + """Test rendering form without delete button""" + self.assert_element_exists( + [self.superuser], self.url_top, 'sodar-pr-btn-delete', False + ) + def test_fields_top(self): """Test rendering of dynamic fields for top level creation view""" self.login_and_redirect(self.superuser, self.url_top) @@ -1897,6 +1903,79 @@ def test_archive_button_archived(self): element = self.selenium.find_element(By.ID, 'sodar-pr-btn-archive') self.assertEqual(element.text, 'Unarchive') + def test_delete_button(self): + """Test rendering delete button""" + self.login_and_redirect(self.superuser, self.url) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertIsNone(elem.get_attribute('disabled')) + + def test_delete_button_category_with_children(self): + """Test rendering delete button with category and children""" + self.login_and_redirect(self.superuser, self.url_cat) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertEqual(elem.get_attribute('disabled'), 'true') + + def test_delete_button_category_no_children(self): + """Test rendering delete button with category and no children""" + self.project.delete() + self.login_and_redirect(self.superuser, self.url_cat) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertIsNone(elem.get_attribute('disabled')) + + def test_delete_button_remote(self): + """Test rendering delete button with non-revoked remote project""" + self.remote_project = self.make_remote_project( + project_uuid=self.project.sodar_uuid, + site=self.remote_site, + level=REMOTE_LEVEL_READ_ROLES, + project=self.project, + ) + self.login_and_redirect(self.superuser, self.url) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertEqual(elem.get_attribute('disabled'), 'true') + + def test_delete_button_remote_revoked(self): + """Test rendering delete button with revoked remote project""" + self.remote_project = self.make_remote_project( + project_uuid=self.project.sodar_uuid, + site=self.remote_site, + level=REMOTE_LEVEL_REVOKED, + project=self.project, + ) + self.login_and_redirect(self.superuser, self.url) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertIsNone(elem.get_attribute('disabled')) + + @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET) + def test_delete_button_remote_target(self): + """Test rendering delete button with non-revoked remote project as target site""" + self.remote_site.mode = SITE_MODE_SOURCE + self.remote_site.save() + self.remote_project = self.make_remote_project( + project_uuid=self.project.sodar_uuid, + site=self.remote_site, + level=REMOTE_LEVEL_READ_ROLES, + project=self.project, + ) + self.login_and_redirect(self.superuser, self.url) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertEqual(elem.get_attribute('disabled'), 'true') + + @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET) + def test_delete_button_remote_revoked_target(self): + """Test rendering delete button with non-revoked remote project as target site""" + self.remote_site.mode = SITE_MODE_SOURCE + self.remote_site.save() + self.remote_project = self.make_remote_project( + project_uuid=self.project.sodar_uuid, + site=self.remote_site, + level=REMOTE_LEVEL_REVOKED, + project=self.project, + ) + self.login_and_redirect(self.superuser, self.url) + elem = self.selenium.find_element(By.ID, 'sodar-pr-btn-delete') + self.assertIsNone(elem.get_attribute('disabled')) + def test_fields_project(self): """Test field visibility for project update""" self.login_and_redirect(self.superuser, self.url) diff --git a/projectroles/tests/test_views.py b/projectroles/tests/test_views.py index 8cdde3d5..a74184f7 100644 --- a/projectroles/tests/test_views.py +++ b/projectroles/tests/test_views.py @@ -78,6 +78,9 @@ LOGIN_MSG, TARGET_CREATE_DISABLED_MSG, SITE_SETTING_UPDATE_MSG, + PROJECT_DELETE_CAT_ERR_MSG, + PROJECT_DELETE_SOURCE_ERR_MSG, + PROJECT_DELETE_TARGET_ERR_MSG, ) from projectroles.context_processors import ( SIDEBAR_ICON_MIN_SIZE, @@ -1168,6 +1171,8 @@ def test_get_project(self): with self.login(self.user): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], True) + self.assertEqual(response.context['project_delete_msg'], '') form = response.context['form'] self.assertIsNotNone(form) self.assertIsInstance(form.fields['type'].widget, HiddenInput) @@ -1195,7 +1200,7 @@ def test_get_remote_site_owner_modifiable_disabled(self): form = response.context['form'] self.assertNotIn(REMOTE_SITE_FIELD, form.fields) - def test_get_remote_project(self): + def test_get_remote(self): """Test GET with remote target project and READ_ROLES perm""" self.make_remote_project( project_uuid=self.project.sodar_uuid, @@ -1206,10 +1211,15 @@ def test_get_remote_project(self): with self.login(self.user): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], False) + self.assertEqual( + response.context['project_delete_msg'], + PROJECT_DELETE_SOURCE_ERR_MSG.format(project_type='project'), + ) form = response.context['form'] self.assertEqual(form.fields[REMOTE_SITE_FIELD].initial, True) - def test_get_remote_projcet_revoked(self): + def test_get_remote_revoked(self): """Test GET with remote target project and REVOKED perm""" self.make_remote_project( project_uuid=self.project.sodar_uuid, @@ -1220,10 +1230,12 @@ def test_get_remote_projcet_revoked(self): with self.login(self.user): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], True) + self.assertEqual(response.context['project_delete_msg'], '') form = response.context['form'] self.assertEqual(form.fields[REMOTE_SITE_FIELD].initial, False) - def test_get_remote_project_read_info(self): + def test_get_remote_read_info(self): """Test GET with remote target project and READ_INFO perm""" self.make_remote_project( project_uuid=self.project.sodar_uuid, @@ -1262,12 +1274,26 @@ def test_get_category(self): with self.login(self.user): response = self.client.get(self.url_cat) self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], False) + self.assertEqual( + response.context['project_delete_msg'], + PROJECT_DELETE_CAT_ERR_MSG.format(project_type='category'), + ) form = response.context['form'] self.assertIsInstance(form.fields['type'].widget, HiddenInput) self.assertNotIsInstance(form.fields['parent'].widget, HiddenInput) self.assertIsInstance(form.fields['owner'].widget, HiddenInput) self.assertNotIn(REMOTE_SITE_FIELD, form.fields) + def test_get_category_no_children(self): + """Test GET with category and no children""" + self.project.delete() + with self.login(self.user): + response = self.client.get(self.url_cat) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], True) + self.assertEqual(response.context['project_delete_msg'], '') + @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET) def test_get_remote_as_target(self): """Test GET with remote project as target""" @@ -1275,6 +1301,11 @@ def test_get_remote_as_target(self): with self.login(self.user): response = self.client.get(self.url) self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], False) + self.assertEqual( + response.context['project_delete_msg'], + PROJECT_DELETE_TARGET_ERR_MSG.format(project_type='project'), + ) form = response.context['form'] self.assertIsInstance(form.fields['title'].widget, HiddenInput) self.assertIsInstance(form.fields['type'].widget, HiddenInput) @@ -1319,6 +1350,19 @@ def test_get_remote_as_target(self): form.fields['settings.projectroles.ip_allowlist'].disabled ) + @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET) + def test_get_remote_as_target_revoked(self): + """Test GET with revoked remote project as target""" + self.set_up_as_target(projects=[self.category, self.project]) + rp = RemoteProject.objects.get(project=self.project) + rp.level = REMOTE_LEVEL_REVOKED + rp.save() + with self.login(self.user): + response = self.client.get(self.url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.context['project_delete_access'], True) + self.assertEqual(response.context['project_delete_msg'], '') + def test_get_not_found(self): """Test GET with invalid project UUID""" with self.login(self.user): diff --git a/projectroles/views.py b/projectroles/views.py index 6455b656..19448992 100644 --- a/projectroles/views.py +++ b/projectroles/views.py @@ -142,8 +142,8 @@ 'level. Revoke remote access on source site to enable deletion.' ) PROJECT_DELETE_SOURCE_ERR_MSG = ( - 'Non-revoked remotes of {project_type} found. Revoke access to them to ' - 'enable deletion.' + 'Non-revoked remotes of {project_type} found on target sites. Revoke ' + 'access to the remotes to enable deletion.' ) TARGET_CREATE_DISABLED_MSG = ( 'PROJECTROLES_TARGET_CREATE=False, creation not allowed.' @@ -1442,6 +1442,145 @@ def form_valid(self, form): return redirect(redirect_url) +class ProjectDeleteAccessMixin: + """ + Mixin to check special access permissions for project deletion. + + Also used for ProjectUpdateView to control access to view link, hence a + separate mixin. + + We want to provide an informative message to the end user and also prevent + superuser access if needed, hence we're not implementing this in rules. + """ + + @classmethod + def check_delete_permission(cls, project): + """ + Check delete permission. Also applies to superusers. + + :param project: Project object + :return: Boolean, string (in case of error) or None + """ + p_type = get_display_name(project.type) + if ( + project.type == PROJECT_TYPE_CATEGORY + and project.get_children().count() > 0 + ): + return False, PROJECT_DELETE_CAT_ERR_MSG.format(project_type=p_type) + # Disallow for remote projects which haven't been revoked + if project.is_remote(): + rp = RemoteProject.objects.filter( + project_uuid=project.sodar_uuid, + site__mode=SITE_MODE_SOURCE, + ).first() + if rp.level != REMOTE_LEVEL_REVOKED: + return ( + False, + PROJECT_DELETE_TARGET_ERR_MSG.format(project_type=p_type), + ) + # Disallow for source projects with non-revoked remote projects + # NOTE: Categories can be deleted + elif project.type == PROJECT_TYPE_PROJECT: + rps = RemoteProject.objects.filter( + project_uuid=project.sodar_uuid, site__mode=SITE_MODE_TARGET + ).exclude(level__in=[REMOTE_LEVEL_NONE, REMOTE_LEVEL_REVOKED]) + if rps.count() > 0: + return ( + False, + PROJECT_DELETE_SOURCE_ERR_MSG.format(project_type=p_type), + ) + return True, None + + +class ProjectDeleteMixin(ProjectModifyPluginViewMixin): + """Mixin for Project deletion in UI and API views""" + + @classmethod + def _create_timeline_event(cls, project, request): + """ + Create timeline summary event for project deletion. Created as a + classified site-wide event only viewable by superusers. + + :param project: Project object + :param request: HttpRequest object + """ + timeline = get_backend_api('timeline_backend') + if not timeline: + return + local_users = { + a.user.username: a.role.name + for a in project.local_roles.order_by( + 'role__rank', 'user__username' + ) + } + parent = str(project.parent.sodar_uuid) if project.parent else None + extra_data = { + 'title': project.title, + 'type': project.type, + 'parent': parent, + 'description': project.description, + 'readme': project.readme.raw, + 'public_guest_access': project.public_guest_access, + 'archive': project.archive, + 'full_title': project.full_title, + 'sodar_uuid': str(project.sodar_uuid), + 'local_roles': local_users, + } + timeline.add_event( + project=None, # No project as it has been deleted + app_name=APP_NAME, + user=request.user, + event_name='project_delete', + description=f'delete {project.type.lower()} ' + f'{project.get_log_title()}', + extra_data=extra_data, + classified=True, + status_type=timeline.TL_STATUS_OK, + ) + + @classmethod + def get_redirect_url(cls, project): + if project.parent: + return reverse( + 'projectroles:detail', + kwargs={'project': project.parent.sodar_uuid}, + ) + else: + return reverse('home') + + def handle_delete(self, project, request): + """ + Handle project deletion. Deletes the object, creates a summary timeline + event and sends out alerts and emails to project members. + + :param project: Project object of project to be deleted + :param request: HttpRequest object + """ + app_alerts = get_backend_api('appalerts_backend') + # Create timeline event + self._create_timeline_event(project, request) + # Create app alerts + if app_alerts: + users = [ + a.user for a in project.get_roles() if a.user != request.user + ] + app_alerts.add_alerts( + app_name=APP_NAME, + alert_name='project_delete', + users=users, + message=PROJECT_DELETE_MSG.format( + project_type=get_display_name(project.type, title=True), + project_title=project.title, + user_name=request.user.username, + ), + ) + # Send email + if SEND_EMAIL: + email.send_project_delete_mail(project, request) + # Actually delete project object + project.delete() + + class ProjectCreateView( LoginRequiredMixin, LoggedInPermissionMixin, @@ -1529,6 +1668,7 @@ class ProjectUpdateView( ProjectModifyPermissionMixin, ProjectContextMixin, ProjectModifyFormMixin, + ProjectDeleteAccessMixin, CurrentUserFormMixin, InvalidFormMixin, UpdateView, @@ -1542,6 +1682,13 @@ class ProjectUpdateView( slug_field = 'sodar_uuid' allow_remote_edit = True + def get_context_data(self, *args, **kwargs): + context = super().get_context_data(*args, **kwargs) + access, msg = self.check_delete_permission(self.get_object()) + context['project_delete_access'] = access + context['project_delete_msg'] = msg or '' + return context + class ProjectArchiveView( LoginRequiredMixin, @@ -1674,100 +1821,12 @@ def post(self, request, **kwargs): return redirect(redirect_url) -class ProjectDeleteMixin(ProjectModifyPluginViewMixin): - """Mixin for Project deletion in UI and API views""" - - @classmethod - def _create_timeline_event(cls, project, request): - """ - Create timeline summary event for project deletion. Created as a - classified site-wide event only viewable by superusers. - - :param project: Project object - :param request: HttpRequest object - """ - timeline = get_backend_api('timeline_backend') - if not timeline: - return - local_users = { - a.user.username: a.role.name - for a in project.local_roles.order_by( - 'role__rank', 'user__username' - ) - } - parent = str(project.parent.sodar_uuid) if project.parent else None - extra_data = { - 'title': project.title, - 'type': project.type, - 'parent': parent, - 'description': project.description, - 'readme': project.readme.raw, - 'public_guest_access': project.public_guest_access, - 'archive': project.archive, - 'full_title': project.full_title, - 'sodar_uuid': str(project.sodar_uuid), - 'local_roles': local_users, - } - timeline.add_event( - project=None, # No project as it has been deleted - app_name=APP_NAME, - user=request.user, - event_name='project_delete', - description=f'delete {project.type.lower()} ' - f'{project.get_log_title()}', - extra_data=extra_data, - classified=True, - status_type=timeline.TL_STATUS_OK, - ) - - @classmethod - def get_redirect_url(cls, project): - if project.parent: - return reverse( - 'projectroles:detail', - kwargs={'project': project.parent.sodar_uuid}, - ) - else: - return reverse('home') - - def handle_delete(self, project, request): - """ - Handle project deletion. Deletes the object, creates a summary timeline - event and sends out alerts and emails to project members. - - :param project: Project object of project to be deleted - :param request: HttpRequest object - """ - app_alerts = get_backend_api('appalerts_backend') - # Create timeline event - self._create_timeline_event(project, request) - # Create app alerts - if app_alerts: - users = [ - a.user for a in project.get_roles() if a.user != request.user - ] - app_alerts.add_alerts( - app_name=APP_NAME, - alert_name='project_delete', - users=users, - message=PROJECT_DELETE_MSG.format( - project_type=get_display_name(project.type, title=True), - project_title=project.title, - user_name=request.user.username, - ), - ) - # Send email - if SEND_EMAIL: - email.send_project_delete_mail(project, request) - # Actually delete project object - project.delete() - - class ProjectDeleteView( LoginRequiredMixin, LoggedInPermissionMixin, ProjectContextMixin, ProjectDeleteMixin, + ProjectDeleteAccessMixin, DeleteView, ): """Project deletion view""" @@ -1777,50 +1836,15 @@ class ProjectDeleteView( slug_field = 'sodar_uuid' slug_url_kwarg = 'project' - def get_context_data(self, *args, **kwargs): - context = super().get_context_data(*args, **kwargs) - return context - def dispatch(self, request, *args, **kwargs): if not request.user.is_authenticated: return self.handle_no_permission() - # Prevent access in certain conditions, even for superusers project = self.get_object() - p_type = get_display_name(project.type) - # Disallow for categories with children - if ( - project.type == PROJECT_TYPE_CATEGORY - and project.get_children().count() > 0 - ): - messages.error( - self.request, - PROJECT_DELETE_CAT_ERR_MSG.format(project_type=p_type), - ) + # Prevent access in certain conditions, even for superusers + access, msg = self.check_delete_permission(project) + if not access: + messages.error(self.request, msg) return redirect('home') - # Disallow for remote projects which haven't been revoked - if project.is_remote(): - rp = RemoteProject.objects.filter( - project_uuid=project.sodar_uuid, - site__mode=SITE_MODE_SOURCE, - ).first() - if rp.level != REMOTE_LEVEL_REVOKED: - messages.error( - self.request, - PROJECT_DELETE_TARGET_ERR_MSG.format(project_type=p_type), - ) - return redirect('home') - # Disallow for source projects with non-revoked remote projects - # NOTE: Categories can be deleted - elif project.type == PROJECT_TYPE_PROJECT: - rps = RemoteProject.objects.filter( - project_uuid=project.sodar_uuid, site__mode=SITE_MODE_TARGET - ).exclude(level__in=[REMOTE_LEVEL_NONE, REMOTE_LEVEL_REVOKED]) - if rps.count() > 0: - messages.error( - self.request, - PROJECT_DELETE_SOURCE_ERR_MSG.format(project_type=p_type), - ) - return redirect('home') return super().dispatch(request, *args, **kwargs) def post(self, *args, **kwargs):