diff --git a/contentcuration/contentcuration/frontend/administration/mixins.js b/contentcuration/contentcuration/frontend/administration/mixins.js index c977f33929..04f3314083 100644 --- a/contentcuration/contentcuration/frontend/administration/mixins.js +++ b/contentcuration/contentcuration/frontend/administration/mixins.js @@ -3,7 +3,6 @@ import difference from 'lodash/difference'; import findKey from 'lodash/findKey'; import intersection from 'lodash/intersection'; import transform from 'lodash/transform'; -import omit from 'lodash/omit'; import pickBy from 'lodash/pickBy'; function _getBooleanVal(value) { @@ -137,10 +136,10 @@ export const tableMixin = { { ...this.$route.query, page_size: pagination.rowsPerPage, - ...omit(pagination, ['rowsPerPage', 'totalItems']), + ...pagination, }, - value => { - return value !== null; + (value, key) => { + return value !== null && key !== 'rowsPerPage' && key !== 'totalItems'; } ); @@ -156,6 +155,17 @@ export const tableMixin = { }); }, }, + fetchParams() { + const params = { + ...this.$route.query, + }; + if (params.sortBy) { + params.ordering = (params.descending ? '-' : '') + params.sortBy; + delete params.sortBy; + delete params.descending; + } + return params; + }, }, watch: { '$route.query'() { @@ -168,7 +178,7 @@ export const tableMixin = { methods: { _loadItems() { this.loading = true; - this.fetch(this.$route.query).then(() => { + this.fetch(this.fetchParams).then(() => { this.loading = false; }); }, diff --git a/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilterBar.vue b/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilterBar.vue index d5bf4d9bbd..02bf30037f 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilterBar.vue +++ b/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilterBar.vue @@ -78,7 +78,7 @@ ), // Channels - ...this.channels.map(channelId => + ...this.channel_id__in.map(channelId => createFilter( channelId, this.getChannelName(channelId), diff --git a/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilters.vue b/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilters.vue index 50b3e1796d..9f9f146bef 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilters.vue +++ b/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchFilters.vue @@ -13,7 +13,7 @@ :menu-props="menuProps" /> { - if (this.channels.length) { - this.channels = []; + if (this.channel_id__in.length) { + this.channel_id__in = []; } this.channelOptions = channels.filter(c => c.id !== this.currentChannelId); diff --git a/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/mixins.js b/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/mixins.js index b6de96ae54..4932d043e2 100644 --- a/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/mixins.js +++ b/contentcuration/contentcuration/frontend/channelEdit/views/ImportFromChannels/mixins.js @@ -4,7 +4,7 @@ import { generateSearchMixin } from 'shared/mixins'; const searchFilters = { kinds: filterTypes.MULTISELECT, resources: filterTypes.BOOLEAN, - channels: filterTypes.MULTISELECT, + channel_id__in: filterTypes.MULTISELECT, languages: filterTypes.MULTISELECT, licenses: filterTypes.MULTISELECT, coach: filterTypes.BOOLEAN, diff --git a/contentcuration/contentcuration/frontend/shared/data/resources.js b/contentcuration/contentcuration/frontend/shared/data/resources.js index c88b0a1931..e29de0ed3d 100644 --- a/contentcuration/contentcuration/frontend/shared/data/resources.js +++ b/contentcuration/contentcuration/frontend/shared/data/resources.js @@ -5,9 +5,11 @@ import flatMap from 'lodash/flatMap'; import get from 'lodash/get'; import isArray from 'lodash/isArray'; import isFunction from 'lodash/isFunction'; +import isNumber from 'lodash/isNumber'; import isString from 'lodash/isString'; import matches from 'lodash/matches'; import overEvery from 'lodash/overEvery'; +import pick from 'lodash/pick'; import sortBy from 'lodash/sortBy'; import uniq from 'lodash/uniq'; import uniqBy from 'lodash/uniqBy'; @@ -47,6 +49,8 @@ const QUERY_SUFFIXES = { LTE: 'lte', }; +const ORDER_FIELD = 'ordering'; + const VALID_SUFFIXES = new Set(Object.values(QUERY_SUFFIXES)); const SUFFIX_SEPERATOR = '__'; @@ -54,6 +58,60 @@ const validPositions = new Set(Object.values(RELATIVE_TREE_POSITIONS)); const EMPTY_ARRAY = Symbol('EMPTY_ARRAY'); +class Paginator { + constructor(params) { + // Get parameters for page number based pagination + Object.assign(this, pick(params, 'page', 'page_size')); + // At a minimum, this pagination style requires a page_size + // parameter, so we check to see if that exists. + if (this.page_size) { + this.pageNumberType = true; + this.page = this.page || 1; + } + // Get parameters for limit offset pagination + Object.assign(this, pick(params, 'limit', 'offset')); + // At a minimum, this pagination style requires a limit + // parameter, so we check to see if that exists. + if (this.limit) { + this.limitOffsetType = true; + this.offset = this.offset || 0; + } + if (this.pageNumberType && this.limitOffsetType) { + console.warn( + 'Specified both page number type pagination and limit offset may get unexpected results' + ); + } + } + paginate(collection) { + let offset; + let limit; + if (this.pageNumberType) { + offset = (this.page - 1) * this.page_size; + limit = this.page_size; + } + if (this.limitOffsetType) { + offset = this.offset; + limit = this.limit; + } + if (isNumber(offset) && isNumber(limit)) { + const countPromise = collection.count(); + const resultPromise = collection + .offset(offset) + .limit(limit) + .toArray(); + return Promise.all([countPromise, resultPromise]).then(([count, results]) => { + const out = { count, results }; + if (this.pageNumberType) { + out.total_pages = Math.ceil(count / this.page_size); + out.page = this.page; + } + return out; + }); + } + return collection.toArray(); + } +} + // Custom uuid4 function to match our dashless uuids on the server side export function uuid4() { return uuidv4().replace(/-/g, ''); @@ -315,6 +373,12 @@ class IndexedDBResource { const arrayParams = {}; // Suffixed parameters - ones that are filtering by [gt/lt](e) const suffixedParams = {}; + // Field to sort by + let sortBy; + let reverse; + + // Setup paginator. + const paginator = new Paginator(params); for (let key of Object.keys(params)) { // Partition our parameters const [rootParam, suffix] = key.split(SUFFIX_SEPERATOR); @@ -329,7 +393,16 @@ class IndexedDBResource { } else if (!suffix) { whereParams[rootParam] = params[key]; } - } else { + } else if (key === ORDER_FIELD) { + const ordering = params[key]; + if (ordering.indexOf('-') === 0) { + sortBy = ordering.substring(1); + reverse = true; + } else { + sortBy = ordering; + } + } else if (!paginator[key]) { + // Don't filter by parameters that are used for pagination filterParams[rootParam] = params[key]; } } @@ -344,7 +417,8 @@ class IndexedDBResource { } return Promise.resolve(EMPTY_ARRAY); } - collection = table.where(Object.keys(arrayParams)[0]).anyOf(Object.values(arrayParams)[0]); + const keyPath = Object.keys(arrayParams)[0]; + collection = table.where(keyPath).anyOf(Object.values(arrayParams)[0]); if (process.env.NODE_ENV !== 'production') { // Flag a warning if we tried to filter by an Array and other where params if (Object.keys(whereParams).length > 1) { @@ -358,6 +432,9 @@ class IndexedDBResource { } } Object.assign(filterParams, whereParams); + if (sortBy === keyPath) { + sortBy = null; + } } else if (Object.keys(arrayParams).length > 1) { if (process.env.NODE_ENV !== 'production') { // Flag a warning if we tried to filter by an Array and other where params @@ -372,8 +449,19 @@ class IndexedDBResource { } } else if (Object.keys(whereParams).length > 0) { collection = table.where(whereParams); + if (whereParams[sortBy] && Object.keys(whereParams).length === 1) { + // If there is only one where parameter, then the collection should already be sorted + // by the index that it was queried by. + // https://dexie.org/docs/Collection/Collection.sortBy()#remarks + sortBy = null; + } } else { - collection = table.toCollection(); + if (sortBy && this.indexFields.has(sortBy) && !reverse) { + collection = table.orderBy(sortBy); + sortBy = null; + } else { + collection = table.toCollection(); + } } let filterFn; if (Object.keys(filterParams).length !== 0) { @@ -421,7 +509,13 @@ class IndexedDBResource { if (filterFn) { collection = collection.filter(filterFn); } - return collection.toArray(); + if (sortBy) { + if (reverse) { + collection = collection.reverse(); + } + collection = collection.sortBy(sortBy); + } + return paginator.paginate(collection); } get(id) { @@ -605,7 +699,7 @@ class Resource extends mix(APIResource, IndexedDBResource) { if (objs === EMPTY_ARRAY) { return []; } - if (!objs.length) { + if (!objs.length && !objs.count) { return this.requestCollection(params); } if (doRefresh) { diff --git a/contentcuration/contentcuration/utils/pagination.py b/contentcuration/contentcuration/utils/pagination.py index 155d31dbe5..0bd5abef97 100644 --- a/contentcuration/contentcuration/utils/pagination.py +++ b/contentcuration/contentcuration/utils/pagination.py @@ -1,47 +1,88 @@ import hashlib from django.core.cache import cache +from django.core.exceptions import EmptyResultSet from django.core.paginator import InvalidPage +from django.core.paginator import Page +from django.core.paginator import Paginator +from django.db.models import QuerySet +from django.utils.functional import cached_property from rest_framework.pagination import NotFound from rest_framework.pagination import PageNumberPagination from rest_framework.response import Response -class CachedListPagination(PageNumberPagination): - def paginate_queryset(self, queryset, request, view=None): - """ - Overrides paginate_queryset from PageNumberPagination - to assign the records count to the paginator. - This count has been previously obtained in a less complex query, - thus it has a better performance. +class ValuesPage(Page): + def __init__(self, object_list, number, paginator): + self.queryset = object_list + self.object_list = object_list + self.number = number + self.paginator = paginator - """ - def cached_count(queryset): +class ValuesViewsetPaginator(Paginator): + def __init__(self, object_list, *args, **kwargs): + if not isinstance(object_list, QuerySet): + raise TypeError("ValuesViewsetPaginator is only intended for use with Querysets") + self.queryset = object_list + object_list = object_list.values_list("pk", flat=True).distinct() + return super(ValuesViewsetPaginator, self).__init__(object_list, *args, **kwargs) + + def _get_page(self, object_list, *args, **kwargs): + pks = list(object_list) + return ValuesPage(self.queryset.filter(pk__in=pks), *args, **kwargs) + + +class CachedValuesViewsetPaginator(ValuesViewsetPaginator): + @cached_property + def count(self): + """ + The count is implemented with this 'double cache' so as to cache the empty results + as well. Because the cache key is dependent on the query string, and that cannot be + generated in the instance that the query_string produces an EmptyResultSet exception. + """ + try: + query_string = str(self.object_list.query).encode("utf8") cache_key = ( "query-count:" - + hashlib.md5(str(queryset.query).encode("utf8")).hexdigest() + + hashlib.md5(query_string).hexdigest() ) value = cache.get(cache_key) if value is None: - value = queryset.values("id").count() + value = super(CachedValuesViewsetPaginator, self).count cache.set(cache_key, value, 300) # save the count for 5 minutes - return value + except EmptyResultSet: + # If the query is an empty result set, then this error will be raised by + # Django - this happens, for example when doing a pk__in=[] query + # In this case, we know the value is just 0! + value = 0 + return value + +class ValuesViewsetPageNumberPagination(PageNumberPagination): + django_paginator_class = ValuesViewsetPaginator + + def paginate_queryset(self, queryset, request, view=None): + """ + Paginate a queryset if required, either returning a + the queryset of a page object so that it can be further annotated for serialization, + or `None` if pagination is not configured for this view. + This is vendored and modified from: + https://github.com/encode/django-rest-framework/blob/master/rest_framework/pagination.py#L191 + """ page_size = self.get_page_size(request) if not page_size: return None paginator = self.django_paginator_class(queryset, page_size) - paginator.count = cached_count(queryset) - page_number = request.query_params.get(self.page_query_param, 1) - if page_number in self.last_page_strings: - page_number = paginator.num_pages + page_number = self.get_page_number(request, paginator) try: self.page = paginator.page(page_number) except InvalidPage as exc: - msg = self.invalid_page_message.format(page_number=page_number, message=exc) + msg = self.invalid_page_message.format( + page_number=page_number, message=str(exc) + ) raise NotFound(msg) if paginator.num_pages > 1 and self.template is not None: @@ -49,36 +90,42 @@ def cached_count(queryset): self.display_page_controls = True self.request = request - return list(self.page) + # This is the only difference between the original function and this implementation + # here, instead of returning the page coerced to a list, we explicitly return the queryset + # of the page, so that we can do further annotations on it - otherwise, once it is coerced + # to a list, the DB read has already occurred. + return self.page.queryset def get_paginated_response(self, data): return Response( { - "next": self.get_next_link(), - "previous": self.get_previous_link(), - "page_number": self.page.number, + "page": self.page.number, "count": self.page.paginator.count, "total_pages": self.page.paginator.num_pages, "results": data, } ) - -def get_order_queryset(request, queryset, field_map): - """ - Function used to extract the field the queryset must be ordered by, - and apply it correctly to the queryset - """ - order = request.GET.get("sortBy", "") - if not order: - order = "undefined" - - if order in field_map and type(field_map[order]) is str: - order = field_map[order] - - if request.GET.get("descending", "true") == "false" and order != "undefined": - order = "-" + order - - queryset = queryset.distinct().order_by() if order == "undefined" else queryset.distinct().order_by(order) - - return (order, queryset) + def get_paginated_response_schema(self, schema): + return { + 'type': 'object', + 'properties': { + 'count': { + 'type': 'integer', + 'example': 123, + }, + 'results': schema, + 'page': { + 'type': 'integer', + 'example': 123, + }, + 'total_pages': { + 'type': 'integer', + 'example': 123, + }, + }, + } + + +class CachedListPagination(ValuesViewsetPageNumberPagination): + django_paginator_class = CachedValuesViewsetPaginator diff --git a/contentcuration/contentcuration/viewsets/assessmentitem.py b/contentcuration/contentcuration/viewsets/assessmentitem.py index 62e13b2bae..5db084a670 100644 --- a/contentcuration/contentcuration/viewsets/assessmentitem.py +++ b/contentcuration/contentcuration/viewsets/assessmentitem.py @@ -2,7 +2,6 @@ from django.db import transaction from django_bulk_update.helper import bulk_update -from django_filters.rest_framework import DjangoFilterBackend from le_utils.constants import exercises from rest_framework.permissions import IsAuthenticated from rest_framework.serializers import ValidationError @@ -196,7 +195,6 @@ class AssessmentItemViewSet(BulkCreateMixin, BulkUpdateMixin, ValuesViewset): queryset = AssessmentItem.objects.all() serializer_class = AssessmentItemSerializer permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = AssessmentItemFilter values = ( "question", diff --git a/contentcuration/contentcuration/viewsets/base.py b/contentcuration/contentcuration/viewsets/base.py index 9ee95420d3..c044043eb1 100644 --- a/contentcuration/contentcuration/viewsets/base.py +++ b/contentcuration/contentcuration/viewsets/base.py @@ -5,7 +5,9 @@ from django.http import Http404 from django_bulk_update.helper import bulk_update from django_filters.constants import EMPTY_VALUES +from django_filters.rest_framework import DjangoFilterBackend from django_filters.rest_framework import FilterSet +from rest_framework.filters import OrderingFilter from rest_framework.generics import get_object_or_404 from rest_framework.response import Response from rest_framework.serializers import ListSerializer @@ -345,12 +347,77 @@ def create(self, validated_data): return created_objects +class ValuesViewsetOrderingFilter(OrderingFilter): + + def get_default_valid_fields(self, queryset, view, context={}): + """ + The original implementation of this makes the assumption that the DRF serializer for the class + encodes all the serialization behaviour for the viewset: + https://github.com/encode/django-rest-framework/blob/version-3.12.2/rest_framework/filters.py#L208 + + With the ValuesViewset, this is no longer the case so here we do an equivalent mapping from the values + defined by the values viewset, with consideration for the mapped fields. + + Importantly, we filter out values that have not yet been annotated on the queryset, so if an annotated + value is requried for ordering, it should be defined in the get_queryset method of the viewset, and not + the annotate_queryset method, which is executed after filtering. + """ + default_fields = set() + # All the fields that we have field maps defined for - this only allows for simple mapped fields + # where the field is essentially a rename, as we have no good way of doing ordering on a field that + # that is doing more complex function based mapping. + mapped_fields = {v: k for k, v in view.field_map.items() if isinstance(v, str)} + # All the fields of the model + model_fields = {f.name for f in queryset.model._meta.get_fields()} + # Loop through every value in the view's values tuple + for field in view.values: + # If the value is for a foreign key lookup, we split it here to make sure that the first relation key + # exists on the model - it's unlikely this would ever not be the case, as otherwise the viewset would + # be returning 500s. + fk_ref = field.split("__")[0] + # Check either if the field is a model field, a currently annotated annotation, or + # is a foreign key lookup on an FK on this model. + if field in model_fields or field in queryset.query.annotations or fk_ref in model_fields: + # If the field is a mapped field, we store the field name as returned to the client + # not the actual internal field - this will later be mapped when we come to do the ordering. + if field in mapped_fields: + default_fields.add((mapped_fields[field], mapped_fields[field])) + else: + default_fields.add((field, field)) + + return default_fields + + def remove_invalid_fields(self, queryset, fields, view, request): + """ + Modified from https://github.com/encode/django-rest-framework/blob/version-3.12.2/rest_framework/filters.py#L259 + to do filtering based on valuesviewset setup + """ + # We filter the mapped fields to ones that do simple string mappings here, any functional maps are excluded. + mapped_fields = {k: v for k, v in view.field_map.items() if isinstance(v, str)} + valid_fields = [item[0] for item in self.get_valid_fields(queryset, view, {'request': request})] + ordering = [] + for term in fields: + if term.lstrip("-") in valid_fields: + if term.lstrip("-") in mapped_fields: + # In the case that the ordering field is a mapped field on the values viewset + # we substitute the serialized name of the field for the database name. + prefix = "-" if term[0] == "-" else "" + new_term = prefix + mapped_fields[term.lstrip("-")] + ordering.append(new_term) + else: + ordering.append(term) + if len(ordering) > 1: + raise ValidationError("Can only define a single ordering field") + return ordering + + class ReadOnlyValuesViewset(SimpleReprMixin, ReadOnlyModelViewSet): """ A viewset that uses a values call to get all model/queryset data in a single database query, rather than delegating serialization to a DRF ModelSerializer. """ + filter_backends = (DjangoFilterBackend, ValuesViewsetOrderingFilter) # A tuple of values to get from the queryset values = None @@ -485,9 +552,6 @@ def get_edit_object(self): def annotate_queryset(self, queryset): return queryset - def prefetch_queryset(self, queryset): - return queryset - def _map_fields(self, item): for key, value in self._field_map.items(): if callable(value): @@ -509,18 +573,22 @@ def serialize(self, queryset): return self.consolidate(list(map(self._map_fields, queryset or [])), queryset) def list(self, request, *args, **kwargs): - queryset = self.filter_queryset(self.prefetch_queryset(self.get_queryset())) - queryset = self._cast_queryset_to_values(queryset) + queryset = self.filter_queryset(self.get_queryset()) + + page_queryset = self.paginate_queryset(queryset) - page = self.paginate_queryset(queryset) + if page_queryset is not None: + queryset = page_queryset + + queryset = self._cast_queryset_to_values(queryset) - if page is not None: - return self.get_paginated_response(self.serialize(page)) + if page_queryset is not None: + return self.get_paginated_response(self.serialize(queryset)) return Response(self.serialize(queryset)) def serialize_object(self, **filter_kwargs): - queryset = self.prefetch_queryset(self.get_queryset()) + queryset = self.get_queryset() try: filter_kwargs = filter_kwargs or self._get_lookup_filter() return self.serialize( diff --git a/contentcuration/contentcuration/viewsets/channel.py b/contentcuration/contentcuration/viewsets/channel.py index 5fa46e104e..e4b9e97c62 100644 --- a/contentcuration/contentcuration/viewsets/channel.py +++ b/contentcuration/contentcuration/viewsets/channel.py @@ -15,7 +15,6 @@ from django_cte import With from django_filters.rest_framework import BooleanFilter from django_filters.rest_framework import CharFilter -from django_filters.rest_framework import DjangoFilterBackend from le_utils.constants import content_kinds from le_utils.constants import roles from rest_framework import serializers @@ -39,7 +38,7 @@ from contentcuration.models import User from contentcuration.tasks import create_async_task from contentcuration.utils.pagination import CachedListPagination -from contentcuration.utils.pagination import get_order_queryset +from contentcuration.utils.pagination import ValuesViewsetPageNumberPagination from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer from contentcuration.viewsets.base import ReadOnlyValuesViewset @@ -55,7 +54,7 @@ from contentcuration.viewsets.sync.utils import generate_update_event -class ChannelListPagination(PageNumberPagination): +class ChannelListPagination(ValuesViewsetPageNumberPagination): page_size = None page_size_query_param = "page_size" max_page_size = 1000 @@ -390,7 +389,6 @@ class ChannelViewSet(ChangeEventMixin, ValuesViewset): queryset = Channel.objects.all() permission_classes = [IsAuthenticated] serializer_class = ChannelSerializer - filter_backends = (DjangoFilterBackend,) pagination_class = ChannelListPagination filterset_class = ChannelFilter @@ -537,15 +535,15 @@ def sync(self, request, pk=None): class CatalogViewSet(ReadOnlyValuesViewset): queryset = Channel.objects.all() serializer_class = ChannelSerializer - filter_backends = (DjangoFilterBackend,) pagination_class = CatalogListPagination filterset_class = BaseChannelFilter permission_classes = [AllowAny] field_map = channel_field_map - values = ("id", "name") - base_values = ( + values = ( + "id", + "name", "description", "thumbnail", "thumbnail_encoding", @@ -561,17 +559,7 @@ def get_queryset(self): return queryset.order_by("name") - def paginate_queryset(self, queryset): - page_results = self.paginator.paginate_queryset( - queryset, self.request, view=self - ) - ids = [result["id"] for result in page_results] - queryset = Channel.objects.filter(id__in=ids) - queryset = self.complete_annotations(queryset) - self.values = self.values + self.base_values - return list(queryset.values(*self.values)) - - def complete_annotations(self, queryset): + def annotate_queryset(self, queryset): queryset = queryset.annotate(primary_token=primary_token_subquery) channel_main_tree_nodes = ContentNode.objects.filter( tree_id=OuterRef("main_tree__tree_id") @@ -638,7 +626,6 @@ class AdminChannelViewSet(ChannelViewSet): permission_classes = [IsAdminUser] serializer_class = AdminChannelSerializer filterset_class = AdminChannelFilter - filter_backends = (DjangoFilterBackend,) field_map = { "published": "main_tree__published", "created": "main_tree__created", @@ -646,11 +633,15 @@ class AdminChannelViewSet(ChannelViewSet): "demo_server_url": format_demo_server_url, } - base_values = ( + values = ( "id", "name", "description", "main_tree__published", + "modified", + "editors_count", + "viewers_count", + "size", "public", "main_tree__created", "main_tree__id", @@ -658,54 +649,23 @@ class AdminChannelViewSet(ChannelViewSet): "deleted", "source_url", "demo_server_url", + "primary_token", ) - values = base_values - - def paginate_queryset(self, queryset): - order, queryset = get_order_queryset(self.request, queryset, self.field_map) - page_results = self.paginator.paginate_queryset( - queryset, self.request, view=self - ) - ids = [result["id"] for result in page_results] - # tree_ids are needed to optimize files size annotation: - self.page_tree_ids = [result["main_tree__tree_id"] for result in page_results] - - self.values = self.base_values - queryset = Channel.objects.filter(id__in=ids).values(*(self.values)) - if order != "undefined": - queryset = queryset.order_by(order) - return self.complete_annotations(queryset) def get_queryset(self): - self.annotations = self.compose_annotations() - order = self.request.GET.get("sortBy", "") - if order in self.annotations: - self.values = self.values + (order,) - return Channel.objects.values("id").order_by("name") - - def annotate_queryset(self, queryset): - # will do it after paginate excepting for order by - order = self.request.GET.get("sortBy", "") - if order in self.annotations: - queryset = queryset.annotate(**{order: self.annotations[order]}) - return queryset - - def compose_annotations(self): channel_main_tree_nodes = ContentNode.objects.filter( tree_id=OuterRef("main_tree__tree_id") ) - - annotations = {} - annotations["primary_token"] = primary_token_subquery - # Add the last modified node modified value as the channel last modified - annotations["modified"] = Subquery( - channel_main_tree_nodes.values("modified").order_by("-modified")[:1] + queryset = Channel.objects.all().annotate( + modified=Subquery( + channel_main_tree_nodes.values("modified").order_by("-modified")[:1] + ), + primary_token=primary_token_subquery, ) - return annotations - - def complete_annotations(self, queryset): + return queryset - queryset = queryset.annotate(**self.annotations) + def annotate_queryset(self, queryset): + page_tree_ids = list(queryset.values_list("main_tree__tree_id", flat=True)) editors_query = ( User.objects.filter(editable_channels__id=OuterRef("id")) @@ -720,7 +680,7 @@ def complete_annotations(self, queryset): nodes = With( ContentNode.objects.values("id", "tree_id") - .filter(tree_id__in=self.page_tree_ids) + .filter(tree_id__in=page_tree_ids) .order_by(), name="nodes", ) diff --git a/contentcuration/contentcuration/viewsets/channelset.py b/contentcuration/contentcuration/viewsets/channelset.py index a958ce4b41..a4e665e2a7 100644 --- a/contentcuration/contentcuration/viewsets/channelset.py +++ b/contentcuration/contentcuration/viewsets/channelset.py @@ -2,7 +2,6 @@ from django.db.models import OuterRef from django.db.models import Q from django_filters.rest_framework import BooleanFilter -from django_filters.rest_framework import DjangoFilterBackend from rest_framework import serializers from rest_framework.permissions import IsAuthenticated @@ -69,7 +68,6 @@ class Meta: class ChannelSetViewSet(ValuesViewset): queryset = ChannelSet.objects.all() serializer_class = ChannelSetSerializer - filter_backends = (DjangoFilterBackend,) filter_class = ChannelSetFilter permission_classes = [IsAuthenticated] values = ("id", "name", "description", "channels", "secret_token__token", "edit") diff --git a/contentcuration/contentcuration/viewsets/clipboard.py b/contentcuration/contentcuration/viewsets/clipboard.py index 6b1fd012f3..2ae7a5ab5d 100644 --- a/contentcuration/contentcuration/viewsets/clipboard.py +++ b/contentcuration/contentcuration/viewsets/clipboard.py @@ -3,7 +3,6 @@ from django.db.models import OuterRef from django.db.models import Subquery from django.db.models.functions import Cast -from django_filters.rest_framework import DjangoFilterBackend from le_utils.constants import content_kinds from rest_framework.permissions import IsAuthenticated from rest_framework.serializers import BooleanField @@ -105,7 +104,6 @@ def update(self, instance, validated_data): class ClipboardViewSet(ValuesViewset): permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = ClipboardFilter serializer_class = ClipboardSerializer values = ( diff --git a/contentcuration/contentcuration/viewsets/contentnode.py b/contentcuration/contentcuration/viewsets/contentnode.py index 2054cf1dc2..7c96d99caa 100644 --- a/contentcuration/contentcuration/viewsets/contentnode.py +++ b/contentcuration/contentcuration/viewsets/contentnode.py @@ -16,7 +16,6 @@ from django.utils.timezone import now from django_cte import CTEQuerySet from django_filters.rest_framework import CharFilter -from django_filters.rest_framework import DjangoFilterBackend from django_filters.rest_framework import UUIDFilter from le_utils.constants import content_kinds from le_utils.constants import exercises @@ -517,7 +516,6 @@ class ContentNodeViewSet(BulkUpdateMixin, ChangeEventMixin, ValuesViewset): queryset = ContentNode.objects.all() serializer_class = ContentNodeSerializer permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = ContentNodeFilter values = ( "id", diff --git a/contentcuration/contentcuration/viewsets/file.py b/contentcuration/contentcuration/viewsets/file.py index 8666cd584c..6c65582a56 100644 --- a/contentcuration/contentcuration/viewsets/file.py +++ b/contentcuration/contentcuration/viewsets/file.py @@ -2,7 +2,6 @@ from django.core.exceptions import PermissionDenied from django.http import HttpResponseBadRequest -from django_filters.rest_framework import DjangoFilterBackend from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -85,7 +84,6 @@ class FileViewSet(BulkDeleteMixin, BulkUpdateMixin, ReadOnlyValuesViewset): queryset = File.objects.all() serializer_class = FileSerializer permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = FileFilter values = ( "id", diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index e7ca9b827a..c46107f7ac 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -1,5 +1,4 @@ from django_filters.rest_framework import CharFilter -from django_filters.rest_framework import DjangoFilterBackend from django_filters.rest_framework import FilterSet from rest_framework import serializers from rest_framework.permissions import IsAuthenticated @@ -95,7 +94,6 @@ def get_sender_name(item): class InvitationViewSet(ValuesViewset): queryset = Invitation.objects.all() permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = InvitationFilter serializer_class = InvitationSerializer values = ( diff --git a/contentcuration/contentcuration/viewsets/task.py b/contentcuration/contentcuration/viewsets/task.py index bb80c1bcda..ee89e8ace7 100644 --- a/contentcuration/contentcuration/viewsets/task.py +++ b/contentcuration/contentcuration/viewsets/task.py @@ -1,7 +1,6 @@ import uuid from celery import states -from django_filters.rest_framework import DjangoFilterBackend from django_filters.rest_framework import UUIDFilter from rest_framework.permissions import IsAuthenticated @@ -31,7 +30,6 @@ class TaskViewSet(ReadOnlyValuesViewset, DestroyModelMixin): order_by = 'created' queryset = Task.objects.order_by(order_by) permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = TaskFilter lookup_field = "task_id" diff --git a/contentcuration/contentcuration/viewsets/user.py b/contentcuration/contentcuration/viewsets/user.py index 036a5d6e25..1a9e664d81 100644 --- a/contentcuration/contentcuration/viewsets/user.py +++ b/contentcuration/contentcuration/viewsets/user.py @@ -13,11 +13,9 @@ from django.db.models.functions import Concat from django_filters.rest_framework import BooleanFilter from django_filters.rest_framework import CharFilter -from django_filters.rest_framework import DjangoFilterBackend from django_filters.rest_framework import FilterSet from rest_framework.decorators import action from rest_framework.exceptions import ValidationError -from rest_framework.pagination import PageNumberPagination from rest_framework.permissions import IsAdminUser from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -26,13 +24,12 @@ from contentcuration.models import boolean_val from contentcuration.models import Channel from contentcuration.models import User -from contentcuration.utils.pagination import get_order_queryset +from contentcuration.utils.pagination import ValuesViewsetPageNumberPagination from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer from contentcuration.viewsets.base import ReadOnlyValuesViewset from contentcuration.viewsets.base import RequiredFilterSet from contentcuration.viewsets.base import ValuesViewset -from contentcuration.viewsets.common import CatalogPaginator from contentcuration.viewsets.common import NotNullArrayAgg from contentcuration.viewsets.common import SQCount from contentcuration.viewsets.common import UUIDFilter @@ -42,23 +39,10 @@ from contentcuration.viewsets.sync.constants import VIEWER_M2M -class UserListPagination(PageNumberPagination): +class UserListPagination(ValuesViewsetPageNumberPagination): page_size = None page_size_query_param = "page_size" max_page_size = 100 - django_paginator_class = CatalogPaginator - - def get_paginated_response(self, data): - return Response( - { - "next": self.get_next_link(), - "previous": self.get_previous_link(), - "page_number": self.page.number, - "count": self.page.paginator.count, - "total_pages": self.page.paginator.num_pages, - "results": data, - } - ) class UserFilter(FilterSet): @@ -123,7 +107,6 @@ class UserViewSet(ReadOnlyValuesViewset): queryset = User.objects.all() serializer_class = UserSerializer permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = UserFilter values = ( "id", @@ -178,7 +161,6 @@ class ChannelUserViewSet(ReadOnlyValuesViewset): queryset = User.objects.all() serializer_class = UserSerializer permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) filterset_class = ChannelUserFilter values = ( "id", @@ -357,8 +339,8 @@ class AdminUserViewSet(ValuesViewset): permission_classes = [IsAdminUser] serializer_class = AdminUserSerializer filterset_class = AdminUserFilter - filter_backends = (DjangoFilterBackend,) - base_values = ( + + values = ( "id", "email", "first_name", @@ -369,42 +351,13 @@ class AdminUserViewSet(ValuesViewset): "date_joined", "is_admin", "is_active", + "name", + "edit_count", + "view_count", ) - values = base_values queryset = User.objects.all() - def paginate_queryset(self, queryset): - order, queryset = get_order_queryset(self.request, queryset, self.field_map) - page_results = self.paginator.paginate_queryset( - queryset, self.request, view=self - ) - ids = [result["id"] for result in page_results] - - self.values = self.base_values - queryset = User.objects.filter(id__in=ids).values(*(self.values)) - if order != "undefined": - queryset = queryset.order_by(order) - return queryset.annotate(**self.annotations) - - def get_queryset(self): - self.annotations = self.compose_annotations() - order = self.request.GET.get("sortBy", "") - if order in self.annotations: - self.values = self.values + (order,) - return User.objects.values("id").order_by("email") - def annotate_queryset(self, queryset): - # will do it after paginate excepting for order by - order = self.request.GET.get("sortBy", "") - if order in self.annotations: - queryset = queryset.annotate(**{order: self.annotations[order]}) - return queryset - - def compose_annotations(self): - annotations = {} - annotations["name"] = Concat( - F("first_name"), Value(" "), F("last_name"), output_field=CharField() - ) edit_channel_query = ( Channel.objects.filter(editors__id=OuterRef("id"), deleted=False) .values_list("id", flat=True) @@ -415,9 +368,15 @@ def compose_annotations(self): .values_list("id", flat=True) .distinct() ) - annotations["edit_count"] = SQCount(edit_channel_query, field="id") - annotations["view_count"] = SQCount(viewonly_channel_query, field="id") - return annotations + + queryset = queryset.annotate( + name=Concat( + F("first_name"), Value(" "), F("last_name"), output_field=CharField() + ), + edit_count=SQCount(edit_channel_query, field="id"), + view_count=SQCount(viewonly_channel_query, field="id"), + ) + return queryset @action(detail=True, methods=("get",)) def metadata(self, request, pk=None): diff --git a/contentcuration/search/viewsets/contentnode.py b/contentcuration/search/viewsets/contentnode.py index 2f6a814035..338968c288 100644 --- a/contentcuration/search/viewsets/contentnode.py +++ b/contentcuration/search/viewsets/contentnode.py @@ -1,7 +1,6 @@ import re from django.db.models import Case -from django.db.models import CharField from django.db.models import F from django.db.models import IntegerField from django.db.models import OuterRef @@ -23,6 +22,8 @@ from contentcuration.viewsets.common import NotNullMapArrayAgg from contentcuration.viewsets.common import SQArrayAgg from contentcuration.viewsets.common import SQCount +from contentcuration.viewsets.common import UUIDFilter +from contentcuration.viewsets.common import UUIDInFilter from contentcuration.viewsets.contentnode import ContentNodeViewSet @@ -51,6 +52,22 @@ class ContentNodeFilter(RequiredFilterSet): resources = BooleanFilter(method="filter_resources") assessments = BooleanFilter(method="filter_assessments") created_after = CharFilter(method="filter_created_after") + channel_id__in = UUIDInFilter(name="channel_id") + channel_list = CharFilter(method="filter_channel_list") + exclude_channel = UUIDFilter(name="channel_id", exclude=True) + + def filter_channel_list(self, queryset, name, value): + user = not self.request.user.is_anonymous() and self.request.user + channel_ids = [] + if value == "public": + channel_ids = Channel.objects.filter(public=True, deleted=False).values_list("id", flat=True) + elif value == "edit" and user: + channel_ids = user.editable_channels.values_list("id", flat=True) + elif value == "bookmark" and user: + channel_ids = user.bookmarked_channels.values_list("id", flat=True) + elif value == "view" and user: + channel_ids = user.view_only_channels.values_list("id", flat=True) + return queryset.filter(channel_id__in=list(channel_ids)) def filter_keywords(self, queryset, name, value): filter_query = Q(title__icontains=value) | Q(description__icontains=value) @@ -126,8 +143,6 @@ class SearchContentNodeViewSet(ContentNodeViewSet): "id", "content_id", "node_id", - ) - base_values = ( "title", "description", "author", @@ -147,55 +162,13 @@ class SearchContentNodeViewSet(ContentNodeViewSet): "original_channel_name", ) - def paginate_queryset(self, queryset): - page_results = self.paginator.paginate_queryset( - queryset, self.request, view=self - ) - ids = [result["id"] for result in page_results] - queryset = self._annotate_channel_id(ContentNode.objects.filter(id__in=ids)) - queryset = self.complete_annotations(queryset) - self.values = self.values + self.base_values - return list(queryset.values(*self.values)) - - def get_accessible_nodes_queryset(self): - # jayoshih: May the force be with you, optimizations team... - user_id = not self.request.user.is_anonymous and self.request.user.id - - # Filter by channel type - channel_type = self.request.query_params.get("channel_list", "public") - if channel_type == "public": - channel_args = {"public": True} - elif channel_type == "edit": - channel_args = {"editors": user_id} - elif channel_type == "bookmark": - channel_args = {"bookmarked_by": user_id} - elif channel_type == "view": - channel_args = {"viewers": user_id} - else: - channel_args = {} - - # Filter by specific channels - if self.request.query_params.get("channels"): - channel_args.update({"pk__in": self.request.query_params["channels"]}) - - return ContentNode.objects.filter( - tree_id__in=Channel.objects.filter(deleted=False, **channel_args) - .exclude(pk=self.request.query_params.get("exclude_channel", "")) - .values_list("main_tree__tree_id", flat=True) - .order_by() - .distinct() - ).annotate(channel_id=Value("", output_field=CharField()),) - - def get_queryset(self): - return self.get_accessible_nodes_queryset() - def annotate_queryset(self, queryset): """ 1. Do a distinct by 'content_id,' using the original node if possible 2. Annotate lists of content node and channel pks """ # Get accessible content nodes that match the content id - content_id_query = self.get_accessible_nodes_queryset().filter( + content_id_query = ContentNode.filter_view_queryset(ContentNode.objects.all(), self.request.user).filter( content_id=OuterRef("content_id") ) @@ -214,9 +187,7 @@ def annotate_queryset(self, queryset): queryset = queryset.filter( pk__in=Subquery(deduped_content_query.values_list("id", flat=True)[:1]) ).order_by() - return queryset - def complete_annotations(self, queryset): thumbnails = File.objects.filter( contentnode=OuterRef("id"), preset__thumbnail=True ) @@ -231,9 +202,6 @@ def complete_annotations(self, queryset): .values("id", "role_visibility", "changed") .order_by() ) - content_id_query = self.get_accessible_nodes_queryset().filter( - content_id=OuterRef("content_id") - ) original_channel_name = Coalesce( Subquery( Channel.objects.filter(pk=OuterRef("original_channel_id")).values( diff --git a/contentcuration/search/viewsets/savedsearch.py b/contentcuration/search/viewsets/savedsearch.py index 11e24f6292..e45900883d 100644 --- a/contentcuration/search/viewsets/savedsearch.py +++ b/contentcuration/search/viewsets/savedsearch.py @@ -1,4 +1,3 @@ -from django_filters.rest_framework import DjangoFilterBackend from rest_framework.permissions import IsAuthenticated from rest_framework.serializers import PrimaryKeyRelatedField from search.models import SavedSearch @@ -37,7 +36,6 @@ class SavedSearchViewSet(ValuesViewset): queryset = SavedSearch.objects.all() serializer_class = SavedSearchSerializer permission_classes = [IsAuthenticated] - filter_backends = (DjangoFilterBackend,) values = ( "id", "name", diff --git a/requirements-dev.txt b/requirements-dev.txt index aa75255077..780dcddf56 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -92,7 +92,7 @@ django==3.2.3 # django-debug-toolbar # djangorestframework # drf-yasg -djangorestframework==3.12.1 +djangorestframework==3.12.4 # via # -c requirements.txt # drf-yasg diff --git a/requirements.in b/requirements.in index e4e858af46..ac6d6ce6c7 100644 --- a/requirements.in +++ b/requirements.in @@ -2,7 +2,7 @@ attrs==19.3.0 django-cte==1.1.5 django-mptt==0.11.0 django-filter==2.4.0 -djangorestframework==3.12.1 +djangorestframework==3.12.4 psycopg2-binary==2.8.6 django-js-reverse==0.9.1 django-registration==3.1.1 diff --git a/requirements.txt b/requirements.txt index 9581f819bc..f09a1e6059 100644 --- a/requirements.txt +++ b/requirements.txt @@ -83,7 +83,7 @@ django==3.2.3 # django-s3-storage # djangorestframework # jsonfield -djangorestframework==3.12.1 +djangorestframework==3.12.4 # via -r requirements.in future==0.18.2 # via -r requirements.in