From d53e3414f513e8e48c2b452b584221b2e98b5093 Mon Sep 17 00:00:00 2001 From: Jacob Rief Date: Wed, 21 Feb 2024 15:59:47 +0100 Subject: [PATCH 1/5] fix #384: Unlock button in toolbar points onto DRAFT version --- djangocms_versioning/cms_toolbars.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/djangocms_versioning/cms_toolbars.py b/djangocms_versioning/cms_toolbars.py index c9d3838a..153f43ba 100644 --- a/djangocms_versioning/cms_toolbars.py +++ b/djangocms_versioning/cms_toolbars.py @@ -125,8 +125,8 @@ def _add_unlock_button(self): if LOCK_VERSIONS and self._is_versioned(): item = ButtonList(side=self.toolbar.RIGHT) proxy_model = self._get_proxy_model() - version = Version.objects.get_for_content(self.toolbar.obj) - if version.check_unlock.as_bool(self.request.user): + version = Version.objects.filter_by_content_grouping_values(self.toolbar.obj).filter(state=DRAFT).first() + if version and version.check_unlock.as_bool(self.request.user): unlock_url = reverse( f"admin:{proxy_model._meta.app_label}_{proxy_model.__name__.lower()}_unlock", args=(version.pk,), From 73d101d3acd4d8c8e5cd75c0c2295caec62b058f Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 12 Mar 2024 17:45:40 +0100 Subject: [PATCH 2/5] fix: consistent logic for action buttons (availability & enabled/disabled) --- djangocms_versioning/admin.py | 31 +++++++++++------------------- djangocms_versioning/conditions.py | 7 ++++++- tests/test_admin.py | 2 +- tests/test_locking.py | 13 +++++++++---- 4 files changed, 27 insertions(+), 26 deletions(-) diff --git a/djangocms_versioning/admin.py b/djangocms_versioning/admin.py index de47bc1a..1ae23915 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 == PUBLISHED or obj.state == 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/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..afa43c96 100644 --- a/tests/test_locking.py +++ b/tests/test_locking.py @@ -270,11 +270,15 @@ 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) + print() + print(response.content.decode("utf-8")) + 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 +396,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) From 2c8e7820821f1183ae3f93d08b1e6f7addce715f Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 12 Mar 2024 17:48:02 +0100 Subject: [PATCH 3/5] fix linting --- djangocms_versioning/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/djangocms_versioning/admin.py b/djangocms_versioning/admin.py index 1ae23915..00898e44 100644 --- a/djangocms_versioning/admin.py +++ b/djangocms_versioning/admin.py @@ -765,7 +765,7 @@ def _get_edit_link(self, obj, request, disabled=False): def _get_revert_link(self, obj, request, disabled=False): """Helper function to get the html link to the revert action """ - if obj.state == PUBLISHED or obj.state == DRAFT: + if obj.state in (PUBLISHED, DRAFT): # Don't display the link if it's a draft or published return "" From 9ed74de8341ee05487ce8a1868dca200d00b7b06 Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 12 Mar 2024 20:43:29 +0100 Subject: [PATCH 4/5] Remove debug statement --- tests/test_locking.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_locking.py b/tests/test_locking.py index afa43c96..bbc72d94 100644 --- a/tests/test_locking.py +++ b/tests/test_locking.py @@ -276,8 +276,6 @@ def test_unlock_link_not_present_for_user_with_no_unlock_privileges(self): ) with self.login_user_context(self.user_has_no_unlock_perms): response = self.client.post(changelist_url) - print() - print(response.content.decode("utf-8")) self.assertInHTML(exprected_disabled_button, response.content.decode("utf-8")) def test_unlock_link_present_for_user_with_privileges(self): From 09e633e1099a05b6b629b1083f47f9f6f129df8d Mon Sep 17 00:00:00 2001 From: Fabian Braun Date: Tue, 12 Mar 2024 20:47:04 +0100 Subject: [PATCH 5/5] Add missing unlock condition --- djangocms_versioning/models.py | 3 +++ 1 file changed, 3 insertions(+) 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")) + ] )