Merge pull request #2517 from YosemiteCrew/fix/settings-scope-review-fixes

Author: aupyay

apps/frontend/src/app/__tests__/pages/Settings/index.test.tsx

@@ -71,6 +71,25 @@ jest.mock('next/dynamic', () => ({
         return <MockDeleteProfile {...props} />;
       }
 
+      // The clinic-wide controls previously fell through to `null`, which let the
+      // whole point of the scope split regress without failing anything. They are
+      // identifiable now so the composition tests below can assert placement.
+      if (source.includes('Sections/AppointmentLockWindowPreference')) {
+        return <div>Appointment Lock Window Preference</div>;
+      }
+
+      if (source.includes('Sections/CrossClinicMessagingPreference')) {
+        return <div>Cross Clinic Messaging Preference</div>;
+      }
+
+      if (source.includes('Sections/YourOrganizations')) {
+        return <div>Your Organizations</div>;
+      }
+
+      if (source.includes('Sections/FederationSection')) {
+        return <div>Federation Section</div>;
+      }
+
       return null;
     };
 
@@ -119,14 +138,102 @@ jest.mock('@/app/features/settings/pages/Settings/Sections/CompanionTerminologyP
   default: () => <div>Companion Terminology</div>,
 }));
 
+const mockHasPermission = jest.fn(() => true);
+jest.mock('@/app/hooks/usePermissions', () => ({
+  __esModule: true,
+  useHasPermission: () => mockHasPermission(),
+}));
+
+/**
+ * Returns the labelled scope band (`<section aria-labelledby>`) that contains
+ * the given control, so a test can assert WHERE a control sits rather than just
+ * that it rendered somewhere on the page.
+ */
+const bandContaining = (text: string): HTMLElement | null =>
+  screen.getByText(text).closest('section[aria-labelledby^="settings-band-"]');
+
 describe('Settings page', () => {
-  it('renders the header with the subtitle and auto-save indicator', () => {
+  beforeEach(() => {
+    mockHasPermission.mockReturnValue(true);
+  });
+
+  it('puts every per-user control in the Personal band', () => {
     render(<Settings />);
 
-    expect(screen.getByRole('heading', { name: 'Settings' })).toBeInTheDocument();
+    for (const control of [
+      'Personal Card',
+      'Timezone Preference',
+      'Default Open Screen Preference',
+      'Companion Terminology',
+      'Your Organizations',
+      'Delete Profile',
+      // Device-scoped, but still the signed-in person's own surface.
+      'Appearance Preference',
+    ]) {
+      expect(bandContaining(control)).toHaveAttribute('aria-labelledby', 'settings-band-Personal');
+    }
+  });
+
+  it('puts every clinic-wide control in the Organisation band', () => {
+    render(<Settings />);
+
+    for (const control of [
+      'Appointment Lock Window Preference',
+      'Cross Clinic Messaging Preference',
+      'Federation Section',
+    ]) {
+      expect(bandContaining(control)).toHaveAttribute(
+        'aria-labelledby',
+        'settings-band-Organisation'
+      );
+    }
+  });
+
+  it('keeps the device theme out of the account-scoped group', () => {
+    render(<Settings />);
+
+    // Appearance does not follow the account to another device, so it must not
+    // sit under the group that promises "your account".
+    const appearanceGroup = screen.getByText('Appearance Preference').closest('section');
+    expect(appearanceGroup).toHaveTextContent('This device');
+    expect(appearanceGroup).not.toHaveTextContent('Only you');
+  });
+
+  // Scoped to the GROUP, not the band. The organisation band mixes two gates -
+  // scheduling goes through updateOrg (teams:edit:any), federation through
+  // integrations:edit:any - so a Supervisor holds one and not the other. A
+  // band-level verdict would be wrong for exactly that role.
+  it('marks the scheduling group read-only when the member cannot edit it', () => {
+    mockHasPermission.mockReturnValue(false);
+    render(<Settings />);
+
+    const group = screen.getByText('Appointment Lock Window Preference').closest('section');
+    expect(group).toHaveTextContent(/Managed by a clinic administrator/);
+  });
+
+  it('does not mark the scheduling group read-only for a member who can edit it', () => {
+    mockHasPermission.mockReturnValue(true);
+    render(<Settings />);
+
+    const group = screen.getByText('Appointment Lock Window Preference').closest('section');
+    expect(group).not.toHaveTextContent(/Managed by a clinic administrator/);
+  });
+
+  it('keeps the organisation band description permission-neutral', () => {
+    mockHasPermission.mockReturnValue(false);
+    render(<Settings />);
+
+    // The band must not claim a single verdict for controls behind two gates.
     expect(
-      screen.getByText('Your preferences, and the clinic settings you administer')
+      screen.getByText('Shared clinic settings. Changes here apply to every colleague.')
     ).toBeInTheDocument();
+  });
+
+  it('renders the header with the subtitle and auto-save indicator', () => {
+    render(<Settings />);
+
+    expect(screen.getByRole('heading', { name: 'Settings' })).toBeInTheDocument();
+    expect(screen.getByText('Your preferences and clinic settings')).toBeInTheDocument();
     expect(screen.getByText('Changes save automatically')).toBeInTheDocument();
   });
 

apps/frontend/src/app/features/settings/pages/Settings/Sections/PreferenceGroup.tsx

@@ -9,10 +9,16 @@ import React from 'react';
  * undifferentiated cards - so an owner could change the whole clinic believing it
  * was their own preference. Groups declare their scope and say so on screen.
  */
-export type PreferenceScope = 'personal' | 'organisation';
+export type PreferenceScope = 'personal' | 'device' | 'organisation';
 
 const SCOPE_COPY: Record<PreferenceScope, { label: string; hint: string }> = {
   personal: { label: 'Only you', hint: 'These apply to your account on this clinic.' },
+  // Distinct from `personal` on purpose. The theme is stored under an
+  // un-namespaced `yc-theme` key in browser localStorage, so it does not follow
+  // the account to another device and does not reset for the next person to use
+  // the same browser. Calling that "your account" would be a promise the storage
+  // does not keep.
+  device: { label: 'This device', hint: 'Saved in this browser, not on your account.' },
   organisation: {
     label: 'Whole clinic',
     hint: 'These apply to everyone at this clinic, not just you.',
@@ -25,6 +31,16 @@ type PreferenceGroupProps = {
   className?: string;
   /** Who the group's controls affect. Renders a scope chip and a one-line hint. */
   scope?: PreferenceScope;
+  /**
+   * The reader can see these settings but not change them.
+   *
+   * Belongs on the GROUP, not the surrounding band: the organisation band mixes
+   * controls behind different permissions - the scheduling ones are gated by
+   * `teams:edit:any` and federation by `integrations:edit:any` - so a Supervisor
+   * can edit one and not the other. A single band-level verdict would be wrong
+   * for exactly that role.
+   */
+  readOnly?: boolean;
 };
 
 /**
@@ -35,7 +51,13 @@ type PreferenceGroupProps = {
  * When `scope` is given the title row also carries a chip naming who the settings
  * affect, plus a faint hint line beneath it.
  */
-export const PreferenceGroup = ({ title, children, className, scope }: PreferenceGroupProps) => {
+export const PreferenceGroup = ({
+  title,
+  children,
+  className,
+  scope,
+  readOnly = false,
+}: PreferenceGroupProps) => {
   const copy = scope ? SCOPE_COPY[scope] : null;
 
   return (
@@ -49,7 +71,11 @@ export const PreferenceGroup = ({ title, children, className, scope }: Preferenc
           <h3 className="text-[14.5px] font-bold text-[var(--ink)]">{title}</h3>
           {copy && <ScopeChip scope={scope!} label={copy.label} />}
         </div>
-        {copy && <p className="m-0! text-[11.5px] text-[var(--ink-faint)]">{copy.hint}</p>}
+        {copy && (
+          <p className="m-0! text-[11.5px] text-[var(--ink-faint)]">
+            {readOnly ? `${copy.hint} Managed by a clinic administrator.` : copy.hint}
+          </p>
+        )}
       </div>
       {children}
     </section>
@@ -64,7 +90,10 @@ const ScopeChip = ({ scope, label }: { scope: PreferenceScope; label: string })
   <span
     className={`flex-none rounded-full border px-2 py-[2px] text-[10.5px] font-semibold tracking-[0.02em] whitespace-nowrap ${
       scope === 'organisation'
-        ? 'border-[var(--blue)] text-[var(--blue)]'
+        ? // --blue is a FILL token and carries no contrast duty; at 10.5px it fails
+          // AA on the bone surfaces. --blue-text is the one that clears 4.5:1, so
+          // the border keeps the fill and the label takes the text token.
+          'border-[var(--blue)] text-[var(--blue-text)]'
         : 'border-[var(--hairline)] text-[var(--ink-faint)]'
     }`}
   >

apps/frontend/src/app/features/settings/pages/Settings/index.tsx

@@ -5,6 +5,8 @@ import React, { useState } from 'react';
 import dynamic from 'next/dynamic';
 import { IoInformationCircleOutline } from 'react-icons/io5';
 
+import { useHasPermission } from '@/app/hooks/usePermissions';
+import { PERMISSIONS } from '@/app/lib/permissions';
 import { PreferenceGroup } from '@/app/features/settings/pages/Settings/Sections/PreferenceGroup';
 import '@/app/features/settings/styles/Settings.css';
 
@@ -98,6 +100,10 @@ const SettingsBand = ({
 const Settings = () => {
   const [profileOpen, setProfileOpen] = useState(false);
   const [hoursOpen, setHoursOpen] = useState(false);
+  // The two updateOrg controls below are gated by teams:edit:any, NOT by
+  // integrations:edit:any - see organization.router.ts. Supervisor holds the
+  // former and not the latter.
+  const canEditClinicPreferences = useHasPermission(PERMISSIONS.TEAMS_EDIT_ANY);
 
   return (
     <div className="yc-page-content">
@@ -115,9 +121,9 @@ const Settings = () => {
               className="flex-none"
             />
           </h1>
-          <span className="yc-settings-subtitle">
-            Your preferences, and the clinic settings you administer
-          </span>
+          {/* Permission-neutral on purpose: most roles can view the organisation
+              band without holding integrations:edit:any. */}
+          <span className="yc-settings-subtitle">Your preferences and clinic settings</span>
         </div>
         <span className="yc-settings-autosave">
           <span className="yc-settings-autosave-dot" aria-hidden="true" />
@@ -140,13 +146,16 @@ const Settings = () => {
             onEditHours={() => setHoursOpen(true)}
           />
 
-          {/* Every control here writes the per-user profile (patchUserProfile) or
-              device-local theme storage — none of it is workspace-wide, which is
-              what the old "Workspace preferences" title wrongly implied. */}
+          {/* Every control here writes the per-user profile via patchUserProfile,
+              so it follows the account to any device. Appearance does NOT — it is
+              deliberately in its own group below. */}
           <PreferenceGroup title="Your preferences" scope="personal">
             <DefaultOpenScreenPreference />
             <TimezonePreference />
             <CompanionTerminologyPreference />
+          </PreferenceGroup>
+
+          <PreferenceGroup title="This browser" scope="device">
             <AppearancePreference />
           </PreferenceGroup>
 
@@ -155,13 +164,22 @@ const Settings = () => {
         </div>
       </SettingsBand>
 
+      {/* The band description stays permission-neutral because the band mixes
+          two different gates: the scheduling controls go through updateOrg
+          (teams:edit:any) and federation through integrations:edit:any. A
+          Supervisor holds the first and not the second, so any single verdict
+          here would be wrong for that role. Each group states its own. */}
       <SettingsBand
         title="Organisation"
         description="Shared clinic settings. Changes here apply to every colleague."
       >
         <div className="flex flex-col gap-3.5">
           {/* Both write the organisation record via updateOrg. */}
-          <PreferenceGroup title="Scheduling & messaging" scope="organisation">
+          <PreferenceGroup
+            title="Scheduling & messaging"
+            scope="organisation"
+            readOnly={!canEditClinicPreferences}
+          >
             <AppointmentLockWindowPreference />
             <CrossClinicMessagingPreference />
           </PreferenceGroup>