-
Notifications
You must be signed in to change notification settings - Fork 699
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
Remove channel labels from non-public ContentNode API #12843
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -828,39 +828,56 @@ class OptionalContentNodePagination(OptionalPagination): | |
def paginate_queryset(self, queryset, request, view=None): | ||
# Record the queryset for use in returning available filters | ||
self.queryset = queryset | ||
self.use_deprecated_channels_labels = ( | ||
request.query_params.get("use_deprecated_channels_labels", "false").lower() | ||
== "true" | ||
) | ||
return super(OptionalContentNodePagination, self).paginate_queryset( | ||
queryset, request, view=view | ||
) | ||
|
||
def get_paginated_response(self, data): | ||
labels = get_available_metadata_labels(self.queryset) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be cleaner to pass through this flag into the get_available_metadata_labels function and have that do the conditional behaviour for adding it to the labels. |
||
if self.use_deprecated_channels_labels: | ||
labels["channels"] = list( | ||
self.queryset.values_list("channel_id", flat=True).distinct() | ||
) | ||
return Response( | ||
OrderedDict( | ||
[ | ||
("more", self.get_more()), | ||
("results", data), | ||
("labels", get_available_metadata_labels(self.queryset)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can remain inline with just the new argument for deprecated channel labels being passed. |
||
("labels", labels), | ||
] | ||
) | ||
) | ||
|
||
def get_paginated_response_schema(self, schema): | ||
return { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to update this at all. |
||
"type": "object", | ||
"properties": { | ||
"more": { | ||
"type": "object", | ||
"nullable": True, | ||
"example": { | ||
"cursor": "asdadshjashjadh", | ||
}, | ||
}, | ||
"results": schema, | ||
"labels": { | ||
"type": "object", | ||
"example": {"accessibility_labels": ["id1", "id2"]}, | ||
properties = { | ||
"more": { | ||
"type": "object", | ||
"nullable": True, | ||
"example": { | ||
"cursor": "asdadshjashjadh", | ||
}, | ||
}, | ||
"results": schema, | ||
"labels": { | ||
"type": "object", | ||
"example": {"accessibility_labels": ["id1", "id2"]}, | ||
}, | ||
} | ||
if self.use_deprecated_channels_labels: | ||
properties["labels"]["example"]["channels"] = ["channel_id1", "channel_id2"] | ||
return { | ||
"type": "object", | ||
"properties": properties, | ||
} | ||
|
||
class DeprecatedChannelsLabelsPagination(OptionalContentNodePagination): | ||
def paginate_queryset(self, queryset, request, view=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of having to set this inside the paginate_queryset call, you can instead just define it as a class property:
|
||
self.use_deprecated_channels_labels = True | ||
return super().paginate_queryset(queryset, request, view) | ||
|
||
|
||
@method_decorator(remote_metadata_cache, name="dispatch") | ||
|
@@ -950,6 +967,18 @@ def recommendations_for(self, request, **kwargs): | |
) | ||
return Response(self.serialize(queryset)) | ||
|
||
def get_paginated_response(self, data): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method doesn't belong on this class - this is extraneous. |
||
labels = get_available_metadata_labels(self.queryset) | ||
return Response( | ||
OrderedDict( | ||
[ | ||
("more", self.get_more()), | ||
("results", data), | ||
("labels", labels), | ||
] | ||
) | ||
) | ||
|
||
|
||
# The max recursed page size should be less than 25 for a couple of reasons: | ||
# 1. At this size the query appears to be relatively performant, and will deliver most of the tree | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,14 +77,22 @@ def _get_available_languages(base_queryset): | |
return list(langs) | ||
|
||
|
||
def _get_available_channels(base_queryset): | ||
def _get_available_channels(base_queryset, include_labels=False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method doesn't need to be updated, the |
||
from kolibri.core.content.models import ChannelMetadata | ||
|
||
return list( | ||
ChannelMetadata.objects.filter( | ||
id__in=base_queryset.values_list("channel_id", flat=True).distinct() | ||
).values("id", "name") | ||
) | ||
channels = ChannelMetadata.objects.filter( | ||
id__in=base_queryset.values_list("channel_id", flat=True).distinct() | ||
).values("id", "name") | ||
|
||
if include_labels: | ||
for channel in channels: | ||
channel["labels"] = list( | ||
ChannelMetadata.objects.filter(id=channel["id"]).values_list( | ||
"labels", flat=True | ||
) | ||
) | ||
|
||
return list(channels) | ||
|
||
|
||
class SQLiteBitwiseORAggregate(Aggregate): | ||
|
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.
Using a query parameter to set this was not asked for in the issue description - this should be removed.