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 <[email protected]>

Author: fsbraun

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,20 +808,14 @@ 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,
             icon="unlock",
             title=_("Unlock"),
             name="unlock",
             action="post",
-            disabled=disabled,
+            disabled=not obj.check_unlock.as_bool(request.user),
         )
 
     def get_actions_list(self):

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
-

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"))
+
         ]
     )
 

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 = [

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'<a class="btn cms-form-post-method cms-action-btn cms-action-unlock js-action js-keep-sideframe" '
+            f'href="{unlock_url}" title="Unlock"><span class="cms-icon cms-icon-unlock"></span></a>'
+        )
         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)