[refactor] Refactor after review comments

Added following
- Added comments

Changed following
- Displays confirm action button only when there are actionable objectsg
- Bulk actions are displayed in same order as page listing buttons

Fixed following
- Fix tests due to change in html of confirmation pages

wagtail/admin/localization.py

@@ -108,6 +108,7 @@ def get_js_translation_strings():
         'WEEKDAYS': [str(WEEKDAYS[d % 7]) for d in range(-1, 6)],
         'WEEKDAYS_SHORT': [str(WEEKDAYS_ABBR[d % 7]) for d in range(-1, 6)],
 
+        # used by bulk actions
         'NUM_PAGES_SELECTED_SINGULAR': _('1 page selected'),
         'NUM_PAGES_SELECTED_PLURAL': _("{0} pages selected"),
         'NUM_PAGES_SELECTED_ALL': _("All {0} pages on this screen selected"),

wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_delete.html

@@ -7,25 +7,27 @@
     {% include "wagtailadmin/shared/header.html" with title=del_str icon="doc-empty-inverse" %}
 
     <div class="nice-padding">
+        {% if pages %}
         <p>{% trans "Are you sure you want to delete these pages?" %}</p>
         <ul>
             {% for page in pages %}
             <li>
                 <a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
-                <ul>
-                    {% if page.descendant_count %}
-                        {% blocktrans count counter=page.descendant_count %}
-                            <li>This will also delete one more subpage.</li>
-                        {% plural %}
-                            <li>This will also delete {{ counter }} more subpages.</li>
-                        {% endblocktrans %}
-                    {% endif %}
-                </ul>
+                {% if page.descendant_count %}
+                    <p>
+                    {% blocktrans count counter=page.descendant_count %}
+                    This will also delete one more subpage.
+                    {% plural %}
+                    This will also delete {{ counter }} more subpages.
+                    {% endblocktrans %}
+                    </p>
+                {% endif %}
             </li>
             {% endfor %}
         </ul>
+        {% endif %}
         {% if pages_with_no_access %}
-        <span>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be deleted{% plural %}The following pages cannot be deleted{% endblocktrans %}</span>
+        <p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be deleted{% plural %}The following pages cannot be deleted{% endblocktrans %}</p>
         <ul>
             {% for page in pages_with_no_access %}
             <li>
@@ -34,10 +36,14 @@
             {% endfor %}
         </ul>
         {% endif %}
+        {% if pages %}
         <form action="{{ submit_url }}" method="POST">
             {% csrf_token %}
             <input type="submit" value="{% trans 'Yes, delete' %}" class="button serious" />
             <a href="{{ next }}" class="button button-secondary">{% trans "No, don't delete" %}</a>
         </form>
+        {% else %}
+        <a href="{{ next }}" class="button button-secondary">{% trans "Go back" %}</a>
+        {% endif %}
     </div>
 {% endblock %}

wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_move.html

@@ -7,21 +7,27 @@
     {% trans "Move" as move_str %}
     {% include "wagtailadmin/shared/header.html" with title=move_str icon="doc-empty-inverse" %}
     <div class="nice-padding">
-        <span>{% trans "Are you sure you want to move these pages?" %}</span>
+        {% if pages %}
+        <p>{% trans "Are you sure you want to move these pages?" %}</p>
         <ul>
             {% for page in pages %}
             <li>
                 <a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
                 {% if not page.page.is_leaf %}
-                <ul>
-                    <li>{% trans "This page has child pages" %}</li>
-                </ul>
+                <p>
+                    {% blocktrans count counter=page.child_pages %}
+                    This page has one child page
+                    {% plural %}
+                    This page has {{ counter }} child pages
+                    {% endblocktrans %}
+                </p>
                 {% endif %}
             </li>
             {% endfor %}
         </ul>
+        {% endif %}
         {% if pages_with_no_access %}
-        <span>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be moved{% plural %}The following pages cannot be moved{% endblocktrans %}</span>
+        <p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be moved{% plural %}The following pages cannot be moved{% endblocktrans %}</p>
         <ul>
             {% for page in pages_with_no_access %}
             <li>
@@ -32,7 +38,7 @@
         {% endif %}
 
         {% if pages_without_destination_access %}
-        <span>{% blocktrans with dest_title=destination.title count counter=pages_with_no_access|length %}The following page cannot be moved to {{dest_title}} {% plural %}The following pages cannot be moved to {{dest_title}}{% endblocktrans %}</span>
+        <p>{% blocktrans with dest_title=destination.title count counter=pages_with_no_access|length %}The following page cannot be moved to {{dest_title}} {% plural %}The following pages cannot be moved to {{dest_title}}{% endblocktrans %}</p>
         <ul>
             {% for page in pages_without_destination_access %}
             <li>
