From 403655e3d2789c29137bff985c9f9d968ef276b4 Mon Sep 17 00:00:00 2001 From: Oliver Date: Wed, 8 Jun 2022 07:45:30 +1000 Subject: [PATCH] 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 --- InvenTree/InvenTree/api.py | 42 ++++++- InvenTree/InvenTree/api_tester.py | 2 +- InvenTree/InvenTree/api_version.py | 7 +- InvenTree/InvenTree/helpers.py | 8 +- InvenTree/InvenTree/metadata.py | 45 ++++---- InvenTree/InvenTree/test_api.py | 53 ++++++++- InvenTree/common/api.py | 13 ++- InvenTree/common/tests.py | 82 ++++++++++++- InvenTree/stock/templates/stock/item.html | 7 +- InvenTree/stock/test_api.py | 18 +++ .../InvenTree/notifications/history.html | 13 ++- .../InvenTree/notifications/inbox.html | 11 +- .../notifications/notifications.html | 108 ++++-------------- InvenTree/templates/attachment_table.html | 2 +- .../templates/js/translated/attachment.js | 8 +- .../templates/js/translated/notification.js | 91 +++++++++++++++ InvenTree/users/fixtures/users.yaml | 1 + 17 files changed, 379 insertions(+), 132 deletions(-) diff --git a/InvenTree/InvenTree/api.py b/InvenTree/InvenTree/api.py index a9f55b23a6..ca4d37dc7c 100644 --- a/InvenTree/InvenTree/api.py +++ b/InvenTree/InvenTree/api.py @@ -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( { diff --git a/InvenTree/InvenTree/api_tester.py b/InvenTree/InvenTree/api_tester.py index ef1007c66b..a95a39d040 100644 --- a/InvenTree/InvenTree/api_tester.py +++ b/InvenTree/InvenTree/api_tester.py @@ -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) diff --git a/InvenTree/InvenTree/api_version.py b/InvenTree/InvenTree/api_version.py index 7bb41bd8c5..e90f60d1b7 100644 --- a/InvenTree/InvenTree/api_version.py +++ b/InvenTree/InvenTree/api_version.py @@ -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 diff --git a/InvenTree/InvenTree/helpers.py b/InvenTree/InvenTree/helpers.py index 34f296c2f8..fdc6380138 100644 --- a/InvenTree/InvenTree/helpers.py +++ b/InvenTree/InvenTree/helpers.py @@ -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 diff --git a/InvenTree/InvenTree/metadata.py b/InvenTree/InvenTree/metadata.py index 924d528fc0..8e4fa13452 100644 --- a/InvenTree/InvenTree/metadata.py +++ b/InvenTree/InvenTree/metadata.py @@ -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 diff --git a/InvenTree/InvenTree/test_api.py b/InvenTree/InvenTree/test_api.py index 46029d1540..0e55f7a559 100644 --- a/InvenTree/InvenTree/test_api.py +++ b/InvenTree/InvenTree/test_api.py @@ -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)) diff --git a/InvenTree/common/api.py b/InvenTree/common/api.py index afc0b1272b..672f341e8f 100644 --- a/InvenTree/common/api.py +++ b/InvenTree/common/api.py @@ -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. diff --git a/InvenTree/common/tests.py b/InvenTree/common/tests.py index f0f06a3e7c..8a087930c0 100644 --- a/InvenTree/common/tests.py +++ b/InvenTree/common/tests.py @@ -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.""" diff --git a/InvenTree/stock/templates/stock/item.html b/InvenTree/stock/templates/stock/item.html index c7c6019ddb..d5ccc21d35 100644 --- a/InvenTree/stock/templates/stock/item.html +++ b/InvenTree/stock/templates/stock/item.html @@ -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" %}', diff --git a/InvenTree/stock/test_api.py b/InvenTree/stock/test_api.py index b0213e5a0b..93f35b78e1 100644 --- a/InvenTree/stock/test_api.py +++ b/InvenTree/stock/test_api.py @@ -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 ) diff --git a/InvenTree/templates/InvenTree/notifications/history.html b/InvenTree/templates/InvenTree/notifications/history.html index 2f6a34d0d7..4623f7fc4f 100644 --- a/InvenTree/templates/InvenTree/notifications/history.html +++ b/InvenTree/templates/InvenTree/notifications/history.html @@ -10,15 +10,22 @@ {% endblock %} {% block actions %} -
- {% trans "Refresh Notification History" %} +
+ {% trans "Delete Notifications" %}
{% endblock %} {% block content %} +
+
+ {% include "filter_list.html" with id="notifications-history" %} +
+
+
- + +
diff --git a/InvenTree/templates/InvenTree/notifications/inbox.html b/InvenTree/templates/InvenTree/notifications/inbox.html index 40ea8f2042..be994727b1 100644 --- a/InvenTree/templates/InvenTree/notifications/inbox.html +++ b/InvenTree/templates/InvenTree/notifications/inbox.html @@ -13,15 +13,18 @@
{% trans "Mark all as read" %}
-
- {% trans "Refresh Pending Notifications" %} -
{% endblock %} {% block content %} +
+
+ {% include "filter_list.html" with id="notifications-inbox" %} +
+
+
- +
diff --git a/InvenTree/templates/InvenTree/notifications/notifications.html b/InvenTree/templates/InvenTree/notifications/notifications.html index fedf8a1448..b225002279 100644 --- a/InvenTree/templates/InvenTree/notifications/notifications.html +++ b/InvenTree/templates/InvenTree/notifications/notifications.html @@ -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 = `${html}`;} - 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 = ""; - } else { - var bDel = ''; - } - var html = "
" + bRead + bDel + "
"; - 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 = ` +
+ {% trans "Delete all read notifications" %} +
`; + + // 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() { diff --git a/InvenTree/templates/attachment_table.html b/InvenTree/templates/attachment_table.html index 2db4c6345f..f47e1f5173 100644 --- a/InvenTree/templates/attachment_table.html +++ b/InvenTree/templates/attachment_table.html @@ -3,7 +3,7 @@
-