mirror
/
wagtail
kopia lustrzana https://github.com/wagtail/wagtail
1
0
Forkuj 0
Kod Zgłoszenia Packages Projekty Wydania Wiki Aktywność

[refactor] Refactor after review commits 

Added following
- Add prepare action method that runs before transaction starts
- Add a default form with a default field

Changed following
- Move permissions check in bulk move from before hook to a method that runs before the transaction starts
- Select all pages now doesn't fetch all ids, but passes id as all and the parent page id
- Change no access permissions

Fixed following
- Fix tests for changes in html
pull/7618/head
Shohan 2021-07-11 15:56:09 +05:30 zatwierdzone przez Matt Westcott
rodzic e95135d5cf
commit a40d4de316
15 zmienionych plików z 123 dodań i 124 usunięć
Pokaż statystyki Ściągnij plik aktualizacji Ściągnij plik porównania Expand all files Collapse all files

29
client/src/entrypoints/admin/bulk-actions.js
Wyświetl plik

@ -1,5 +1,4 @@
/* global wagtailConfig */
/* global $ */
const BULK_ACTION_PAGE_CHECKBOX_INPUT = 'bulk-action-checkbox';
const BULK_ACTION_SELECT_ALL_CHECKBOX_TH = 'bulk-actions-filter-checkbox';
@ -12,7 +11,6 @@ const TABLE_HEADERS_TR = 'table-headers';
const checkedState = {
checkedPages: new Set(),
numPages: 0,
listingPageIds: [],
selectAllInListing: false
};
@ -39,7 +37,7 @@ function SelectBulkActionsCheckboxes(e) {
}
if (checkedState.checkedPages.size === 0) {
/* when all checboxes are unchecked */
/* when all checkboxes are unchecked */
document.querySelectorAll(`.${TABLE_HEADERS_TR} > th`).forEach(el => el.classList.remove('u-hidden'));
document.querySelector(`.${BULK_ACTION_CHOICES_DIV}`).classList.add('u-hidden');
document.querySelectorAll(`.${BULK_ACTION_PAGE_CHECKBOX_INPUT}`).forEach(el => el.classList.remove('show'));
@ -47,7 +45,6 @@ function SelectBulkActionsCheckboxes(e) {
} else if (checkedState.checkedPages.size === 1 && prevLength === 0) {
/* when 1 checkbox is checked for the first time */
document.querySelectorAll(`.${BULK_ACTION_PAGE_CHECKBOX_INPUT}`).forEach(el => {
el.classList.remove('show');
el.classList.add('show');
});
document.querySelectorAll(`.${TABLE_HEADERS_TR} > th`).forEach(el => el.classList.add('u-hidden'));
@ -85,16 +82,7 @@ function SelectBulkActionsCheckboxes(e) {
}
}
function fetchAllPageIdsInListing(e) {
if (checkedState.listingPageIds.length === 0) {
e.preventDefault();
$.ajax({
url: '?fetchPageIds=1',
success: (response) => {
checkedState.listingPageIds = response.page_ids;
},
});
}
function selectAllPageIdsInListing() {
checkedState.selectAllInListing = true;
document.querySelector(`.${BULK_ACTION_NUM_PAGES_SPAN}`).
textContent = wagtailConfig.STRINGS.NUM_PAGES_SELECTED_ALL_IN_LISTING;
@ -138,9 +126,14 @@ function BulkActionEventListeners(e) {
e.preventDefault();
const url = e.target.getAttribute('href');
let queryString = '';
(checkedState.selectAllInListing ? checkedState.listingPageIds : checkedState.checkedPages).forEach(pageId => {
queryString += `&id=${pageId}`;
});
if (checkedState.selectAllInListing) {
const parentPageId = document.querySelector(`.${BULK_ACTION_SELECT_ALL_CHECKBOX_TH}`).dataset.parentId;
queryString += `&id=all&childOf=${parentPageId}`;
} else {
checkedState.checkedPages.forEach(pageId => {
queryString += `&id=${pageId}`;
});
}
window.location.href = url + queryString;
}
@ -159,7 +152,7 @@ function addBulkActionListeners() {
document.querySelectorAll(`.${BULK_ACTION_CHOICES_DIV} > ul > li > a`).forEach(
elem => elem.addEventListener('click', BulkActionEventListeners)
);
document.querySelector(`.${BULK_ACTION_NUM_PAGES_IN_LISTING}`).addEventListener('click', fetchAllPageIdsInListing);
document.querySelector(`.${BULK_ACTION_NUM_PAGES_IN_LISTING}`).addEventListener('click', selectAllPageIdsInListing);
}
addBulkActionListeners();

15
wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_delete.html
Wyświetl plik

@ -12,26 +12,31 @@
<ul>
{% for page in pages %}
<li>
<a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
<a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{page.page.title }}</a>
{% if page.descendant_count %}
<p>
<p>
{% blocktrans count counter=page.descendant_count %}
This will also delete one more subpage.
{% plural %}
This will also delete {{ counter }} more subpages.
{% endblocktrans %}
</p>
</p>
{% endif %}
</li>
{% endfor %}
</ul>
{% endif %}
{% if pages_with_no_access %}
<p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be deleted{% plural %}The following pages cannot be deleted{% endblocktrans %}</p>
<p>{% blocktrans count counter=pages_with_no_access|length %}You don't have permission to delete this page{% plural %}
You don't have permission to delete these pages{% endblocktrans %}</p>
<ul>
{% for page in pages_with_no_access %}
<li>
<a href="{% url 'wagtailadmin_pages:edit' page.id %}" target="_blank" rel="noopener noreferrer">{{ page.title }}</a>
{% if page.can_edit %}
<a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
{% else %}
{{ page.page.title }}
{% endif %}
</li>
{% endfor %}
</ul>

19
wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_move.html
Wyświetl plik

@ -27,11 +27,15 @@
</ul>
{% endif %}
{% if pages_with_no_access %}
<p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be moved{% plural %}The following pages cannot be moved{% endblocktrans %}</p>
<p>{% blocktrans count counter=pages_with_no_access|length %}You don't have permission to move this page{% plural %}You don't have permission to move these pages{% endblocktrans %}</p>
<ul>
{% for page in pages_with_no_access %}
<li>
<a href="{% url 'wagtailadmin_pages:edit' page.id %}" target="_blank" rel="noopener noreferrer">{{ page.title }}</a>
{% if page.can_edit %}
<a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
{% else %}
{{ page.page.title }}
{% endif %}
</li>
{% endfor %}
</ul>
@ -65,16 +69,7 @@
<ul class="fields">
{% include "wagtailadmin/shared/field_as_li.html" with field=form.chooser %}
{% if pages_without_destination_access or pages_with_duplicate_slugs %}
<li>
<div class="field boolean_field checkbox_input">
<div class="field-content">
<div class="input">
<input id="id_move_applicable" name="move_applicable" type="checkbox">
<label for="id_move_applicable" class="plain-checkbox-label">{% trans "Move only applicable pages" %}</label>
</div>
</div>
</div>
</li>
{% include "wagtailadmin/shared/field_as_li.html" with field=form.move_applicable %}
{% endif %}
</ul>
<input type="submit" value="{% trans 'Yes, move these pages' %}" class="button">

19
wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_publish.html
Wyświetl plik

@ -26,11 +26,15 @@
</ul>
{% endif %}
{% if pages_with_no_access %}
<p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be published{% plural %}The following pages cannot be published{% endblocktrans %}</p>
<p>{% blocktrans count counter=pages_with_no_access|length %}You don't have permission to publish this page{% plural %}You don't have permission to publish these pages{% endblocktrans %}</p>
<ul>
{% for page in pages_with_no_access %}
<li>
<a href="{% url 'wagtailadmin_pages:edit' page.id %}" target="_blank" rel="noopener noreferrer">{{ page.title }}</a>
{% if page.can_edit %}
<a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
{% else %}
{{ page.page.title }}
{% endif %}
</li>
{% endfor %}
</ul>
@ -40,16 +44,7 @@
{% csrf_token %}
{% if has_draft_descendants %}
<ul class="fields">
<li>
<div class="field boolean_field checkbox_input">
<div class="field-content">
<div class="input">
<input id="id_include_descendants" name="include_descendants" type="checkbox">
<label for="id_include_descendants" class="plain-checkbox-label">{% trans "Publish subpages" %}</label>
</div>
</div>
</div>
</li>
{% include "wagtailadmin/shared/field_as_li.html" with field=form.include_descendants %}
</ul>
{% endif %}
<input type="submit" value="{% trans 'Yes, publish' %}" class="button" />

19
wagtail/admin/templates/wagtailadmin/pages/bulk_actions/confirm_bulk_unpublish.html
Wyświetl plik

@ -26,11 +26,15 @@
</ul>
{% endif %}
{% if pages_with_no_access %}
<p>{% blocktrans count counter=pages_with_no_access|length %}The following page cannot be unpublished{% plural %}The following pages cannot be unpublished{% endblocktrans %}</p>
<p>{% blocktrans count counter=pages_with_no_access|length %}You don't have permission to unpublish this page{% plural %}You don't have permission to unpublish these pages{% endblocktrans %}</p>
<ul>
{% for page in pages_with_no_access %}
<li>
<a href="{% url 'wagtailadmin_pages:edit' page.id %}" target="_blank" rel="noopener noreferrer">{{ page.title }}</a>
{% if page.can_edit %}
<a href="{% url 'wagtailadmin_pages:edit' page.page.id %}" target="_blank" rel="noopener noreferrer">{{ page.page.title }}</a>
{% else %}
{{ page.page.title }}
{% endif %}
</li>
{% endfor %}
</ul>
@ -40,16 +44,7 @@
{% csrf_token %}
{% if has_live_descendants %}
<ul class="fields">
<li>
<div class="field boolean_field checkbox_input">
<div class="field-content">
<div class="input">
<input id="id_include_descendants" name="include_descendants" type="checkbox">
<label for="id_include_descendants" class="plain-checkbox-label">{% trans "Unpublish subpages" %}</label>
</div>
</div>
</div>
</li>
{% include "wagtailadmin/shared/field_as_li.html" with field=form.include_descendants %}
</ul>
{% endif %}
<input type="submit" value="{% trans 'Yes, unpublish' %}" class="button" />

3
wagtail/admin/templates/wagtailadmin/pages/search.html
Wyświetl plik

@ -1,5 +1,5 @@
{% extends "wagtailadmin/base.html" %}
{% load i18n %}
{% load wagtailadmin_tags i18n %}
{% block titletag %}{% trans 'Search' %}{% endblock %}
{% block extra_js %}
{{ block.super }}
@ -11,6 +11,7 @@
targetOutput: "#page-results"
}
</script>
<script defer src="{% versioned_static 'wagtailadmin/js/bulk-actions.js' %}"></script>
{% endblock %}
{% block content %}

2
wagtail/admin/templatetags/wagtailadmin_tags.py
Wyświetl plik

@ -497,7 +497,7 @@ def bulk_action_filters(context):
for hook in button_hooks:
buttons.extend(hook())
buttons.sort()
buttons.sort(key=lambda x: x.priority)
return {'buttons': buttons}

8
wagtail/admin/tests/pages/test_bulk_actions/test_bulk_delete.py
Wyświetl plik

@ -103,17 +103,15 @@ class TestBulkDelete(TestCase, WagtailTestUtils):
html = response.content.decode()
self.assertInHTML('<p>The following pages cannot be deleted</p>', html)
self.assertInHTML("<p>You don't have permission to delete these pages</p>", html)
needle = '<ul>'
for child_page in self.pages_to_be_deleted:
needle += '<li><a href="{edit_page_url}" target="_blank" rel="noopener noreferrer">{page_title}</a></li>'.format(
edit_page_url=reverse('wagtailadmin_pages:edit', args=[child_page.id]),
page_title=child_page.title
)
needle += '<li>{page_title}</li>'.format(page_title=child_page.title)
needle += '</ul>'
self.assertInHTML(needle, html)
self.assertTagInHTML('''<form action="{}" method="POST"></form>'''.format(self.url), html, count=0)
def test_bulk_delete_post(self):
# Connect a mock signal handler to page_unpublished signal

13
wagtail/admin/tests/pages/test_bulk_actions/test_bulk_move.py
Wyświetl plik

@ -96,18 +96,17 @@ class TestBulkMove(TestCase, WagtailTestUtils):
html = response.content.decode()
self.assertInHTML('<p>The following pages cannot be moved</p>', html)
self.assertInHTML("<p>You don't have permission to move these pages</p>", html)
needle = '<ul>'
for child_page in self.pages_to_be_moved:
needle += '<li><a href="{edit_page_url}" target="_blank" rel="noopener noreferrer">{page_title}</a></li>'.format(
edit_page_url=reverse('wagtailadmin_pages:edit', args=[child_page.id]),
page_title=child_page.title
)
needle += '<li>{page_title}</li>'.format(page_title=child_page.title)
needle += '</ul>'
self.assertInHTML(needle, html)
self.assertTagInHTML('''<form action="{}" method="POST"></form>'''.format(self.url), html, count=0)
def test_user_without_bulk_delete_permission_can_move(self):
# to verify that a user without bulk delete permission is able to move a page with a child page
@ -144,7 +143,7 @@ class TestBulkMove(TestCase, WagtailTestUtils):
needle += '</ul>'
self.assertInHTML(needle, html)
self.assertContains(response, '<input id="id_move_applicable" name="move_applicable" type="checkbox">')
self.assertContains(response, '<input type="checkbox" name="move_applicable" id="id_move_applicable">')
def test_bulk_move_slug_already_taken(self):
temp_page_1 = SimplePage(title="Hello world!", slug="hello-world-b", content="hello")
@ -167,7 +166,7 @@ class TestBulkMove(TestCase, WagtailTestUtils):
needle += '</ul>'
self.assertInHTML(needle, html)
self.assertContains(response, '<input id="id_move_applicable" name="move_applicable" type="checkbox">')
self.assertContains(response, '<input type="checkbox" name="move_applicable" id="id_move_applicable">')
def test_bulk_move_ignore_permission_errors(self):
temp_page_1 = SimplePage(title="Hello world!", slug="hello-world-b", content="hello")

9
wagtail/admin/tests/pages/test_bulk_actions/test_bulk_publish.py
Wyświetl plik

@ -74,14 +74,11 @@ class TestBulkPublish(TestCase, WagtailTestUtils):
html = response.content.decode()
self.assertInHTML('<p>The following pages cannot be published</p>', html)
self.assertInHTML("<p>You don't have permission to publish these pages</p>", html)
needle = '<ul>'
for child_page in self.pages_to_be_published:
needle += '<li><a href="{edit_page_url}" target="_blank" rel="noopener noreferrer">{page_title}</a></li>'.format(
edit_page_url=reverse('wagtailadmin_pages:edit', args=[child_page.id]),
page_title=child_page.title
)
needle += '<li>{page_title}</li>'.format(page_title=child_page.title)
needle += '</ul>'
self.assertInHTML(needle, html)
@ -213,7 +210,7 @@ class TestBulkPublishIncludingDescendants(TestCase, WagtailTestUtils):
self.assertEqual(response.status_code, 200)
self.assertTemplateUsed(response, 'wagtailadmin/pages/bulk_actions/confirm_bulk_publish.html')
# Check the form contains the checkbox field include_descendants
self.assertContains(response, '<input id="id_include_descendants" name="include_descendants" type="checkbox">')
self.assertContains(response, '<input type="checkbox" name="include_descendants" id="id_include_descendants">')
def test_publish_include_children_view_post(self):
"""

11
wagtail/admin/tests/pages/test_bulk_actions/test_bulk_unpublish.py
Wyświetl plik

@ -75,14 +75,11 @@ class TestBulkUnpublish(TestCase, WagtailTestUtils):
html = response.content.decode()
self.assertInHTML('<p>The following pages cannot be unpublished</p>', html)
self.assertInHTML("<p>You don't have permission to unpublish these pages</p>", html)
needle = '<ul>'
for child_page in self.pages_to_be_unpublished:
needle += '<li><a href="{edit_page_url}" target="_blank" rel="noopener noreferrer">{page_title}</a></li>'.format(
edit_page_url=reverse('wagtailadmin_pages:edit', args=[child_page.id]),
page_title=child_page.title
)
needle += '<li>{page_title}</li>'.format(page_title=child_page.title)
needle += '</ul>'
self.assertInHTML(needle, html)
@ -167,7 +164,7 @@ class TestBulkUnpublish(TestCase, WagtailTestUtils):
self.assertEqual(response.status_code, 200)
self.assertTemplateUsed(response, 'wagtailadmin/pages/bulk_actions/confirm_bulk_unpublish.html')
# Check the form does not contain the checkbox field include_descendants
self.assertNotContains(response, '<input id="id_include_descendants" name="include_descendants" type="checkbox">')
self.assertContains(response, '<input type="checkbox" name="include_descendants" id="id_include_descendants">', count=0)
class TestBulkUnpublishIncludingDescendants(TestCase, WagtailTestUtils):
@ -216,7 +213,7 @@ class TestBulkUnpublishIncludingDescendants(TestCase, WagtailTestUtils):
self.assertEqual(response.status_code, 200)
self.assertTemplateUsed(response, 'wagtailadmin/pages/bulk_actions/confirm_bulk_unpublish.html')
# Check the form contains the checkbox field include_descendants
self.assertContains(response, '<input id="id_include_descendants" name="include_descendants" type="checkbox">')
self.assertContains(response, '<input type="checkbox" name="include_descendants" id="id_include_descendants">')
def test_unpublish_include_children_view_post(self):
"""

23
wagtail/admin/views/bulk_action.py
Wyświetl plik

@ -1,4 +1,4 @@
from abc import ABC, abstractproperty
from abc import ABC, abstractmethod
from django.db import transaction
from django.shortcuts import get_list_or_404, redirect
@ -10,15 +10,18 @@ from wagtail.core import hooks
class BulkAction(ABC, TemplateView):
@abstractproperty
@property
@abstractmethod
def display_name(self):
pass
@abstractproperty
@property
@abstractmethod
def action_type(self):
pass
@abstractproperty
@property
@abstractmethod
def aria_label(self):
pass
@ -78,7 +81,11 @@ class BulkAction(ABC, TemplateView):
def get_actionable_objects(self):
objects = []
objects_with_no_access = []
object_ids = list(map(int, self.request.GET.getlist('id')))
object_ids = self.request.GET.getlist('id')
if 'all' in object_ids:
parent_page_id = int(self.request.GET.get('childOf'))
object_ids = self.model.objects.get(id=parent_page_id).get_children().values_list('id', flat=True)
object_ids = list(map(int, object_ids))
for obj in self.get_queryset(object_ids):
if not self.check_perm(obj):
@ -99,8 +106,14 @@ class BulkAction(ABC, TemplateView):
'submit_url': self.request.path + '?' + self.request.META['QUERY_STRING']
}
def prepare_action(self, objects):
return
def post(self, request):
objects, _ = self.get_actionable_objects()
resp = self.prepare_action(objects)
if hasattr(resp, 'status_code'):
return resp
with transaction.atomic():
before_hook_result = self.__run_before_hooks(self.action_type, request, objects)
if before_hook_result is not None:

