Add channel token support to importchannel management command by rtibbles · Pull Request #14056 · learningequality/kolibri

rtibbles · 2026-01-12T03:59:49Z

Summary

Adds support for using channel tokens in addition to channel IDs when importing channels via the importchannel management command.

Key features:

Token resolution via Kolibri Studio's channel lookup API
Automatic detection of tokens vs channel IDs using UUID validation
Clear error when token resolves to multiple channels (lists all options)
Full backward compatibility with existing channel ID usage

Example usage:

kolibri manage importchannel network tahid-modal

While addressing review comments from rtibblesbot, it became apparent that our existing channel lookup URL function was returning a full path, which was at adds with how it is used as an argument for NetworkClient.get - simplified it to just return the path, not the fully qualified URL.

References

Fixes #3733

Reviewer guidance

Token resolution only applies to the network subcommand; disk requires UUIDs since tokens need network access to resolve
Test coverage includes unit tests for resolve_channel_token() and integration tests in test_import_export.py

github-actions · 2026-01-13T04:02:57Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.4.dev0_git.20260325025118.pex
Windows Installer (EXE)	kolibri-0.19.4.dev0+git.20260325025118-windows-setup-unsigned.exe
Debian Package	kolibri_0.19.4.dev0+git.20260325025118-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.4.dev0+git.20260325025118.dmg
Android Package (APK)	kolibri-0.19.4.dev0+git.20260325025118-0.1.7-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.4.dev0+git.20260325025118.zip
TAR file	kolibri-0.19.4.dev0+git.20260325025118.tar.gz
WHL file	kolibri-0.19.4.dev0+git.20260325025118-py2.py3-none-any.whl

Smoke test screenshot

Download screenshot

rtibblesbot

Clean implementation of channel token resolution for the importchannel command, addressing #3733.

CI passing. Two suggestions below; no blocking issues found.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
Architecture: duplicated concerns, minimal interfaces, composition over inheritance
Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
Checked CI status and linked issue acceptance criteria
For UI changes: inspected screenshots for layout, visual completeness, and consistency

rtibblesbot · 2026-02-23T23:57:53Z

+    baseurl = baseurl or CENTRAL_CONTENT_BASE_URL
+    lookup_url = get_channel_lookup_url(identifier=token, baseurl=baseurl)
+    client = NetworkClient.build_for_address(baseurl)
+    response = client.get(lookup_url)


suggestion: get_channel_lookup_url returns a fully-qualified URL (scheme + host + path), but NetworkClient.request() does url = join_url(self.base_url, path) — so it joins the base URL with the full URL again. This happens to work because urljoin treats an absolute second argument as-is, but it's fragile and non-idiomatic for this client.

Consider passing just the path portion instead, e.g.:

lookup_path = "api/public/v1/channels/lookup/{}?".format(token) response = client.get(lookup_path)

Or use get_channel_lookup_url without baseurl and strip the prefix — whichever fits the existing patterns better.

rtibblesbot · 2026-02-23T23:57:54Z

+            channel_id, all_channels = resolve_channel_token(
+                identifier, baseurl=baseurl
+            )
+        except (NetworkLocationConnectionFailure, NetworkLocationNotFound):


suggestion: NetworkClient.request() can also raise NetworkLocationResponseTimeout (when the server is reachable but the request times out). This isn't caught here, so it would surface as an unhandled traceback instead of a clean CommandError. Consider adding it to this except clause.

rtibblesbot · 2026-02-23T23:57:54Z

        )

    def copy_channel(self, channel_id, source_path, no_upgrade, content_dir):
+        if not is_valid_uuid(channel_id):


praise: Good guardrail — rejecting tokens early for disk imports with a clear error message avoids a confusing failure later in the pipeline.

AlexVelezLl

Code changes look good to me, and manual QA checks out. Not approving yet, since notes from rtibblesbot seem valuable.

rtibblesbot

Prior findings

Resolved:

Use path instead of full URL with NetworkClient (suggestion) — get_channel_lookup_url replaced with inline path construction at importchannel.py:33
Catch NetworkLocationResponseTimeout (suggestion) — added to except clause at importchannel.py:134

2/2 prior suggestions resolved.

Clean implementation addressing both prior suggestions. CI still running. No new issues found.

The inline path "api/public/v1/channels/lookup/{}?" duplicates knowledge from get_channel_lookup_url in paths.py, but given the URL-joining issue that motivated the change, this is a reasonable trade-off.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-24T21:49:19Z

+    :raises: ValueError if response is invalid
+    """
+    baseurl = baseurl or CENTRAL_CONTENT_BASE_URL
+    lookup_path = "api/public/v1/channels/lookup/{}?".format(token)


suggestion: The trailing ? is carried over from get_channel_lookup_url where it precedes query parameters (keyword, language). Since this call site never appends query params, the ? is unnecessary. It's harmless (servers ignore empty query strings), but could be dropped for clarity:

lookup_path = "api/public/v1/channels/lookup/{}".format(token)

rtibblesbot · 2026-03-24T21:49:19Z

+        # Verify token was resolved (NetworkClient was called)
+        mock_network_client_class.build_for_address.assert_called()
+        # Verify the import proceeded
+        import_channel_mock.assert_called_once()


praise: Thorough end-to-end test — mocks the full chain from token resolution through file download, confirming the token path integrates correctly with the existing import machinery.

rtibblesbot

Clean implementation of token support for importchannel. Both prior suggestions resolved.

Prior findings

2 prior findings unchanged (2 resolved).

All prior findings remain resolved — no changes since last review.

CI: prior run passed; latest run still in progress. No UI changes.

1 suggestion (inline)
1 praise (inline)

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-24T23:42:20Z

+
+    if not channels or not isinstance(channels, list):
+        raise NetworkLocationResponseFailure(
+            "Token '{}' not found on content server".format(token)


suggestion: NetworkLocationResponseFailure expects a response keyword argument (see errors.py:41 — self.response = kwargs.pop("response", None)). Raising it here without one sets self.response = None, which would cause an AttributeError if any caller tried e.response.status_code. That doesn't happen today because _resolve_channel_identifier catches it generically, but it's a fragile contract. A plain ValueError (consistent with the other raises in this function) would be more appropriate here — then _resolve_channel_identifier could catch ValueError for both "not found" and "invalid response" cases.

rtibblesbot · 2026-03-24T23:42:20Z

-    version="1", identifier=None, baseurl=None, keyword=None, language=None
-):
-    content_server_path = "api/public/v{}/channels".format(version)
+def get_channel_lookup_url(version="1", identifier=None, keyword=None, language=None):


praise: Good cleanup — removing the baseurl parameter and the unconditional trailing ? makes this function a pure path builder. All four callers were updated consistently.

rtibblesbot

No new commits since last review. The suggestion about NetworkLocationResponseFailure remains unaddressed.

Prior findings

3 prior findings unchanged (3 resolved).

Still unaddressed:

NetworkLocationResponseFailure raised without response kwarg (suggestion) — no reply, no code change. Re-raised below.

3/4 prior findings resolved. 1 re-raised below.

CI passing. No UI changes.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-25T00:01:28Z

+
+    if not channels or not isinstance(channels, list):
+        raise NetworkLocationResponseFailure(
+            "Token '{}' not found on content server".format(token)


suggestion: (re-raised from prior review) NetworkLocationResponseFailure expects a response keyword argument (self.response = kwargs.pop("response", None) in errors.py:41). Raising it here without one sets self.response = None, which would break any caller that accesses e.response.status_code. A plain ValueError would be more appropriate here — _resolve_channel_identifier already catches ValueError, so this simplifies the exception handling while avoiding the fragile contract.

rtibblesbot

Clean implementation of channel token support for importchannel, addressing #3733.

Prior findings

4 prior findings unchanged (3 resolved, 1 suggestion now also resolved).

The NetworkLocationResponseFailure suggestion (previously re-raised) is now resolved — resolve_channel_token only raises ValueError, which was the recommended fix.

4/4 prior findings resolved.

CI in progress. No UI changes.

1 suggestion (inline)
1 praise (inline)

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-25T02:33:02Z

+    """
+    baseurl = baseurl or CENTRAL_CONTENT_BASE_URL
+    client = NetworkClient.build_for_address(baseurl)
+    response = client.get(get_channel_lookup_url(identifier=token))


suggestion: get_channel_lookup_url can raise NetworkLocationResponseFailure (from the client.get call) but _resolve_channel_identifier doesn't catch it. If the server returns a non-2xx response (e.g., 500), the exception propagates as an unhandled traceback rather than a clean CommandError.

The existing caller in resource_import.py:52 catches NetworkLocationResponseFailure and checks e.response.status_code == 404. Consider catching it here too:

except ( NetworkLocationConnectionFailure, NetworkLocationNotFound, NetworkLocationResponseTimeout, NetworkLocationResponseFailure, ):

Or, if you want to distinguish 404 from other server errors, handle them separately.

rtibblesbot · 2026-03-25T02:33:02Z

-    version="1", identifier=None, baseurl=None, keyword=None, language=None
-):
-    content_server_path = "api/public/v{}/channels".format(version)
+def get_channel_lookup_url(version="1", identifier=None, keyword=None, language=None):


praise: Clean refactor — removing baseurl from this function and the unconditional trailing ? makes it a pure path builder. All callers updated consistently.

