From 3ab35722421d1a0acb79d234242d9fa19f7b78b6 Mon Sep 17 00:00:00 2001 From: Samy Ait-Ouakli Date: Wed, 12 Jul 2023 01:30:22 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=E2=9C=A8(backend)=20add=20bulk=20delete=20?= =?UTF-8?q?route?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add route to admin api to bulk delete objects --- CHANGELOG.md | 1 + src/backend/joanie/core/api/admin.py | 12 ++-- src/backend/joanie/core/mixins/__init__.py | 5 ++ src/backend/joanie/core/mixins/admin.py | 59 ++++++++++++++++++ .../test_api_admin_certificate_definitions.py | 62 ++++++++++++++++++- .../tests/core/test_api_admin_course_runs.py | 20 +++++- .../tests/core/test_api_admin_courses.py | 43 ++++++++++++- .../core/test_api_admin_organizations.py | 22 ++++++- .../tests/core/test_api_admin_products.py | 39 +++++++++++- 9 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 src/backend/joanie/core/mixins/__init__.py create mode 100644 src/backend/joanie/core/mixins/admin.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 81f2e326b..e531d0370 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to ## Added +- Add admin endpoint to bulk delete objects - Remove djmoney and Moneyfields - Add admin endpoints to create/update/delete organization/course accesses - Add admin endpoint to search users diff --git a/src/backend/joanie/core/api/admin.py b/src/backend/joanie/core/api/admin.py index 30dbe144f..8d3a4b6a9 100755 --- a/src/backend/joanie/core/api/admin.py +++ b/src/backend/joanie/core/api/admin.py @@ -1,14 +1,16 @@ """ Admin API Endpoints """ + import django_filters.rest_framework from rest_framework import authentication, mixins, permissions, viewsets from joanie.core import filters, models, serializers +from joanie.core.mixins import admin as admin_mixins # pylint: disable=too-many-ancestors -class OrganizationViewSet(viewsets.ModelViewSet): +class OrganizationViewSet(viewsets.ModelViewSet, admin_mixins.BulkDeleteMixin): """ Admin Organization ViewSet """ @@ -29,7 +31,7 @@ def get_serializer_class(self): return self.serializer_class -class ProductViewSet(viewsets.ModelViewSet): +class ProductViewSet(viewsets.ModelViewSet, admin_mixins.BulkDeleteMixin): """ Admin Product ViewSet """ @@ -41,7 +43,7 @@ class ProductViewSet(viewsets.ModelViewSet): filterset_class = filters.ProductAdminFilterSet -class CourseViewSet(viewsets.ModelViewSet): +class CourseViewSet(viewsets.ModelViewSet, admin_mixins.BulkDeleteMixin): """ Admin Course ViewSet """ @@ -62,7 +64,7 @@ def get_serializer_class(self): return self.serializer_class -class CourseRunViewSet(viewsets.ModelViewSet): +class CourseRunViewSet(viewsets.ModelViewSet, admin_mixins.BulkDeleteMixin): """ Admin CourseRun ViewSet """ @@ -75,7 +77,7 @@ class CourseRunViewSet(viewsets.ModelViewSet): filterset_class = filters.CourseRunAdminFilterSet -class CertificateDefinitionViewSet(viewsets.ModelViewSet): +class CertificateDefinitionViewSet(viewsets.ModelViewSet, admin_mixins.BulkDeleteMixin): """ Admin Certificate ViewSet """ diff --git a/src/backend/joanie/core/mixins/__init__.py b/src/backend/joanie/core/mixins/__init__.py new file mode 100644 index 000000000..a32e3723c --- /dev/null +++ b/src/backend/joanie/core/mixins/__init__.py @@ -0,0 +1,5 @@ +"""Mixins for Joanie Core app.""" +# flake8: noqa +# pylint: disable=wildcard-import + +from .admin import * diff --git a/src/backend/joanie/core/mixins/admin.py b/src/backend/joanie/core/mixins/admin.py new file mode 100644 index 000000000..ce9900500 --- /dev/null +++ b/src/backend/joanie/core/mixins/admin.py @@ -0,0 +1,59 @@ +""" +Mixins for admin api +""" + +from django.db import IntegrityError + +from rest_framework import mixins +from rest_framework.response import Response + + +class BulkDeleteMixin(mixins.DestroyModelMixin): + """ + Mixin for classes that need to be able to bulk delete + """ + + def delete(self, request, *args, **kwargs): + """ + Performs a deletion. Handles cases where an id is present + in the url (single deletion), if no id is present in url + and ids are in the body, performs bulk delete + """ + if self.kwargs.get("id", None): + return super().delete(request, *args, **kwargs) + + id_to_delete = request.data.get("id", []) + elements_to_delete = self.queryset.filter(id__in=id_to_delete) + (deleted_elements, error_elements) = self.perform_bulk_delete( + elements_to_delete + ) + + response = {} + if len(deleted_elements) > 0: + response["deleted"] = deleted_elements + if len(error_elements) > 0: + response["error"] = error_elements + + return Response(response) + + def perform_bulk_delete(self, instances): + """ + Bulk deletion of model + data = {"id": [, ]} + """ + error_elements = [] + deleted_elements = [] + for element in instances.all(): + try: + deleted_element_id = element.id + element.delete() + deleted_elements.append(deleted_element_id) + except IntegrityError as error: + error_elements.append( + { + "id": element.id, + "error": str(error), + } + ) + + return (deleted_elements, error_elements) diff --git a/src/backend/joanie/tests/core/test_api_admin_certificate_definitions.py b/src/backend/joanie/tests/core/test_api_admin_certificate_definitions.py index a254b8d16..2f1142bc8 100644 --- a/src/backend/joanie/tests/core/test_api_admin_certificate_definitions.py +++ b/src/backend/joanie/tests/core/test_api_admin_certificate_definitions.py @@ -5,7 +5,7 @@ from django.test import TestCase -from joanie.core import factories +from joanie.core import factories, models class CertificateDefinitionAdminApiTest(TestCase): @@ -250,3 +250,63 @@ def test_admin_api_certificate_definition_delete(self): ) self.assertEqual(response.status_code, 204) + + def test_admin_api_certificate_definition_bulk_delete(self): + """ + Staff user should be able to delete multiple certificate_definitions. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + certificate_definitions = factories.CertificateDefinitionFactory.create_batch(3) + factories.CertificateDefinitionFactory() + data = { + "id": [ + str(certificate_definition.id) + for certificate_definition in certificate_definitions + ] + } + response = self.client.delete( + "/api/v1.0/admin/certificate-definitions/", + data=data, + content_type="application/json", + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.CertificateDefinition.objects.count(), 1) + self.assertEqual(len(content["deleted"]), 3) + self.assertFalse("error" in content) + + def test_admin_api_certificate_definition_bulk_delete_error(self): + """ + CertificateDefinition linked to a Certificate cannot be deleted. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + certificate_definitions = factories.CertificateDefinitionFactory.create_batch(2) + factories.CertificateFactory(certificate_definition=certificate_definitions[0]) + data = { + "id": [ + str(certificate_definition.id) + for certificate_definition in certificate_definitions + ] + } + response = self.client.delete( + "/api/v1.0/admin/certificate-definitions/", + data=data, + content_type="application/json", + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertTrue( + models.CertificateDefinition.objects.filter( + id=certificate_definitions[0].id + ).exists() + ) + self.assertFalse( + models.CertificateDefinition.objects.filter( + id=certificate_definitions[1].id + ).exists() + ) + self.assertEqual(len(content["deleted"]), 1) + self.assertEqual(content["deleted"][0], str(certificate_definitions[1].id)) + self.assertEqual(len(content["error"]), 1) diff --git a/src/backend/joanie/tests/core/test_api_admin_course_runs.py b/src/backend/joanie/tests/core/test_api_admin_course_runs.py index f1ac63de5..fc57c7642 100644 --- a/src/backend/joanie/tests/core/test_api_admin_course_runs.py +++ b/src/backend/joanie/tests/core/test_api_admin_course_runs.py @@ -5,7 +5,7 @@ from django.test import TestCase -from joanie.core import factories +from joanie.core import factories, models class CourseRunAdminApiTest(TestCase): @@ -233,3 +233,21 @@ def test_admin_api_course_runs_delete(self): response = self.client.delete(f"/api/v1.0/admin/course-runs/{course_run.id}/") self.assertEqual(response.status_code, 204) + + def test_admin_api_course_runs_bulk_delete(self): + """ + Staff user should be able to delete multiple course runs. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + course_runs = factories.CourseRunFactory.create_batch(3) + factories.CourseRunFactory() + data = {"id": [str(course_run.id) for course_run in course_runs]} + response = self.client.delete( + "/api/v1.0/admin/course-runs/", data=data, content_type="application/json" + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.CourseRun.objects.count(), 1) + self.assertEqual(len(content["deleted"]), 3) + self.assertFalse("error" in content) diff --git a/src/backend/joanie/tests/core/test_api_admin_courses.py b/src/backend/joanie/tests/core/test_api_admin_courses.py index afc0276a3..31b837045 100644 --- a/src/backend/joanie/tests/core/test_api_admin_courses.py +++ b/src/backend/joanie/tests/core/test_api_admin_courses.py @@ -5,7 +5,7 @@ from django.test import TestCase -from joanie.core import factories +from joanie.core import factories, models class CourseAdminApiTest(TestCase): @@ -309,3 +309,44 @@ def test_admin_api_course_delete(self): response = self.client.delete(f"/api/v1.0/admin/courses/{course.id}/") self.assertEqual(response.status_code, 204) + + def test_admin_api_course_bulk_delete(self): + """ + Staff user should be able to delete multiple courses. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + courses = factories.CourseFactory.create_batch(3) + factories.CourseFactory() + data = {"id": [str(course.id) for course in courses]} + response = self.client.delete( + "/api/v1.0/admin/courses/", data=data, content_type="application/json" + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Course.objects.count(), 1) + self.assertEqual(len(content["deleted"]), 3) + self.assertFalse("error" in content) + + def test_admin_api_course_bulk_delete_error(self): + """ + Courses with linked CourseRun cannot be deleted. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + courses = factories.CourseFactory.create_batch(3) + factories.CourseFactory() + factories.CourseRunFactory(course=courses[0]) + data = {"id": [str(course.id) for course in courses]} + response = self.client.delete( + "/api/v1.0/admin/courses/", data=data, content_type="application/json" + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Course.objects.count(), 2) + self.assertEqual(len(content["deleted"]), 2) + self.assertEqual(len(content["error"]), 1) + self.assertEqual(content["error"][0]["id"], str(courses[0].id)) + self.assertTrue( + "through protected foreign keys" in content["error"][0]["error"] + ) diff --git a/src/backend/joanie/tests/core/test_api_admin_organizations.py b/src/backend/joanie/tests/core/test_api_admin_organizations.py index 0c10cd4cd..f7acb546d 100755 --- a/src/backend/joanie/tests/core/test_api_admin_organizations.py +++ b/src/backend/joanie/tests/core/test_api_admin_organizations.py @@ -10,7 +10,7 @@ import factory.django -from joanie.core import factories +from joanie.core import factories, models from joanie.core.serializers import fields @@ -345,3 +345,23 @@ def test_admin_api_organization_delete(self): ) self.assertEqual(response.status_code, 204) + + def test_admin_api_organization_bulk_delete(self): + """ + Staff user should be able to delete multiple organizations. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + organizations = factories.OrganizationFactory.create_batch(3) + factories.OrganizationFactory() + data = {"id": [str(organization.id) for organization in organizations]} + response = self.client.delete( + "/api/v1.0/admin/organizations/", + data=data, + content_type="application/json", + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Organization.objects.count(), 1) + self.assertEqual(len(content["deleted"]), 3) + self.assertFalse("error" in content) diff --git a/src/backend/joanie/tests/core/test_api_admin_products.py b/src/backend/joanie/tests/core/test_api_admin_products.py index bd5323f4f..698eb368c 100644 --- a/src/backend/joanie/tests/core/test_api_admin_products.py +++ b/src/backend/joanie/tests/core/test_api_admin_products.py @@ -5,7 +5,7 @@ from django.test import TestCase -from joanie.core import factories +from joanie.core import factories, models class ProductAdminApiTest(TestCase): @@ -148,3 +148,40 @@ def test_admin_api_product_delete(self): response = self.client.delete(f"/api/v1.0/admin/products/{product.id}/") self.assertEqual(response.status_code, 204) + + def test_admin_api_product_bulk_delete(self): + """ + Staff user should be able to delete multiple products. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + products = factories.ProductFactory.create_batch(3) + factories.ProductFactory() + data = {"id": [str(product.id) for product in products]} + response = self.client.delete( + "/api/v1.0/admin/products/", data=data, content_type="application/json" + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Product.objects.count(), 1) + self.assertEqual(len(content["deleted"]), 3) + self.assertFalse("error" in content) + + def test_admin_api_product_bulk_delete_error(self): + """ + Products linked to Order cannot be deleted + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + products = factories.ProductFactory.create_batch(3) + factories.OrderFactory(product=products[0]) + data = {"id": [str(product.id) for product in products]} + response = self.client.delete( + "/api/v1.0/admin/products/", data=data, content_type="application/json" + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Product.objects.count(), 1) + self.assertTrue(models.Product.objects.filter(id=products[0].id).exists()) + self.assertEqual(len(content["deleted"]), 2) + self.assertEqual(len(content["error"]), 1) From 1d1ca176f2a95907b40558d80029e34a9a8c165a Mon Sep 17 00:00:00 2001 From: Samy Ait-Ouakli Date: Mon, 31 Jul 2023 19:15:12 +0200 Subject: [PATCH 2/2] f --- src/backend/joanie/core/mixins/admin.py | 27 +++++++++++++------ .../joanie/core/models/certifications.py | 15 +++++++++++ src/backend/joanie/core/models/courses.py | 15 +++++++++++ src/backend/joanie/core/models/products.py | 15 +++++++++++ .../tests/core/test_api_admin_courses.py | 24 +++++++++++++++-- .../core/test_api_admin_organizations.py | 23 ++++++++++++++++ 6 files changed, 109 insertions(+), 10 deletions(-) diff --git a/src/backend/joanie/core/mixins/admin.py b/src/backend/joanie/core/mixins/admin.py index ce9900500..0ce32bf0b 100644 --- a/src/backend/joanie/core/mixins/admin.py +++ b/src/backend/joanie/core/mixins/admin.py @@ -3,6 +3,7 @@ """ from django.db import IntegrityError +from django.utils.translation import gettext_lazy as _ from rest_framework import mixins from rest_framework.response import Response @@ -19,13 +20,15 @@ def delete(self, request, *args, **kwargs): in the url (single deletion), if no id is present in url and ids are in the body, performs bulk delete """ + if self.kwargs.get("id", None): return super().delete(request, *args, **kwargs) + user = request.user id_to_delete = request.data.get("id", []) elements_to_delete = self.queryset.filter(id__in=id_to_delete) (deleted_elements, error_elements) = self.perform_bulk_delete( - elements_to_delete + elements_to_delete, user ) response = {} @@ -36,7 +39,7 @@ def delete(self, request, *args, **kwargs): return Response(response) - def perform_bulk_delete(self, instances): + def perform_bulk_delete(self, instances, user): """ Bulk deletion of model data = {"id": [, ]} @@ -44,15 +47,23 @@ def perform_bulk_delete(self, instances): error_elements = [] deleted_elements = [] for element in instances.all(): - try: - deleted_element_id = element.id - element.delete() - deleted_elements.append(deleted_element_id) - except IntegrityError as error: + if element.get_abilities(user)["delete"]: + try: + deleted_element_id = element.id + element.delete() + deleted_elements.append(deleted_element_id) + except IntegrityError as error: + error_elements.append( + { + "id": element.id, + "error": str(error), + } + ) + else: error_elements.append( { "id": element.id, - "error": str(error), + "error": _("Not authorized to delete this element"), } ) diff --git a/src/backend/joanie/core/models/certifications.py b/src/backend/joanie/core/models/certifications.py index d82015ee4..3e5332686 100644 --- a/src/backend/joanie/core/models/certifications.py +++ b/src/backend/joanie/core/models/certifications.py @@ -45,6 +45,21 @@ class Meta: def __str__(self): return self.safe_translation_getter("title", any_language=True) + + def get_abilities(self, user): + """ + Compute and return abilities for a given user taking into account + the current state of the object. + """ + + # TODO : Who can delete a CertificateDefinition? + return { + "delete": True, + "get": True, + "patch": True, + "put": True, + "set_role_to": True, + } class Certificate(BaseModel): diff --git a/src/backend/joanie/core/models/courses.py b/src/backend/joanie/core/models/courses.py index 42ad0a72e..a63d252ea 100644 --- a/src/backend/joanie/core/models/courses.py +++ b/src/backend/joanie/core/models/courses.py @@ -613,6 +613,21 @@ class CourseRun(parler_models.TranslatableModel, BaseModel): ), ) + def get_abilities(self, user): + """ + Compute and return abilities for a given user taking into account + the current state of the object. + """ + + # TODO : Who can delete a CourseRun? + return { + "delete": True, + "get": True, + "patch": True, + "put": True, + "set_role_to": True, + } + class Meta: db_table = "joanie_course_run" verbose_name = _("Course run") diff --git a/src/backend/joanie/core/models/products.py b/src/backend/joanie/core/models/products.py index d470d5a7f..396cf6602 100644 --- a/src/backend/joanie/core/models/products.py +++ b/src/backend/joanie/core/models/products.py @@ -230,6 +230,21 @@ def clean(self): ) super().clean() + def get_abilities(self, user): + """ + Compute and return abilities for a given user taking into account + the current state of the object. + """ + + # TODO : Who can delete a Product? + return { + "delete": True, + "get": True, + "patch": True, + "put": True, + "set_role_to": True, + } + class ProductTargetCourseRelation(BaseModel): """ diff --git a/src/backend/joanie/tests/core/test_api_admin_courses.py b/src/backend/joanie/tests/core/test_api_admin_courses.py index 31b837045..0f2d838a0 100644 --- a/src/backend/joanie/tests/core/test_api_admin_courses.py +++ b/src/backend/joanie/tests/core/test_api_admin_courses.py @@ -317,6 +317,8 @@ def test_admin_api_course_bulk_delete(self): admin = factories.UserFactory(is_staff=True, is_superuser=True) self.client.login(username=admin.username, password="password") courses = factories.CourseFactory.create_batch(3) + for course in courses: + factories.UserCourseAccessFactory(user=admin, course=course, role="owner") factories.CourseFactory() data = {"id": [str(course.id) for course in courses]} response = self.client.delete( @@ -328,6 +330,23 @@ def test_admin_api_course_bulk_delete(self): self.assertEqual(len(content["deleted"]), 3) self.assertFalse("error" in content) + def test_admin_api_course_bulk_delete_no_authorization(self): + """ + Unauthorized user should be able to delete multiple courses. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + courses = factories.CourseFactory.create_batch(3) + factories.CourseFactory() + data = {"id": [str(course.id) for course in courses]} + response = self.client.delete( + "/api/v1.0/admin/courses/", data=data, content_type="application/json" + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Course.objects.count(), 4) + self.assertEqual(len(content["error"]), 3) + def test_admin_api_course_bulk_delete_error(self): """ Courses with linked CourseRun cannot be deleted. @@ -335,15 +354,16 @@ def test_admin_api_course_bulk_delete_error(self): admin = factories.UserFactory(is_staff=True, is_superuser=True) self.client.login(username=admin.username, password="password") courses = factories.CourseFactory.create_batch(3) - factories.CourseFactory() factories.CourseRunFactory(course=courses[0]) + for course in courses: + factories.UserCourseAccessFactory(user=admin, course=course, role="owner") data = {"id": [str(course.id) for course in courses]} response = self.client.delete( "/api/v1.0/admin/courses/", data=data, content_type="application/json" ) content = response.json() self.assertEqual(response.status_code, 200) - self.assertEqual(models.Course.objects.count(), 2) + self.assertEqual(models.Course.objects.count(), 1) self.assertEqual(len(content["deleted"]), 2) self.assertEqual(len(content["error"]), 1) self.assertEqual(content["error"][0]["id"], str(courses[0].id)) diff --git a/src/backend/joanie/tests/core/test_api_admin_organizations.py b/src/backend/joanie/tests/core/test_api_admin_organizations.py index f7acb546d..b26ad47ca 100755 --- a/src/backend/joanie/tests/core/test_api_admin_organizations.py +++ b/src/backend/joanie/tests/core/test_api_admin_organizations.py @@ -354,6 +354,8 @@ def test_admin_api_organization_bulk_delete(self): self.client.login(username=admin.username, password="password") organizations = factories.OrganizationFactory.create_batch(3) factories.OrganizationFactory() + for organization in organizations: + factories.UserOrganizationAccessFactory(user=admin, organization=organization, role="owner") data = {"id": [str(organization.id) for organization in organizations]} response = self.client.delete( "/api/v1.0/admin/organizations/", @@ -365,3 +367,24 @@ def test_admin_api_organization_bulk_delete(self): self.assertEqual(models.Organization.objects.count(), 1) self.assertEqual(len(content["deleted"]), 3) self.assertFalse("error" in content) + + + def test_admin_api_organization_bulk_delete_no_authorization(self): + """ + Unauthorized user should be able to delete multiple organizations. + """ + admin = factories.UserFactory(is_staff=True, is_superuser=True) + self.client.login(username=admin.username, password="password") + organizations = factories.OrganizationFactory.create_batch(3) + factories.OrganizationFactory() + data = {"id": [str(organization.id) for organization in organizations]} + response = self.client.delete( + "/api/v1.0/admin/organizations/", + data=data, + content_type="application/json", + ) + content = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(models.Organization.objects.count(), 4) + self.assertEqual(len(content["error"]), 3) + self.assertFalse("deleted" in content)