From 53c119f554e362a7a26379e8815679bd42194237 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Tue, 15 Jul 2025 19:30:01 +0200 Subject: [PATCH 1/7] Add action to resolve Community Library Submissions --- .../constants/community_library_submission.py | 16 + ...communitylibrarysubmission_admin_fields.py | 74 ++++ contentcuration/contentcuration/models.py | 15 + .../test_community_library_submission.py | 384 ++++++++++++++++++ contentcuration/contentcuration/urls.py | 8 + .../viewsets/community_library_submission.py | 132 ++++++ 6 files changed, 629 insertions(+) create mode 100644 contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py diff --git a/contentcuration/contentcuration/constants/community_library_submission.py b/contentcuration/contentcuration/constants/community_library_submission.py index 640b804f92..4e02bc5f6e 100644 --- a/contentcuration/contentcuration/constants/community_library_submission.py +++ b/contentcuration/contentcuration/constants/community_library_submission.py @@ -1,11 +1,27 @@ STATUS_PENDING = "PENDING" STATUS_APPROVED = "APPROVED" STATUS_REJECTED = "REJECTED" +STATUS_SUPERSEDED = "SUPERSEDED" STATUS_LIVE = "LIVE" status_choices = ( (STATUS_PENDING, "Pending"), (STATUS_APPROVED, "Approved"), (STATUS_REJECTED, "Rejected"), + (STATUS_SUPERSEDED, "Superseded"), (STATUS_LIVE, "Live"), ) + +REASON_INVALID_LICENSING = "INVALID_LICENSING" +REASON_TECHNICAL_QUALITY_ASSURANCE = "TECHNICAL_QUALITY_ASSURANCE" +REASON_INVALID_METADATA = "INVALID_METADATA" +REASON_PORTABILITY_ISSUES = "PORTABILITY_ISSUES" +REASON_OTHER = "OTHER" + +resolution_reason_choices = ( + (REASON_INVALID_LICENSING, "Invalid Licensing"), + (REASON_TECHNICAL_QUALITY_ASSURANCE, "Technical Quality Assurance"), + (REASON_INVALID_METADATA, "Invalid Metadata"), + (REASON_PORTABILITY_ISSUES, "Portability Issues"), + (REASON_OTHER, "Other"), +) diff --git a/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py b/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py new file mode 100644 index 0000000000..e88594c47a --- /dev/null +++ b/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py @@ -0,0 +1,74 @@ +# Generated by Django 3.2.24 on 2025-07-15 19:12 +import django.db.models.deletion +from django.conf import settings +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ( + "contentcuration", + "0155_communitylibrarysubmission_submission_date_created_idx", + ), + ] + + operations = [ + migrations.AddField( + model_name="communitylibrarysubmission", + name="date_resolved", + field=models.DateTimeField(blank=True, null=True), + ), + migrations.AddField( + model_name="communitylibrarysubmission", + name="feedback_notes", + field=models.TextField(blank=True, null=True), + ), + migrations.AddField( + model_name="communitylibrarysubmission", + name="internal_notes", + field=models.TextField(blank=True, null=True), + ), + migrations.AddField( + model_name="communitylibrarysubmission", + name="resolution_reason", + field=models.CharField( + choices=[ + ("INVALID_LICENSING", "Invalid Licensing"), + ("TECHNICAL_QUALITY_ASSURANCE", "Technical Quality Assurance"), + ("INVALID_METADATA", "Invalid Metadata"), + ("PORTABILITY_ISSUES", "Portability Issues"), + ("OTHER", "Other"), + ], + max_length=50, + null=True, + ), + ), + migrations.AddField( + model_name="communitylibrarysubmission", + name="resolved_by", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="resolved_community_library_submissions", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterField( + model_name="communitylibrarysubmission", + name="status", + field=models.CharField( + choices=[ + ("PENDING", "Pending"), + ("APPROVED", "Approved"), + ("REJECTED", "Rejected"), + ("SUPERSEDED", "Superseded"), + ("LIVE", "Live"), + ], + default="PENDING", + max_length=20, + ), + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index d174261fa8..f0c7305962 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -2558,11 +2558,26 @@ class CommunityLibrarySubmission(models.Model): ) categories = models.JSONField(blank=True, null=True) date_created = models.DateTimeField(auto_now_add=True) + date_resolved = models.DateTimeField(blank=True, null=True) status = models.CharField( max_length=20, choices=community_library_submission.status_choices, default=community_library_submission.STATUS_PENDING, ) + resolved_by = models.ForeignKey( + User, + related_name="resolved_community_library_submissions", + blank=True, + null=True, + on_delete=models.SET_NULL, + ) + resolution_reason = models.CharField( + max_length=50, + choices=community_library_submission.resolution_reason_choices, + null=True, + ) + feedback_notes = models.TextField(blank=True, null=True) + internal_notes = models.TextField(blank=True, null=True) def save(self, *args, **kwargs): # Validate on save that the submission author is an editor of the channel diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index d4de9e7d6b..cf21d9534a 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -1,7 +1,13 @@ +import datetime +from unittest import mock from urllib.parse import urlencode +import pytz from django.urls import reverse +from contentcuration.constants import ( + community_library_submission as community_library_submission_constants, +) from contentcuration.models import CommunityLibrarySubmission from contentcuration.tests import testdata from contentcuration.tests.base import StudioAPITestCase @@ -422,3 +428,381 @@ def test_delete_submission__is_forbidden(self): id=self.existing_submission1.id ).exists() ) + + +class AdminViewSetTestCase(StudioAPITestCase): + def setUp(self): + super().setUp() + + self.submission = testdata.community_library_submission() + self.submission.channel.version = 3 + self.submission.channel.save() + self.submission.channel_version = 2 + self.submission.save() + + self.editor_user = self.submission.channel.editors.first() + + self.superseded_submission = CommunityLibrarySubmission.objects.create( + channel=self.submission.channel, + author=self.editor_user, + status=community_library_submission_constants.STATUS_PENDING, + date_created=datetime.datetime(2023, 1, 1, tzinfo=pytz.utc), + channel_version=1, + ) + self.not_superseded_submission = CommunityLibrarySubmission.objects.create( + channel=self.submission.channel, + author=self.editor_user, + status=community_library_submission_constants.STATUS_PENDING, + date_created=datetime.datetime(2024, 1, 1, tzinfo=pytz.utc), + channel_version=3, + ) + self.submission_for_other_channel = testdata.community_library_submission() + self.submission_for_other_channel.channel_version = 1 + self.submission_for_other_channel.save() + + self.feedback_notes = "Feedback" + self.internal_notes = "Internal notes" + + self.resolve_approve_metadata = { + "status": community_library_submission_constants.STATUS_APPROVED, + "feedback_notes": self.feedback_notes, + "internal_notes": self.internal_notes, + } + self.resolve_reject_metadata = { + "status": community_library_submission_constants.STATUS_REJECTED, + "resolution_reason": community_library_submission_constants.REASON_INVALID_METADATA, + "feedback_notes": self.feedback_notes, + "internal_notes": self.internal_notes, + } + + self.resolved_time = datetime.datetime(2023, 10, 1, tzinfo=pytz.utc) + self.patcher = mock.patch( + "contentcuration.viewsets.community_library_submission.timezoned_datetime_now", + return_value=self.resolved_time, + ) + self.mock_datetime = self.patcher.start() + + def tearDown(self): + self.patcher.stop() + super().tearDown() + + def _manually_resolve_submission(self): + self.submission.status = community_library_submission_constants.STATUS_REJECTED + self.submission.resolved_by = self.admin_user + self.submission.resolution_reason = ( + community_library_submission_constants.REASON_INVALID_METADATA + ) + self.submission.feedback_notes = self.feedback_notes + self.submission.internal_notes = self.internal_notes + self.submission.date_resolved = self.resolved_time + self.submission.save() + + def _refresh_submissions_from_db(self): + self.submission.refresh_from_db() + self.superseded_submission.refresh_from_db() + self.not_superseded_submission.refresh_from_db() + self.submission_for_other_channel.refresh_from_db() + + def test_list_submissions__admin(self): + self.client.force_authenticate(user=self.admin_user) + + self._manually_resolve_submission() + + response = self.client.get( + reverse("community-library-submission-admin-list"), + ) + self.assertEqual(response.status_code, 200, response.content) + + results = response.data + self.assertEqual(len(results), 4) + rejected_results = [ + result + for result in results + if result["status"] + == community_library_submission_constants.STATUS_REJECTED + ] + self.assertEqual(len(rejected_results), 1) + result = rejected_results[0] + + self.assertEqual(result["resolved_by_id"], self.admin_user.id) + self.assertEqual(result["resolved_by_first_name"], self.admin_user.first_name) + self.assertEqual(result["resolved_by_last_name"], self.admin_user.last_name) + self.assertEqual(result["internal_notes"], self.internal_notes) + + def test_list_submissions__editor(self): + self.client.force_authenticate(user=self.editor_user) + + self._manually_resolve_submission() + + response = self.client.get( + reverse("community-library-submission-admin-list"), + ) + self.assertEqual(response.status_code, 403, response.content) + + def test_submission_detail__admin(self): + self.client.force_authenticate(user=self.admin_user) + + self._manually_resolve_submission() + + response = self.client.get( + reverse( + "community-library-submission-admin-detail", + args=[self.submission.id], + ), + ) + self.assertEqual(response.status_code, 200, response.content) + + result = response.data + self.assertEqual(result["id"], self.submission.id) + self.assertEqual(result["resolved_by_id"], self.admin_user.id) + self.assertEqual(result["resolved_by_first_name"], self.admin_user.first_name) + self.assertEqual(result["resolved_by_last_name"], self.admin_user.last_name) + self.assertEqual(result["internal_notes"], self.internal_notes) + + def test_submission_detail__editor(self): + self.client.force_authenticate(user=self.editor_user) + + self._manually_resolve_submission() + + response = self.client.get( + reverse( + "community-library-submission-admin-detail", + args=[self.submission.id], + ), + ) + self.assertEqual(response.status_code, 403, response.content) + + def test_update_submission(self): + self.client.force_authenticate(user=self.admin_user) + + response = self.client.put( + reverse( + "community-library-submission-admin-detail", + args=[self.submission.id], + ), + {}, + format="json", + ) + self.assertEqual(response.status_code, 405, response.content) + + def test_partial_update_submission(self): + self.client.force_authenticate(user=self.admin_user) + + response = self.client.patch( + reverse( + "community-library-submission-admin-detail", + args=[self.submission.id], + ), + {}, + format="json", + ) + self.assertEqual(response.status_code, 405, response.content) + + def test_destroy_submission(self): + self.client.force_authenticate(user=self.admin_user) + + response = self.client.delete( + reverse( + "community-library-submission-admin-detail", + args=[self.submission.id], + ), + format="json", + ) + self.assertEqual(response.status_code, 405, response.content) + + def test_resolve_submission__editor(self): + self.client.force_authenticate(user=self.editor_user) + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_approve_metadata, + format="json", + ) + self.assertEqual(response.status_code, 403, response.content) + + def test_resolve_submission__accept_correct(self): + self.client.force_authenticate(user=self.admin_user) + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_approve_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + resolved_submission = CommunityLibrarySubmission.objects.get( + id=self.submission.id + ) + self.assertEqual( + resolved_submission.status, + community_library_submission_constants.STATUS_APPROVED, + ) + self.assertEqual(resolved_submission.feedback_notes, self.feedback_notes) + self.assertEqual(resolved_submission.internal_notes, self.internal_notes) + self.assertEqual(resolved_submission.resolved_by, self.admin_user) + self.assertEqual(resolved_submission.date_resolved, self.resolved_time) + + def test_resolve_submission__reject_correct(self): + self.client.force_authenticate(user=self.admin_user) + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_reject_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + resolved_submission = CommunityLibrarySubmission.objects.get( + id=self.submission.id + ) + self.assertEqual( + resolved_submission.status, + community_library_submission_constants.STATUS_REJECTED, + ) + self.assertEqual( + resolved_submission.resolution_reason, + community_library_submission_constants.REASON_INVALID_METADATA, + ) + self.assertEqual(resolved_submission.feedback_notes, self.feedback_notes) + self.assertEqual(resolved_submission.internal_notes, self.internal_notes) + self.assertEqual(resolved_submission.resolved_by, self.admin_user) + self.assertEqual(resolved_submission.date_resolved, self.resolved_time) + + def test_resolve_submission__reject_missing_resolution_reason(self): + self.client.force_authenticate(user=self.admin_user) + metadata = self.resolve_reject_metadata.copy() + del metadata["resolution_reason"] + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + metadata, + format="json", + ) + self.assertEqual(response.status_code, 400, response.content) + + def test_resolve_submission__reject_missing_feedback_notes(self): + self.client.force_authenticate(user=self.admin_user) + metadata = self.resolve_reject_metadata.copy() + del metadata["feedback_notes"] + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + metadata, + format="json", + ) + self.assertEqual(response.status_code, 400, response.content) + + def test_resolve_submission__invalid_status(self): + self.client.force_authenticate(user=self.admin_user) + metadata = self.resolve_approve_metadata.copy() + metadata["status"] = (community_library_submission_constants.STATUS_PENDING,) + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + metadata, + format="json", + ) + self.assertEqual(response.status_code, 400, response.content) + + def test_resolve_submission__not_pending(self): + self.client.force_authenticate(user=self.admin_user) + self.submission.status = community_library_submission_constants.STATUS_APPROVED + self.submission.save() + + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_approve_metadata, + format="json", + ) + self.assertEqual(response.status_code, 400, response.content) + + def test_resolve_submission__overrite_categories(self): + self.client.force_authenticate(user=self.admin_user) + categories = ["Category 1"] + self.resolve_approve_metadata["categories"] = categories + + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_approve_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + resolved_submission = CommunityLibrarySubmission.objects.get( + id=self.submission.id + ) + self.assertListEqual(resolved_submission.categories, categories) + + def test_resolve_submission__accept_mark_superseded(self): + self.client.force_authenticate(user=self.admin_user) + + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_approve_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + self._refresh_submissions_from_db() + + self.assertEqual( + self.superseded_submission.status, + community_library_submission_constants.STATUS_SUPERSEDED, + ) + self.assertEqual( + self.not_superseded_submission.status, + community_library_submission_constants.STATUS_PENDING, + ) + self.assertEqual( + self.submission_for_other_channel.status, + community_library_submission_constants.STATUS_PENDING, + ) + + def test_resolve_submission__reject_do_not_mark_superseded(self): + self.client.force_authenticate(user=self.admin_user) + + response = self.client.post( + reverse( + "community-library-submission-admin-resolve", + args=[self.submission.id], + ), + self.resolve_reject_metadata, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + + self._refresh_submissions_from_db() + + self.assertEqual( + self.superseded_submission.status, + community_library_submission_constants.STATUS_PENDING, + ) + self.assertEqual( + self.not_superseded_submission.status, + community_library_submission_constants.STATUS_PENDING, + ) + self.assertEqual( + self.submission_for_other_channel.status, + community_library_submission_constants.STATUS_PENDING, + ) diff --git a/contentcuration/contentcuration/urls.py b/contentcuration/contentcuration/urls.py index b948effc8b..7216eb972d 100644 --- a/contentcuration/contentcuration/urls.py +++ b/contentcuration/contentcuration/urls.py @@ -38,6 +38,9 @@ from contentcuration.viewsets.channel import ChannelViewSet from contentcuration.viewsets.channelset import ChannelSetViewSet from contentcuration.viewsets.clipboard import ClipboardViewSet +from contentcuration.viewsets.community_library_submission import ( + CommunityLibrarySubmissionAdminViewSet, +) from contentcuration.viewsets.community_library_submission import ( CommunityLibrarySubmissionViewSet, ) @@ -88,6 +91,11 @@ def get_redirect_url(self, *args, **kwargs): CommunityLibrarySubmissionViewSet, basename="community-library-submission", ) +router.register( + r"communitylibrary_submission_admin", + CommunityLibrarySubmissionAdminViewSet, + basename="community-library-submission-admin", +) urlpatterns = [ re_path(r"^api/", include(router.urls)), diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 2aef31bfba..7610ae323e 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -1,8 +1,16 @@ +import datetime + from django_filters.rest_framework import DjangoFilterBackend +from rest_framework.decorators import action +from rest_framework.exceptions import MethodNotAllowed from rest_framework.permissions import IsAuthenticated from rest_framework.relations import PrimaryKeyRelatedField +from rest_framework.response import Response from rest_framework.serializers import ValidationError +from contentcuration.constants import ( + community_library_submission as community_library_submission_constants, +) from contentcuration.models import Channel from contentcuration.models import CommunityLibrarySubmission from contentcuration.models import Country @@ -14,6 +22,7 @@ from contentcuration.viewsets.base import RESTDestroyModelMixin from contentcuration.viewsets.base import RESTUpdateModelMixin from contentcuration.viewsets.common import UserFilteredPrimaryKeyRelatedField +from contentcuration.viewsets.user import IsAdminUser class CommunityLibrarySubmissionSerializer(BulkModelSerializer): @@ -82,6 +91,62 @@ def update(self, instance, validated_data): return super().update(instance, validated_data) +def timezoned_datetime_now(): + """ + Simple wrapper around datetime.datetime.now. The point of using a wrapper + is that mocking datetime.datetime in tests runs into issues with + isinstance in the serializer, and it is a lot easier to just mock this + wrapper. + """ + return datetime.datetime.now(datetime.timezone.utc) + + +class CommunityLibrarySubmissionResolveSerializer(CommunityLibrarySubmissionSerializer): + class Meta(CommunityLibrarySubmissionSerializer.Meta): + fields = CommunityLibrarySubmissionSerializer.Meta.fields + [ + "status", + "resolution_reason", + "feedback_notes", + "internal_notes", + ] + + def create(self, validated_data): + raise ValidationError( + "Cannot create a community library submission with this serializer. " + "Use the standard CommunityLibrarySubmissionSerializer instead." + ) + + def update(self, instance, validated_data): + if instance.status != community_library_submission_constants.STATUS_PENDING: + raise ValidationError( + "Cannot resolve a community library submission that is not pending." + ) + + if "status" not in validated_data or validated_data["status"] not in [ + community_library_submission_constants.STATUS_APPROVED, + community_library_submission_constants.STATUS_REJECTED, + ]: + raise ValidationError( + "Status must be either APPROVED or REJECTED when resolving a submission." + ) + + if ( + "status" not in validated_data + or validated_data["status"] + == community_library_submission_constants.STATUS_REJECTED + ): + if "resolution_reason" not in validated_data: + raise ValidationError( + "Resolution reason must be provided when rejecting a submission." + ) + if "feedback_notes" not in validated_data: + raise ValidationError( + "Feedback notes must be provided when rejecting a submission." + ) + + return super().update(instance, validated_data) + + class CommunityLibrarySubmissionPagination(ValuesViewsetCursorPagination): ordering = "-date_created" page_size_query_param = "max_results" @@ -105,6 +170,9 @@ class CommunityLibrarySubmissionViewSet( "categories", "date_created", "status", + "resolution_reason", + "feedback_notes", + "date_resolved", ) field_map = { "author_first_name": "author__first_name", @@ -130,3 +198,67 @@ def consolidate(self, items, queryset): item["countries"] = countries.get(item["id"], []) return items + + +class CommunityLibrarySubmissionAdminViewSet(CommunityLibrarySubmissionViewSet): + permission_classes = [IsAdminUser] + + values = CommunityLibrarySubmissionViewSet.values + ( + "resolved_by_id", + "resolved_by__first_name", + "resolved_by__last_name", + "internal_notes", + ) + field_map = CommunityLibrarySubmissionViewSet.field_map.copy() + field_map.update( + { + "resolved_by_first_name": "resolved_by__first_name", + "resolved_by_last_name": "resolved_by__last_name", + } + ) + + def create(self, request, *args, **kwargs): + raise MethodNotAllowed( + "Cannot create a community library submission with this viewset. " + "Use the standard CommunityLibrarySubmissionViewSet instead." + ) + + def update(self, instance, *args, **kwargs): + raise MethodNotAllowed( + "Cannot update a community library submission with this viewset. " + "Use the standard CommunityLibrarySubmissionViewSet instead." + ) + + def destroy(self, instance, *args, **kwargs): + raise MethodNotAllowed( + "Cannot delete a community library submission with this viewset. " + "Use the standard CommunityLibrarySubmissionViewSet instead." + ) + + def _mark_previous_pending_submissions_as_superseded(self, submission): + CommunityLibrarySubmission.objects.filter( + status=community_library_submission_constants.STATUS_PENDING, + channel=submission.channel, + channel_version__lt=submission.channel_version, + ).update(status=community_library_submission_constants.STATUS_SUPERSEDED) + + @action( + methods=["post"], + detail=True, + serializer_class=CommunityLibrarySubmissionResolveSerializer, + ) + def resolve(self, request, pk=None): + instance = self.get_edit_object() + serializer = self.get_serializer(instance, data=request.data, partial=True) + serializer.is_valid(raise_exception=True) + + date_resolved = timezoned_datetime_now() + submission = serializer.save( + date_resolved=date_resolved, + resolved_by=request.user, + ) + + if submission.status == community_library_submission_constants.STATUS_APPROVED: + self._mark_previous_pending_submissions_as_superseded(submission) + + return Response(serializer.data) From c6d22070a5ed08d951587fa15ea68928f53597b5 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Tue, 15 Jul 2025 22:17:44 +0200 Subject: [PATCH 2/7] Allow resolution reason to be blank --- ...nitylibrarysubmission_resolution_reason.py | 29 +++++++++++++++++++ contentcuration/contentcuration/models.py | 1 + 2 files changed, 30 insertions(+) create mode 100644 contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py diff --git a/contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py b/contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py new file mode 100644 index 0000000000..09a4f7d190 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py @@ -0,0 +1,29 @@ +# Generated by Django 3.2.24 on 2025-07-15 20:11 +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("contentcuration", "0156_communitylibrarysubmission_admin_fields"), + ] + + operations = [ + migrations.AlterField( + model_name="communitylibrarysubmission", + name="resolution_reason", + field=models.CharField( + blank=True, + choices=[ + ("INVALID_LICENSING", "Invalid Licensing"), + ("TECHNICAL_QUALITY_ASSURANCE", "Technical Quality Assurance"), + ("INVALID_METADATA", "Invalid Metadata"), + ("PORTABILITY_ISSUES", "Portability Issues"), + ("OTHER", "Other"), + ], + max_length=50, + null=True, + ), + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index f0c7305962..b62441402a 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -2574,6 +2574,7 @@ class CommunityLibrarySubmission(models.Model): resolution_reason = models.CharField( max_length=50, choices=community_library_submission.resolution_reason_choices, + blank=True, null=True, ) feedback_notes = models.TextField(blank=True, null=True) From 9cddb63f73882d39f60bdc833480a6d5d172a162 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 21 Jul 2025 15:50:55 +0200 Subject: [PATCH 3/7] Address PR feedback --- ...communitylibrarysubmission_admin_fields.py | 1 + .../test_community_library_submission.py | 64 +++++++------ contentcuration/contentcuration/urls.py | 10 +- .../viewsets/community_library_submission.py | 96 +++++++++---------- 4 files changed, 84 insertions(+), 87 deletions(-) diff --git a/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py b/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py index e88594c47a..e7af358f71 100644 --- a/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py +++ b/contentcuration/contentcuration/migrations/0156_communitylibrarysubmission_admin_fields.py @@ -34,6 +34,7 @@ class Migration(migrations.Migration): model_name="communitylibrarysubmission", name="resolution_reason", field=models.CharField( + blank=True, choices=[ ("INVALID_LICENSING", "Invalid Licensing"), ("TECHNICAL_QUALITY_ASSURANCE", "Technical Quality Assurance"), diff --git a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py index cf21d9534a..4d2808658f 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py +++ b/contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py @@ -282,8 +282,10 @@ def test_get_single_submission__author_name(self): self.assertEqual(response.status_code, 200, response.content) result = response.data - self.assertEqual(result["author_first_name"], self.author_user.first_name) - self.assertEqual(result["author_last_name"], self.author_user.last_name) + self.assertEqual( + result["author_name"], + f"{self.author_user.first_name} {self.author_user.last_name}", + ) def test_update_submission__is_author(self): self.client.force_authenticate(user=self.author_user) @@ -477,7 +479,7 @@ def setUp(self): self.resolved_time = datetime.datetime(2023, 10, 1, tzinfo=pytz.utc) self.patcher = mock.patch( - "contentcuration.viewsets.community_library_submission.timezoned_datetime_now", + "contentcuration.viewsets.community_library_submission.timezone.now", return_value=self.resolved_time, ) self.mock_datetime = self.patcher.start() @@ -486,7 +488,7 @@ def tearDown(self): self.patcher.stop() super().tearDown() - def _manually_resolve_submission(self): + def _manually_reject_submission(self): self.submission.status = community_library_submission_constants.STATUS_REJECTED self.submission.resolved_by = self.admin_user self.submission.resolution_reason = ( @@ -506,10 +508,10 @@ def _refresh_submissions_from_db(self): def test_list_submissions__admin(self): self.client.force_authenticate(user=self.admin_user) - self._manually_resolve_submission() + self._manually_reject_submission() response = self.client.get( - reverse("community-library-submission-admin-list"), + reverse("admin-community-library-submission-list"), ) self.assertEqual(response.status_code, 200, response.content) @@ -525,28 +527,30 @@ def test_list_submissions__admin(self): result = rejected_results[0] self.assertEqual(result["resolved_by_id"], self.admin_user.id) - self.assertEqual(result["resolved_by_first_name"], self.admin_user.first_name) - self.assertEqual(result["resolved_by_last_name"], self.admin_user.last_name) + self.assertEqual( + result["resolved_by_name"], + f"{self.admin_user.first_name} {self.admin_user.last_name}", + ) self.assertEqual(result["internal_notes"], self.internal_notes) def test_list_submissions__editor(self): self.client.force_authenticate(user=self.editor_user) - self._manually_resolve_submission() + self._manually_reject_submission() response = self.client.get( - reverse("community-library-submission-admin-list"), + reverse("admin-community-library-submission-list"), ) self.assertEqual(response.status_code, 403, response.content) def test_submission_detail__admin(self): self.client.force_authenticate(user=self.admin_user) - self._manually_resolve_submission() + self._manually_reject_submission() response = self.client.get( reverse( - "community-library-submission-admin-detail", + "admin-community-library-submission-detail", args=[self.submission.id], ), ) @@ -555,18 +559,20 @@ def test_submission_detail__admin(self): result = response.data self.assertEqual(result["id"], self.submission.id) self.assertEqual(result["resolved_by_id"], self.admin_user.id) - self.assertEqual(result["resolved_by_first_name"], self.admin_user.first_name) - self.assertEqual(result["resolved_by_last_name"], self.admin_user.last_name) + self.assertEqual( + result["resolved_by_name"], + f"{self.admin_user.first_name} {self.admin_user.last_name}", + ) self.assertEqual(result["internal_notes"], self.internal_notes) def test_submission_detail__editor(self): self.client.force_authenticate(user=self.editor_user) - self._manually_resolve_submission() + self._manually_reject_submission() response = self.client.get( reverse( - "community-library-submission-admin-detail", + "admin-community-library-submission-detail", args=[self.submission.id], ), ) @@ -577,7 +583,7 @@ def test_update_submission(self): response = self.client.put( reverse( - "community-library-submission-admin-detail", + "admin-community-library-submission-detail", args=[self.submission.id], ), {}, @@ -590,7 +596,7 @@ def test_partial_update_submission(self): response = self.client.patch( reverse( - "community-library-submission-admin-detail", + "admin-community-library-submission-detail", args=[self.submission.id], ), {}, @@ -603,7 +609,7 @@ def test_destroy_submission(self): response = self.client.delete( reverse( - "community-library-submission-admin-detail", + "admin-community-library-submission-detail", args=[self.submission.id], ), format="json", @@ -614,7 +620,7 @@ def test_resolve_submission__editor(self): self.client.force_authenticate(user=self.editor_user) response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_approve_metadata, @@ -626,7 +632,7 @@ def test_resolve_submission__accept_correct(self): self.client.force_authenticate(user=self.admin_user) response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_approve_metadata, @@ -650,7 +656,7 @@ def test_resolve_submission__reject_correct(self): self.client.force_authenticate(user=self.admin_user) response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_reject_metadata, @@ -680,7 +686,7 @@ def test_resolve_submission__reject_missing_resolution_reason(self): del metadata["resolution_reason"] response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), metadata, @@ -694,7 +700,7 @@ def test_resolve_submission__reject_missing_feedback_notes(self): del metadata["feedback_notes"] response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), metadata, @@ -708,7 +714,7 @@ def test_resolve_submission__invalid_status(self): metadata["status"] = (community_library_submission_constants.STATUS_PENDING,) response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), metadata, @@ -723,7 +729,7 @@ def test_resolve_submission__not_pending(self): response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_approve_metadata, @@ -738,7 +744,7 @@ def test_resolve_submission__overrite_categories(self): response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_approve_metadata, @@ -756,7 +762,7 @@ def test_resolve_submission__accept_mark_superseded(self): response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_approve_metadata, @@ -784,7 +790,7 @@ def test_resolve_submission__reject_do_not_mark_superseded(self): response = self.client.post( reverse( - "community-library-submission-admin-resolve", + "admin-community-library-submission-resolve", args=[self.submission.id], ), self.resolve_reject_metadata, diff --git a/contentcuration/contentcuration/urls.py b/contentcuration/contentcuration/urls.py index 7216eb972d..7ccb5b53a5 100644 --- a/contentcuration/contentcuration/urls.py +++ b/contentcuration/contentcuration/urls.py @@ -39,7 +39,7 @@ from contentcuration.viewsets.channelset import ChannelSetViewSet from contentcuration.viewsets.clipboard import ClipboardViewSet from contentcuration.viewsets.community_library_submission import ( - CommunityLibrarySubmissionAdminViewSet, + AdminCommunityLibrarySubmissionViewSet, ) from contentcuration.viewsets.community_library_submission import ( CommunityLibrarySubmissionViewSet, @@ -87,14 +87,14 @@ def get_redirect_url(self, *args, **kwargs): basename="recommendations-interaction", ) router.register( - r"communitylibrary_submission", + r"communitylibrary-submission", CommunityLibrarySubmissionViewSet, basename="community-library-submission", ) router.register( - r"communitylibrary_submission_admin", - CommunityLibrarySubmissionAdminViewSet, - basename="community-library-submission-admin", + r"admin_communitylibrary_submission", + AdminCommunityLibrarySubmissionViewSet, + basename="admin-community-library-submission", ) urlpatterns = [ diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 7610ae323e..6d45e0c4e4 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -1,8 +1,6 @@ -import datetime - +from django.utils import timezone from django_filters.rest_framework import DjangoFilterBackend from rest_framework.decorators import action -from rest_framework.exceptions import MethodNotAllowed from rest_framework.permissions import IsAuthenticated from rest_framework.relations import PrimaryKeyRelatedField from rest_framework.response import Response @@ -91,16 +89,6 @@ def update(self, instance, validated_data): return super().update(instance, validated_data) -def timezoned_datetime_now(): - """ - Simple wrapper around datetime.datetime.now. The point of using a wrapper - is that mocking datetime.datetime in tests runs into issues with - isinstance in the serializer, and it is a lot easier to just mock this - wrapper. - """ - return datetime.datetime.now(datetime.timezone.utc) - - class CommunityLibrarySubmissionResolveSerializer(CommunityLibrarySubmissionSerializer): class Meta(CommunityLibrarySubmissionSerializer.Meta): fields = CommunityLibrarySubmissionSerializer.Meta.fields + [ @@ -135,11 +123,11 @@ def update(self, instance, validated_data): or validated_data["status"] == community_library_submission_constants.STATUS_REJECTED ): - if "resolution_reason" not in validated_data: + if not validated_data.get("resolution_reason", "").strip(): raise ValidationError( "Resolution reason must be provided when rejecting a submission." ) - if "feedback_notes" not in validated_data: + if not validated_data.get("feedback_notes", "").strip(): raise ValidationError( "Feedback notes must be provided when rejecting a submission." ) @@ -153,12 +141,16 @@ class CommunityLibrarySubmissionPagination(ValuesViewsetCursorPagination): max_page_size = 100 -class CommunityLibrarySubmissionViewSet( - RESTCreateModelMixin, - RESTUpdateModelMixin, - RESTDestroyModelMixin, - ReadOnlyValuesViewset, -): +def get_author_name(item): + return "{} {}".format(item["author__first_name"], item["author__last_name"]) + + +class CommunityLibrarySubmissionViewSetMixin: + """ + Mixin with logic shared between the CommunityLibrarySubmissionViewSet and + AdminCommunityLibrarySubmissionViewSet. + """ + values = ( "id", "description", @@ -175,14 +167,8 @@ class CommunityLibrarySubmissionViewSet( "date_resolved", ) field_map = { - "author_first_name": "author__first_name", - "author_last_name": "author__last_name", + "author_name": get_author_name, } - filter_backends = [DjangoFilterBackend] - filterset_fields = ["channel"] - permission_classes = [IsAuthenticated] - pagination_class = CommunityLibrarySubmissionPagination - serializer_class = CommunityLibrarySubmissionSerializer queryset = CommunityLibrarySubmission.objects.all().order_by("-date_created") def consolidate(self, items, queryset): @@ -200,41 +186,45 @@ def consolidate(self, items, queryset): return items -class CommunityLibrarySubmissionAdminViewSet(CommunityLibrarySubmissionViewSet): +class CommunityLibrarySubmissionViewSet( + CommunityLibrarySubmissionViewSetMixin, + RESTCreateModelMixin, + RESTUpdateModelMixin, + RESTDestroyModelMixin, + ReadOnlyValuesViewset, +): + filter_backends = [DjangoFilterBackend] + filterset_fields = ["channel"] + permission_classes = [IsAuthenticated] + pagination_class = CommunityLibrarySubmissionPagination + serializer_class = CommunityLibrarySubmissionSerializer + + +def get_resolved_by_name(item): + return "{} {}".format( + item["resolved_by__first_name"], item["resolved_by__last_name"] + ) + + +class AdminCommunityLibrarySubmissionViewSet( + CommunityLibrarySubmissionViewSetMixin, + ReadOnlyValuesViewset, +): permission_classes = [IsAdminUser] - values = CommunityLibrarySubmissionViewSet.values + ( + values = CommunityLibrarySubmissionViewSetMixin.values + ( "resolved_by_id", "resolved_by__first_name", "resolved_by__last_name", "internal_notes", ) - field_map = CommunityLibrarySubmissionViewSet.field_map.copy() + field_map = CommunityLibrarySubmissionViewSetMixin.field_map.copy() field_map.update( { - "resolved_by_first_name": "resolved_by__first_name", - "resolved_by_last_name": "resolved_by__last_name", + "resolved_by_name": get_resolved_by_name, } ) - def create(self, request, *args, **kwargs): - raise MethodNotAllowed( - "Cannot create a community library submission with this viewset. " - "Use the standard CommunityLibrarySubmissionViewSet instead." - ) - - def update(self, instance, *args, **kwargs): - raise MethodNotAllowed( - "Cannot update a community library submission with this viewset. " - "Use the standard CommunityLibrarySubmissionViewSet instead." - ) - - def destroy(self, instance, *args, **kwargs): - raise MethodNotAllowed( - "Cannot delete a community library submission with this viewset. " - "Use the standard CommunityLibrarySubmissionViewSet instead." - ) - def _mark_previous_pending_submissions_as_superseded(self, submission): CommunityLibrarySubmission.objects.filter( status=community_library_submission_constants.STATUS_PENDING, @@ -252,7 +242,7 @@ def resolve(self, request, pk=None): serializer = self.get_serializer(instance, data=request.data, partial=True) serializer.is_valid(raise_exception=True) - date_resolved = timezoned_datetime_now() + date_resolved = timezone.now() submission = serializer.save( date_resolved=date_resolved, resolved_by=request.user, @@ -261,4 +251,4 @@ def resolve(self, request, pk=None): if submission.status == community_library_submission_constants.STATUS_APPROVED: self._mark_previous_pending_submissions_as_superseded(submission) - return Response(serializer.data) + return Response(self.serialize_object(pk=submission.id)) From ff53d60c24dc3ff15f029d8d17fbb0a153ac2227 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 21 Jul 2025 19:02:07 +0200 Subject: [PATCH 4/7] Use the same case for both community library routes --- contentcuration/contentcuration/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/urls.py b/contentcuration/contentcuration/urls.py index 7ccb5b53a5..f65d9fa35c 100644 --- a/contentcuration/contentcuration/urls.py +++ b/contentcuration/contentcuration/urls.py @@ -87,7 +87,7 @@ def get_redirect_url(self, *args, **kwargs): basename="recommendations-interaction", ) router.register( - r"communitylibrary-submission", + r"communitylibrary_submission", CommunityLibrarySubmissionViewSet, basename="community-library-submission", ) From 73ad4cbc6d951789f7d1a5a4beb1c31da39bbb68 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 21 Jul 2025 19:05:36 +0200 Subject: [PATCH 5/7] Do not pass pk explicitly to serialize_object --- .../contentcuration/viewsets/community_library_submission.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index 6d45e0c4e4..eb5e2334c3 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -251,4 +251,4 @@ def resolve(self, request, pk=None): if submission.status == community_library_submission_constants.STATUS_APPROVED: self._mark_previous_pending_submissions_as_superseded(submission) - return Response(self.serialize_object(pk=submission.id)) + return Response(self.serialize_object()) From 72bbef69118ea84deddee00144574da272cbb5a7 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Mon, 21 Jul 2025 19:07:41 +0200 Subject: [PATCH 6/7] Move community library filtering and pagination to mixin --- .../viewsets/community_library_submission.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contentcuration/contentcuration/viewsets/community_library_submission.py b/contentcuration/contentcuration/viewsets/community_library_submission.py index eb5e2334c3..163ab50e77 100644 --- a/contentcuration/contentcuration/viewsets/community_library_submission.py +++ b/contentcuration/contentcuration/viewsets/community_library_submission.py @@ -170,6 +170,9 @@ class CommunityLibrarySubmissionViewSetMixin: "author_name": get_author_name, } queryset = CommunityLibrarySubmission.objects.all().order_by("-date_created") + filter_backends = [DjangoFilterBackend] + filterset_fields = ["channel"] + pagination_class = CommunityLibrarySubmissionPagination def consolidate(self, items, queryset): countries = {} @@ -193,10 +196,7 @@ class CommunityLibrarySubmissionViewSet( RESTDestroyModelMixin, ReadOnlyValuesViewset, ): - filter_backends = [DjangoFilterBackend] - filterset_fields = ["channel"] permission_classes = [IsAuthenticated] - pagination_class = CommunityLibrarySubmissionPagination serializer_class = CommunityLibrarySubmissionSerializer From 7a3c46e6c21d86563fc4ebe46ab9d00600eccfc6 Mon Sep 17 00:00:00 2001 From: Jakub Komarek Date: Thu, 24 Jul 2025 22:39:27 +0200 Subject: [PATCH 7/7] Remove leftover migration --- ...nitylibrarysubmission_resolution_reason.py | 29 ------------------- 1 file changed, 29 deletions(-) delete mode 100644 contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py diff --git a/contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py b/contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py deleted file mode 100644 index 09a4f7d190..0000000000 --- a/contentcuration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py +++ /dev/null @@ -1,29 +0,0 @@ -# Generated by Django 3.2.24 on 2025-07-15 20:11 -from django.db import migrations -from django.db import models - - -class Migration(migrations.Migration): - - dependencies = [ - ("contentcuration", "0156_communitylibrarysubmission_admin_fields"), - ] - - operations = [ - migrations.AlterField( - model_name="communitylibrarysubmission", - name="resolution_reason", - field=models.CharField( - blank=True, - choices=[ - ("INVALID_LICENSING", "Invalid Licensing"), - ("TECHNICAL_QUALITY_ASSURANCE", "Technical Quality Assurance"), - ("INVALID_METADATA", "Invalid Metadata"), - ("PORTABILITY_ISSUES", "Portability Issues"), - ("OTHER", "Other"), - ], - max_length=50, - null=True, - ), - ), - ]