-
Notifications
You must be signed in to change notification settings - Fork 231
Add a public filter to the kolibri-public's ChannelMetadata viewset #5486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Conversation
…into add-public-filter fix bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @taoerman! This solution looks good, and it is fixing the error of returning the Community Channels in the public channels endpoint! I just noticed one potential improvement in the channelmetadata viewset test suite, and a question to get a more idiomatic implementation for the default filter value. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in this test suite are meant to be run for Community Library channels (e.g. they are being set the countries
field, but this only makes sense in Community Library Channels); therefore, instead of setting the channels' public field to true, I think a better option would be to update the client.get
method to filter by public channels with the public
field set to false. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alex, I've update the client.get method.
self.assertEqual(response.data[0]["name"], community_metadata.name) | ||
self.assertFalse(response.data[0]["public"]) | ||
|
||
def test_channelmetadata_public_filter_mixed_channels(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
queryset = models.ChannelMetadata.objects.all() | ||
# By default, only return public channels unless explicitly filtered | ||
request = getattr(self, "request", None) | ||
if request is not None and "public" not in request.query_params: | ||
return queryset.filter(public=True) | ||
return queryset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Non-blocking) Did you have a chance to look at the using initial values as default for Filterset fields? It seems like a more idiomatic way to set the public filter if it's not present. Let's chat! If this doesn't look like the best solution, I think a better option would be to implement this in filter_queryset
rather than in the get_queryset
? Just to declare the intention of filtering the queryset more idiomatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've added a new filter according to the document and added default value as well.
Summary
Added a public filter to the ChannelMetadataViewSet that defaults to only returning public channels, preventing community channels from appearing in Kolibri's main Library listing. The filter allows explicit access to community channels via ?public=false while maintaining backward compatibility.
Added comprehensive tests to verify the filtering behavior works correctly for both public and community channels.
I also used postman and local Kolibri to manually test public filter.
References
#5263
Reviewer guidance
run unit tests.