Remove channel labels from non-public ContentNode API #12843
Conversation
github-actions
bot
added
the
DEV: backend
|
Hi @Pulkitxm, welcome and thanks for your effort. @nucleogenesis would you please have a look and decide whether we should review and if so, if the changes make sense? @Pulkitxm Looking at #12842 it wasn't originally indicated as open for contributions. I wanted to provide some guidance so next time you can contribute successfully. Please find an unassigned 'help wanted' issue and before working on it, message us there and wait for an assignment. It's best to familiarize yourself with the contributing guidelines:
Then you can see the list of unassigned "help wanted" or "good first issue" issues across all repositories. |
Build Artifacts
|
There was a problem hiding this comment.
This needs some cleaning up - this may achieve the end desired end result, but has more code changes than I would expect, and adds additional code changes that are not needed.
This rather has the appearance of having been automatically generated by a generative AI, due to these extra, seemingly duplicative, changes. Feel free to use AI, however make sure you can understand the task and adjust the AI output in a meaningful manner, and provide with the smallest diff needed to achieve the task at hand.
| @@ -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() | |||
There was a problem hiding this comment.
Using a query parameter to set this was not asked for in the issue description - this should be removed.
| 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.
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.
| return Response( | ||
| OrderedDict( | ||
| [ | ||
| ("more", self.get_more()), | ||
| ("results", data), | ||
| ("labels", get_available_metadata_labels(self.queryset)), |
There was a problem hiding this comment.
This can remain inline with just the new argument for deprecated channel labels being passed.
| ] | ||
| ) | ||
| ) | ||
|
|
||
| def get_paginated_response_schema(self, schema): | ||
| return { |
There was a problem hiding this comment.
No need to update this at all.
| @@ -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.
This method doesn't belong on this class - this is extraneous.
| } | ||
|
|
||
| class DeprecatedChannelsLabelsPagination(OptionalContentNodePagination): | ||
| def paginate_queryset(self, queryset, request, view=None): |
There was a problem hiding this comment.
Instead of having to set this inside the paginate_queryset call, you can instead just define it as a class property:
class DeprecatedChannelsLabelsPagination(OptionalContentNodePagination):
use_deprecated_channels_labels = True
| @@ -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.
This method doesn't need to be updated, the get_available_metadata_labels method should be updated to conditionally call this and add the result to the returned labels.
|
Hi @Pulkitxm - any further guidance needed here? Feel free to ask any follow up questions! |
There was a problem hiding this comment.
@Pulkitxm do you think you'll be able to address the feedback given above? If so please let us know, even if you need some additional time, otherwise we'll probably close this PR and reassign the related issue.
Fixes
Closes #12842
Summary
ContentNodeViewsetAPI.use_deprecated_channels_labelsto theOptionalContentNodePaginationclass to handle the inclusion of channel labels.References
Reviewer guidance
ContentNodeViewsetAPI to ensure that channel labels are no longer returned.Testing checklist
PR process
Reviewer checklist
yarnandpip)