@@ -43,7 +49,7 @@
         {% endif %}
 
         {% if pages_with_duplicate_slugs %}
-        <span>{% blocktrans count counter=pages_with_duplicate_slugs|length %}The following page cannot be moved due to duplicate slug{% plural %}The following pages cannot be moved due to duplicate slugs{% endblocktrans %}</span>
+        <p>{% blocktrans count counter=pages_with_duplicate_slugs|length %}The following page cannot be moved due to duplicate slug{% plural %}The following pages cannot be moved due to duplicate slugs{% endblocktrans %}</p>
         <ul>
             {% for page in pages_with_duplicate_slugs %}
             <li>
@@ -53,6 +59,7 @@
         </ul>
         {% endif %}
 
+        {% if pages %}
         <form action="{{ submit_url }}" method="POST">
             {% csrf_token %}
             <ul class="fields">
@@ -71,7 +78,11 @@
             {% endif %}
             </ul>
             <input type="submit" value="{% trans 'Yes, move these pages' %}" class="button">
+            <a href="{{ next }}" class="button button-secondary">{% trans "No, don't move" %}</a>
         </form>
+        {% else %}
+        <a href="{{ next }}" class="button button-secondary">{% trans "Go back" %}</a>
+        {% endif %}
     </div>
 {% endblock %}
 
@@ -79,4 +90,4 @@
     {{ block.super }}
     {% include "wagtailadmin/pages/_editor_js.html" %}
     {{ form.media.js }}
-{% endblock %}
+{% endblock %}

wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_publish.html

@@ -6,25 +6,27 @@
     {% include "wagtailadmin/shared/header.html" with title=publish_str icon="doc-empty-inverse" %}
 
     <div class="nice-padding">
-        <p>{% trans "Are you sure you want to publish these pages?" %}</p>
-        <ul>
-            {% for page in pages %}
-            <li>
-                <a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
-                <ul>
+        {% if pages %}
+            <p>{% trans "Are you sure you want to publish these pages?" %}</p>
+            <ul>
+                {% for page in pages %}
+                <li>
+                    <a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
                     {% if page.draft_descendant_count %}
+                    <p>
                         {% blocktrans count counter=page.draft_descendant_count %}
-                            <li>This page has one unpublished subpage</li>
+                        This page has one unpublished subpage
                         {% plural %}
-                            <li>This page has {{ counter }} unpublished subpages</li>
+                        This page has {{ counter }} unpublished subpages
                         {% endblocktrans %}
+                    </p>
                     {% endif %}
-                </ul>
-            </li>
-            {% endfor %}
-        </ul>
+                </li>
+                {% endfor %}
+            </ul>
+        {% endif %}
         {% if pages_with_no_access %}
-        <span>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be published{% plural %}The following pages cannot be published{% endblocktrans %}</span>
+        <p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be published{% plural %}The following pages cannot be published{% endblocktrans %}</p>
         <ul>
             {% for page in pages_with_no_access %}
             <li>
@@ -33,6 +35,7 @@
             {% endfor %}
         </ul>
         {% endif %}
+        {% if pages %}
         <form action="{{ submit_url }}" method="POST">
             {% csrf_token %}
             {% if has_draft_descendants %}
@@ -52,5 +55,8 @@
             <input type="submit" value="{% trans 'Yes, publish' %}" class="button" />
             <a href="{{ next }}" class="button button-secondary">{% trans "No, don't publish" %}</a>
         </form>
+        {% else %}
+        <a href="{{ next }}" class="button button-secondary">{% trans "Go back" %}</a>
+        {% endif %}
     </div>
 {% endblock %}

wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_unpublish.html

@@ -6,25 +6,27 @@
     {% include "wagtailadmin/shared/header.html" with title=unpublish_str icon="doc-empty-inverse" %}
 
     <div class="nice-padding">
+        {% if pages %}
         <p>{% trans "Are you sure you want to unpublish these pages?" %}</p>
         <ul>
             {% for page in pages %}
             <li>
                 <a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
-                <ul>
+                <p>
                     {% if page.live_descendant_count %}
                         {% blocktrans count counter=page.live_descendant_count %}
-                            <li>This page has one subpage</li>
+                        This page has one subpage
                         {% plural %}
-                            <li>This page has {{ counter }} subpages</li>
+                        This page has {{ counter }} subpages
                         {% endblocktrans %}
                     {% endif %}
