From f43c9235346a456a22a1e1931b851c1a0470ea43 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Thu, 28 Mar 2024 09:27:08 +0100 Subject: [PATCH] fix: Consistent use of action buttons (#392) * fix #384: Unlock button in toolbar points onto DRAFT version * fix: consistent logic for action buttons (availability & enabled/disabled) * fix linting * Remove debug statement * Add missing unlock condition --------- Co-authored-by: Jacob Rief --- djangocms_versioning/admin.py | 31 +++++++++++------------------- djangocms_versioning/conditions.py | 7 ++++++- djangocms_versioning/models.py | 3 +++ tests/test_admin.py | 2 +- tests/test_locking.py | 11 +++++++---- 5 files changed, 28 insertions(+), 26 deletions(-) diff --git a/djangocms_versioning/admin.py b/djangocms_versioning/admin.py index de47bc1a..00898e44 100644 --- a/djangocms_versioning/admin.py +++ b/djangocms_versioning/admin.py @@ -685,13 +685,13 @@ def _get_archive_link(self, obj, request, disabled=False): icon="archive", title=_("Archive"), name="archive", - disabled=not obj.can_be_archived(), + disabled=not obj.check_archive.as_bool(request.user), ) def _get_publish_link(self, obj, request): """Helper function to get the html link to the publish action """ - if not obj.check_publish.as_bool(request.user): + if not obj.can_be_published(): # Don't display the link if it can't be published return "" publish_url = reverse( @@ -704,14 +704,14 @@ def _get_publish_link(self, obj, request): title=_("Publish"), name="publish", action="post", - disabled=not obj.can_be_published(), + disabled=not obj.check_publish.as_bool(request.user), keepsideframe=False, ) def _get_unpublish_link(self, obj, request, disabled=False): """Helper function to get the html link to the unpublish action """ - if not obj.check_unpublish.as_bool(request.user): + if not obj.can_be_unpublished(): # Don't display the link if it can't be unpublished return "" unpublish_url = reverse( @@ -723,15 +723,12 @@ def _get_unpublish_link(self, obj, request, disabled=False): icon="unpublish", title=_("Unpublish"), name="unpublish", - disabled=not obj.can_be_unpublished(), + disabled=not obj.check_unpublish.as_bool(request.user), ) def _get_edit_link(self, obj, request, disabled=False): """Helper function to get the html link to the edit action """ - if not obj.check_edit_redirect.as_bool(request.user): - return "" - # Only show if no draft exists if obj.state == PUBLISHED: pks_for_grouper = obj.versionable.for_content_grouping_values( @@ -761,14 +758,14 @@ def _get_edit_link(self, obj, request, disabled=False): title=_("Edit") if icon == "pencil" else _("New Draft"), name="edit", action="post", - disabled=disabled, + disabled=not obj.check_edit_redirect.as_bool(request.user) or disabled, keepsideframe=keepsideframe, ) def _get_revert_link(self, obj, request, disabled=False): """Helper function to get the html link to the revert action """ - if not obj.check_revert.as_bool(request.user): + if obj.state in (PUBLISHED, DRAFT): # Don't display the link if it's a draft or published return "" @@ -781,13 +778,13 @@ def _get_revert_link(self, obj, request, disabled=False): icon="undo", title=_("Revert"), name="revert", - disabled=disabled, + disabled=not obj.check_revert.as_bool(request.user) or disabled, ) def _get_discard_link(self, obj, request, disabled=False): """Helper function to get the html link to the discard action """ - if not obj.check_discard.as_bool(request.user): + if obj.state != DRAFT: # Don't display the link if it's not a draft return "" @@ -800,7 +797,7 @@ def _get_discard_link(self, obj, request, disabled=False): icon="bin", title=_("Discard"), name="discard", - disabled=disabled, + disabled=not obj.check_discard.as_bool(request.user) or disabled, ) def _get_unlock_link(self, obj, request): @@ -811,12 +808,6 @@ def _get_unlock_link(self, obj, request): if not conf.LOCK_VERSIONS or obj.state != DRAFT or not version_is_locked(obj): return "" - disabled = True - # Check whether the lock can be removed - # Check that the user has unlock permission - if request.user.has_perm("djangocms_versioning.delete_versionlock"): - disabled = False - unlock_url = reverse(f"admin:{obj._meta.app_label}_{self.model._meta.model_name}_unlock", args=(obj.pk,)) return self.admin_action_button( unlock_url, @@ -824,7 +815,7 @@ def _get_unlock_link(self, obj, request): title=_("Unlock"), name="unlock", action="post", - disabled=disabled, + disabled=not obj.check_unlock.as_bool(request.user), ) def get_actions_list(self): diff --git a/djangocms_versioning/conditions.py b/djangocms_versioning/conditions.py index fe9c9012..fd76a007 100644 --- a/djangocms_versioning/conditions.py +++ b/djangocms_versioning/conditions.py @@ -77,6 +77,12 @@ def inner(version, user): raise ConditionFailed(message) return inner +def user_can_unlock(message: str) -> callable: + def inner(version, user): + if not user.has_perm("djangocms_versioning.delete_versionlock"): + raise ConditionFailed(message) + return inner + def user_can_publish(message: str) -> callable: def inner(version, user): if not version.has_publish_permission(user): @@ -89,4 +95,3 @@ def inner(version, user): if not version.has_change_permission(user): raise ConditionFailed(message) return inner - diff --git a/djangocms_versioning/models.py b/djangocms_versioning/models.py index f6347008..0f4a3956 100644 --- a/djangocms_versioning/models.py +++ b/djangocms_versioning/models.py @@ -18,6 +18,7 @@ is_not_locked, user_can_change, user_can_publish, + user_can_unlock, ) from .conf import ALLOW_DELETING_VERSIONS, LOCK_VERSIONS from .operations import send_post_version_operation, send_pre_version_operation @@ -492,6 +493,7 @@ def _has_permission(self, perm: str, user) -> bool: [ in_state([constants.DRAFT], not_draft_error), draft_is_not_locked(lock_draft_error_message), + user_can_unlock(permission_error_message), ] ) check_revert = Conditions( @@ -529,6 +531,7 @@ def _has_permission(self, perm: str, user) -> bool: [ in_state([constants.DRAFT, constants.PUBLISHED], not_draft_error), draft_is_locked(_("Draft version is not locked")) + ] ) diff --git a/tests/test_admin.py b/tests/test_admin.py index 7751b329..cb59fc33 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -729,7 +729,7 @@ class StateActionsTestCase(CMSTestCase): def test_archive_in_state_actions_for_draft_version(self): version = factories.PollVersionFactory(state=constants.DRAFT) request = RequestFactory().get("/admin/polls/pollcontent/") - request.user = factories.UserFactory() + request.user = self.get_superuser() # Get the version model proxy from the main admin site # Trying to test this on the plain Version model throws exceptions version_model_proxy = [ diff --git a/tests/test_locking.py b/tests/test_locking.py index 08860c00..bbc72d94 100644 --- a/tests/test_locking.py +++ b/tests/test_locking.py @@ -270,11 +270,13 @@ def test_unlock_link_not_present_for_user_with_no_unlock_privileges(self): locked_by=self.user_author) changelist_url = version_list_url(poll_version.content) unlock_url = self.get_admin_url(self.versionable.version_model_proxy, "unlock", poll_version.pk) - + exprected_disabled_button = ( + f'' + ) with self.login_user_context(self.user_has_no_unlock_perms): response = self.client.post(changelist_url) - - self.assertNotContains(response, unlock_url) + self.assertInHTML(exprected_disabled_button, response.content.decode("utf-8")) def test_unlock_link_present_for_user_with_privileges(self): poll_version = factories.PollVersionFactory( @@ -392,11 +394,12 @@ def test_edit_action_link_disabled_state(self): author_request.user = self.user_author otheruser_request = RequestFactory() otheruser_request.user = self.superuser + expected_disabled_state = "inactive" actual_disabled_state = self.version_admin._get_edit_link(version, otheruser_request) self.assertFalse(version.check_edit_redirect.as_bool(self.superuser)) - self.assertEqual("", actual_disabled_state) + self.assertIn(expected_disabled_state, actual_disabled_state) @override_settings(DJANGOCMS_VERSIONING_LOCK_VERSIONS=True)