Migrate the learn plugin onto the new resource methods - #15155
Conversation
|
Acceptance criteria from #15059, with the evidence for each. Posted here because this account cannot edit an issue body (
Anchored on
No
New spec Two things found on the way, both left alone:
|
45ab7a0 to
083a490
Compare
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks! Just a couple of nitpicks. Also smoke tested the Learn pluggin, and didn't find any regression. So, after the fixes this should be ready for QA! Please add instructions for QA in the PR description. We will need QA pretty much on the entire plugin, but specially mention areas that need more attention.
| const studio = await RemoteChannelResource.getKolibriStudioStatus_v2(); | ||
| set(allDevices, [ | ||
| { | ||
| ...response.data, | ||
| ...studio, |
There was a problem hiding this comment.
Could we rename this to studioStatus or something more meaningful?
There was a problem hiding this comment.
Renamed to studioStatus.
| }, | ||
| // Overrides `Resource.retrieve` to read through this resource's `cache`, which any of its | ||
| // fetch methods may have populated. As in `fetchModel` above, a cache hit ignores `params`. | ||
| async retrieve(id, { params } = {}) { | ||
| if (!id) { | ||
| throw TypeError('An id must be specified'); | ||
| } | ||
| if (this.cache[id]) { | ||
| return cloneDeep(this.cache[id]); | ||
| } | ||
| return this._requestAndCache({ action: 'detail', routeParams: id, params }); | ||
| }, | ||
| // Overrides `Resource.list` to populate this resource's `cache` with every node it returns. | ||
| async list(params = {}) { | ||
| return this._requestAndCache({ params }); | ||
| }, | ||
| async _requestAndCache(options) { | ||
| const { data } = await this.request(options); | ||
| this.cacheData(data); | ||
| return data; | ||
| }, |
There was a problem hiding this comment.
I think the _requestAndCache is a good idea, but I think the way this PR handles this is sligthly better, because there we actually call the underlying method, which, for now its equivalent to the code change here, but if in the future we change something in the base methods, we will need to recall to update it here too.
There was a problem hiding this comment.
Agreed — retrieve and list now call Resource.prototype.retrieve/list and cache what comes back, so a later change to the base methods reaches this resource without an edit here. _requestAndCache survives for fetchTree_v2 and fetchBookmarks_v2, whose custom endpoints have no base method to delegate to.
|
Both nitpicks addressed on the current head, and a QA section is now in the description. Thanks for the smoke test! (No commit SHA cited on purpose — this branch is periodically folded and force-pushed, which kills any SHA link. Read the current diff instead.) |
Build Artifacts
Smoke test screenshot |
fef56a6 to
fa831a0
Compare
|
The red
|
177f25b to
c8e86f3
Compare
|
Update: |
c8e86f3 to
350b27d
Compare
|
@rtibblesbot Reiterating here the need for a concise reviewer guidance specific for Manual QA in the PR body. It might be the best to, if the PR requires manual QA, always specify testing workflows separately (under their own headings): one for the code review, and another suitable for manual QA reviewers, not engineers. Code review
Manual QA
|
|
Split into Manual QA is now written for a non-engineer: no code identifiers, five highest-risk workflows spelled out as steps, then the sweep list for the rest of the plugin. Behaviour is unchanged everywhere, so the whole plugin does need a pass — the five are where a regression would hide. |
The shared resources get _v2 variants over request, with the originals left intact for consumers not yet migrated. Those built on getListEndpoint resolve with response.data instead of the whole axios response, so their call sites unwrap. LearnerCourseResource is learn-only, so getResumeData is rewritten in place onto request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
force is dropped from useLearnerResources' four public fetchers. Without the Model/Collection cache it bypassed nothing, so advertising it would promise behaviour the code no longer has. ClassAssignmentsPage's 30s poll is unaffected: every tick hits the network either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fetchLessonResources stays: it targets usercontentnode_list, a different resource name than request can resolve, and already bypasses the Model/Collection layer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CustomContentRenderer's sendRandomCollection still reads .results off what the random endpoint returns as a bare array. That expression was already undefined before this change, and stays undefined after; it is a pre-existing bug in the sandbox custom-channel API, left for its own issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With no Model/Collection cache there is nothing stale for the quiz renderers' LearnerClassroomResource.clearCache() to clear: the next list() always hits the network. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
350b27d to
d72a0f9
Compare
|
Thanks @AlexVelezLl - no regressions observed while manually testing the Learn plugin - good to go! |
radinamatic
left a comment
There was a problem hiding this comment.
Manual QA passes, good to go! ![]()
Summary
Every resource call in
kolibri/plugins/learnnow uses the newResourcemethods. Sharedkolibri-commoncustom methods gained_v2variants; theclearCache()calls are gone. Reads that the retired cache served now hit the network —requestcoalesces only concurrent identical GETs.References
Fixes #15059. Recipe: #15057. New methods: #15056. Evidence per criterion.
learn— two greps return nothing._v2additions, originals untouched;learn-onlygetResumeDatarewritten in place.useRetrieve/useListwhere possible — nowhere. The threeuseFetchsites inCourseUnitView/index.vuecompose or wrap custom endpoints, anduseListdrives only the default list endpoint. Every other read is one-shot.pnpm test-jestgreen forlearn— 35 suites, 930 tests.PATCH /issues/15059returns403 Must have admin rights to Repository, so a maintainer has to tick the issue body.Code review
useLearnerResources.js:300—forcegone from the four public fetchers; the cache they bypassed is gone.ContentNodeResource.retrieve's cache hit ignoresparams,baseurlincluded — the same node on two devices resolves to whichever was fetched first. Unchanged fromfetchModel, and why the two-device QA below.CustomContentRenderer.vue:359reads.resultsoff a bare array —undefinedbefore and after. Pre-existing.Manual QA
Nothing in Learn should look or behave differently — this is a plumbing change with no new UI. Every screen in the plugin was touched, so it all needs a pass. These five carry the most risk:
Normal depth everywhere else: library and topic browsing including "show more", bookmarks, the resource-info side panel, recommendations after finishing a resource, downloads, pinning and unpinning devices, lesson playlists, the quiz and exam players, and course units.
Deviations from the issue spec
ContentNodeResource.fetchResumeandfetchLessonResources— called fromlearn, left on the raw client. They hitkolibri:core:usercontentnode_list, a resource namethis.requestcannot resolve, so neither recipe branch applies.AI usage
Used Claude Code to apply the recipe call site by call site. Verified with the Jest suites and greps for every retired name.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟡 Waiting for feedback
Last updated: 2026-08-11 14:22 UTC