-                </ul>
+                </p>
             </li>
             {% endfor %}
         </ul>
+        {% endif %}
         {% if pages_with_no_access %}
-        <span>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be unpublished{% plural %}The following pages cannot be unpublished{% endblocktrans %}</span>
+        <p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be unpublished{% plural %}The following pages cannot be unpublished{% endblocktrans %}</p>
         <ul>
             {% for page in pages_with_no_access %}
             <li>
@@ -33,6 +35,7 @@
             {% endfor %}
         </ul>
         {% endif %}
+        {% if pages %}
         <form action="{{ submit_url }}" method="POST">
             {% csrf_token %}
             {% if has_live_descendants %}
@@ -52,5 +55,8 @@
             <input type="submit" value="{% trans 'Yes, unpublish' %}" class="button" />
             <a href="{{ next }}" class="button button-secondary">{% trans "No, don't unpublish" %}</a>
         </form>
+        {% else %}
+        <a href="{{ next }}" class="button button-secondary">{% trans "Go back" %}</a>
+        {% endif %}
     </div>
 {% endblock %}

wagtail/admin/templates/wagtailadmin/pages/index.html

@@ -29,6 +29,11 @@
     {% comment %} modal-workflow is required by the view restrictions interface {% endcomment %}
     <script src="{% versioned_static 'wagtailadmin/js/modal-workflow.js' %}"></script>
     <script src="{% versioned_static 'wagtailadmin/js/privacy-switch.js' %}"></script>
+    
+    {% comment %}
+    The first column will display checkboxes only if ordering is not being carried out, in which case
+    that column will have the drag and drop buttons to enable ordering
+    {% endcomment %}
     {% if not show_ordering_column %}
     <script defer src="{% versioned_static 'wagtailadmin/js/bulk-actions.js' %}"></script>
     {% endif %}

wagtail/admin/templates/wagtailadmin/pages/listing/_list.html

@@ -18,8 +18,7 @@
         {% if parent_page %}
             {% page_permissions parent_page as parent_page_perms %}
             <tr class="index {% if not parent_page.live %} unpublished{% endif %} {% block parent_page_row_classname %}{% endblock %}">
-                <td></td>
-                <td class="title" {% if show_ordering_column %}colspan="2"{% endif %}>
+                <td class="title" colspan="2">
                     {% block parent_page_title %}
                     {% endblock %}
                 </td>
@@ -81,4 +80,4 @@
             {% block no_results %}{% endblock %}
         {% endif %}
     </tbody>
-</table>
+</table>

wagtail/admin/templates/wagtailadmin/pages/listing/_table_headers_explore.html

@@ -38,7 +38,7 @@ ordering: the current sort parameter
             {% bulk_action_filters %}
             <div class="bulk-actions-choices u-hidden">
                 <span class="num-pages"></span>
-                <a class="num-pages-in-listing u-hidden" href='#'>{{ select_all_page_text }}</a>
+                <a class="num-pages-in-listing u-hidden" href='#'>{% trans "Select all pages in listing" %}</a>
                 <ul>{% page_bulk_action_choices %}</ul>
             </div>
         </div>

wagtail/admin/tests/pages/test_bulk_actions/test_bulk_delete.py

@@ -65,14 +65,15 @@ class TestBulkDelete(TestCase, WagtailTestUtils):
                 edit_page_url=reverse('wagtailadmin_pages:edit', args=[child_page.id]),
                 page_title=child_page.title
             )
-            needle += '<ul>'
             descendants = len(self.grandchildren_pages.get(child_page, []))
             if descendants:
+                needle += '<p>'
                 if descendants == 1:
-                    needle += '<li>This will also delete one more subpage.</li>'
+                    needle += 'This will also delete one more subpage.'
                 else:
-                    needle += f'<li>This will also delete {descendants} more subpages.</li>'
-            needle += '</ul></li>'
+                    needle += f'This will also delete {descendants} more subpages.'
+                needle += '</p>'
+            needle += '</li>'
             self.assertInHTML(needle, html)
 
     def test_page_delete_specific_admin_title(self):
