Add action to resolve Community Library Submissions by Jakoma02 · Pull Request #5178 · learningequality/studio

Jakoma02 · 2025-07-15T19:26:08Z

Summary

This PR extends the CommunityLibrarySubmission model with new internal fields, adds a new CommunityLibraryAdminViewSet that exposes these fields and introduces a resolve action for administrators.

Detailed changes:

Added date_resolved, resolved_by, resolved_reason, feedback_notes and internal_notes to the CommunityLibrarySubmission model
Exposed resolved_reason, feedback_notes and date_resolved in the user CommunityLibrarySubmissionViewset
Added a new Superseded submission state
Added resolution_reason_choices constants
Added CommunityLibrarySubmissionAdminViewSet extending CommunityLibrarySubmissionViewSet, also exposing resolved_by_id, resolved_by_first_name, resolved_by_last_name and internal_notes
Added resolve action with semantics as in the linked issue
Added CommunityLibrarySubmissionResolveSerializer for deserializing resolution requests, extending CommunityLibrarySubmissionSerializer
Added AdminViewSetTestCase testing the functionality

No manual testing besides designing the tests and making sure that they pass was performed.

References

Solves #5170.

This PR depends on unmerged changes from #5167, so it should only be merged after that PR is merged and should be rebased to incorporate any changes to that PR introduced during the review process.

Reviewer guidance

~~See the note above about the merging order.~~ Otherwise, not any.

AlexVelezLl

Thanks @Jakoma02! This is looking great so far! I have left a couple of things I noted, mostly nitpicks, but the most important thing is to ensure the consistency in the returned value of the resolve action. Apart from that eveything looks good!

AlexVelezLl · 2025-07-18T15:31:27Z

...ration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py

