From 523aaeca4c5afde3ab3d989dda5c970ab973a2be Mon Sep 17 00:00:00 2001 From: Alex Sorafumo Date: Fri, 2 Oct 2026 13:12:34 +1000 Subject: [PATCH 1/2] fix(signage-manager): keep group selection and lists stable - Open the first group once, so clearing the selection on mobile works. - Keep group, user and zone lists while they reload or fail to reload. - Show loading and error states in the user search modal. - shareItems reports a failure instead of rejecting. - Build the group tree from the loaded index and keep row levels. - Guard the groups page, read every page of users and zones, and send one shared group request for admins. - Accessibility and translation fixes for tabs, breadcrumbs and labels. --- apps/signage-manager/USER_STORIES.md | 9 +- apps/signage-manager/src/app/app.config.ts | 3 +- .../signage-group-access-modal.component.ts | 47 +-- .../app/groups/signage-group-admin.service.ts | 182 +++++------ .../signage-group-edit-modal.component.ts | 22 +- .../signage-group-features-modal.component.ts | 20 +- .../groups/signage-group-list.component.ts | 282 ++++-------------- ...gnage-group-permission-labels.component.ts | 28 ++ .../groups/signage-group-tabs.component.ts | 38 ++- ...gnage-group-user-select-modal.component.ts | 31 +- .../groups/signage-group-users.component.ts | 26 +- .../groups/signage-group-zones.component.ts | 26 +- .../app/shared/group-breadcrumbs.component.ts | 3 + .../shared/group-select-modal.component.ts | 3 +- .../src/app/signage-access.guard.ts | 24 +- .../src/app/signage-context.service.ts | 123 +++++--- ...gnage-group-access-modal.component.spec.ts | 3 +- .../signage-group-admin.service.spec.ts | 179 ++++++++++- ...signage-group-edit-modal.component.spec.ts | 22 +- .../signage-group-list.component.spec.ts | 221 ++++++++------ .../signage-group-tabs.component.spec.ts | 19 ++ ...-group-user-select-modal.component.spec.ts | 55 +++- .../src/tests/signage-access.guard.spec.ts | 75 ++++- .../src/tests/signage-group-context.spec.ts | 63 ++++ shared/assets/locale/en-AU.json | 3 + shared/assets/locale/en-GB.json | 3 + shared/assets/locale/en-US.json | 3 + 27 files changed, 895 insertions(+), 618 deletions(-) create mode 100644 apps/signage-manager/src/app/groups/signage-group-permission-labels.component.ts diff --git a/apps/signage-manager/USER_STORIES.md b/apps/signage-manager/USER_STORIES.md index f771cd95106..e8ce7068ac9 100644 --- a/apps/signage-manager/USER_STORIES.md +++ b/apps/signage-manager/USER_STORIES.md @@ -311,10 +311,11 @@ These stories cover the current app workflows: **Acceptance Criteria:** -- The groups page is available only when the user can manage signage groups. +- The groups page is available only when the user can manage signage groups. Other users who open its address go to the media library. - Manageable groups appear in a searchable tree. -- Expanding a group loads and shows child groups. -- Selecting a group opens its users and zones panels. +- Expanding a group shows its child groups. +- Selecting a group opens its users and zones panels. On mobile, the back button returns to the group list. +- After a save, the selected group and the expanded groups stay as they were, also when the group list cannot load again. When the group list cannot load at all, the page shows an error. - Users with manage-all-groups permission can create a new group. - Selected groups can be edited or removed. @@ -329,7 +330,7 @@ These stories cover the current app workflows: **Acceptance Criteria:** - The users panel lists assigned users with name, email, and permission labels. -- Users can add a user not already assigned to the group. The user gets the default permissions of the group. +- Users can add a user not already assigned to the group. The user gets the default permissions of the group. The user search shows a loading state, and an error when the search fails. - Users can edit an assigned user's signage permissions. - Users can remove an assigned user from the group. - Empty state appears when no users are assigned. diff --git a/apps/signage-manager/src/app/app.config.ts b/apps/signage-manager/src/app/app.config.ts index 7ddcd1798ac..37169a415ed 100644 --- a/apps/signage-manager/src/app/app.config.ts +++ b/apps/signage-manager/src/app/app.config.ts @@ -19,7 +19,7 @@ import { UnauthorisedComponent, } from '@placeos/components'; import { environment } from '../environments/environment'; -import { signageAccessGuard } from './signage-access.guard'; +import { manageGroupsGuard, signageAccessGuard } from './signage-access.guard'; import { templatesEnabledGuard } from './templates-enabled.guard'; import { templateUnsavedGuard } from './templates/template-unsaved.guard'; @@ -121,6 +121,7 @@ const APP_ROUTES: Routes = [ { path: 'branding', redirectTo: 'manage/branding' }, { path: 'groups', + canActivate: [manageGroupsGuard], loadComponent: () => import('./groups/groups.component').then( (m) => m.GroupsSectionComponent, diff --git a/apps/signage-manager/src/app/groups/signage-group-access-modal.component.ts b/apps/signage-manager/src/app/groups/signage-group-access-modal.component.ts index ba66a0dfd4b..d9a13447b9a 100644 --- a/apps/signage-manager/src/app/groups/signage-group-access-modal.component.ts +++ b/apps/signage-manager/src/app/groups/signage-group-access-modal.component.ts @@ -28,9 +28,9 @@ import { PlaceGroup, PlaceGroupAdMappings } from '@placeos/ts-client'; import { adGroupKey } from '../signage-group-access'; import { dialogClosed } from '../signage-service.util'; import { SignageGroupAdminService } from './signage-group-admin.service'; +import { SignageGroupPermissionLabelsComponent } from './signage-group-permission-labels.component'; import { GROUP_PERMISSION_FLAGS, - groupPermissionLabels, SignageGroupPermissionsModalComponent, } from './signage-group-permissions-modal.component'; @@ -133,26 +133,9 @@ interface AdGroupRow {
- @let labels = - permissionLabels( - row.permissions - ); - @if (labels.length) { - @for ( - label of labels; - track label - ) { - {{ label | translate }} - @if (!$last) { - , - } - } - } @else { - {{ - 'SIGNAGE_MANAGER.DEFAULT_PERMISSIONS' - | translate - }} - } +
} - + `, }) export class SignageGroupTabsComponent { private readonly _group_admin = inject(SignageGroupAdminService); + private readonly _element = inject>(ElementRef); public readonly active_tab = this._group_admin.managed_group_tab; - public readonly tabs = [ - { id: 'users' as const, label: 'SIGNAGE_MANAGER.TAB_USERS' }, - { id: 'zones' as const, label: 'SIGNAGE_MANAGER.TAB_ZONES' }, + public readonly tabs: { id: GroupTab; label: string }[] = [ + { id: 'users', label: 'SIGNAGE_MANAGER.TAB_USERS' }, + { id: 'zones', label: 'SIGNAGE_MANAGER.TAB_ZONES' }, ]; + + /** Arrow keys, Home and End move between the tabs, as in a tab list */ + public onKeydown(event: KeyboardEvent) { + const index = this.tabs.findIndex(({ id }) => id === this.active_tab()); + const last = this.tabs.length - 1; + const targets: Record = { + ArrowLeft: index > 0 ? index - 1 : last, + ArrowRight: index < last ? index + 1 : 0, + Home: 0, + End: last, + }; + const next = targets[event.key]; + if (next === undefined) return; + event.preventDefault(); + const tab = this.tabs[next].id; + this.active_tab.set(tab); + this._element.nativeElement + .querySelector(`#group-${tab}-tab`) + ?.focus(); + } } diff --git a/apps/signage-manager/src/app/groups/signage-group-user-select-modal.component.ts b/apps/signage-manager/src/app/groups/signage-group-user-select-modal.component.ts index eba645c89ac..7505f4b1efc 100644 --- a/apps/signage-manager/src/app/groups/signage-group-user-select-modal.component.ts +++ b/apps/signage-manager/src/app/groups/signage-group-user-select-modal.component.ts @@ -11,6 +11,7 @@ import { MatRippleModule } from '@angular/material/core'; import { MAT_DIALOG_DATA, MatDialogModule } from '@angular/material/dialog'; import { MatFormFieldModule } from '@angular/material/form-field'; import { MatInputModule } from '@angular/material/input'; +import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; import { IconComponent, TranslatePipe } from '@placeos/components'; import { SignageGroupAdminService } from './signage-group-admin.service'; @@ -49,7 +50,21 @@ import { SignageGroupAdminService } from './signage-group-admin.service'; " /> - @if (users().length > 0) { + @if (loading()) { +
+ +
+ } @else if (failed()) { + + } @else if (users().length > 0) { @for (user of users(); track user.id || user.email) {
+ @if (failed() && users().length) { + + } @if (users().length) { @for (row of users(); track row.user_id) {
+ @if (failed() && zones().length) { + + } @if (zones().length) { @for (row of zones(); track row.zone_id) {
( } /** - * Share one request per key, so resources that reload on the same change hit - * the endpoint once. A failed request is dropped, so a later key can retry. + * Share one request per key and user, so resources that reload on the same + * change hit the endpoint once. A new user gets a new request. A failed + * request is dropped, so a later key can retry. + * @param user Signed in user, read when a request starts */ -function sharedRequest(load: () => Promise) { - let last: { key: number; promise: Promise } | null = null; +function sharedRequest(load: () => Promise, user: () => unknown) { + let last: { key: number; user: unknown; promise: Promise } | null = null; return (key: number) => { - if (last?.key === key) return last.promise; - const promise = load().catch((error: unknown) => { - if (last?.key === key) last = null; + const current_user = user(); + if (last?.key === key && last.user === current_user) { + return last.promise; + } + const request = { key, user: current_user, promise: load() }; + request.promise = request.promise.catch((error: unknown) => { + if (last === request) last = null; throw error; }); - last = { key, promise }; - return promise; + last = request; + return request.promise; }; } @@ -367,14 +374,16 @@ export class SignageContextService { } // Several resources need the signage groups on page load and after a - // save. Share one request per `groups_change`. + // save. Share one request per `groups_change` and user. /** Signage groups of the current user, one request per `groups_change` */ - public readonly currentSignageGroups = sharedRequest(() => - currentGroups({ subsystem: 'signage' }), + public readonly currentSignageGroups = sharedRequest( + () => currentGroups({ subsystem: 'signage' }), + () => untracked(this.active_user)?.email, ); /** Every signage group, for admins, one request per `groups_change` */ - public readonly allSignageGroups = sharedRequest(() => - this.queryManageableGroups(), + public readonly allSignageGroups = sharedRequest( + () => this.queryManageableGroups(), + () => untracked(this.active_user)?.email, ); /** ID of the selected group, empty for "All groups" */ diff --git a/apps/signage-manager/src/tests/groups/signage-group-admin.service.spec.ts b/apps/signage-manager/src/tests/groups/signage-group-admin.service.spec.ts index b735129dd01..f552277b566 100644 --- a/apps/signage-manager/src/tests/groups/signage-group-admin.service.spec.ts +++ b/apps/signage-manager/src/tests/groups/signage-group-admin.service.spec.ts @@ -513,6 +513,40 @@ describe('SignageGroupAdminService', () => { expect(queryGroups).toHaveBeenCalledTimes(1); }); + it('opens the first group again for a new user', async () => { + const service = createService(); + await settle(); + service.managed_group_id.set(''); + await settle(); + + vi.mocked(currentGroups).mockResolvedValue([manager('c')]); + setCurrentUser( + new StaffUser({ id: 'other', email: 'other@place.tech' }), + ); + await settle(); + + expect( + service.manageable_signage_groups().map(({ id }) => id), + ).toEqual(['c']); + expect(service.managed_group_id()).toBe('c'); + }); + + it('keeps the users on screen when their reload fails', async () => { + const service = createService(); + await settle(); + service.managed_group_id.set('b'); + await settle(); + const [row] = service.managed_group_users(); + + vi.mocked(updateGroupUser).mockResolvedValue(row); + vi.mocked(queryGroupUsers).mockRejectedValue(new Error('down')); + await service.updateManagedGroupUser(row, READ | MANAGE); + await settle(); + + expect(service.managed_group_users()).toEqual([row]); + expect(service.managed_group_users_failed()).toBe(true); + }); + it('reads every page of the group users', async () => { vi.mocked(queryGroupUsers).mockReturnValue( page([member('user-1', 'a')], () => diff --git a/apps/signage-manager/src/tests/groups/signage-group-users.component.spec.ts b/apps/signage-manager/src/tests/groups/signage-group-users.component.spec.ts index bb0f2d396ad..1f6ed7addbe 100644 --- a/apps/signage-manager/src/tests/groups/signage-group-users.component.spec.ts +++ b/apps/signage-manager/src/tests/groups/signage-group-users.component.spec.ts @@ -1,6 +1,9 @@ -import { signal } from '@angular/core'; +import { Component, input, Pipe, PipeTransform, signal } from '@angular/core'; import { TestBed } from '@angular/core/testing'; +import { MatRippleModule } from '@angular/material/core'; import { MatDialog } from '@angular/material/dialog'; +import { MatProgressSpinnerModule } from '@angular/material/progress-spinner'; +import { MatTooltipModule } from '@angular/material/tooltip'; import { SignageGroupAdminService } from '../../app/groups/signage-group-admin.service'; import { SignageGroupPermissionsModalComponent } from '../../app/groups/signage-group-permissions-modal.component'; import { SignageGroupUserSelectModalComponent } from '../../app/groups/signage-group-user-select-modal.component'; @@ -17,14 +20,33 @@ function dialogRef(value: unknown) { }; } +@Component({ selector: 'icon', template: '' }) +class IconStubComponent {} + +@Component({ selector: 'signage-group-permission-labels', template: '' }) +class PermissionLabelsStubComponent { + public readonly permissions = input(0); +} + +@Pipe({ name: 'translate' }) +class TranslateStubPipe implements PipeTransform { + public transform(key: string) { + return key; + } +} + describe('SignageGroupUsersComponent', () => { const managed_group_users = signal([]); const add_user = vi.fn(); const update_user = vi.fn(); const remove_user = vi.fn(); const dialog = { open: vi.fn() }; + const managed_group_users_loading = signal(false); + const managed_group_users_failed = signal(false); const service_stub = { managed_group_users, + managed_group_users_loading, + managed_group_users_failed, addManagedGroupUser: add_user, updateManagedGroupUser: update_user, removeManagedGroupUser: remove_user, @@ -43,8 +65,34 @@ describe('SignageGroupUsersComponent', () => { .componentInstance; } + /** Renders the real template with stubbed icons, labels and text */ + async function render() { + TestBed.configureTestingModule({ + providers: [ + { provide: SignageGroupAdminService, useValue: service_stub }, + { provide: MatDialog, useValue: dialog }, + ], + }).overrideComponent(SignageGroupUsersComponent, { + set: { + imports: [ + MatProgressSpinnerModule, + MatRippleModule, + MatTooltipModule, + IconStubComponent, + PermissionLabelsStubComponent, + TranslateStubPipe, + ], + }, + }); + const fixture = TestBed.createComponent(SignageGroupUsersComponent); + await fixture.whenStable(); + return fixture.nativeElement as HTMLElement; + } + beforeEach(() => { vi.clearAllMocks(); + managed_group_users_loading.set(false); + managed_group_users_failed.set(false); managed_group_users.set([ { user_id: 'user-1', permissions: 1 }, { user_id: 'user-2', permissions: 0 }, @@ -108,6 +156,16 @@ describe('SignageGroupUsersComponent', () => { expect(update_user).not.toHaveBeenCalled(); }); + it('shows the load error above the rows it kept', async () => { + managed_group_users_failed.set(true); + const element = await render(); + + expect(element.querySelector('[role="alert"]')?.textContent).toContain( + 'SIGNAGE_MANAGER.USERS_LOAD_ERROR', + ); + expect(element.textContent).toContain('user-1'); + }); + it('removes a user through the service', () => { const component = make(); const row = { user_id: 'user-1' } as any;