Insert checkbox column in Facility > Users KTable to allow user selection #13479

AllanOXDi · 2025-06-11T20:58:42Z

Summary

This Add checkbox column User table to enable individual and multiple user selection

Screen.Recording.2025-06-11.at.23.51.58.mov

References

#13426

Reviewer guidance

Check if

Selection state persists correctly across user interactions
Select all functionality works with paginated data
Individual selections update the select all state appropriately
Accessibility features work with screen readers and keyboard navigation
Bulk actions can access selected user data correctly

github-actions · 2025-06-11T21:31:04Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.1a0.dev0_git.134.g3124d6c2.pex
Windows Installer (EXE)	kolibri-0.18.1a0.dev0+git.134.g3124d6c2-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.1a0.dev0+git.134.g3124d6c2-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.1a0.dev0+git.134.g3124d6c2.dmg
Android Package (APK)	kolibri-0.18.1a0.dev0+git.134.g3124d6c2-0.1.4-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.18.1a0.dev0+git.134.g3124d6c2.zip
TAR file	kolibri-0.18.1a0.dev0+git.134.g3124d6c2.tar.gz
WHL file	kolibri-0.18.1a0.dev0+git.134.g3124d6c2-py2.py3-none-any.whl

nucleogenesis

Thanks Allan, I see the selection working in manual testing. I made some suggestions for clean-up and noted some necessary changes.

nucleogenesis · 2025-06-11T22:40:36Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+              />
+              <KButton
+                appearance="basic-link"
+                text="filter"


I missed this in the strings file, can you add it to the file please?

nucleogenesis · 2025-06-11T22:40:59Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+
+              <KButton
+                appearance="basic-link"
+                text="clear "


CommonCoreStrings.clearAction will work here.

