Use manifest.json files to download collections #436
Conversation
danigm
force-pushed
the
manifest-import
branch
2 times, most recently
from
8d3a5fe to
fed0380
Compare
|
This is a custom build of the plugin for testing: |
danigm
force-pushed
the
manifest-import
branch
2 times, most recently
from
fad508a to
ce50571
Compare
| @downloadCollection="downloadCollection" | ||
| @goBack="visibleModal = 'grade'" | ||
| /> | ||
| <InstallContentModal | ||
| :visible="contentModalVisible" | ||
| :collection="downloadingCollection" | ||
| :grade="grade" |
There was a problem hiding this comment.
I feel like "grade" is an awkward word to use here since it's very specific to our current implementation, but at the same time the alternatives I can think of ("group", "category") would be terrrible and confusing, so 🤷
kolibri_explore_plugin/views.py
Outdated
| for _grade, collections in self.COLLECTIONS.items(): | ||
| for _k, v in collections.items(): |
There was a problem hiding this comment.
Two small nitpicks: this collections variable is a narrower scope than COLLECTIONS, so I think it would be clearer if it was named grade_collections; and I'm wondering why we're using COLLECTIONS.items() instead of COLLECTIONS.values()?
There was a problem hiding this comment.
You're right, it's better to use values instead of items and ignoring the key.
kolibri_explore_plugin/views.py
Outdated
| else: | ||
| # Fallback, download full collection using the token | ||
| token = self.COLLECTIONS[grade][collection]["token"] | ||
|
|
||
| channel_viewset = RemoteChannelViewSet() | ||
| channels = channel_viewset._make_channel_endpoint_request( | ||
| identifier=token | ||
| ) |
There was a problem hiding this comment.
Should we keep this fallback around? I can see the awkwardness we're avoiding since the manifest files are included as extra data, but the fallback only really kicks in if something goes wrong, and it behaves differently from the regular case. I can see it leading to confusion.
There was a problem hiding this comment.
Yes, it's better to remove this fallback, because as you said it could hide some error and will make it more difficult to debug. I'll replace with an error in the interface.
| # Done this way to convert [] to None | ||
| node_ids=channel.get("include_node_ids") or None, | ||
| # Done this way to convert [] to None | ||
| exclude_node_ids=channel.get("exclude_node_ids") or None, |
There was a problem hiding this comment.
👍 there's a subtle difference in how node_ids=[] is handled between the code that copies data and the code that marks nodes as available, so this is a useful way to deal with that.
It's worth noting this behaviour will be different with learningequality/kolibri#9460. Setting include_node_ids: [] is the same as saying "no nodes", while include_node_ids: None means "the default, which is all of the nodes".
Once we have Kolibri with my changes applied, the manifest file code should actually never create a manifest file with the latter: if it includes all the nodes from a channel, it ends up with include_node_ids: [${channel_id}]. If a manifest file has include_node_ids: [], it's probably wrong but there's no need arguing with it.
Hopefully in a future iteration we can pass a manifest file directly to the remoteimport task so we don't need to deal with all this :)
|
Just a couple small suggestions, but nothing showstopping :) Feel free to merge this when you're happy with it, @danigm! |
This patch install the collection using the token if the manifest file is not present or can't be found. https://phabricator.endlessm.com/T33570
This patch fixes the bug of the carousel items not shown when a channel is available after download. https://phabricator.endlessm.com/T33570
https://phabricator.endlessm.com/T33570