refactor: [EDXOLDMNG-200] refactor CourseCertificateSerializer and fi…

…x tests

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

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,29 +292,28 @@ 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
 
 
 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

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)

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)