Repository navigation
fix: search class groups by display name and abbreviation - #15
Merged
Merged
Conversation
Member
|
Tested the search-related changes in the test environment, where they worked as expected. The original group name (gid/Nextcloud groupID) and group abbreviation (cn) are no longer displayed in the search results. Only the display_name is shown in the search results, as intended. From this perspective, the PR is ready to be merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Teachers could not find some class groups in the share dialog. Groups that had been renamed since they were first synced were unfindable by the name shown in the UI, while unrenamed groups in the same school worked. Typing part of the group's old name found it, which was the clue.
Root cause
ClassGroupMapper::searchEntriesInner()matched the search term against thegidcolumn only, but the picker labels each hit withIGroup::getDisplayName().For LDAP groups those two are not the same string. Nextcloud has no stable internal id for groups: the gid is frozen at whatever
ldapGroupDisplayNameheld whenuser_ldapfirst mapped the group, and a later rename updates only the display name. So the app was searching a name no user has seen since the rename.Changes
Version1001Date20260909120000adds nullabledisplay_nameandabbreviationcolumns togroupsharemachine_groups.LdapSyncstores both on every sync:display_namefrom the configuredldapGroupDisplayNameattribute (so it matches what Nextcloud shows),abbreviationfromcn. Both come out of the LDAP record already being fetched — no extra queries.ClassGroupMappermatchesdisplay_name, and nothing else. Results are ordered byCOALESCE(display_name, gid), i.e. by the name the picker actually shows.TeacherClassSearchPlugintreats only the display name as an exact match, so a stale gid can no longer rank an impostor above the real hit.occ upgraderuns the migration.The rule the picker now follows is one sentence: you find a group by the name you see. The two other names in the table are deliberately not searched.
Why not the gid
Keeping the gid in the predicate looks harmless but breaks the year rollover. When
4. classis renamed to5. classand a new4. classis created,user_ldapgives the newcomer gid4. class_2— so the frozen gid4. classis now another group's current name. Matching it would answer a search for4. classwith the cohort labelled5. class: two entries that look like the same class, one of them wrong, with no way for the teacher to tell them apart.The gid is used only as a fallback while
display_nameis still NULL — that is, betweenocc upgradeand the first sync — so no group becomes unfindable during that window. The label is always rendered from the live group, so even then the entry a teacher sees and the group they share with are the same group.Why not the abbreviation
It is stored but not searched. The picker label is
displayName (school)and never shows thecn, so a hit on the abbreviation is a result the teacher cannot account for. It also adds nothing when it overlaps the display name — and when it does not, it is typically a slug frozen at the group's original year (cn: 1klon a group now displayed5. klasse), carrying the same staleness as the gid.The column stays in the table and is still synced, so surfacing it in the label, or making it searchable again, needs no further migration.
Testing
dev/ldap/rename-group.sh(new) reproduces the condition locally by renaming a seed group after it has been mapped.gid = 1A/display_name = Klasse 1Ais found by "Klasse" and returned asKlasse 1A (Alpha School)whileshareWithstays the gid1A; searching the abbreviationalpha-1areturns nothing. School scoping still holds — the other school's groups stay hidden.occ groupsharemachine:diagnosereports the share itself as allowed.ClassGroupMapperTestfor the year rollover, the NULL-display_namefallback, the result ordering and the unsearchable abbreviation, plus one inTeacherClassSearchPluginTestfor the exact-match bucket.phpunit47/47, Psalm clean,php-cs-fixerclean.Deployment
The search does not improve until the new columns are populated:
Between those two steps the gid fallback keeps every group findable, so there is no window where the picker comes up empty.
Note for operators: after the sync, searching a group's old name stops working, and so does searching its abbreviation. That is the point of the change — a group is found by the name on screen, and an old name now belongs to whichever group carries it today, if any.
Known issue, not fixed here
LdapSync::safeSearch()returns[]both when a page is genuinely empty and when the LDAP search throws. An empty page ends the paging loop, so a failed search would leave$keptempty anddeleteNotIn([])would delete every row, blanking the picker for all teachers until the next successful sync.This is a reading of the code, not an observed failure — the empty tables seen during development turned out to be the test suite truncating them, not a failed search. Still worth a follow-up that distinguishes the two cases and skips the prune when the search did not complete.