Skip to content

fix: search class groups by display name and abbreviation - #15

Merged
eluttine merged 7 commits into
mainfrom
fix/class-group-search
Oct 8, 2026
Merged

eluttine merged 7 commits into
mainfrom
fix/class-group-search

Conversation

@eluttine

@eluttine eluttine commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

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 the gid column only, but the picker labels each hit with IGroup::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 ldapGroupDisplayName held when user_ldap first 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

  • Migration Version1001Date20260909120000 adds nullable display_name and abbreviation columns to groupsharemachine_groups.
  • LdapSync stores both on every sync: display_name from the configured ldapGroupDisplayName attribute (so it matches what Nextcloud shows), abbreviation from cn. Both come out of the LDAP record already being fetched — no extra queries.
  • ClassGroupMapper matches display_name, and nothing else. Results are ordered by COALESCE(display_name, gid), i.e. by the name the picker actually shows.
  • TeacherClassSearchPlugin treats only the display name as an exact match, so a stale gid can no longer rank an impostor above the real hit.
  • Version bumped to 1.0.1 so occ upgrade runs 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. class is renamed to 5. class and a new 4. class is created, user_ldap gives the newcomer gid 4. class_2 — so the frozen gid 4. class is now another group's current name. Matching it would answer a search for 4. class with the cohort labelled 5. 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_name is still NULL — that is, between occ upgrade and 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 the cn, 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: 1kl on a group now displayed 5. 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.
  • Verified end to end on Nextcloud 33 against the docker LDAP, through the real sharee API as an LDAP teacher: gid = 1A / display_name = Klasse 1A is found by "Klasse" and returned as Klasse 1A (Alpha School) while shareWith stays the gid 1A; searching the abbreviation alpha-1a returns nothing. School scoping still holds — the other school's groups stay hidden. occ groupsharemachine:diagnose reports the share itself as allowed.
  • New regression tests in ClassGroupMapperTest for the year rollover, the NULL-display_name fallback, the result ordering and the unsearchable abbreviation, plus one in TeacherClassSearchPluginTest for the exact-match bucket.
  • phpunit 47/47, Psalm clean, php-cs-fixer clean.

Deployment

The search does not improve until the new columns are populated:

occ upgrade
occ groupsharemachine:sync

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 $kept empty and deleteNotIn([]) 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.

@eluttine eluttine added the bug Something isn't working label Sep 9, 2026
@cjoohs

cjoohs commented Oct 8, 2026

Copy link
Copy Markdown
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.

@eluttine
eluttine merged commit e668d2e into main Oct 8, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants