Add channel token support to importchannel management command#14056
Add channel token support to importchannel management command#14056rtibbles wants to merge 1 commit intorelease-v0.19.xfrom
Conversation
github-actions
bot
added
DEV: dev-ops
rtibbles
force-pushed
the
claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn
branch
from
7f66ea8 to
f9d039f
Compare
Build Artifacts
|
rtibbles
removed
SIZE: very large
DEV: dev-ops
rtibbles
force-pushed
the
claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn
branch
from
f9d039f to
a9dbaa8
Compare
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
rtibbles
force-pushed
the
claude/plan-import-issue-01PLz3FuYjwoyrk7gttdFysn
branch
from
a9dbaa8 to
940d830
Compare
nucleogenesis
modified the milestones:
Kolibri 0.19: Planned Patch 2,
Kolibri 0.19: Planned Patch 3
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
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
| 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) |
There was a problem hiding this comment.
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.
| channel_id, all_channels = resolve_channel_token( | ||
| identifier, baseurl=baseurl | ||
| ) | ||
| except (NetworkLocationConnectionFailure, NetworkLocationNotFound): |
There was a problem hiding this comment.
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.
| ) | ||
|
|
||
| def copy_channel(self, channel_id, source_path, no_upgrade, content_dir): | ||
| if not is_valid_uuid(channel_id): |
There was a problem hiding this comment.
praise: Good guardrail — rejecting tokens early for disk imports with a clear error message avoids a confusing failure later in the pipeline.
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Code changes look good to me, and manual QA checks out. Not approving yet, since notes from rtibblesbot seem valuable.
5 participants
Summary
Adds support for using channel tokens in addition to channel IDs when importing channels via the
importchannelmanagement command.Key features:
Example usage:
References
Fixes #3733
Reviewer guidance
networksubcommand;diskrequires UUIDs since tokens need network access to resolveresolve_channel_token()and integration tests intest_import_export.py