diff --git a/credentials/apps/api/v2/permissions.py b/credentials/apps/api/v2/permissions.py index 3f2587bfb..94d72ff3d 100644 --- a/credentials/apps/api/v2/permissions.py +++ b/credentials/apps/api/v2/permissions.py @@ -53,11 +53,10 @@ def has_permission(self, request, view): class IsAdminUserOrReadOnly(permissions.BasePermission): """ - Grants access to edit only the administrator. + Grants access to edit only the staff. + Grants read access to all users. """ - SAFE_METHODS = ('GET', 'HEAD', 'OPTIONS') - def has_permission(self, request, view): is_admin_user = request.user and (request.user.is_superuser or request.user.is_staff) - return request.method in self.SAFE_METHODS or is_admin_user + return is_admin_user or request.method in permissions.SAFE_METHODS diff --git a/credentials/apps/api/v2/serializers.py b/credentials/apps/api/v2/serializers.py index e6c259101..49b0b9ca2 100644 --- a/credentials/apps/api/v2/serializers.py +++ b/credentials/apps/api/v2/serializers.py @@ -3,9 +3,10 @@ """ import logging from pathlib import Path +from typing import Dict, Generator, List, Union from django.core.exceptions import ObjectDoesNotExist -from django.db.models.query import QuerySet +from django.core.files.uploadedfile import InMemoryUploadedFile from django.utils.datastructures import MultiValueDict from rest_framework import serializers from rest_framework.exceptions import ValidationError @@ -27,7 +28,6 @@ logger = logging.getLogger(__name__) -DATETIME_FORMAT = '%Y-%m-%dT%H:%M:%S' class CredentialField(serializers.Field): @@ -292,21 +292,21 @@ def create(self, validated_data): class SignatorySerializer(serializers.ModelSerializer): - """Serializer used to create Signatory objects.""" + """Serializer for Signatory objects.""" class Meta: model = Signatory fields = ( + "image", "name", - "title", "organization_name_override", - "image", + "title", ) def validate_image(self, value): if value: extension = Path(value.name).suffix[1:].lower() - if extension != 'png': + if extension != "png": raise ValidationError("Only PNG files can be uploaded. Please select a file ending in .png to upload.") return value @@ -314,7 +314,6 @@ def validate_image(self, value): class CourseCertificateSerializer(serializers.ModelSerializer): course_run = CourseRunField(read_only=True) certificate_available_date = serializers.DateTimeField( - format=DATETIME_FORMAT, required=False, allow_null=True, ) @@ -336,7 +335,7 @@ class Meta: ) read_only_fields = ("id", "course_run", "site") - def create(self, validated_data): + def create(self, validated_data) -> CourseCertificate: site = self.context["request"].site # A course run may not exist, but if it does, we want to find it. There may be times where course # staff will change the date on a newly created course before credentials has pulled it from the catalog. @@ -362,41 +361,38 @@ def create(self, validated_data): }, ) - cert.signatories.all().delete() + cert.signatories.clear() self.save_related_signatories(cert) return cert - def save_related_signatories(self, certificate: CourseCertificate): + def save_related_signatories(self, certificate: CourseCertificate) -> None: """ - Generates new signatures and relates them to the certificate. + Creates new signatures and relates them to the certificate. """ - raw_signatories_data = self.context["request"].data.get("signatories") - files = self.context["request"].FILES + signatories_data = self.context["request"].data.get("signatories") # pylint: disable=no-member + files = self.context["request"].FILES # pylint: disable=no-member - if raw_signatories_data and files: - signatories_parsed_data = self.parse_signatories_in_files(raw_signatories_data, files) - certificate.signatories.set(self.validate_create_signatories(signatories_parsed_data)) + if signatories_data and files: + signatories_populated_data = self.populate_signatories(signatories_data, files) + signatories = self.create_signatories(list(signatories_populated_data)) + certificate.signatories.set(signatories) @staticmethod - def validate_create_signatories(signatories_data: list) -> QuerySet[Signatory]: - """ - Validates and creates new signatures objects. - """ + def create_signatories(signatories_data: List[Dict[str, Union[str, InMemoryUploadedFile]]]) -> List[Signatory]: serializer = SignatorySerializer(data=signatories_data, many=True) serializer.is_valid(raise_exception=True) - return serializer.create(serializer.validated_data) + return serializer.save() @staticmethod - def parse_signatories_in_files(raw_signatories_data: str, files: MultiValueDict) -> list: + def populate_signatories( + signatories_data: str, + files: MultiValueDict, + ) -> Generator[Dict[str, Union[str, InMemoryUploadedFile]], None, None]: """ - Collects the data and files from the request into a list of signatories with image files. + Populates a list of signature data with files. """ - signatories = [] - - for signatory in json.loads(raw_signatories_data): + for signatory in json.loads(signatories_data): if signatory_file := files.get(signatory.get("image"), None): signatory["image"] = signatory_file - signatories.append(signatory) - - return signatories + yield signatory diff --git a/credentials/apps/api/v2/tests/test_serializers.py b/credentials/apps/api/v2/tests/test_serializers.py index d62d320b3..bbbf090e3 100644 --- a/credentials/apps/api/v2/tests/test_serializers.py +++ b/credentials/apps/api/v2/tests/test_serializers.py @@ -1,10 +1,10 @@ +import json from collections import namedtuple from datetime import datetime from logging import WARNING from uuid import uuid4 import ddt -import json import pytz from django.test import TestCase from django.urls import reverse @@ -26,11 +26,11 @@ from credentials.apps.credentials.models import CourseCertificate from credentials.apps.credentials.tests.factories import ( CourseCertificateFactory, - create_test_image, ProgramCertificateFactory, SignatoryFactory, UserCredentialAttributeFactory, UserCredentialFactory, + create_image, ) from credentials.apps.records.tests.factories import UserGradeFactory @@ -302,7 +302,6 @@ def test_course_credential(self): class SignatorySerializerTests(SiteMixin, TestCase): def test_create_signatory(self): signatory = SignatoryFactory() - actual = SignatorySerializer(signatory).data expected = { "name": signatory.name, @@ -312,9 +311,8 @@ def test_create_signatory(self): } self.assertEqual(actual, expected) - def test_create_signatory_missing_image(self): + def test_create_signatory_with_missing_image(self): signatory = SignatoryFactory(image=None) - actual = SignatorySerializer(signatory).data expected = { "name": signatory.name, @@ -325,28 +323,17 @@ def test_create_signatory_missing_image(self): self.assertEqual(actual, expected) def test_validation(self): - png_image = create_test_image("png") - data = { - "image": png_image, - "name": "signatory 1", - "organization": "edX", - "title": "title" - } + png_image = create_image("png") + data = {"image": png_image, "name": "signatory 1", "organization": "edX", "title": "title"} actual = SignatorySerializer(data=data).is_valid() - - self.assertEqual(actual, True) + self.assertTrue(actual) def test_validation_with_wrong_image_extension(self): - jpg_image = create_test_image("jpg") - data = { - "image": jpg_image, - "name": "signatory 1", - "organization": "edX", - "title": "title" - } + jpg_image = create_image("jpg") + data = {"image": jpg_image, "name": "signatory 1", "organization": "edX", "title": "title"} actual = SignatorySerializer(data=data).is_valid() - self.assertEqual(actual, False) + self.assertFalse(actual) class CourseCertificateSerializerTests(SiteMixin, TestCase): @@ -424,36 +411,29 @@ def test_create_without_course_run_raises_warning(self): ) def test_parse_signatories_in_files(self): - png_image = create_test_image("png") - signatories_data = [{ - "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image.png", - "name": "signatory 1", - "organization": "edX", - "title": "title" - }, - { - "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image+1.png", - "name": "signatory 2", - "organization": "edX", - "title": "title" - }] + png_image = create_image("png") + signatories_data = [ + { + "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image.png", + "name": "signatory 1", + "organization": "edX", + "title": "title", + }, + { + "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image+1.png", + "name": "signatory 2", + "organization": "edX", + "title": "title", + }, + ] files = { "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image.png": png_image, "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image+1.png": png_image, } - actual = CourseCertificateSerializer.parse_signatories_in_files(json.dumps(signatories_data), files) - expected = [{ - "image": png_image, - "name": "signatory 1", - "organization": "edX", - "title": "title" - }, - { - "image": png_image, - "name": "signatory 2", - "organization": "edX", - "title": "title" - }] - self.assertEqual(actual, expected) - + actual = CourseCertificateSerializer.populate_signatories(json.dumps(signatories_data), files) + expected = [ + {"image": png_image, "name": "signatory 1", "organization": "edX", "title": "title"}, + {"image": png_image, "name": "signatory 2", "organization": "edX", "title": "title"}, + ] + self.assertEqual(list(actual), expected) diff --git a/credentials/apps/api/v2/tests/test_views.py b/credentials/apps/api/v2/tests/test_views.py index debd96cb0..a8f815147 100644 --- a/credentials/apps/api/v2/tests/test_views.py +++ b/credentials/apps/api/v2/tests/test_views.py @@ -33,6 +33,7 @@ from credentials.apps.records.models import UserGrade from credentials.apps.records.tests.factories import UserGradeFactory + JSON_CONTENT_TYPE = "application/json" LOGGER_NAME = "credentials.apps.credentials.issuers" LOGGER_NAME_SERIALIZER = "credentials.apps.api.v2.serializers" @@ -701,8 +702,6 @@ def test_existing_and_non_existing_users(self): class CourseCertificateViewSetTests(SiteMixin, APITestCase): """Tests CourseCertificateViewSet""" - SERVICE_USERNAME = "test_replace_username_service_worker" - def setUp(self): super().setUp() self.user = UserFactory() @@ -718,24 +717,28 @@ def setUp(self): "certificate_available_date": self.certificate.certificate_available_date, "is_active": self.certificate.is_active, "signatories": list(self.certificate.signatories.all()), - "title": self.certificate.title + "title": self.certificate.title, } self.valid_data = { "course_id": "course-v1:edX+DemoX+Demo_Course", "certificate_type": "honor", "title": "Name of the certificate", - "signatories": json.dumps([{ - "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image.png", - "name": "signatory 1", - "organization": "edX", - "title": "title" - }, - { - "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image+1.png", - "name": "signatory 2", - "organization": "edX", - "title": "title" - }]), + "signatories": json.dumps( + [ + { + "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image.png", + "name": "signatory 1", + "organization": "edX", + "title": "title", + }, + { + "image": "/asset-v1:edX+DemoX+Demo_Course+type@asset+block@images_course_image+1.png", + "name": "signatory 2", + "organization": "edX", + "title": "title", + }, + ] + ), "is_active": True, } @@ -745,35 +748,25 @@ def authenticate_user(self, user): self.client.login(username=user.username, password=USER_PASSWORD) def test_get_for_anonymous(self): - params = { - "course_id": self.certificate.course_id, - "certificate_type": self.certificate.certificate_type - } + params = {"course_id": self.certificate.course_id, "certificate_type": self.certificate.certificate_type} response = self.client.get(self.url, data=params) - self.assertEqual(response.status_code, 200) self.assertEqual(response.data, self.certificate_data) def test_get_for_staff(self): self.authenticate_user(self.superuser) - params = { - "course_id": self.certificate.course_id, - "certificate_type": self.certificate.certificate_type - } + params = {"course_id": self.certificate.course_id, "certificate_type": self.certificate.certificate_type} response = self.client.get(self.url, data=params) - self.assertEqual(response.status_code, 200) self.assertEqual(response.data, self.certificate_data) def test_post_for_anonymous(self): response = self.client.post(self.url, data=self.valid_data) - self.assertEqual(response.status_code, 401) def test_post_for_staff(self): self.authenticate_user(self.superuser) response = self.client.post(self.url, data=self.valid_data) - self.assertEqual(response.status_code, 201) def test_delete_for_anonymous(self): @@ -784,8 +777,8 @@ def test_delete_for_anonymous(self): "signatories": [], "is_active": True, } + self.assertEqual(CourseCertificate.objects.first(), self.certificate) response = self.client.delete(self.url, data=data) - self.assertEqual(response.status_code, 401) self.assertEqual(CourseCertificate.objects.first(), self.certificate) @@ -798,7 +791,7 @@ def test_delete_for_staff(self): "signatories": [], "is_active": True, } + self.assertEqual(CourseCertificate.objects.first(), self.certificate) response = self.client.delete(self.url, data=data) - self.assertEqual(response.status_code, 204) self.assertEqual(CourseCertificate.objects.first(), None) diff --git a/credentials/apps/api/v2/views.py b/credentials/apps/api/v2/views.py index 26cb81547..f3216bb58 100644 --- a/credentials/apps/api/v2/views.py +++ b/credentials/apps/api/v2/views.py @@ -293,10 +293,9 @@ class CourseCertificateViewSet( serializer_class = CourseCertificateSerializer permission_classes = (IsAdminUserOrReadOnly,) lookup_field = "course_id" - SAFE_METHODS = ("GET", "HEAD", "OPTIONS") def get_object(self) -> CourseCertificate: - data = self.request.GET if self.request.method in self.SAFE_METHODS else self.request.data + data = self.request.GET if self.request.method in permissions.SAFE_METHODS else self.request.data return get_object_or_404( CourseCertificate, @@ -305,8 +304,5 @@ def get_object(self) -> CourseCertificate: ) def perform_destroy(self, instance: CourseCertificate) -> None: - """ - Overriden perform_destroy to delete related signatories. - """ - instance.signatories.all().delete() + instance.signatories.clear() instance.delete() diff --git a/credentials/apps/credentials/tests/factories.py b/credentials/apps/credentials/tests/factories.py index 12ae36274..1ceeb3c92 100644 --- a/credentials/apps/credentials/tests/factories.py +++ b/credentials/apps/credentials/tests/factories.py @@ -2,9 +2,8 @@ import uuid import factory -from factory.django import ImageField - from django.core.files.base import ContentFile +from factory.django import ImageField from credentials.apps.core.tests.factories import SiteFactory from credentials.apps.credentials import constants, models @@ -13,8 +12,11 @@ PASSWORD = "dummy-password" -def create_test_image(extension): - return ContentFile(ImageField()._make_data({"width": 1289, "height": 720}), f"example.{extension}") +def create_image(extension: str) -> ContentFile: + return ContentFile( + ImageField()._make_data({"width": 1289, "height": 720}), # pylint: disable=protected-access + f"example.{extension}", + ) class AbstractCertificateFactory(factory.django.DjangoModelFactory): @@ -73,4 +75,4 @@ class Meta: name = factory.Faker("name") title = factory.Faker("job") - image = factory.LazyAttribute(lambda _: create_test_image("png")) + image = factory.LazyAttribute(lambda _: create_image("png"))