[Fixes #1299] Restore thumbnail data to public channels API. #1367
Conversation
…ls API. Also fix content type value for public JSON endpoints.
| information, fall back to the channel thumbnail data if icon_encoding is not set. | ||
| """ | ||
| if channel.icon_encoding: | ||
| return channel.icon_encoding |
There was a problem hiding this comment.
Is it always safe to just return the thumbnail_encoding? If so this could be removed.
There was a problem hiding this comment.
This is so that the thumbnail used when the channel was published remains the same despite the user changing the thumbnail after publishing
There was a problem hiding this comment.
It currently isn't being set, although from my reading of the code I think that was unintentional and may not have always been the case, so I didn't want to break the old behavior without more discussion about whether or not we want to use this field going forward.
| @@ -762,11 +762,27 @@ class PublicChannelSerializer(ChannelFieldMixin, serializers.ModelSerializer): | |||
| """ | |||
| kind_count = serializers.SerializerMethodField('generate_kind_count') | |||
| matching_tokens = serializers.SerializerMethodField('match_tokens') | |||
| icon_encoding = serializers.SerializerMethodField('get_thumbnail_encoding') | |||
There was a problem hiding this comment.
If we decide it is safe to always return thumbnail_encoding, we could simplify this to:
icon_encoding = serializers.JSONField(source='thumbnail_encoding')
| @@ -59,7 +58,7 @@ def get_public_channel_list(request, version): | |||
| channel_list = _get_channel_list(version, request.query_params) | |||
| except LookupError: | |||
| return HttpResponseNotFound(_("Api endpoint {} is not available").format(version)) | |||
| return HttpResponse(json.dumps(PublicChannelSerializer(channel_list, many=True).data)) | |||
| @@ -72,7 +71,7 @@ def get_public_channel_lookup(request, version, identifier): | |||
| return HttpResponseNotFound(_("Api endpoint {} is not available").format(version)) | |||
| if not channel_list.exists(): | |||
| return HttpResponseNotFound(_("No channel matching {} found").format(identifier)) | |||
| return HttpResponse(json.dumps(PublicChannelSerializer(channel_list, many=True).data)) | |||
| return Response(PublicChannelSerializer(channel_list, many=True).data) | |||
There was a problem hiding this comment.
Just want to make sure, does this break any versions of Kolibri since the type returned will be different (string vs. array)?
There was a problem hiding this comment.
The output of the endpoint should not be any different, rest_framework.Response serializes the data to JSON internally. It also returns a correct Content-Type header of application/json, unlike HttpResponse, which returns text/html, which is why I made the change.
There was a problem hiding this comment.
Will this affect any front-end code? When I did a similar thing changing HttpResponse to rest_framework.Response, I had to remove any JSON.parse instances from the front end to get it to work
There was a problem hiding this comment.
Is any Studio frontend code using the Kolibri public channel list API? I wasn't able to find any code paths in the Studio frontend using it.
On a more general note, we should actually look at any places we need to explicitly parse and make fixing those part of our tech debt. It means we're not properly declaring the return type, which will cause problems when writing tests, etc.
There was a problem hiding this comment.
Alright, I think it should be fine. Worst case, we just revert back to the json dumps code :)
jayoshih
added
the
changelog
jayoshih
removed
qa-ready
Also fix content type value for public JSON endpoints.
Description
The public channel list API gets its thumbnail from the
icon_encodingfield in the Channel model. However, in Studio, we never set this field. We only ever set it on the Kolibri sqlite db. In Studio, the thumbnail comes from thethumbnail_encodingfield instead. As a result, Kolibri import always shows blank thumbnails for Studio content sources available to import.To fix this, this PR updates the channel public list API serializer to return the
thumbnail_encodingthumbnail if theicon_encodingfield is not set.Issue Addressed (if applicable)
Addresses #1299
Steps to Test
Checklist
Reviewers
If you are looking to assign a reviewer, here are some options: