Add support for channel version tokens on v1 public channel endpoint#5772
Add support for channel version tokens on v1 public channel endpoint#5772AlexVelezLl wants to merge 1 commit intolearningequality:unstablefrom
Conversation
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean implementation that adds channel version token resolution to the public v1 channel lookup endpoint.
CI pending. No UI changes.
- suggestion:
select_relatedwould be more efficient thanprefetch_relatedfor FK fields (see inline) - suggestion:
_get_thumbnail_encodingduplicates serializer logic (see inline) - praise: Key-parity test is a solid contract safeguard (see inline)
@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
| # If channels doesnt exist with the given token, check if this is a token of | ||
| # a channel version. | ||
| channel_version = ChannelVersion.objects.prefetch_related( | ||
| "secret_token", "channel" |
There was a problem hiding this comment.
suggestion: secret_token and channel are ForeignKey fields, not M2M. select_related would be more efficient here — it uses a single JOIN query instead of the separate queries that prefetch_related issues for each FK.
channel_version = ChannelVersion.objects.select_related(
"secret_token", "channel"
).filter(...)| if channel.thumbnail_encoding: | ||
| base64 = channel.thumbnail_encoding.get("base64") | ||
| if base64: | ||
| return base64 |
There was a problem hiding this comment.
suggestion: This function is an exact copy of PublicChannelSerializer.get_thumbnail_encoding (in contentcuration/serializers.py:43). Consider extracting it to a shared module-level function that both this code and the serializer call, to keep the fallback logic in one place.
| self.assertEqual(len(channel_version_response.data), 1) | ||
| self.assertEqual(len(channel_response.data), 1) | ||
|
|
||
| self.assertSetEqual( |
There was a problem hiding this comment.
praise: Good contract test — asserting key parity between the two serialization paths catches drift early. The version-specific filtering in the first test (version_notes only up to version 4) is also well-targeted.
AlexVelezLl
force-pushed
the
support-channel-version-tokens
branch
from
2a17a6e to
8fa622c
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Resolved:
select_relatedvsprefetch_relatedfor FK fields (suggestion) — code atviews_v1.py:100already usesselect_related
Unaddressed (re-raised below):
_get_thumbnail_encodingduplication (suggestion) — no reply, no code change
1/2 prior findings resolved. 1 re-raised below.
Clean implementation overall. The version-filtering in _get_version_notes, the select_related usage, and the key-parity test are all solid.
CI in progress.
@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
| base64 = channel.thumbnail_encoding.get("base64") | ||
| if base64: | ||
| return base64 | ||
|
|
There was a problem hiding this comment.
suggestion: This is still an exact copy of the module-level get_thumbnail_encoding in contentcuration/serializers.py (which this PR extracted — nice). _serialize_channel_version could import and call that shared function directly instead of maintaining a second copy:
from contentcuration.serializers import get_thumbnail_encodingMinor difference: the serializer version explicitly return None, this one doesn't. Functionally identical in Python, but importing the shared function avoids the drift risk entirely.
AlexVelezLl
force-pushed
the
support-channel-version-tokens
branch
from
8fa622c to
16109dd
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
Resolved:
select_relatedvsprefetch_relatedfor FK fields (suggestion) — already resolved in prior delta_get_thumbnail_encodingduplication (suggestion) — resolved: the function was extracted to module level inserializers.py(line 25) andviews_v1.pyimports it (line 24, used at line 56). No duplicate copy exists.
0 findings re-raised.
Clean implementation. All prior findings are resolved. No new issues found.
CI: Python unit tests still pending; linting, frontend build, and message files all pass.
@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
| "language": channel.language_id, | ||
| "public": channel.public, | ||
| "description": channel.description, | ||
| "icon_encoding": get_thumbnail_encoding(channel), |
There was a problem hiding this comment.
praise: Good call extracting get_thumbnail_encoding to a shared module-level function and importing it here — resolves the duplication concern from the prior review cleanly.
2 participants
Summary
Reviewer guidance
GET /api/public/v1/channels/lookup/{token}and check the information is relevant to the specific version.AI usage
Used AI to generate the tests for the new additions.