Add bitmask fields for ChannelMetadata categories#5272
Add bitmask fields for ChannelMetadata categories#5272AlexVelezLl merged 3 commits intolearningequality:unstablefrom
Conversation
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks @Jakoma02! The bitmasks annotation, and the categories filter are working as expected! And thanks for fixing the bug in the bitmask_lookup computation! I just noted one little bug with the countries field in the values list, but we can fix this in a follow up if it makes more sense!
| for key, labels in metadata_lookup.items(): | ||
| bitmask_lookup = {} | ||
| i = 0 | ||
| while (chunk := labels[i : i + 64]) : |
| "total_resource_count", | ||
| "published_size", | ||
| "categories", | ||
| "countries", |
There was a problem hiding this comment.
There is a small problem with having the countries value here, this makes the channels query to join the country table, and if we have a channel with multiple countries, the query will return duplicated records for each channel. We sometimes solve this by grouping the entity instances and group the multi-value field into an array (e.g. here).
But this method isn't that elegant, and if we filter by that field (in this case, the countries), this countries array will just contain the filtered values, not the actual set of countries of that channel. So thats why we sometimes prefer to query these multi-value fields separately and annotate them in the consolidate method (it can also be in the annotate method), just like we did to set the countries in the Community Library Submission viewset here.
There was a problem hiding this comment.
Also, now that we are returning the countries here. Could you also add a countries filter here, please? It should be a CharInFilter that filters by the country code. I know this issue wasn't originally intended for this 😅, but now that we are here, it'd be nice if we include it too, it should be just a couple of lines. We can also do it in a follow up if that makes more sense!
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Code changes look good, manual QA confirms this is working as expected, and tests give more confidence. Thanks @Jakoma02!
2 participants
Summary
This PR adds bitmask fields for ChannelMetadata categories in
kolibri_publicand allows filtering the channels by the categories.Detailed changes:
ContentNodeandChannelMetadatabitmasks insearch.py_populate_bitmask_datamethodhas_all_labelslogic under searchsearch.pyset_channel_metadata_fieldsChannelMetadataQuerysetandChannelMetadataManagerwith bitmasks supportbitmasks_contains_andto be a top-level functioncategoriesandcountriesto fields returned byChannelMetadataViewSet(this seems to have been forgotten earlier)ChannelMetadataViewSetDateTimeTzFieldand default active databasereverse_with_querymethod to thehelpers.pyfileNo manual testing apart from running automated tests was done.
References
Resolves #5208.
A WIP branch with an alternative implementation using a mixin (ended up not being pursued) can be found at https://github.com/Jakoma02/studio/tree/wip-channel-categories-bit-masks-mixin.
Reviewer guidance
Nothing.