Repository navigation
Conversation
3715d35 to
456055a
Compare
b07ec95 to
7cde310
Compare
7cde310 to
1ae8d53
Compare
🟡 Waiting for changesOutcome: skipped — rebased to Last updated: 2026-10-28 16:48 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6096 breaks the mass-email recipient line, which now reads "All [object Object] users". Fix that before merge. The storage fix itself works end to end.
CI passes. Manual QA passed: storage request → CSV, all filters, Clear filters, CSV download, row selection, and responsive/RTL layout. The IconButton :ariaLabel fix gives every shared icon button a real accessible name. axe found nothing new except the Clear filters contrast.
- blocking: mass-email recipient line reads "All [object Object] users" (inline)
- suggestion: User Type "All" counts as an active filter
- suggestion: Clear filters drops focus to
<body>, and its link fails contrast - suggestion: Clear filters duplicates
useTable's query keys - suggestion: search scope hint dropped
- suggestion: CSV test never checks filter params
- nitpick:
record_storage_requestcan be private; magic pixel values; UTC filename date
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6096: two new suggestions on Clear filters. 10 prior findings: 6 resolved, 4 acknowledged.
CI passing. Manual QA passed for the user type select, mass email To line, CSV export, search field and storage request round-trip.
Prior-finding status
RESOLVED — UserTable.vue:368 — mass-email To line reads "All [object Object] users"
RESOLVED — UserTable.vue — User Type "All" counts as an active filter
RESOLVED — UserTable.vue:126 — Clear filters drops focus to <body>
ACKNOWLEDGED — UserTable.vue:126 — Clear filters link fails contrast (KDS primary token)
RESOLVED — UserTable.vue — tableStateQueryParams duplicates useTable's query keys
ACKNOWLEDGED — UserTable.vue:64 — search scope hint dropped
RESOLVED — userTable.spec.js:415 — CSV test never checks filter params
ACKNOWLEDGED — UserTable.vue:520 — magic pixel values
ACKNOWLEDGED — UserTable.vue:477 — UTC filename date
RESOLVED — settings.py — record_storage_request should be private
@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 via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
Superseded by my review of cc02e3b: no blocking findings remain.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6096: all 11 prior findings are resolved. No new findings.
CI passing. Manual QA did not run. The UI is unverified.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue:365 — mass-email To line reads All [object Object] users
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue — User Type → All counts as an active filter
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue:123 — Clear filters drops keyboard focus to <body>
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue — tableStateQueryParams duplicates useTable's query keys
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue:65 — search hint no longer mentions channels
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/tests/userTable.spec.js:437 — test never checks filter params
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue:519 — magic pixel values
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue:476 — CSV filename date off by one near midnight
RESOLVED — contentcuration/contentcuration/views/settings.py — helper should be private
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue:432 — Clear filters doesn't cancel a pending keyword search
RESOLVED — contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue — Clear filters focuses an unnamed div
@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 via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
A storage request only ever sent an email, so the admin Users CSV "Storage needed" column could only show what a user typed at registration. Users who requested storage afterwards showed blank. Persist the requested amount to user.information and prefer it over the registration answer when building the CSV row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Has Studio edits" matched any Change row the user ever created, so it reported activity rather than edits; renamed to "Has Studio activity" in the filter row and the CSV header. On wide viewports the two checkboxes sat in separate quarter-width columns, leaving a large gap between them. They now share one half-width flex row alongside a "Clear filters" action, which resets every filter while preserving pagination and sorting. The action is enabled only once a filter differs from the default its control already displays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spec drove the component through wrapper.vm — assigning filters and reading computed properties — so it asserted internals rather than behaviour. Rewritten against Vue Testing Library, per the frontend testing guidelines, querying by label and role and asserting on the dispatched fetch payload. Two problems surfaced once the queries went through the accessibility tree. "Clear filters" used appearance="basic-link", which KButton renders as an anchor; disabled is not a valid attribute there and KButton's click handler does not guard on it, so the greyed-out link was still focusable and still fired. It is now shown only when there is something to clear. Studio's IconButton passes ariaLabel="text" unbound, leaving icon buttons with no usable accessible name, so those two are still reached by their existing data-test hooks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every user-facing string on the page was hardcoded English, including the browser tab title, the filter option labels, the table headers and the CSV snackbars. Moved them into a usersStrings translator module, each with a context note for translators, and replaced the count strings' ternaries with ICU plurals. The spec references the message keys rather than the English text, per the frontend testing guidelines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replace the Vuetify layout and form controls above the users table with their Kolibri Design System equivalents: VLayout/VFlex become KGrid and KGridItem, VSelect becomes KSelect, VTextField becomes KTextbox with a prepended KIcon, and VProgressLinear becomes KLinearLoader. KSelect emits `select` only on user action, so the filters bind `:value` and `@select` rather than v-model, which would otherwise write the defaults back into the URL and undo "Clear filters". VDataTable, CountryField and UserItem still wrap Vuetify and are left for a later pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The component mixed an Options API shell with composables. Move it wholly into `<script setup>`: the Vuex getters become computeds over useStore, routerMixin's tab title becomes an onMounted call to updateTabTitle, and the responsive headers become a plain function of the breakpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four comments restated what the code already said: two doc blocks naming their own helpers, and a pointer to renderWithFilters repeated above two calls to it. The remaining comments each cite behaviour of code outside this file that a test has to work around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CountryField gains fullWidth, which switches its own max-width from 500px to 100%. KTextbox caps the inner UiTextbox at max-width: 400px, which a pass-through class cannot reach, so it is overridden through the component's appearanceOverrides prop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IconButton passed ariaLabel as a literal string, so every icon button in Studio announced the word "text" to a screen reader. Bind the prop. The Users table tests now find the email and CSV buttons by role rather than by test id, which proves the label reaches the button. isoDateMonthsAgo returned a date in the wrong month. On 31 March the "Last month" window landed on 3 March, because JavaScript rolls an out of range day forward into the next month. Fall back to the last day of the target month instead. filterDefaults repeated the default value that each filter already declares, and nothing kept the two in step. Name the two shared defaults once. Restore the comment that records why the location filter map is built after mount, and add the missing test for unticking select all.
The KDS migration removed the wrapper that turned the user type filter into a plain string, so UserTable passed the whole option object to EmailUsersDialog. The dialog declares that prop as a String and builds its To line by interpolation, which produced "All [object Object] users". Pass the option value instead. Two tests now open the dialog and read that line. "All" was not the declared default, so picking it wrote userType=all and offered "Clear filters" on a list that was not filtered. Download CSV then failed with 412, because the same selection sends no query at all. Make "all" the default, which also fills the select on first load. Clearing filters now removes the filter keys rather than keeping a second copy of the table state keys, so a key useTable gains later is no longer dropped. It also moves focus to the filter actions, because the button that had focus unmounts itself. The search string is a label rather than a placeholder, so drop its trailing ellipsis and correct its context.
Typing schedules a debounced write 500ms later and does not touch the URL until it fires. Clearing filters before then removed nothing, so the watcher that resets the input never ran, and the pending write put the keyword back afterwards. Cancel the debounce and reset the input. Clearing also moved focus to a bare div, which a screen reader cannot name. Focus the search field instead, through the public focus method KTextbox provides. KSelect exposes no such method.
f669728 to
7a24a42
Compare
Summary
This fixes QA follow-ups from #5946, all on the admin Users page.
information["space_needed"]is only written at registration, and only when the user ticks "Storing materials for private or local use" — the post-registration storage request flow just sent an email and persisted nothing. So users who requested storage after signing up showed blank, while older users who answered the registration question showed a value.StorageSettingsViewnow records the requested amount on the user, and the CSV prefers it over the registration answer. No migration:informationis an existingJSONField.Exists(Change.objects.filter(created_by=...)), which matches anyChangerow the user ever created — creating a channel qualifies, and deleting it later doesn't undo it. That is activity, not edits, which is what QA observed. Renamed in the filter row and the CSV header; the query param and backend annotation are unchanged, so existing bookmarked URLs still work.On sign-up (storage specified)
On request storage (storage requested)
UI updates
Before
After
References
Fixes #5946
Reviewer guidance
Note on Target location. It is the one control in the filter row that this PR does not migrate, so it draws a darker box than the KSelect and KTextbox beside it. That difference is visible in the After screenshot and is expected.
CountryFieldis shared with other pages, so migrating it belongs in its own change. The only change here is passing its existingfullWidthprop so that it fills the grid cell.AI usage
Claude Code (Opus 5) diagnosed the reported bugs and proposed fixes and a refactoring plan. The code and approach were reviewed and verified by @akolson.