nucleogenesis · 2025-06-11T22:42:13Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

      return {
        selectedUser: null,
        modalShown: null,
+        selectedUsers: new Set(),


Let's move everything from the data() method into the setup() method and use refs for them - ie, const selectedUser = ref(null)

nucleogenesis · 2025-06-11T22:45:10Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

      tableHeaders() {
        return [
+          {
+            label: '',


I think we still want there to be a label on this which should be passed to the KCheckbox and w/ the :showLabel="false" we ensure there is an accessible label still present, even if it's not visible.

The label for this should be "Select all" which I think at this point can be added to the common core strings file.

Once we merge this, we can make a follow-up issue to update the 4 other instances that define their own "Select all" messages (ie, quiz management, user table)

nucleogenesis · 2025-06-11T23:05:01Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+        if (selectedVisibleUsers.length === 0) {
+          return { checked: false, indeterminate: false };
+        } else if (selectedVisibleUsers.length === visibleUserIds.length) {
+          return { checked: true, indeterminate: false };
+        } else {
+          return { checked: false, indeterminate: true };
+        }


I like the feel of this that you can check selectAllState.(checked|indeterminate) to get the current value in the component above.

The if/else/if/else though makes it a little hard to mentally map what conditions result in which values.

I think the core part of the logic can be written like:

checked: true if the number of visible users equal to number of selected users, otherwise checked: false

indeterminate: true if checked == false and the number of selected users is greater than 0; otherwise indeterminate: false

This is a bit easier to read than:

Are no users selected? Then checked: false, indeterminate: false

Are the same # of users selected than are visible? Then checked: true, indeterminate: false

Otherwise, checked:false, indeterminate: true

Could you try reworking this function to read a bit more like the first narrative? I think returning the object still would be great, but it might be nice to clean up the if/else chain. Happy to chat about this on Slack or a call if that'd be helpful.

nucleogenesis · 2025-06-11T23:05:49Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

-        // Otherwise, only non-SUs can be edited.
-        return this.isSuperuser || !user.is_superuser;
-      },
+      // manageUserOptions(userId) {


Are these no longer needed?

marcellamaki · 2025-06-12T14:28:25Z

@radinamatic - Jacob has left some code changes for Allan, so I think we will wait to do the full manual QA feedback until the code review is ✅, but I'm adding you as a reviewer for a11y purposes. I'm going to do an initial pass as I do my code review/manual review now, just to catch anything that I can spot, and then I will ask for you to jump in and do the more thorough one :)

nucleogenesis

Thanks Allan! I've added a few comments and questions - the aria-label stuff is blocking but the other questions may not be.

One thing though that I was thinking about - particularly if you're running into reactivity issues where you do something like this.selectedUsers = new Set(this.selectedUsers) - I wonder if maybe these things you're adding to the methods: {} section could be written instead in the setup() method as functions.

Then you can just refer directly to selectedUsers.value for the getting/setting stuff.

I don't think we've explicitly decided to move away from the Vue Options API - but I do think that keeping it consistent within a single file is a good idea wherever we can.

nucleogenesis · 2025-06-12T23:18:22Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+        const isChecked =
+          selectedVisibleUsers.length === visibleUserIds.length && selectedVisibleUsers.length > 0;
+        const isIndeterminate =
+          selectedVisibleUsers.length > 0 && selectedVisibleUsers.length < visibleUserIds.length;


This could be simplified even further using !isChecked as the second half of your boolean expression here.

nucleogenesis · 2025-06-12T23:32:41Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+            <KIconButton icon="assignCoaches" />
+            <KIconButton icon="add" />
+            <KIconButton icon="remove" />
+            <KIconButton icon="download" />
+            <KIconButton icon="trash" />


These should have an :ariaLabel and :tooltip attr added that use the strings here -- also you can remove the "download" icon as we won't be implementing that here.

nucleogenesis · 2025-06-12T23:33:53Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

            dataType: 'string',
            minWidth: '300px',
-            width: '40%',
+            width: '35%',


Why did these need to be changed?

nucleogenesis · 2025-06-12T23:35:49Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+          this.selectedUsers.add(user.id);
+        }
+
+        this.selectedUsers = new Set(this.selectedUsers);


Is this necessary to do? Did you run into an issue with reactivity or something that led you to directly assign a new Set to this.selectedUsers?

nucleogenesis · 2025-06-12T23:36:44Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+        this.selectedUsers = new Set(this.selectedUsers);
+      },
+      clearSelectedUsers() {
+        this.selectedUsers = new Set();


There's a clear() method available to sets so should be able to do this.selectedUsers.clear() here.

marcellamaki · 2025-06-13T13:55:39Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+                :ariaLabel="selectAllLabel$()"
+                @change="handleSelectAllToggle"
+              >
+                <span class="visuallyhidden">{{ selectAllLabel$() }}</span>


@radinamatic - it seems to me we don't need both the aria level and the visually hidden span here. Is there one or the other that we should prefer?

marcellamaki · 2025-06-13T13:57:39Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+
          <template #cell="{ content, colIndex, row }">
            <span v-if="colIndex === 0">
+              <KCheckbox


We do need either a visually-hidden label or an aria-label (please follow whichever @radinamatic indicates is the preferred option based on the question above) for these checkboxes

Each row checkbox should announce its label by screen readers: e.g. “Select John Doe, coach.”

LianaHarris360

I just left a few clean-up comments, adding the checkbox column is a super important piece of the 0.19 feature work, thank you for working on this Allan!

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

LianaHarris360 · 2025-06-12T20:55:56Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+            label: '',
            dataType: 'undefined',
-            minWidth: '150px',
+            minWidth: '180px',


This is causing the last column to be wider than necessary. Because this minWidth property is optional, we can either remove it completely, or set it to a smaller value because the OPTIONS button that goes in this column takes up less space than it previously did.

This isn't a blocker for this PR at all, but this last column is still wider than it needs to be.

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

marcellamaki

Hi @AllanOXDi - really great work so far. Thanks for already addressing so much feedback. I left one small comment regarding using translated strings in aria labels, and I'm going to hand it over to @radinamatic for more robust accessibility testing.

One other (very small) issue I noticed that is mostly a pre-existing problem that has just been exacerbated here is that the tooltip for the identifier is covered by the table cell

It exists in Kolibri-demo as well, but because of the spacing there, the text is legible.

It's a small piece of cleanup, but I think worth doing now

@radinamatic - over to you for a11y testing! (Please note that the icon buttons at the top of the table are not intended to be interactive yet).

marcellamaki · 2025-06-16T22:47:44Z

kolibri/plugins/facility/assets/src/views/UserPage/index.vue

+              <KCheckbox
+                :checked="isUserSelected(row[6])"
+                :showLabel="false"
+                :aria-label="row[1] + ',' + row[6].kind"


Right now we're passing the .kind string directly to the aria label. But, we need to make sure that we are passing a translated string. It should match the label for the name in all cases (i.e. follow these conventions for admin, super admin, coach types, etc.) and then add in "learner"

radinamatic · 2025-06-17T22:17:14Z

@AllanOXDi Good work so far, still a few rough edges to smoothen 🙂

The main thing, as you can see in the screenshot, is that the checkbox does not seem to have a label, and NVDA announces:

check box  not checked

Seems that the strategy to add the aria-label to the checkbox container div is not giving us the results we need... Can you transfer the content of that aria-label to the actual checkbox <label> that is now empty?

Another issue is that there is a superfluous (first) Select announced by NVDA in each of the cells of the first column.

Select  row 2  Select  column 1

This seems to happen because of the aria-label that is added to the container div. If we eliminate that one completely, that should solve the problem.

AllanOXDi · 2025-06-19T12:40:34Z

Hi @AllanOXDi - really great work so far. Thanks for already addressing so much feedback. I left one small comment regarding using translated strings in aria labels, and I'm going to hand it over to @radinamatic for more robust accessibility testing.

One other (very small) issue I noticed that is mostly a pre-existing problem that has just been exacerbated here is that the tooltip for the identifier is covered by the table cell

It exists in Kolibri-demo as well, but because of the spacing there, the text is legible. It's a small piece of cleanup, but I think worth doing now

@radinamatic - over to you for a11y testing! (Please note that the icon buttons at the top of the table are not intended to be interactive yet).

Hi @marcellamaki , the issue seems to have come from https://github.com/BabyElias/kolibri-design-system/blob/0613a2163071512a86da80591101eb97ba50aa94/lib/KTable/index.vue#L341

radinamatic · 2025-06-25T12:40:09Z

@AllanOXDi, apologies for the late review 🙏🏽

Checkbox for all the individual users in rows below the header is now properly displaying the wording Select, yey! 👏🏽

Select  check box  checked

However, the column header cell is still read out as Select all, instead of just Select:

Select all  column header  row 1  column 1
Select all  check box  not checked

The second line in the above screen reader output is correct, because it is for the actual checkbox inside the first cell, but the header cell itself (and consequently all the cells below in that column) must have as the label just Select.

Technically, we should not even be putting the aria-label on the generic div element (k-checkbox-container). We should, instead place a with the text Select as the first thing directly inside the respective <th> element, same as for the rest of column headers, something like:

<span data...="" data...="" class="">Select</span>

AllanOXDi · 2025-06-25T12:42:30Z

Okay! Thanks @radinamatic let me fix that

radinamatic · 2025-06-25T19:06:23Z

More progress, still one thing to fix 😅

We should be good with the header row now, but the output from the 2º (and the following) sounds like this:

Abby L.,admin  row 2  Select column 1
Select  check box  not checked

We should be injecting the Abby L.,admin part - and mind to put a space after the comma, so it's read separately
(Abby L., admin) - INSIDE the <label> element for the checkbox.

Output should sound like this:

row 2  Select column 1
Select Abby L., admin check box  not checked

radinamatic · 2025-06-26T20:40:10Z

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

Select Abby L., Facility coach  check box  checked

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

radinamatic

Thank you @AllanOXDi , ready to roll! 💯 🎉

resolved

github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: large labels Jun 11, 2025

nucleogenesis requested changes Jun 11, 2025

View reviewed changes

AllanOXDi marked this pull request as ready for review June 12, 2025 00:20

marcellamaki requested a review from radinamatic June 12, 2025 14:23

github-actions bot added the SIZE: medium label Jun 12, 2025

AllanOXDi requested a review from nucleogenesis June 12, 2025 16:13

AllanOXDi added this to the Kolibri 0.19: Bulk User Management milestone Jun 12, 2025

nucleogenesis previously requested changes Jun 12, 2025

View reviewed changes

marcellamaki reviewed Jun 13, 2025

View reviewed changes

This was referenced Jun 13, 2025

Bulk User: Fundamental routing & data architecture for users page #13484

Closed

Setup bulk user action buttons on the Facility Users page #13477

Closed

LianaHarris360 requested changes Jun 16, 2025

View reviewed changes

marcellamaki previously requested changes Jun 16, 2025

View reviewed changes

marcellamaki self-assigned this Jun 17, 2025

LianaHarris360 linked an issue Jun 17, 2025 that may be closed by this pull request

Insert checkbox column in Facility > Users KTable to allow user selection #13426

Closed

AllanOXDi force-pushed the add-checkbox-to-Ktable branch 2 times, most recently from d2aec1e to 0a83573 Compare June 19, 2025 13:30

nucleogenesis added the TODO: needs QA re-review For stale issues that need to be revisited label Jun 24, 2025

AllanOXDi mentioned this pull request Jun 25, 2025

Setup bulk user action buttons on the Facility Users page #13513

Merged

AllanOXDi force-pushed the add-checkbox-to-Ktable branch from 1037cf4 to d33057b Compare June 25, 2025 13:34

LianaHarris360 approved these changes Jun 26, 2025

View reviewed changes

AllanOXDi force-pushed the add-checkbox-to-Ktable branch from 1d38405 to 19baf44 Compare June 26, 2025 15:38

AllanOXDi added 21 commits June 26, 2025 12:29

add checkbox functionality

b428381

remove unused class

ad3f427

adds filter label to bulkuse management strings

5936877

shifts data props to setup section

ab27aa0

utlises select all string from enhancedQuizmanagement strings

196525f

refines the selectALlState function

c2604f8

can reset password, edit and delete user

199e199

reduces border radius of usertype badge

3755f1e

Add accessible label to select all checkbox header

e580470

adds arialabels

14b2993

adds tooltip text

54c9e0d

reverts changes

ec51f67

simplifies selection state function

852cff6

fixes reactivity at clicking the checkbox

4c9e74f

adds a more descriptive arialabel to KChecbox

3f87fac

adds aria label to KCheckbox

68a6868

a bit of clean up

1c477f2

adds aria labels and adjusts column width

6ebf88a

fix more accesiblity issue

01ef139

more accessibility fix

91884b0

adds the strings to bulkUserManagementStrings

3d7af12

nucleogenesis force-pushed the add-checkbox-to-Ktable branch from 19baf44 to 3d7af12 Compare June 26, 2025 19:31

radinamatic approved these changes Jun 26, 2025

View reviewed changes

radinamatic removed the TODO: needs QA re-review For stale issues that need to be revisited label Jun 27, 2025

AllanOXDi merged commit 915e90f into learningequality:develop Jun 30, 2025
51 checks passed

Insert checkbox column in Facility > Users KTable to allow user selection #13479

Insert checkbox column in Facility > Users KTable to allow user selection #13479

Uh oh!

Conversation

AllanOXDi commented Jun 11, 2025

Summary

References

Reviewer guidance

Uh oh!

github-actions bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nucleogenesis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nucleogenesis Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcellamaki commented Jun 12, 2025

Uh oh!

nucleogenesis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LianaHarris360 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcellamaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radinamatic commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AllanOXDi commented Jun 19, 2025

Uh oh!

radinamatic commented Jun 25, 2025

Uh oh!

AllanOXDi commented Jun 25, 2025

Uh oh!

radinamatic commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radinamatic commented Jun 26, 2025

Uh oh!

radinamatic left a comment

github-actions bot commented Jun 11, 2025 •

edited

Loading

nucleogenesis Jun 11, 2025 •

edited

Loading

radinamatic commented Jun 17, 2025 •

edited

Loading

radinamatic commented Jun 25, 2025 •

edited

Loading