+        ("contentcuration", "0156_communitylibrarysubmission_admin_fields"),
+    ]
+
+    operations = [


I think we can collapse this migration with the previous one, as I see the only value that is being altered is the `blank=True)

You are, of course, right. I incorrectly thought the previous migration was created in an earlier PR, so it was necessary to alter it. It definitely does not make sense to create and alter the field in the same PR. I fixed this in 9cddb63.

Seems like we still need to remove this file, as its not needed anymore 😅

AlexVelezLl · 2025-07-18T15:32:23Z

contentcuration/contentcuration/urls.py

 )
+router.register(
+    r"communitylibrary_submission_admin",
+    CommunityLibrarySubmissionAdminViewSet,


Just for consistency with other admin viewsets, I think it may be a better idea to name this viewset AdminCommunityLibrarySubmissionViewset and add admin as prefix of the route.

That makes sense, I changed this in 9cddb63. I am not sure if underscores or dashes should be preferred because I see both in the codebase, I leave underscores for now because that is what was approved for the user-facing community library viewset.

I dont think there is a preferred way, I think thats okay :)

AlexVelezLl · 2025-07-18T15:32:55Z

contentcuration/contentcuration/viewsets/community_library_submission.py

        return super().update(instance, validated_data)


+def timezoned_datetime_now():


Hmm, did you tried using django's timezone.now method instead? And then mock "django.utils.timezone.now" in the tests? Just gave it a quick try and seemed to be working okay

I was not aware of django.utils.timezone. Directly mocking that function works well, thanks! Changed in 9cddb63.

AlexVelezLl · 2025-07-18T15:37:38Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+            )
+
+        if (
+            "status" not in validated_data


(nitpick) If we reach to this point, we have already ensured that status is in validated_data thanks to the previous check.

I was aware that status will always be in the validated data after the first if statement, but I intentionally left it because it seemed easier for the reader to understand that this is indeed not accessing a potentially undefined value, and the performance impact is practically non-existent. But I have no strong feelings about this. We can leave the explicit check out if that seems better to you.

No strong feelings neither, if it was intentional, then its fine!

AlexVelezLl · 2025-07-18T15:37:51Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+            or validated_data["status"]
+            == community_library_submission_constants.STATUS_REJECTED
+        ):
+            if "resolution_reason" not in validated_data:


Could we also check if "resolution_reason" and "feedback_notes" are not empty strings?

You are right, I missed that possibility. Added in 9cddb63.

AlexVelezLl · 2025-07-18T15:40:13Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+    field_map = CommunityLibrarySubmissionViewSet.field_map.copy()
+    field_map.update(
+        {
+            "resolved_by_first_name": "resolved_by__first_name",


Now that I think more about this, I think it would be better to just map both first and last name to a resolved_by_name field, as this is what we will always show in the frontend. We can pass functions to field_map fields, just like this example. If you can also update the author field in the CommunityLibrarySubmissionViewSet that'd be nice :)

Changed in 9cddb63.

AlexVelezLl · 2025-07-18T15:49:35Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+        if submission.status == community_library_submission_constants.STATUS_APPROVED:
+            self._mark_previous_pending_submissions_as_superseded(submission)
+
+        return Response(serializer.data)


Since we use serializers just for write-only changes, we don't set read-only values to the serializer, therefore, here we are not returning the fields date_resolved, resolved_by_id, etc. So here we can instead return the self.serialize_object() value to get the standard response taking into account the values viewset property

I did not think of doing it this way, thanks for suggesting this! Changed in 9cddb63.

AlexVelezLl · 2025-07-18T15:54:03Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+        }
+    )
+
+    def create(self, request, *args, **kwargs):


Here if we want to block all these requests, then it will be cleaner to create a Mixin that defines the common properties and methods we want to share between these two viewsets, and then have the CommunityLibrarySubmissionViewSet inherit from that mixin + the other REST operations. And have the CommunityLibrarySubmissionViewSet just inherit from the mixin and the ReadOnlyValuesViewset. That is a cleaner way to not override these methods if we don't want to allow them in the first place.

In Kolibri we have an example of this: https://github.com/learningequality/kolibri/blob/538fb63ddc7218d849e5fc7df1bf515d301eac4a/kolibri/core/auth/api.py#L542

I have thought about separating the common functionality and then reusing it in both viewsets, but it seemed to be a bit too complicated and confusing and it seemed simpler to just override the methods. But I can see your point that extracting the common pieces into a mixin is somewhat cleaner, I refactored it that way in 9cddb63.

AlexVelezLl · 2025-07-18T16:06:17Z

contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py

+        self.patcher.stop()
+        super().tearDown()
+
+    def _manually_resolve_submission(self):


We can name this helper to _manually_reject_submission, so that is clearer why we then check the status == REJECTED in the tests.

Done in 9cddb63.

AlexVelezLl · 2025-07-21T15:30:26Z

contentcuration/contentcuration/urls.py

+    r"communitylibrary-submission",
    CommunityLibrarySubmissionViewSet,
    basename="community-library-submission",
 )
+router.register(
+    r"admin_communitylibrary_submission",


Either if we use snake case or kebab case for these new routes, I think we should use the same case for both routes.

Right, this was not intentional. Fixed in ff53d60.

AlexVelezLl · 2025-07-21T15:33:55Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+        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))


(AFAIK 😅) In theory we shouldn't need to pass the pk argument to this method, it should behave the same as the get_object methods for detail=True actions.

That makes sense, I removed the explicit pk argument in 73ad4cb.

AlexVelezLl · 2025-07-21T15:37:42Z

contentcuration/contentcuration/viewsets/community_library_submission.py

+    ReadOnlyValuesViewset,
+):
+    filter_backends = [DjangoFilterBackend]
+    filterset_fields = ["channel"]


The AdminCommunityLibrarySubmissionViewSet should also allow filtering by channels, and provide paginated responses, so I think we can make these fields part of the CommunityLibrarySubmissionViewSetMixin too.

You are right that it makes sense to support the same filtering and pagination for the admin viewset as well, changed in 72bbef6.

AlexVelezLl

Thanks @Jakoma02! We are almost there! Just one little cleanup! After this we can proceed with the merge :)

AlexVelezLl · 2025-07-23T21:36:19Z

...ration/contentcuration/migrations/0157_alter_communitylibrarysubmission_resolution_reason.py

+        ("contentcuration", "0156_communitylibrarysubmission_admin_fields"),
+    ]
+
+    operations = [


Seems like we still need to remove this file, as its not needed anymore 😅

Jakoma02 · 2025-07-24T20:40:37Z

Seems like we still need to remove this file, as its not needed anymore 😅

You are of course right, done in 7a3c46e.

AlexVelezLl

Thanks @Jakoma02! Great work! This looks good to me! Merging!

Jakoma02 added 2 commits July 17, 2025 16:08

Add action to resolve Community Library Submissions

53c119f

Allow resolution reason to be blank

c6d2207

Jakoma02 force-pushed the resolve_submission_action branch from 02827cf to c6d2207 Compare July 17, 2025 14:11

AlexVelezLl requested changes Jul 18, 2025

View reviewed changes

Address PR feedback

9cddb63

AlexVelezLl reviewed Jul 21, 2025

View reviewed changes

Jakoma02 added 3 commits July 21, 2025 19:02

Use the same case for both community library routes

ff53d60

Do not pass pk explicitly to serialize_object

73ad4cb

Move community library filtering and pagination to mixin

72bbef6

Jakoma02 requested a review from AlexVelezLl July 21, 2025 17:11

rtibbles assigned AlexVelezLl Jul 22, 2025

AlexVelezLl requested changes Jul 23, 2025

View reviewed changes

Remove leftover migration

7a3c46e

Jakoma02 requested a review from AlexVelezLl July 24, 2025 20:41

AlexVelezLl approved these changes Jul 28, 2025

View reviewed changes

AlexVelezLl merged commit 4863c93 into learningequality:community-channels Jul 28, 2025
11 checks passed

AlexVelezLl mentioned this pull request Jul 28, 2025

ESoCC: Add action to resolve Community Library Submissions #5170

Closed

3 tasks

		return super().update(instance, validated_data)


		def timezoned_datetime_now():

Comments

Conversation

Jakoma02 commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakoma02 commented Jul 24, 2025

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Jakoma02 commented Jul 15, 2025 •

edited

Loading

AlexVelezLl Jul 21, 2025 •

edited

Loading