Thread token-resolved metadata (library, version) through channel import pipeline#14598
Conversation
Build Artifacts
Smoke test screenshot |
|
The Build APK file / build_apk failure is a CI infrastructure issue unrelated to this PR's changes. Root cause: When python-for-android builds the Why this isn't caused by our changes:
This is a known flakiness in the p4a build environment with |
|
Suggestions from the pending self-review have been addressed:
|
|
Both suggestions (re: |
rtibbles
left a comment
There was a problem hiding this comment.
A good start, but some changes needed.
| } | ||
| ] | ||
| ) | ||
| from kolibri.core.content.utils.resource_import import ( |
There was a problem hiding this comment.
All of these inline imports are completely unnecessary, don't do this.
There was a problem hiding this comment.
Removed all inline imports and moved them to the top-level import block.
| peer = serializers.PrimaryKeyRelatedField( | ||
| required=False, queryset=NetworkLocation.objects.all().values("base_url", "id") | ||
| ) | ||
| token = serializers.CharField(required=False, allow_null=True, allow_blank=True) |
There was a problem hiding this comment.
We can add some validation here too - a token is 10 characters optionally with a - in the middle. We should coerce it to lowercase as well.
There was a problem hiding this comment.
Added validate_token to RemoteImportMixin that coerces to lowercase and validates against ^[a-z0-9]{10}$|^[a-z0-9]{5}-[a-z0-9]{5}$.
kolibri/core/content/upgrade.py
Outdated
|
|
||
|
|
||
| @version_upgrade(old_version="<0.19.5") | ||
| def populate_channel_library(): |
There was a problem hiding this comment.
Let's change this to populate_channel_library_field.
There was a problem hiding this comment.
Renamed to populate_channel_library_field.
kolibri/core/content/upgrade.py
Outdated
| backfill_content_request_priority.enqueue_if_not_active() | ||
|
|
||
|
|
||
| @version_upgrade(old_version="<0.19.5") |
There was a problem hiding this comment.
0.19.4 is the next version of Kolibri to be released, so this needs to be <0.19.4 - the value is just already set in the code.
There was a problem hiding this comment.
Changed to <0.19.4.
kolibri/core/content/tasks.py
Outdated
| def remotechannelimport(channel_id, baseurl=None, peer_id=None): | ||
| def remotechannelimport(channel_id, baseurl=None, peer_id=None, token=None): | ||
| transfer_channel(channel_id, DOWNLOAD_METHOD, baseurl=baseurl) | ||
| if token: |
There was a problem hiding this comment.
I don't understand why we are handling this near identical code block three times in the tasks, when this could be consolidated in an import manager (including updating remotechannelimport to use an import manager class, but without importing any resources).
There was a problem hiding this comment.
Added a RemoteChannelDatabaseImportManager class and moved the set_channel_metadata_fields call into RemoteResourceImportManagerBase.run(). The three tasks (remotechannelimport, remotecontentimport, remoteimport) now all delegate to the manager's run() without any post-run metadata handling in the tasks themselves.
kolibri/core/content/tasks.py
Outdated
| token=token, | ||
| ) | ||
| manager.run() | ||
| if token and manager.listing_found: |
There was a problem hiding this comment.
This should be handled internally to the import manager.
There was a problem hiding this comment.
Moved into RemoteResourceImportManagerBase.run().
kolibri/core/content/tasks.py
Outdated
| token=token, | ||
| ) | ||
| manager.run() | ||
| if token and manager.listing_found: |
There was a problem hiding this comment.
This should be handled internally to the import manager.
There was a problem hiding this comment.
Moved into RemoteResourceImportManagerBase.run().
|
The |
Yes - there is no need to resolve this in the scope of this PR. |
rtibblesbot
left a comment
There was a problem hiding this comment.
All acceptance criteria met. CI is still running (some unit test jobs pending, no failures yet). No UI files changed.
| # Verify token was resolved (NetworkClient was called) | ||
| mock_network_client_class.build_for_address.assert_called() | ||
| # Verify token was resolved via lookup_channel_listing_status | ||
| mock_lookup.assert_called_once() |
There was a problem hiding this comment.
suggestion: mock_set_fields is patched but never asserted. If the set_channel_metadata_fields call in download_channel were accidentally dropped or its arguments changed, this test would still pass. Adding mock_set_fields.assert_called_once_with(self.the_channel_id, library="KOLIBRI", version=5) would close the gap for the CLI path. The equivalent assertion is already present in RemoteChannelImportTaskTestCase.test_token_causes_library_to_be_persisted for the tasks path.
| "library": "KOLIBRI", | ||
| } | ||
| call_command("importchannel", "network", "my-channel-token") | ||
| mock_lookup.assert_called_once() |
There was a problem hiding this comment.
suggestion: Same gap as test_network_import_with_token — mock_set_fields is captured but never asserted. Since this test patches transfer_channel directly and sets up a full lookup return value with library="KOLIBRI", version=5, it's the cleaner place to add mock_set_fields.assert_called_once_with(the_channel_id, library="KOLIBRI", version=5). That would give the CLI path a full round-trip assertion.
|
|
||
| return { | ||
| "id": channel_info.get("id"), | ||
| "public": channel_info.get("public"), |
There was a problem hiding this comment.
praise: The four distinct response cases (404/None, token-channel mismatch, multi-channel token, single channel) are each handled explicitly and exercised by dedicated tests in LookupChannelListingStatusTest. The draft-version sentinel (returning None for version → callers store 0) also has its own path in test_zero_version_stored_as_zero. Thorough edge-case coverage for the central resolution function.
1b6ea5e to
06af9df
Compare
rtibbles
left a comment
There was a problem hiding this comment.
This is nearly there - just one small piece of cleanup needed.
| self.assertTrue(validator.is_valid(), validator.errors) | ||
|
|
||
|
|
||
| class RemoteChannelImportTaskTestCase(TestCase): |
There was a problem hiding this comment.
I don't think this test class is needed - it's just asserting an implementation detail about the task.
|
The |
f8569ee to
ef90ce8
Compare
…factor lookup_channel_listing_status
Bump le-utils to 0.2.17 which provides library metadata in channel listing
responses. Add channel_versions parameter to get_channel_lookup_url so callers
can filter by version. Refactor lookup_channel_listing_status to accept
channel_id/token/baseurl directly and return a full metadata dict
{id, public, version, library} instead of a bare boolean.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fields Add ChannelMetadata.library CharField to store the Kolibri library name a channel belongs to (e.g. KOLIBRI). Add a migration for the new field. Update set_channel_metadata_fields to accept library and version parameters so they can be persisted alongside other channel metadata after import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… import managers Add token parameter to RemoteResourceImportManagerBase, which is used to fetch channel metadata after a transfer completes and persist library/version via set_channel_metadata_fields. Extract RemoteChannelDatabaseImportManager as a dedicated class for channel database downloads (without resource import). Add token support to ContentDownloadRequestResourceImportManager. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l command Add token field and validate_token method to RemoteImportMixin (validates 10-char or 5-5 hyphenated format, lowercases). Use RemoteChannelDatabaseImportManager in remotechannelimport task. Add token parameter to remotecontentimport and remoteimport tasks, passing it through to the import managers. Simplify importchannel command by replacing resolve_channel_token with lookup_channel_listing_status and calling set_channel_metadata_fields after channel database download. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add populate_channel_library_field upgrade function (runs on <0.19.4) that sets ChannelMetadata.library=KOLIBRI for all channels already marked as public, covering channels imported before the library field was introduced. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ef90ce8 to
18acfe2
Compare
|
The |
rtibbles
left a comment
There was a problem hiding this comment.
Excellent - implemented to spec, good to go.
2c7c840
into
learningequality:release-v0.19.x
Plan: Thread token-resolved metadata (version, library, public) from Studio's v1 channel lookup endpoint through the full import pipeline, and persist it on
ChannelMetadata.channel_versionsto URL builderlookup_channel_listing_statusto accept a token and return full metadatalibraryfield toChannelMetadata, migration, andset_channel_metadata_fieldstokentoRemoteResourceImportManagerBaseand store metadatatokento validators and tasks; callset_channel_metadata_fieldsafter importresolve_channel_tokenwithlookup_channel_listing_statuslibrary="KOLIBRI"for existing public channelsSummary
Previously,
lookup_channel_listing_statusonly returned whether a channel waspublic, discarding token-specific metadata (version, library) after resolution. Token resolution in the CLI and frontend import paths was also siloed, and the core import utilities re-fetched channel info by channel ID — losing the original token context entirely.This makes
lookup_channel_listing_statusthe single token-resolution point: it now accepts a token or channel ID, calls the v1 endpoint withchannel_versions=truewhen a token is given, and returns a full metadata dict (version, library, public). Import managers, tasks, and validators pass the token through so resolved library and version are stored onChannelMetadataafter import. Draft channel tokens (where Studio returnsversion: null) are stored asversion=0. Existingpublic=Truechannels are backfilled tolibrary="KOLIBRI"via an upgrade task.This PR also includes unrelated changes in two areas:
Coach plugin: Internal status helpers in
class_summary_api.pyhave been extracted into reusable functions (get_content_log_values,get_log_status,fetch_notification_maps), and a newUnitLessonProgressViewSetendpoint has been added atcoursesession/<id>/unit/<id>/lessonprogress/.Locale middleware / redirect views:
RootURLRedirectViewandGuestRedirectViewincore/views.pyhave been refactored from plainViewsubclasses toRedirectViewsubclasses (withGuestRedirectViewnow extendingRootURLRedirectView).KolibriLocaleMiddlewaregains a_language_url()helper extracted from the 404-handling path, and a new optimisation: when a language-less request resolves (with the language prefix) to aRedirectViewsubclass, the middleware rewritesrequest.path_infobefore dispatch so the two-hop chain (language-prefix redirect → view redirect) is collapsed into a single redirect response.References
Closes #14588
See also learningequality/studio#5772
Reviewer guidance
0045_channelmetadata_library.pyadds a nullablelibrarycolumn — verify it is reversible and handles existing rows correctlylookup_channel_listing_statusinresource_import.pyis now the central token-resolution point — review error handling when a token resolves to a different channel ID than the one provided, and thechannel_versions=trueparameter threadingupgrade.pybackfillslibrary="KOLIBRI"for existing public channels on server startup — verify idempotency so it won't re-run on subsequent restartsclass_summary_api.pyrefactored to exposeget_content_log_values,get_log_status, andfetch_notification_mapsas module-level helpers; newUnitLessonProgressViewSetinunit_lesson_progress_api.pywired intoapi_urls.py. Note: these changes are not related to issue Update core channel import utilities to handle tokens directly #14588 and may warrant a separate PR.RootURLRedirectViewandGuestRedirectViewconverted toRedirectViewsubclasses;KolibriLocaleMiddlewarenow collapses two-hop redirect chains by rewritingrequest.path_infowhen the prefixed path resolves to aRedirectView. Verify the one-shot redirect tests intest_locale_middleware.pyand confirm the 404-fallback behaviour is preserved for non-RedirectViewviews.AI usage
Implemented with Claude Code following a pre-approved plan using test-driven development. I reviewed the plan, approved each phase, and verified the test suite passed before the final commit.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?