52
wagtail/admin/views/pages/bulk_actions/move.py
Wyświetl plik

@ -20,30 +20,10 @@ class MoveForm(forms.Form):
widget=widgets.AdminPageChooser(can_choose_root=True, user_perms='move_to'),
label=_("Select a new parent page"),
)
def before_bulk_move(request, action_type, pages, action_instance):
if action_type != 'move':
return
move_applicable = request.POST.dict().get("move_applicable", False)
if move_applicable:
return
destination_page_id = request.POST.dict()['chooser']
destination = get_object_or_404(Page, id=destination_page_id)
pages_without_destination_access = []
pages_with_duplicate_slugs = []
for page in pages:
if not page.permissions_for_user(request.user).can_move_to(destination):
pages_without_destination_access.append(page)
if not Page._slug_is_available(page.slug, destination, page=page):
pages_with_duplicate_slugs.append(page)
if pages_without_destination_access or pages_with_duplicate_slugs:
return TemplateResponse(request, action_instance.template_name, {
'pages_without_destination_access': pages_without_destination_access,
"pages_with_duplicate_slugs": pages_with_duplicate_slugs,
'destination': destination,
**action_instance.get_context_data(destination=destination)
})
self.fields['move_applicable'] = forms.BooleanField(
label=_("Move only applicable pages"),
required=False
)
class MoveBulkAction(PageBulkAction):
@ -77,6 +57,28 @@ class MoveBulkAction(PageBulkAction):
context['form'] = MoveForm(destination=destination)
return context
def prepare_action(self, pages):
request = self.request
move_applicable = request.POST.dict().get("move_applicable", False)
if move_applicable:
return
destination_page_id = request.POST.dict()['chooser']
destination = get_object_or_404(Page, id=destination_page_id)
pages_without_destination_access = []
pages_with_duplicate_slugs = []
for page in pages:
if not page.permissions_for_user(request.user).can_move_to(destination):
pages_without_destination_access.append(page)
if not Page._slug_is_available(page.slug, destination, page=page):
pages_with_duplicate_slugs.append(page)
if pages_without_destination_access or pages_with_duplicate_slugs:
return TemplateResponse(request, self.template_name, {
'pages_without_destination_access': pages_without_destination_access,
"pages_with_duplicate_slugs": pages_with_duplicate_slugs,
'destination': destination,
**self.get_context_data(destination=destination)
})
def execute_action(cls, pages):
destination_page_id = cls.request.POST.dict().pop("chooser")
destination = get_object_or_404(Page, id=destination_page_id)
@ -89,8 +91,6 @@ class MoveBulkAction(PageBulkAction):
page.move(destination, pos='last-child', user=cls.request.user)
cls.num_parent_objects += 1
# register temporary hook to check for move related permissions
@hooks.register_temporarily('before_bulk_action', before_bulk_move)
def post(self, request):
return super().post(request)

18
wagtail/admin/views/pages/bulk_actions/page_bulk_action.py
Wyświetl plik

@ -1,7 +1,25 @@
from django import forms
from wagtail.admin.views.bulk_action import BulkAction
from wagtail.core.models import Page
class DefaultForm(forms.Form):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.fields['include_descendants'] = forms.BooleanField(
required=False
)
class PageBulkAction(BulkAction):
model = Page
object_key = 'page'
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context['form'] = DefaultForm()
context['pages_with_no_access'] = [
{'page': page, 'can_edit': page.permissions_for_user(self.request.user).can_edit()} for page in context['pages_with_no_access']
]
return context

7
wagtail/admin/views/pages/listing.py
Wyświetl plik

@ -1,7 +1,6 @@
from django.conf import settings
from django.core.paginator import Paginator
from django.db.models import Count
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
@ -40,12 +39,6 @@ def index(request, parent_page_id=None):
& user_perms.explorable_pages()
)
fetch_page_ids = request.GET.get('fetchPageIds', '0')
if fetch_page_ids == '1':
return JsonResponse({
'page_ids': list(pages.values_list('id', flat=True))
})
# Get page ordering
ordering = request.GET.get('ordering', '-latest_revision_created_at')
if ordering not in [