Implements support for using channel tokens in addition to channel IDs when importing channels via the importchannel command, addressing issue #3733. Key features: - Token resolution via channel lookup API (works with Studio or any Kolibri instance) - Automatic detection of tokens vs channel IDs using UUID validation - Interactive multi-channel selection when token resolves to multiple channels - Non-interactive mode shows error with all channel options - Comprehensive error handling with user-friendly messages - Full backward compatibility with existing channel ID usage Implementation details: - Added resolve_channel_token() utility in paths.py using duck typing - Returns tuple of (channel_id, all_channels) for flexible handling - Command detects TTY to determine interactive vs non-interactive mode - Interactive mode prompts user to choose from numbered list - Non-interactive mode fails with clear error listing all options - Only network subcommand supports tokens (disk requires UUIDs) Error handling improvements: - JSON parse errors caught with helpful messages - Network failures, invalid tokens, malformed responses all handled - Validates all channels have IDs before returning - Clear error messages guide users to resolution Test coverage: - Token resolution (single and multiple channels) - Invalid JSON responses - Missing channel IDs - Default and custom baseurl - All error paths covered Fixes #3733

All callers pass the result to NetworkClient.get(), which joins it with its own base_url. Returning a full URL caused a fragile double-join via urljoin. Now returns just the path (e.g. /api/public/v1/channels/...) so the NetworkClient can join it correctly with whatever base URL it was built for.

rtibblesbot

No new commits since last review. All prior findings remain resolved.

Prior findings

5 prior findings unchanged (4 resolved, 1 incorrect).

Resolved:

Use path instead of full URL with NetworkClient (suggestion) — resolved in get_channel_lookup_url refactor
Catch NetworkLocationResponseTimeout (suggestion) — added to except clause
Trailing ? without query params (suggestion) — resolved, now conditional
NetworkLocationResponseFailure raised without response kwarg (suggestion) — resolved, resolve_channel_token now raises ValueError instead

Withdrawn (incorrect):

"get_channel_lookup_url can raise NetworkLocationResponseFailure" (suggestion from review 5) — get_channel_lookup_url is now a pure path builder that returns a string; it cannot raise network errors. The client.get() call that can raise NetworkLocationResponseFailure is already caught by _resolve_channel_identifier at line 131.

5/5 prior findings accounted for. CI passing (Raspberry Pi image build pending). No UI changes.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-25T03:09:53Z

+                "Use a channel ID instead.".format(identifier, channel_list)
+            )
+
+        return channel_id


praise: Clean separation of concerns — resolve_channel_token handles network/parsing and raises ValueError, while _resolve_channel_identifier translates those into CommandError with user-friendly messages. The network exception catch at line 128-133 covers all four failure modes consistently.

AlexVelezLl

Code changes make sense, rtibblesbot has ✅, tests provide good coverage on new code, affected parts due to the refactor of get_channel_lookup_url have been properly refactored, and manual QA checks out. lgmt!

github-actions Bot added DEV: dev-ops Continuous integration & deployment DEV: backend Python, databases, networking, filesystem... APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: very large labels Jan 12, 2026

rtibbles changed the base branch from develop to release-v0.19.x January 12, 2026 05:09

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from 7f66ea8 to f9d039f Compare January 13, 2026 03:24

github-actions Bot added the SIZE: medium label Jan 13, 2026

rtibbles assigned AlexVelezLl Jan 13, 2026

rtibbles removed SIZE: very large DEV: dev-ops Continuous integration & deployment DEV: frontend APP: Learn Re: Learn App (content, quizzes, lessons, etc.) labels Jan 13, 2026

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from f9d039f to a9dbaa8 Compare January 13, 2026 15:38

rtibbles added this to the Kolibri 0.19: Planned Patch 2 milestone Jan 13, 2026

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from a9dbaa8 to 940d830 Compare January 13, 2026 17:42

rtibbles marked this pull request as ready for review January 13, 2026 18:13

nucleogenesis requested a review from rtibblesbot February 23, 2026 23:54

nucleogenesis modified the milestones: Kolibri 0.19: Planned Patch 2, Kolibri 0.19: Planned Patch 3 Feb 23, 2026

rtibblesbot reviewed Feb 23, 2026

View reviewed changes

AlexVelezLl reviewed Mar 17, 2026

View reviewed changes

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from 940d830 to 798d43d Compare March 24, 2026 21:41

rtibblesbot reviewed Mar 24, 2026

View reviewed changes

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from 4b3bfe0 to e4c6efc Compare March 24, 2026 23:43

rtibblesbot reviewed Mar 25, 2026

View reviewed changes

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from e4c6efc to 09c932d Compare March 25, 2026 02:13

rtibblesbot reviewed Mar 25, 2026

View reviewed changes

claude and others added 2 commits March 24, 2026 19:49

rtibbles force-pushed the claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn branch from 09c932d to 70aac0b Compare March 25, 2026 02:51

rtibblesbot approved these changes Mar 25, 2026

View reviewed changes

AlexVelezLl approved these changes Apr 7, 2026

View reviewed changes

AlexVelezLl merged commit 560eb16 into release-v0.19.x Apr 7, 2026
56 checks passed

rtibbles mentioned this pull request Apr 7, 2026

allow command-line import to take a token as a parameter #3733

Closed

Uh oh!

Conversation

rtibbles commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Uh oh!

github-actions Bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexVelezLl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rtibbles commented Jan 12, 2026 •

edited

Loading

github-actions Bot commented Jan 13, 2026 •

edited

Loading