Add Organizations' User Invitation UI - #6126
Conversation
|
👋 Hi @yasinelmi, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
1b1371e to
d7e3b63
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesLast updated: 2026-09-17 22:09 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — CI passing; manual QA did not run.
@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
| (SessionAuthentication, BasicAuthentication, TokenAuthentication) | ||
| ) | ||
| @permission_classes((IsAuthenticated,)) | ||
| def send_organization_invitation_email(request): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: No send_mail — nobody is notified; Resend invitation mails nothing. Mirror templates/permissions/permissions_email*.txt.
| const organizations = ref([]); | ||
|
|
||
| onMounted(() => { | ||
| Organization.fetchCollection({ page_size: MAX_PAGE_SIZE }) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Unfiltered — filter_view_queryset (models.py:1969) returns public=True orgs too. Add a member filter to OrganizationFilter.
| self.assertEqual(response.status_code, 400, response.content) | ||
| invitation.refresh_from_db() | ||
| self.assertFalse(invitation.accepted) | ||
| self.assertFalse(self.channel.editors.filter(pk=self.invited_user.id).exists()) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Restore — assertFalse(invitation.accepted) alone misses the guard's effect.
| * Pass a falsy organizationId to use this in "create a new organization" mode: | ||
| * the fetch is skipped and `create` becomes usable instead of `update`. | ||
| */ | ||
| export function useOrganization(organizationId) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Captured at setup(); RouterView reuses the instance, so Save after Create calls Organization.update('', data). Take a getter.
| email, | ||
| shareMode: this.role.value, | ||
| }) | ||
| .then(() => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: No .catch here or at OrganizationDetailsTab.vue:139, OrganizationUsersTable.vue:169,199, useOrganizationInvitations.js:34,40; shared/client.js re-rejects — use handleMembershipError.
| v-if="!isNew" | ||
| class="tabs-nav" | ||
| > | ||
| <KTabsList |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: No KTabsPanel, so aria-controls (KTabsList.vue:201) targets a missing id.
| if not invitation: | ||
| invitation = Invitation.objects.create(**fields) | ||
|
|
||
| invitation.share_mode = share_mode |
There was a problem hiding this comment.
suggestion: Unvalidated share_mode → 500 at accept (models.py:3920).
| INVITATION, | ||
| {"revoked": True}, | ||
| channel_id=invitation.channel_id, | ||
| user_id=request.user.id, |
There was a problem hiding this comment.
suggestion: user_id is the revoker; the invitee never receives this.
| > | ||
| <template #menu> | ||
| <KDropdownMenu | ||
| :options="menuOptions(rows[rowIndex][3])" |
There was a problem hiding this comment.
suggestion: rowIndex indexes KTable's sortedRows, not rows — wrong target once sortable.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — 6 of 9 prior findings resolved; 3 open, 2 new (1 blocking).
CI pending; manual QA did not run.
Still open, see existing threads: unvalidated share_mode (views/users.py:155); revoke notifies the revoker (viewsets/invitation.py:268); rowIndex indexes sortedRows (OrganizationUsersTable.vue:29).
Prior-finding status
RESOLVED — contentcuration/contentcuration/views/users.py:122 — Resend invitation mails nothing
RESOLVED — contentcuration/contentcuration/frontend/channelList/composables/useOrganizationList.js:14 — unfiltered org list
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:802 — editors assertion
RESOLVED — contentcuration/contentcuration/frontend/channelList/composables/useOrganization.js:10 — organizationId captured at setup()
RESOLVED — contentcuration/contentcuration/frontend/channelList/views/Organization/InviteOrganizationUserForm.vue:92 — missing .catch
RESOLVED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationEditPage.vue:17 — KTabsPanel aria-controls
UNADDRESSED — contentcuration/contentcuration/views/users.py:155 — unvalidated share_mode
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:268 — revoke notifies the revoker
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationUsersTable.vue:29 — rowIndex indexes sortedRows
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| {% if user and user.is_active %} | ||
| {% translate "Please sign in to Kolibri Studio to accept or decline your invitation:" %} | ||
| {{ domain }}{% url 'channels' %} |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: {% url 'channels' %} lands on CHANNELS_EDITABLE; the org invitation banner only exists at /my-organizations ({% else %} branch redirects to base, same problem).
Org invitations also reach that page as broken channel invitations: filter_invited (viewsets/invitation.py:142) matches email alone, loadInvitationList (vuex/channelList/actions.js:45) returns them, StudioMyChannels/index.vue:119 filters only share_mode === 'edit' — so ChannelInvitation.vue renders a null channel name, ChannelListIndex.vue:196 inflates the badge, and accepting dispatches channel/loadChannel(null).
Both branches should link {{ domain }}{% url 'channels' %}#/my-organizations, and loadInvitationList should drop organization-bearing rows, as useOrganizationInvitations.js:20 already does.
| @@ -0,0 +1,5 @@ | |||
| export function getApiErrorMessage(error, fallback) { | |||
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: OrganizationUsersTable.handleMembershipError (211-215) is still the verbatim body of this util — 3 of 4 call sites converted.
| }, | ||
| mixins: [routerMixin], | ||
| setup(props) { | ||
| const { loading, organization, update, create } = useOrganization(() => props.organizationId); |
There was a problem hiding this comment.
suggestion: Untested. OrganizationEditPage.spec.js passes static props, so it passes either way; a repro needs RouterView with props: true, then Save on the details tab asserting Organization.update gets org-2.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — 7 of 12 prior findings resolved; 5 still open.
CI pending; manual QA did not run.
Still open:
views/users.py:155—share_modeunvalidatedviewsets/invitation.py:268— mails revokerOrganizationUsersTable.vue:29—rowIndex/sortedRowschannelList/utils.js:1— duplicatehandleMembershipErrorOrganizationEditPage.vue:77— untested
Prior-finding status
RESOLVED — contentcuration/contentcuration/views/users.py:122 — send_mail
RESOLVED — contentcuration/contentcuration/frontend/channelList/composables/useOrganizationList.js — public=True
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:802 — assertFalse guard
RESOLVED — contentcuration/contentcuration/frontend/channelList/composables/useOrganization.js — stale id
RESOLVED — contentcuration/contentcuration/frontend/channelList/views/Organization/InviteOrganizationUserForm.vue:92 — .catch
RESOLVED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationEditPage.vue:17 — aria-controls
RESOLVED — contentcuration/contentcuration/templates/permissions/organization_permissions_email.txt — link target
UNADDRESSED — contentcuration/contentcuration/views/users.py:155 — share_mode
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:268 — user_id
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationUsersTable.vue:29 — rowIndex
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/utils.js:1 — duplicate
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationEditPage.vue:77 — untested
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| {{ domain }}{% url 'channels' %}#/my-organizations | ||
| {% else %} | ||
| {% translate "Please follow this link to create an account (you must be signed in to accept or decline invitations):" %} | ||
| {{ domain }}{% url 'accept_invitation_and_registration' email %}#/my-organizations |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The redirect chain drops this fragment: new_user_redirect (views/users.py:444,459) sends new invitees to /accounts/#/create?email=..., and a UA keeps the original fragment only when Location has none (RFC 7231 §7.1.2). Drop it, or pass the target as a query param.
|
|
||
| self.send_invitation("active-invitee@example.com", "edit") | ||
|
|
||
| self.assertIn("#/my-organizations", mail.outbox[0].body) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Both branches carry #/my-organizations, so the active-User setup buys nothing and the registration branch stays untested. Assert /channels/#/my-organizations plus a no-User case.
| ]); | ||
| }); | ||
| }); | ||
| it('should exclude organization invitations', () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Asserts getter output, not the filter.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — all 7 prior blocking findings resolved; 7 earlier suggestion threads remain open on code untouched by this delta.
CI pending. Manual QA did not run, so nothing here is visually verified. One new suggestion inline on the sole-admin guard.
Prior-finding status
RESOLVED — contentcuration/contentcuration/views/users.py:122 — no send_mail on resend
RESOLVED — contentcuration/contentcuration/frontend/channelList/composables/useOrganizationList.js — unfiltered org queryset
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:802 — restore guard assertion
RESOLVED — contentcuration/contentcuration/frontend/channelList/composables/useOrganization.js — id captured at setup()
RESOLVED — contentcuration/contentcuration/frontend/channelList/views/Organization/InviteOrganizationUserForm.vue:92 — missing .catch handlers
RESOLVED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationEditPage.vue:17 — aria-controls targets a missing id
RESOLVED — contentcuration/contentcuration/templates/permissions/organization_permissions_email.txt — wrong invitation link target
UNADDRESSED — contentcuration/contentcuration/views/users.py:155 — unvalidated share_mode
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:268 — revoke change addressed to the revoker
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationUsersTable.vue:29 — rowIndex indexes sortedRows
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/utils.js:1 — handleMembershipError duplicates getApiErrorMessage
UNADDRESSED — contentcuration/contentcuration/frontend/channelList/views/Organization/OrganizationEditPage.vue:77 — organizationId reactivity untested
UNADDRESSED — contentcuration/contentcuration/templates/permissions/organization_permissions_email.txt:19 — redirect chain drops the fragment
UNADDRESSED — contentcuration/contentcuration/tests/test_organization_invitation.py:66 — both branches assert the same URL fragment
ACKNOWLEDGED — contentcuration/contentcuration/frontend/channelList/vuex/channelList/tests/module.spec.js:55 — praise
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| if (target.type !== 'member') { | ||
| return false; | ||
| } | ||
| const activeAdmins = this.members.filter(member => member.role === OrganizationRoles.ADMIN); |
There was a problem hiding this comment.
suggestion: members is one page, not the org.
useOrganizationMembers.js:18 fetches with page_size: 100 against a viewset capped at max_page_size = 100 (viewsets/organization.py:131) and never pages further. In an org with >100 active members whose second admin falls outside page 1, activeAdmins.length === 1 is true for an admin who is not the sole admin: the options button disappears and there is no way to demote or remove them, silently. The server counts all active admins, so it would have allowed the action.
Derive the flag from a count the server supplies, or page the fetch to completion.
fb78f38 to
262edf4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — 11 of 16 prior findings resolved; 5 open: views/users.py:155, viewsets/invitation.py:268, OrganizationUsersTable.vue:29, OrganizationEditPage.vue:77, OrganizationUsersTable.vue:158.
CI pending; manual QA did not run.
Prior-finding status
RESOLVED — views/users.py:122 — send_mail
RESOLVED — useOrganizationList.js — member
RESOLVED — test_invitation.py:802 — editors
RESOLVED — useOrganization.js — organizationId
RESOLVED — InviteOrganizationUserForm.vue:92 — .catch
RESOLVED — OrganizationEditPage.vue:17 — KTabsPanel
RESOLVED — organization_permissions_email.txt — #/my-organizations
RESOLVED — organization_permissions_email.txt — new_user_redirect
RESOLVED — test_organization_invitation.py — {% else %}
RESOLVED — channelList/utils.js:1 — handleMembershipError
RESOLVED — module.spec.js:55 — getters
UNADDRESSED — views/users.py:155 — share_mode
UNADDRESSED — viewsets/invitation.py:268 — user_id
UNADDRESSED — OrganizationUsersTable.vue:29 — rowIndex
UNADDRESSED — OrganizationEditPage.vue:77 — useOrganization
UNADDRESSED — OrganizationUsersTable.vue:158 — members
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| export default async function startApp({ store, router, index }) { | ||
| trackInputModality(); | ||
| await initiateServiceWorker(); | ||
| try { |
There was a problem hiding this comment.
suggestion: Out of scope — changes startup for every bundle; own PR.
| @@ -0,0 +1,136 @@ | |||
| <template> | |||
There was a problem hiding this comment.
suggestion: Near-verbatim fork of Channel/ChannelInvitation.vue — template, decline modal, .invitation SCSS, $trs. Vuex blocks reuse; make it presentational.
| invitation.refresh_from_db() | ||
| self.assertTrue(invitation.revoked) | ||
|
|
||
| def test_sender_can_revoke_even_if_no_longer_admin(self): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Neither guard branch is isolated: organization_role defaults to status=ACTIVE, and this admin is the sender. Drop is_org_admin or the sender_id clause — all four pass.
| } | ||
|
|
||
| # Need to break into two steps to avoid MultipleObjectsReturned error | ||
| invitation = Invitation.objects.filter( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Comment dropped from untouched code — why filter().first(), not get_or_create. Also router.js, ChannelListIndex.vue.
| adminRole: 'Admin', | ||
| editorRole: 'Editor', | ||
| viewerRole: 'Viewer', | ||
| pendingRole: 'Pending {role}', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Pending {role} nests $tr('adminRole') — untranslatable gender and order. Three flat strings instead; line 146 also calls $tr('').
dfed586 to
ced4d89
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — every finding from the last round is resolved. One new suggestion below; seven suggestions from earlier rounds are still open in their existing threads and are not restated here.
Reviewed at dfed586. The head has since advanced to ced4d89, a squash whose tree is identical — no unreviewed content. CI was pending and manual QA did not run, so this is a comment rather than an approval.
Prior-finding status
RESOLVED — views/users.py:123 — no send_mail on organization invitations
RESOLVED — composables/useOrganizationList.js — unfiltered filter_view_queryset returns public orgs
RESOLVED — tests/viewsets/test_invitation.py:802 — restore the dropped guard assertion
RESOLVED — composables/useOrganization.js — stale id captured at setup()
RESOLVED — InviteOrganizationUserForm.vue:92 — missing .catch on invitation/membership promises
RESOLVED — OrganizationEditPage.vue:17 — aria-controls targets a missing id
RESOLVED — organization_permissions_email.txt — {% url 'channels' %} lands on the wrong page
RESOLVED — channelList/utils.js:1 — handleMembershipError duplicated the util verbatim
RESOLVED — organization_permissions_email.txt — redirect chain drops the fragment
RESOLVED — tests/test_organization_invitation.py — redundant active-User setup
RESOLVED — vuex/channelList/tests/module.spec.js:55 — asserts getter output, not the filter
RESOLVED — views/users.py:76 — comments dropped from untouched code (also router.js, ChannelListIndex.vue)
RESOLVED — tests/viewsets/test_invitation.py:836 — revoke guard branches not isolated
RESOLVED — OrganizationUsersTable.vue — Pending {role} nested translation
UNADDRESSED — views/users.py:156 — unvalidated share_mode → 500 at accept
UNADDRESSED — viewsets/invitation.py:268 — user_id is the revoker; the invitee never receives this
UNADDRESSED — OrganizationUsersTable.vue:29 — rowIndex indexes sortedRows, not rows
UNADDRESSED — OrganizationEditPage.vue:77 — untested
UNADDRESSED — OrganizationUsersTable.vue:164 — members is one page, not the org
UNADDRESSED — shared/app.js:313 — out of scope; own PR
UNADDRESSED — OrganizationInvitation.vue:1 — near-verbatim fork of ChannelInvitation.vue
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| invitation = Invitation.objects.create(**fields) | ||
|
|
||
| # Handle these values separately as different users might invite the same user again | ||
| invitation.share_mode = share_mode |
There was a problem hiding this comment.
suggestion: A fourth dropped comment is still missing above this line — # Handle these values separately as different users might invite the same user again (b2379a2, line 86). The code it annotates is unchanged. send_organization_invitation_email repeats the same two-step lookup at 145-157 with neither comment; worth carrying both across.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — 14 of 22 prior findings resolved; 8 open.
CI passing; manual QA did not run.
Open: views/users.py:156, views/users.py:87, viewsets/invitation.py:268, OrganizationUsersTable.vue:29, OrganizationUsersTable.vue:164, OrganizationEditPage.vue:77, OrganizationInvitation.vue:1, shared/app.js:313.
New findings inline.
Prior-finding status
RESOLVED — views/users.py:123 — send_mail
RESOLVED — useOrganizationList.js — member=true
RESOLVED — test_invitation.py:802 — editors
RESOLVED — useOrganization.js — getOrganizationId()
RESOLVED — InviteOrganizationUserForm.vue:92 — .catch
RESOLVED — OrganizationEditPage.vue:17 — KTabsPanel
RESOLVED — organization_permissions_email.txt — {% url 'channels' %}
RESOLVED — organization_permissions_email.txt — #/my-organizations
RESOLVED — channelList/utils.js:1 — handleMembershipError
RESOLVED — test_organization_invitation.py — new_user_redirect
RESOLVED — test_invitation.py:836 — revoke_invitation
RESOLVED — views/users.py:76 — # comments
RESOLVED — OrganizationUsersTable.vue — Pending {role}
RESOLVED — module.spec.js:55 — getters
UNADDRESSED — views/users.py:156 — share_mode
UNADDRESSED — viewsets/invitation.py:268 — user_id
UNADDRESSED — OrganizationUsersTable.vue:29 — sortedRows
UNADDRESSED — OrganizationUsersTable.vue:164 — members page
UNADDRESSED — OrganizationEditPage.vue:77 — organizationId
UNADDRESSED — OrganizationInvitation.vue:1 — ChannelInvitation.vue
UNADDRESSED — shared/app.js:313 — registerServiceWorker
UNADDRESSED — views/users.py:87 — # comment
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| query: { last: RouteNames.MY_ORGANIZATIONS }, | ||
| }); | ||
| }, | ||
| acceptInvitation(invitationId) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: accept() (useOrganizationInvitations.js:34-38) drops the invitation, but nothing refetches useOrganizationList, so noOrganizationsFound stays until reload. Export refresh and await it here, as vuex/channelList/actions.js:57-75 does.
| if (!getOrganizationId()) { | ||
| return; | ||
| } | ||
| load().finally(() => { |
There was a problem hiding this comment.
suggestion: No .catch — a failed fetch leaves organization null, indistinguishable from empty. Same in useOrganizationList.js:14, useOrganizationMembers.js:24, useOrganizationInvitations.js:28, all unhandled (client.js:113). Compare useChannelList.js:32-46.
| }); | ||
| } | ||
|
|
||
| function close(roleId) { |
There was a problem hiding this comment.
suggestion: close hard-deletes, losing joined_at; PATCH {status: 'inactive'} keeps it and hits the same guard (viewsets/organization.py:308-313). OrganizationRoleStatuses.INACTIVE (constants.js:63) is unused — intended?
| @@ -0,0 +1,5 @@ | |||
| export function getApiErrorMessage(error, fallback) { | |||
| const data = error && error.response && error.response.data; | |||
| const message = Array.isArray(data) ? data[0] : null; | |||
There was a problem hiding this comment.
suggestion: data[0] is untranslated DRF text (viewsets/organization.py:281-283) that displaces the $tr fallback.
Adds the frontend for issue learningequality#6088: a My Organizations landing page (pending-invitations banner + org card grid), an organization edit page with Details and Sharing tabs, an invite-by-email form, and a unified members/pending-invites table with role change, remove, resend, and revoke actions. Reuses StudioImmersiveModal for the edit page so it matches the channel edit/details page pattern, including its focus trap and dark banner header. Also supports creating a new organization through the same page. All new state is Composition API composables (no new Vuex), and every new component uses Kolibri Design System components exclusively (no Vuetify). Backend: adds a send_organization_invitation_email endpoint, a revoke action and organization support on InvitationViewSet, and a role annotation on top of learningequality#6080's OrganizationViewSet so the frontend can show the current user's role in an organization without an extra request per organization. Also includes fixes from review: the invitation email actually sends (with organization-specific templates), the organization list only shows orgs you're a member of, organization invitations no longer leak into the channel invitation UI, Save-after-Create reuses the newly created id instead of a stale one, network errors surface via snackbar instead of failing silently, KTabsList's aria-controls targets a real KTabsPanel, the sole active admin of an org can't be offered a way to remove or demote themselves that would only fail server-side, pending role labels no longer compose translated strings inside another translation, a revoke-permission test properly isolates the path it's meant to cover, and accepting an invitation refreshes the organization list instead of leaving the empty state stale until reload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
8deb2fd to
17f2865
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — 15/26 prior findings resolved, 11 unchanged. CI green; manual QA did not run.
Prior-finding status
RESOLVED — views/users.py:123 — send_mail
RESOLVED — useOrganizationList.js — member=
RESOLVED — tests/viewsets/test_invitation.py:802 — assertFalse(invitation.accepted)
RESOLVED — useOrganization.js — props.organizationId
RESOLVED — InviteOrganizationUserForm.vue:92 — .catch
RESOLVED — OrganizationEditPage.vue:17 — KTabsPanel
RESOLVED — organization_permissions_email.txt — {% url 'channels' %}
RESOLVED — channelList/utils.js:1 — handleMembershipError
RESOLVED — organization_permissions_email.txt — #/my-organizations
RESOLVED — tests/test_organization_invitation.py — setUp
RESOLVED — vuex/channelList/__tests__/module.spec.js:55 — getters
RESOLVED — tests/viewsets/test_invitation.py:836 — organization_role
RESOLVED — views/users.py:76 — MultipleObjectsReturned
RESOLVED — OrganizationUsersTable.vue — Pending {role}
RESOLVED — StudioMyOrganizations.vue:126 — accept() + refresh()
UNADDRESSED — views/users.py:156 — share_mode → 500
UNADDRESSED — viewsets/invitation.py:268 — user_id
UNADDRESSED — OrganizationUsersTable.vue:29 — sortedRows
UNADDRESSED — OrganizationEditPage.vue:77 — props
UNADDRESSED — OrganizationUsersTable.vue:164 — members
UNADDRESSED — shared/app.js:313 — serviceWorker
UNADDRESSED — OrganizationInvitation.vue:1 — ChannelInvitation.vue
UNADDRESSED — views/users.py:87 — # Handle these values separately
UNADDRESSED — useOrganization.js:25 — .catch
UNADDRESSED — useOrganizationMembers.js:37 — joined_at
UNADDRESSED — channelList/utils.js:3 — data[0]
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| } = useOrganizationInvitations({ organization: props.organizationId }); | ||
|
|
||
| function sendOrganizationInvitation({ organizationId, email, shareMode }) { | ||
| return Invitation.sendOrganizationInvitation({ organizationId, email, shareMode }).then( |
There was a problem hiding this comment.
suggestion: refreshInvitations() may resolve from the fetchCollection cache (resources.js:809-820, REFRESH_INTERVAL = 5) — a resend within 5s shows no new row. Update invitations locally, as revoke does.
| expect(await screen.findByText('Email is required')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('sends the invitation with the entered email and selected role', async () => { |
There was a problem hiding this comment.
suggestion: role keeps its created() default in both cases, asserting only shareMode: 'view' — a swapped roleOptions pair passes. Add an Admin case asserting shareMode: 'admin'.
| expect(await screen.findByText('Acme')).toBeInTheDocument(); | ||
| expect(screen.getByRole('textbox', { name: 'Organization name' })).toBeInTheDocument(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
suggestion: Passes even when OrganizationSharingTab throws or renders empty. fetchModel mocks no role: assert the non-admin notice.
| $trs: { | ||
| editText: '{sender} has invited you to edit {organization}', | ||
| viewText: '{sender} has invited you to view {organization}', | ||
| ownText: '{sender} has invited you to own {organization}', |
There was a problem hiding this comment.
suggestion: ownText says ownership; elsewhere Admin, Pending Admin, administer an organization. One term per concept.
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
Hi @yasinelmi!
Overall, this is looking really good. You did a nice job with the implementation, great use KComponents, and an initial round of accessibility testing looked good. :) The main feedback I have is around some of the LLM-driven frontend tests and strings. I left an inline comment just as a particular example of some broader feedback/approach I should have shared about this earlier. We can chat more with the group in the meeting on Tuesday. There isn't documentation for this anywhere I don't think, because we're still trying to figure out best practices here, and there are inconsistent examples throughout the codebase, which doesn't help. 🙃
First, let's create a file like organizationStrings.js where all of the strings for the feature will live, rather than within each component file. (I realize there is a mix of this -- we've been trying to move to this over time, even though there are still a majority of component files that are not like this). Here is an example where you can see how the file is set up, and then see how these strings are referenced elsewhere.
contentcuration/contentcuration/frontend/shared/strings/searchRecommendationsStrings.js
Second, with LLM-written frontend tests, we have found, tend to over-index on using specific strings and having the tests be essentially a "getByText".
This can be challenging for a couple of reasons. 1) it doesn't necessarily test the interaction well (unless the getByText is being used to identify an element and trigger something like a click). 2) Before a feature is released, we often adjust the strings slightly when we prepare for translation. Having lots of tests across many files be specific-string dependent is just too brittle for this process because slight wording changes mean failing tests.
So, all of that to say.
- I'd like you to review the tests and take a look at which ones are really testing meaningful interactions. I know "meaningful" is somewhat subjective -- broadly, things like interactions, validation messages, confirmation messages, etc. Generally, for us this means things that are end to end, not just about display. Let's please prioritize those. Things that are display only, particularly display of text only (things existing on the page) can be removed.
- Then, I'd as you to please go through and update the string usage. For strings, it's fine to use a specific string when you're are referencing the test data within the test file (say, "My Test Organization 1"), but otherwise, using the strings references (for example:
adminAccessRequiredForEdits$instead of "Only organization admins can edit these details.") makes our translation and pre-release process much more manageable.
I know this is a lot of information, and apologies again that I didn't think of sharing it sooner. As I mentioned, we are still really working on adapting practices around LLM-driven coding. Happy to talk through any questions or approaches you might have on slack or tomorrow in our meeting!
| <KCheckbox | ||
| :checked="isPublic" | ||
| :label="$tr('publicLabel')" | ||
| :description="$tr('publicDescription')" |
There was a problem hiding this comment.
I think it's because we are copying much of the Channels UI (which is correct!) but I don't think we should add this here, because (at least as of now), we are not extending the UI to have a browsable list of organizations. If nothing else in your PR is conditionalized on this, let's remove it, at least for now. We can always add it back in later if we need to.
| @@ -0,0 +1,128 @@ | |||
| <template> | |||
|
|
|||
| <KCard | |||
| expect(screen.getByRole('textbox', { name: 'Organization name' })).toBeDisabled(); | ||
| expect(screen.queryByRole('button', { name: 'Save changes' })).not.toBeInTheDocument(); | ||
| expect( | ||
| screen.getByText('Only organization admins can edit these details.'), |
There was a problem hiding this comment.
Citing this because it's the only one that would need to be updated, but I think because it's a better example to imagine changing the wording slightly, than something like a button that says "Save".
…ic toggle Per review feedback: - All $trs blocks across the 8 Organization components are consolidated into shared/strings/organizationStrings.js (createTranslator pattern, matching shared/strings/searchRecommendationsStrings.js), so translators work from one file instead of eight, and adjusting wording pre-release no longer means finding it scattered across components. - Removes the "Public" checkbox and all related code from OrganizationDetailsTab (isPublic state, the organization.public watch, the public field in the save payload). Nothing else in the feature reads organization.public, and there's no browsable-organizations page for it to matter to yet. The backend public field defaults to False, so omitting it from the payload is a no-op there. - Removed 6 tests whose assertions were already implied by a stronger neighboring test in the same file (a shallow "emits an event" or "renders initial state" check subsumed by the full interaction/ end-to-end test right next to it), plus both tests in OrganizationSharingTab.spec.js (pure text-presence checks with no interaction, and the file is deleted since nothing remains). One unique assertion (no Sharing tab while creating) was folded into the test that already covers the rest of that flow. - Updated the remaining tests to assert against organizationStrings references instead of hardcoded literal UI text, so a wording tweak during translation prep doesn't silently break test coverage. Test fixture data (org names, emails made up for a test) stays literal, per the same guidance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6126 — delta re-review: 15 of 30 prior findings resolved, 15 open elsewhere. CI pending, QA skipped.
Prior-finding status
RESOLVED — views/users.py:123 — send_mail
RESOLVED — composables/useOrganizationList.js — public=True
RESOLVED — tests/viewsets/test_invitation.py:802 — assertRaises
RESOLVED — composables/useOrganization.js — organizationId
RESOLVED — InviteOrganizationUserForm.vue:93 — .catch
RESOLVED — OrganizationEditPage.vue:23 — aria-controls
RESOLVED — organization_permissions_email.txt — {% url 'channels' %}
RESOLVED — organization_permissions_email.txt:19 — #/my-organizations
RESOLVED — channelList/utils.js:1 — handleMembershipError
RESOLVED — tests/test_organization_invitation.py — new_user_redirect
RESOLVED — tests/viewsets/test_invitation.py:836 — organization_role
RESOLVED — views/users.py:76 — filter().first()
RESOLVED — OrganizationUsersTable.vue — Pending {role}
RESOLVED — StudioMyOrganizations.vue:128 — accept()
RESOLVED — vuex/channelList/__tests__/module.spec.js:55 — getters
UNADDRESSED — views/users.py:156 — share_mode
UNADDRESSED — viewsets/invitation.py:268 — user_id
UNADDRESSED — OrganizationUsersTable.vue:29 — sortedRows
UNADDRESSED — OrganizationEditPage.vue:84 — organizationId
UNADDRESSED — OrganizationUsersTable.vue:165 — members
UNADDRESSED — shared/app.js:313 — initState
UNADDRESSED — OrganizationInvitation.vue:1 — ChannelInvitation.vue
UNADDRESSED — views/users.py:87 — # Handle these values separately
UNADDRESSED — composables/useOrganization.js:25 — .catch
UNADDRESSED — composables/useOrganizationMembers.js:37 — close
UNADDRESSED — channelList/utils.js:3 — data[0]
UNADDRESSED — OrganizationSharingTab.vue:63 — refreshInvitations()
UNADDRESSED — __tests__/InviteOrganizationUserForm.spec.js:44 — role
UNADDRESSED — __tests__/OrganizationEditPage.spec.js:79 — OrganizationSharingTab
UNADDRESSED — OrganizationInvitation.vue:93 — ownText
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- 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
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| const memberRows = this.members.map(member => [ | ||
| member.user_name || member.user_email, | ||
| member.user_email, | ||
| organizationStrings[`${roleLabels[member.role] || member.role}$`](), |
There was a problem hiding this comment.
suggestion: Unknown member.role builds undefined$; the call throws in rows and the table never renders. Drop || member.role and index an explicit map (my flat-strings request produced this form).
| }); | ||
|
|
||
| const user = userEvent.setup(); | ||
| const nameInput = screen.getByRole('textbox', { name: 'Organization name' }); |
There was a problem hiding this comment.
suggestion: 'Organization name' also at :36,97,118,143 and OrganizationEditPage.spec.js:77,90,126,131; use nameLabel$().
| }); | ||
|
|
||
| describe('OrganizationCard', () => { | ||
| it('navigates to the organization edit page from the options menu', async () => { |
There was a problem hiding this comment.
suggestion: Two pruned tests were behavioral: card click drives navigation; deleted OrganizationSharingTab.spec.js covered the non-admin gate.
| message: 'Users', | ||
| context: 'Heading and accessible caption for the organization members table', | ||
| }, | ||
| name: { |
There was a problem hiding this comment.
suggestion: name sits six lines from nameLabel; prefer memberNameHeader-style keys.
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
Thanks @yasinelmi ! Appreciate the edits here on the tests and strings
4 participants
Summary
Implements the frontend for #6088: a My Organizations landing page, an organization
edit page with Details and Sharing tabs (invite/manage members), and organization
creation — following the existing channel edit/details page pattern.
Closes #6088
New pages and features
Framework compliance (per the issue's Frameworks checklist)
(
KCard,KButton,KTextbox,KCheckbox,KSelect,KTable,KTabsList,KModal,KDropdownMenu,StudioImmersiveModal).(
useOrganization,useOrganizationList,useOrganizationMembers,useOrganizationInvitations).Accessibility
ariaLabel/tooltip, each carrying a per-itemdistinguishing label rather than a generic repeated one — e.g. "More options for
{org name}" on organization cards, "Options for {email}" on each row of the
members/invitations table, and "Accept/Decline invitation to {organization}" on
each pending-invitation banner entry.
StudioImmersiveModal'sKFocusTrap), with Escape closing it — matching the existing channel details page.cue that the SPA navigated even though there's no real page reload.
a visual dimming.
Test plan
pytest— organization, organization-role-annotation, and invitation tests pass(102 backend tests)
pnpm test— full channelList frontend suite passes (226 tests)pre-commit run --all-fileson all touched fileschanges, resend/revoke invites, member removal (including the last-active-admin
guard), organization creation, and the non-admin read-only states
Notes for reviewers
composables) per the issue's Frameworks requirements.
🤖 Generated with Claude Code