CommunityLibrarySubmission viewset#5167
Conversation
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Jakoma02! This is looking great! I've left just a couple minor comments, the more important one is the potential 500 error that we can have the update method of the serializer. Apart from that everything looks great!
| ), | ||
| ] | ||
| indexes = [ | ||
| # Useful for cursor pagination |
|
|
||
| class Meta: | ||
| indexes = [ | ||
| # Useful for cursor pagination |
There was a problem hiding this comment.
Seems like we introduced and unwanted change here
| from contentcuration.tests.base import StudioAPITestCase | ||
|
|
||
|
|
||
| def reverse_with_query( |
| cursor = more["cursor"] | ||
|
|
||
| response = self.client.get( | ||
| reverse_with_query( | ||
| "community-library-submission-list", | ||
| query={"max_results": 1, "cursor": cursor}, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Not super important, but just noting that usually the returned more object can be used as the new query param, as it already returns the max_results, and all filters we have applied to the previous request (e.g. if we are filtering by channel, it already returns the channel filter too). So we usually just do something like:
response = self.client.get(
reverse_with_query(
"community-library-submission-list",
query=more
)
)There was a problem hiding this comment.
That is a neat feature :) Changed in 6ab6775.
| return instance | ||
|
|
||
| def update(self, instance, validated_data): | ||
| if instance.channel.id != validated_data["channel"].id: |
There was a problem hiding this comment.
This will throw a 500 error for patch operations that don't send the channel in the request.
I know this is different from what is written in the issue, but another Idea would be to just silently remove the channel from the validated_data if it exists, and just prevent channel changes updates altogether. What do you think?
There was a problem hiding this comment.
You are right; thanks for catching this! I forgot that partial updates are also possible. This is fixed in ba77607.
I kept the provided channel in validated_data so that a more descriptive error is given in case of failure, but I can silently remove it if you like that approach better.
| countries = validated_data.pop("countries", []) | ||
| instance = super().create(validated_data) | ||
|
|
||
| for country in countries: |
There was a problem hiding this comment.
(nitpick) I think here we can just do instance.countries.set(countries) too right?
There was a problem hiding this comment.
Right, I have only discovered the set method later and forgot to use it for create as well. Changed in 6ab6775.
| def get_queryset(self): | ||
| base_queryset = CommunityLibrarySubmission.objects.all() | ||
| user = self.request.user | ||
| queryset = CommunityLibrarySubmission.filter_view_queryset( | ||
| base_queryset, user | ||
| ).order_by("-date_created") | ||
|
|
||
| return queryset | ||
|
|
||
| def get_edit_queryset(self): | ||
| base_queryset = CommunityLibrarySubmission.objects.all() | ||
| user = self.request.user | ||
| queryset = CommunityLibrarySubmission.filter_edit_queryset(base_queryset, user) | ||
|
|
||
| return queryset |
There was a problem hiding this comment.
Oh, we don't need to override these methods, we already have them thanks to our BaseValuesViewset from which the ReadOnlyValuesViewset inherits: https://github.com/jakoma02/studio/blob/9e7c0f7fff6f749bc04b8acb8635f848eaa5823d/contentcuration/contentcuration/viewsets/base.py#L581-L595.
So we can define our base queryset directly as a class property. (e.g. https://github.com/jakoma02/studio/blob/9e7c0f7fff6f749bc04b8acb8635f848eaa5823d/contentcuration/contentcuration/viewsets/assessmentitem.py#L247) and we can apply the order_by there.
There was a problem hiding this comment.
I have completely missed that! Thanks, changed in 6ab6775.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
This looks great! One more excellent PR! Thanks @Jakoma02!
AlexVelezLl
merged commit 4a34ccc
into
learningequality:community-channels
2 participants
Summary
The PR implements a viewset for managing community library submissions.
Detailed changes:
CommunityLibrarySubmissionViewsetdate_createdfield to support pagination betterCommunityLibrarySubmissionViewsetThe functionality was verified by checking that the new tests pass and by briefly manually testing using the browsable API.
References
Resolves #5157
Reviewer guidance
Nothing