@@ -102,7 +103,7 @@ class TestBulkDelete(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>The following pages cannot be deleted</span>', html)
+        self.assertInHTML('<p>The following pages cannot be deleted</p>', html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_deleted:

wagtail/admin/tests/pages/test_bulk_actions/test_bulk_move.py

@@ -69,7 +69,7 @@ class TestBulkMove(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>Are you sure you want to move these pages?</span>', html)
+        self.assertInHTML('<p>Are you sure you want to move these pages?</p>', html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_moved:
@@ -96,7 +96,7 @@ class TestBulkMove(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>The following pages cannot be moved</span>', html)
+        self.assertInHTML('<p>The following pages cannot be moved</p>', html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_moved:
@@ -133,7 +133,7 @@ class TestBulkMove(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>The following pages cannot be moved to {}</span>'.format(page.title), html)
+        self.assertInHTML('<p>The following pages cannot be moved to {}</p>'.format(page.title), html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_moved:
@@ -156,7 +156,7 @@ class TestBulkMove(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>The following pages cannot be moved due to duplicate slugs</span>', html)
+        self.assertInHTML('<p>The following pages cannot be moved due to duplicate slugs</p>', html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_moved:

wagtail/admin/tests/pages/test_bulk_actions/test_bulk_publish.py

@@ -74,7 +74,7 @@ class TestBulkPublish(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>The following pages cannot be published</span>', html)
+        self.assertInHTML('<p>The following pages cannot be published</p>', html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_published:

wagtail/admin/tests/pages/test_bulk_actions/test_bulk_unpublish.py

@@ -75,7 +75,7 @@ class TestBulkUnpublish(TestCase, WagtailTestUtils):
 
         html = response.content.decode()
 
-        self.assertInHTML('<span>The following pages cannot be unpublished</span>', html)
+        self.assertInHTML('<p>The following pages cannot be unpublished</p>', html)
 
         needle = '<ul>'
         for child_page in self.pages_to_be_unpublished:

wagtail/admin/views/pages/bulk_actions/delete.py

@@ -10,6 +10,7 @@ class DeleteBulkAction(PageBulkAction):
     action_type = "delete"
     aria_label = "Delete pages"
     template_name = "wagtailadmin/pages/bulk_actions/confirm_bulk_delete.html"
+    action_priority = 30
 
     def check_perm(self, page):
         return page.permissions_for_user(self.request.user).can_delete()

wagtail/admin/views/pages/bulk_actions/move.py

@@ -51,6 +51,7 @@ class MoveBulkAction(PageBulkAction):
     action_type = "move"
     aria_label = "Move pages"
     template_name = "wagtailadmin/pages/bulk_actions/confirm_bulk_move.html"
+    action_priority = 10
 
     def check_perm(self, page):
         return page.permissions_for_user(self.request.user).can_move()
@@ -65,6 +66,11 @@ class MoveBulkAction(PageBulkAction):
         }
         return success_message
 
+    def object_context(self, obj):
+        context = super().object_context(obj)
+        context['child_pages'] = context['page'].get_descendants().count()
+        return context
+
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
         destination = kwargs.get('destination', Page.get_first_root_node())

wagtail/admin/views/pages/bulk_actions/publish.py

@@ -10,6 +10,7 @@ class PublishBulkAction(PageBulkAction):
     action_type = "publish"
     aria_label = _("Publish pages")
     template_name = "wagtailadmin/pages/bulk_actions/confirm_bulk_publish.html"
+    action_priority = 40
 
     def check_perm(self, page):
         return page.permissions_for_user(self.request.user).can_publish()

wagtail/admin/views/pages/bulk_actions/unpublish.py

@@ -10,6 +10,7 @@ class UnpublishBulkAction(PageBulkAction):
     action_type = "unpublish"
     aria_label = _("Unpublish pages")
     template_name = "wagtailadmin/pages/bulk_actions/confirm_bulk_unpublish.html"
+    action_priority = 50
 
     def check_perm(self, page):
         return page.permissions_for_user(self.request.user).can_unpublish()

wagtail/admin/views/pages/listing.py

@@ -5,7 +5,6 @@ from django.http.response import JsonResponse
 from django.shortcuts import get_object_or_404, redirect
 from django.template.response import TemplateResponse
 from django.urls import reverse
-from django.utils.translation import gettext_lazy as _
 
 from wagtail.admin.auth import user_has_any_page_permission, user_passes_test
 from wagtail.admin.navigation import get_explorable_root_page
@@ -111,8 +110,7 @@ def index(request, parent_page_id=None):
         'do_paginate': do_paginate,
         'locale': None,
         'translations': [],
-        'show_ordering_column': 'ordering' in request.GET.dict(),
-        'select_all_page_text': _("Select all pages in listing")
+        'show_ordering_column': 'ordering' in request.GET.dict()
     }
 
     if getattr(settings, 'WAGTAIL_I18N_ENABLED', False) and not parent_page.is_root():