Adding bulk deletion endpoint for notifications (#3154)

* Catch DoesNotExist error

* Move notificationtable function to js file

* Fix for custom metadata class

- Previously only worked if a POST or PUT action was available on the endpoint
- So, a ListAPIView endpoint would not actually work!
- Adding in a BulkDelete mixin to a ListAPIView caused failure

* Add unit test to ensure new OPTIONS metadata updates are checked

* Expand functionality of the existing BulkDelete mixin

- Allow deletion by custom filters
- Allow each implementing class to implement custom filters
- Adds more unit testing for BulkDelete mixin class

* Add bulk delete operation for Notification API

- Ensure users can only delete their *own* notifications

* Improve notification tables / buttons / etc

* Adds unit testing for bulk delete of notifications

- Fixed API permissions for notifications list endpoint

* Update BulkDelete operations for the StockItemTestResult table

* Use filters parameter in attachments table to ensure that only correct attachments are deleted

* JS linting

* Fixes for unit tests
This commit is contained in:
Oliver 2022-06-08 07:45:30 +10:00 committed by GitHub
parent c0148c0a38
commit 403655e3d2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 379 additions and 132 deletions

View File

@ -64,6 +64,10 @@ class BulkDeleteMixin:
- Speed (single API call and DB query)
"""
def filter_delete_queryset(self, queryset, request):
"""Provide custom filtering for the queryset *before* it is deleted"""
return queryset
def delete(self, request, *args, **kwargs):
"""Perform a DELETE operation against this list endpoint.
@ -81,18 +85,46 @@ class BulkDeleteMixin:
except AttributeError:
items = request.data.get('items', None)
if items is None or type(items) is not list or not items:
# Extract the filters from the request body
try:
filters = request.data.getlist('filters', None)
except AttributeError:
filters = request.data.get('filters', None)
if not items and not filters:
raise ValidationError({
"non_field_errors": ["List of items must be provided for bulk deletion"]
"non_field_errors": ["List of items or filters must be provided for bulk deletion"],
})
if items and type(items) is not list:
raise ValidationError({
"items": ["'items' must be supplied as a list object"]
})
if filters and type(filters) is not dict:
raise ValidationError({
"filters": ["'filters' must be supplied as a dict object"]
})
# Keep track of how many items we deleted
n_deleted = 0
with transaction.atomic():
objects = model.objects.filter(id__in=items)
n_deleted = objects.count()
objects.delete()
# Start with *all* models and perform basic filtering
queryset = model.objects.all()
queryset = self.filter_delete_queryset(queryset, request)
# Filter by provided item ID values
if items:
queryset = queryset.filter(id__in=items)
# Filter by provided filters
if filters:
queryset = queryset.filter(**filters)
n_deleted = queryset.count()
queryset.delete()
return Response(
{

View File

@ -146,7 +146,7 @@ class InvenTreeAPITestCase(UserMixin, APITestCase):
if data is None:
data = {}
response = self.client.delete(url, data=data, foramt=format)
response = self.client.delete(url, data=data, format=format)
if expected_code is not None:
self.assertEqual(response.status_code, expected_code)

View File

@ -2,11 +2,16 @@
# InvenTree API version
INVENTREE_API_VERSION = 58
INVENTREE_API_VERSION = 59
"""
Increment this API version number whenever there is a significant change to the API that any clients need to know about
v59 -> 2022-06-07 : https://github.com/inventree/InvenTree/pull/3154
- Adds further improvements to BulkDelete mixin class
- Fixes multiple bugs in custom OPTIONS metadata implementation
- Adds 'bulk delete' for Notifications
v58 -> 2022-06-06 : https://github.com/inventree/InvenTree/pull/3146
- Adds a BulkDelete API mixin class for fast, safe deletion of multiple objects with a single API request

View File

@ -682,6 +682,7 @@ def get_objectreference(obj, type_ref: str = 'content_type', object_ref: str = '
The method name must always be the name of the field prefixed by 'get_'
"""
model_cls = getattr(obj, type_ref)
obj_id = getattr(obj, object_ref)
@ -691,7 +692,12 @@ def get_objectreference(obj, type_ref: str = 'content_type', object_ref: str = '
# resolve referenced data into objects
model_cls = model_cls.model_class()
item = model_cls.objects.get(id=obj_id)
try:
item = model_cls.objects.get(id=obj_id)
except model_cls.DoesNotExist:
return None
url_fnc = getattr(item, 'get_absolute_url', None)
# create output

View File

@ -44,7 +44,7 @@ class InvenTreeMetadata(SimpleMetadata):
if str2bool(request.query_params.get('context', False)):
if hasattr(self.serializer, 'get_context_data'):
if hasattr(self, 'serializer') and hasattr(self.serializer, 'get_context_data'):
context = self.serializer.get_context_data()
metadata['context'] = context
@ -70,33 +70,36 @@ class InvenTreeMetadata(SimpleMetadata):
actions = metadata.get('actions', None)
if actions is not None:
if actions is None:
actions = {}
check = users.models.RuleSet.check_table_permission
check = users.models.RuleSet.check_table_permission
# Map the request method to a permission type
rolemap = {
'POST': 'add',
'PUT': 'change',
'PATCH': 'change',
'DELETE': 'delete',
}
# Map the request method to a permission type
rolemap = {
'POST': 'add',
'PUT': 'change',
'PATCH': 'change',
'DELETE': 'delete',
}
# Remove any HTTP methods that the user does not have permission for
for method, permission in rolemap.items():
# Remove any HTTP methods that the user does not have permission for
for method, permission in rolemap.items():
result = check(user, table, permission)
result = check(user, table, permission)
if method in actions and not result:
del actions[method]
if method in actions and not result:
del actions[method]
# Add a 'DELETE' action if we are allowed to delete
if 'DELETE' in view.allowed_methods and check(user, table, 'delete'):
actions['DELETE'] = True
# Add a 'DELETE' action if we are allowed to delete
if 'DELETE' in view.allowed_methods and check(user, table, 'delete'):
actions['DELETE'] = True
# Add a 'VIEW' action if we are allowed to view
if 'GET' in view.allowed_methods and check(user, table, 'view'):
actions['GET'] = True
# Add a 'VIEW' action if we are allowed to view
if 'GET' in view.allowed_methods and check(user, table, 'view'):
actions['GET'] = True
metadata['actions'] = actions
except AttributeError:
# We will assume that if the serializer class does *not* have a Meta

View File

@ -215,15 +215,15 @@ class APITests(InvenTreeAPITestCase):
actions = self.getActions(url)
# No actions, as there are no permissions!
self.assertEqual(len(actions), 0)
# Even without permissions, GET action is available
self.assertEqual(len(actions), 1)
# Assign a new role
self.assignRole('part.view')
actions = self.getActions(url)
# As we don't have "add" permission, there should be no available API actions
self.assertEqual(len(actions), 0)
# As we don't have "add" permission, there should be only the GET API action
self.assertEqual(len(actions), 1)
# But let's make things interesting...
# Why don't we treat ourselves to some "add" permissions
@ -244,7 +244,8 @@ class APITests(InvenTreeAPITestCase):
actions = self.getActions(url)
# No actions, as we do not have any permissions!
self.assertEqual(len(actions), 0)
self.assertEqual(len(actions), 1)
self.assertIn('GET', actions.keys())
# Add a 'add' permission
# Note: 'add' permission automatically implies 'change' also
@ -266,3 +267,45 @@ class APITests(InvenTreeAPITestCase):
self.assertIn('GET', actions.keys())
self.assertIn('PUT', actions.keys())
self.assertIn('DELETE', actions.keys())
class BulkDeleteTests(InvenTreeAPITestCase):
"""Unit tests for the BulkDelete endpoints"""
superuser = True
def test_errors(self):
"""Test that the correct errors are thrown"""
url = reverse('api-stock-test-result-list')
# DELETE without any of the required fields
response = self.delete(
url,
{},
expected_code=400
)
self.assertIn('List of items or filters must be provided for bulk deletion', str(response.data))
# DELETE with invalid 'items'
response = self.delete(
url,
{
'items': {"hello": "world"},
},
expected_code=400,
)
self.assertIn("'items' must be supplied as a list object", str(response.data))
# DELETE with invalid 'filters'
response = self.delete(
url,
{
'filters': [1, 2, 3],
},
expected_code=400,
)
self.assertIn("'filters' must be supplied as a dict object", str(response.data))

View File

@ -16,6 +16,7 @@ from rest_framework.views import APIView
import common.models
import common.serializers
from InvenTree.api import BulkDeleteMixin
from InvenTree.helpers import inheritors
from plugin.models import NotificationUserSetting
from plugin.serializers import NotificationUserSettingSerializer
@ -258,12 +259,16 @@ class NotificationUserSettingsDetail(generics.RetrieveUpdateAPIView):
]
class NotificationList(generics.ListAPIView):
class NotificationList(BulkDeleteMixin, generics.ListAPIView):
"""List view for all notifications of the current user."""
queryset = common.models.NotificationMessage.objects.all()
serializer_class = common.serializers.NotificationMessageSerializer
permission_classes = [
permissions.IsAuthenticated,
]
filter_backends = [
DjangoFilterBackend,
filters.SearchFilter,
@ -298,6 +303,12 @@ class NotificationList(generics.ListAPIView):
queryset = queryset.filter(user=user)
return queryset
def filter_delete_queryset(self, queryset, request):
"""Ensure that the user can only delete their *own* notifications"""
queryset = queryset.filter(user=request.user)
return queryset
class NotificationDetail(generics.RetrieveUpdateDestroyAPIView):
"""Detail view for an individual notification object.

View File

@ -14,7 +14,8 @@ from plugin.models import NotificationUserSetting, PluginConfig
from .api import WebhookView
from .models import (ColorTheme, InvenTreeSetting, InvenTreeUserSetting,
NotificationEntry, WebhookEndpoint, WebhookMessage)
NotificationEntry, NotificationMessage, WebhookEndpoint,
WebhookMessage)
CONTENT_TYPE_JSON = 'application/json'
@ -665,6 +666,10 @@ class WebhookMessageTests(TestCase):
class NotificationTest(InvenTreeAPITestCase):
"""Tests for NotificationEntriy."""
fixtures = [
'users',
]
def test_check_notification_entries(self):
"""Test that notification entries can be created."""
# Create some notification entries
@ -684,9 +689,84 @@ class NotificationTest(InvenTreeAPITestCase):
def test_api_list(self):
"""Test list URL."""
url = reverse('api-notifications-list')
self.get(url, expected_code=200)
# Test the OPTIONS endpoint for the 'api-notification-list'
# Ref: https://github.com/inventree/InvenTree/pull/3154
response = self.options(url)
self.assertIn('DELETE', response.data['actions'])
self.assertIn('GET', response.data['actions'])
self.assertNotIn('POST', response.data['actions'])
self.assertEqual(response.data['description'], 'List view for all notifications of the current user.')
# POST action should fail (not allowed)
response = self.post(url, {}, expected_code=405)
def test_bulk_delete(self):
"""Tests for bulk deletion of user notifications"""
from error_report.models import Error
# Create some notification messages by throwing errors
for _ii in range(10):
Error.objects.create()
# Check that messsages have been created
messages = NotificationMessage.objects.all()
# As there are three staff users (including the 'test' user) we expect 30 notifications
self.assertEqual(messages.count(), 30)
# Only 10 messages related to *this* user
my_notifications = messages.filter(user=self.user)
self.assertEqual(my_notifications.count(), 10)
# Get notification via the API
url = reverse('api-notifications-list')
response = self.get(url, {}, expected_code=200)
self.assertEqual(len(response.data), 10)
# Mark some as read
for ntf in my_notifications[0:3]:
ntf.read = True
ntf.save()
# Read out via API again
response = self.get(
url,
{
'read': True,
},
expected_code=200
)
# Check validity of returned data
self.assertEqual(len(response.data), 3)
for ntf in response.data:
self.assertTrue(ntf['read'])
# Now, let's bulk delete all 'unread' notifications via the API,
# but only associated with the logged in user
response = self.delete(
url,
{
'filters': {
'read': False,
}
},
expected_code=204,
)
# Only 7 notifications should have been deleted,
# as the notifications associated with other users must remain untouched
self.assertEqual(NotificationMessage.objects.count(), 23)
self.assertEqual(NotificationMessage.objects.filter(user=self.user).count(), 3)
class LoadingTest(TestCase):
"""Tests for the common config."""

View File

@ -280,9 +280,7 @@
// Ensure that we are only deleting the correct test results
response.forEach(function(result) {
if (result.stock_item == {{ item.pk }}) {
items.push(result.pk);
}
items.push(result.pk);
});
var html = `
@ -293,6 +291,9 @@
constructForm(url, {
form_data: {
items: items,
filters: {
stock_item: {{ item.pk }},
}
},
method: 'DELETE',
title: '{% trans "Delete Test Data" %}',

View File

@ -968,10 +968,28 @@ class StockTestResultTest(StockAPITestCase):
)
# Now, let's delete all the newly created items with a single API request
# However, we will provide incorrect filters
response = self.delete(
url,
{
'items': tests,
'filters': {
'stock_item': 10,
}
},
expected_code=204
)
self.assertEqual(StockItemTestResult.objects.count(), n + 50)
# Try again, but with the correct filters this time
response = self.delete(
url,
{
'items': tests,
'filters': {
'stock_item': 1,
}
},
expected_code=204
)

View File

@ -10,15 +10,22 @@
{% endblock %}
{% block actions %}
<div class='btn btn-secondary' type='button' id='history-refresh' title='{% trans "Refresh Notification History" %}'>
<span class='fa fa-sync'></span> {% trans "Refresh Notification History" %}
<div class='btn btn-danger' type='button' id='history-delete' title='{% trans "Delete Notifications" %}'>
<span class='fas fa-trash-alt'></span> {% trans "Delete Notifications" %}
</div>
{% endblock %}
{% block content %}
<div id='history-buttons'>
<div class='btn-group' role='group'>
{% include "filter_list.html" with id="notifications-history" %}
</div>
</div>
<div class='row'>
<table class='table table-striped table-condensed' id='history-table'>
<table class='table table-striped table-condensed' id='history-table' data-toolbar='#history-buttons'>
</table>
</div>

View File

@ -13,15 +13,18 @@
<div class='btn btn-outline-secondary' type='button' id='mark-all' title='{% trans "Mark all as read" %}'>
<span class='fa fa-bookmark'></span> {% trans "Mark all as read" %}
</div>
<div class='btn btn-secondary' type='button' id='inbox-refresh' title='{% trans "Refresh Pending Notifications" %}'>
<span class='fa fa-sync'></span> {% trans "Refresh Pending Notifications" %}
</div>
{% endblock %}
{% block content %}
<div id='inbox-buttons'>
<div class='btn-group' role='group'>
{% include "filter_list.html" with id="notifications-inbox" %}
</div>
</div>
<div class='row'>
<table class='table table-striped table-condensed' id='inbox-table'>
<table class='table table-striped table-condensed' id='inbox-table' data-toolbar='#inbox-buttons'>
</table>
</div>

View File

@ -29,83 +29,6 @@ function updateNotificationTables() {
// this allows the global notification panel to update the tables
window.updateNotifications = updateNotificationTables
function loadNotificationTable(table, options={}, enableDelete=false) {
var params = options.params || {};
var read = typeof(params.read) === 'undefined' ? true : params.read;
$(table).inventreeTable({
url: options.url,
name: options.name,
groupBy: false,
search: true,
queryParams: {
ordering: 'age',
read: read,
},
paginationVAlign: 'bottom',
formatNoMatches: options.no_matches,
columns: [
{
field: 'pk',
title: '{% trans "ID" %}',
visible: false,
switchable: false,
},
{
field: 'age',
title: '{% trans "Age" %}',
sortable: 'true',
formatter: function(value, row) {
return row.age_human
}
},
{
field: 'category',
title: '{% trans "Category" %}',
sortable: 'true',
},
{
field: 'target',
title: '{% trans "Item" %}',
sortable: 'true',
formatter: function(value, row, index, field) {
if (value == null) {
return '';
}
var html = `${value.model}: ${value.name}`;
if (value.link ) {html = `<a href='${value.link}'>${html}</a>`;}
return html;
}
},
{
field: 'name',
title: '{% trans "Name" %}',
},
{
field: 'message',
title: '{% trans "Message" %}',
},
{
formatter: function(value, row, index, field) {
var bRead = getReadEditButton(row.pk, row.read)
if (enableDelete) {
var bDel = "<button title='{% trans "Delete Notification" %}' class='notification-delete btn btn-outline-secondary' type='button' pk='" + row.pk + "'><span class='fas fa-trash-alt icon-red'></span></button>";
} else {
var bDel = '';
}
var html = "<div class='btn-group float-right' role='group'>" + bRead + bDel + "</div>";
return html;
}
}
]
});
$(table).on('click', '.notification-read', function() {
updateNotificationReadState($(this));
});
}
loadNotificationTable("#inbox-table", {
name: 'inbox',
@ -116,10 +39,6 @@ loadNotificationTable("#inbox-table", {
no_matches: function() { return '{% trans "No unread notifications found" %}'; },
});
$("#inbox-refresh").on('click', function() {
$("#inbox-table").bootstrapTable('refresh');
});
$("#mark-all").on('click', function() {
inventreeGet(
'{% url "api-notifications-readall" %}',
@ -140,8 +59,31 @@ loadNotificationTable("#history-table", {
no_matches: function() { return '{% trans "No notification history found" %}'; },
}, true);
$("#history-refresh").on('click', function() {
$("#history-table").bootstrapTable('refresh');
$('#history-delete').click(function() {
var html = `
<div class='alert alert-block alert-danger'>
{% trans "Delete all read notifications" %}
</div>`;
// Perform a bulk delete of all 'read' notifications for this user
constructForm(
'{% url "api-notifications-list" %}',
{
method: 'DELETE',
preFormContent: html,
title: '{% trans "Delete Notifications" %}',
onSuccess: function() {
$('#history-table').bootstrapTable('refresh');
},
form_data: {
filters: {
read: true,
}
}
}
);
});
$("#history-table").on('click', '.notification-delete', function() {

View File

@ -3,7 +3,7 @@
<div id='attachment-buttons'>
<div class='btn-group' role='group'>
<div class='btn-group'>
<button class='btn btn-primary dropdown-toggle' type='buton' data-bs-toggle='dropdown' title='{% trans "Actions" %}'>
<button class='btn btn-primary dropdown-toggle' type='button' data-bs-toggle='dropdown' title='{% trans "Actions" %}'>
<span class='fas fa-tools'></span> <span class='caret'></span>
</button>
<ul class='dropdown-menu'>

View File

@ -113,6 +113,7 @@ function deleteAttachments(attachments, url, options={}) {
preFormContent: html,
form_data: {
items: ids,
filters: options.filters,
},
onSuccess: function() {
// Refresh the table once all attachments are deleted
@ -128,6 +129,9 @@ function reloadAttachmentTable() {
}
/* Load a table of attachments against a specific model.
* Note that this is a 'generic' table which is used for multiple attachment model classes
*/
function loadAttachmentTable(url, options) {
var table = options.table || '#attachment-table';
@ -141,7 +145,7 @@ function loadAttachmentTable(url, options) {
var attachments = getTableData(table);
if (attachments.length > 0) {
deleteAttachments(attachments, url);
deleteAttachments(attachments, url, options);
}
});
@ -182,7 +186,7 @@ function loadAttachmentTable(url, options) {
var pk = $(this).attr('pk');
var attachment = $(table).bootstrapTable('getRowByUniqueId', pk);
deleteAttachments([attachment], url);
deleteAttachments([attachment], url, options);
});
},
columns: [

View File

@ -1,6 +1,7 @@
{% load i18n %}
/* exported
loadNotificationTable,
showAlertOrCache,
showCachedAlerts,
startNotificationWatcher,
@ -9,6 +10,96 @@
closeNotificationPanel,
*/
/*
* Load notification table
*/
function loadNotificationTable(table, options={}, enableDelete=false) {
var params = options.params || {};
var read = typeof(params.read) === 'undefined' ? true : params.read;
setupFilterList(`notifications-${options.name}`, table);
$(table).inventreeTable({
url: options.url,
name: options.name,
groupBy: false,
search: true,
queryParams: {
ordering: 'age',
read: read,
},
paginationVAlign: 'bottom',
formatNoMatches: options.no_matches,
columns: [
{
field: 'pk',
title: '{% trans "ID" %}',
visible: false,
switchable: false,
},
{
field: 'age',
title: '{% trans "Age" %}',
sortable: 'true',
formatter: function(value, row) {
return row.age_human;
}
},
{
field: 'category',
title: '{% trans "Category" %}',
sortable: 'true',
},
{
field: 'target',
title: '{% trans "Item" %}',
sortable: 'true',
formatter: function(value, row, index, field) {
if (value == null) {
return '';
}
var html = `${value.model}: ${value.name}`;
if (value.link ) {
html = `<a href='${value.link}'>${html}</a>`;
}
return html;
}
},
{
field: 'name',
title: '{% trans "Name" %}',
},
{
field: 'message',
title: '{% trans "Message" %}',
},
{
formatter: function(value, row, index, field) {
var bRead = getReadEditButton(row.pk, row.read);
if (enableDelete) {
var bDel = `<button title='{% trans "Delete Notification" %}' class='notification-delete btn btn-outline-secondary' type='button' pk='${row.pk}'><span class='fas fa-trash-alt icon-red'></span></button>`;
} else {
var bDel = '';
}
var html = `<div class='btn-group float-right' role='group'>${bRead}${bDel}</div>`;
return html;
}
}
]
});
$(table).on('click', '.notification-read', function() {
updateNotificationReadState($(this));
});
}
/*
* Add a cached alert message to sesion storage
*/

View File

@ -36,6 +36,7 @@
first_name: "Alan"
last_name: "Allgroup"
is_active: false
is_staff: true
groups:
- 1
- 2