From 07bf4c2112f331bd068ff864244e1d8df2b94473 Mon Sep 17 00:00:00 2001
From: James Griffin <james@unsupervised.ca>
Date: Wed, 8 Apr 2026 19:56:20 -0300
Subject: [PATCH] Replace volunteer ID input with checkbox picker on shift edit

The edit form now loads the volunteer list and groups them by
operational role. Template role requirements are shown with a
count indicator (selected/required) that turns green when filled.
Volunteers are selected via checkboxes instead of typing IDs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 web/src/pages/Schedules.test.tsx |   9 +-
 web/src/pages/Schedules.tsx      | 226 ++++++++++++++++++++++++-------
 2 files changed, 184 insertions(+), 51 deletions(-)

diff --git a/web/src/pages/Schedules.test.tsx b/web/src/pages/Schedules.test.tsx
index 4f7edd8..49fe025 100644
--- a/web/src/pages/Schedules.test.tsx
+++ b/web/src/pages/Schedules.test.tsx
@@ -9,6 +9,7 @@ jest.mock('../api', () => ({
   api: {
     listShifts: jest.fn(),
     listShiftTemplates: jest.fn(),
+    listVolunteers: jest.fn(),
     generateShifts: jest.fn(),
     publishShifts: jest.fn(),
     unpublishShifts: jest.fn(),
@@ -140,10 +141,16 @@ describe('Schedules (admin shifts view)', () => {
     expect(screen.getByText('Edit')).toBeInTheDocument();
   });
 
-  it('opens edit form when Edit is clicked', async () => {
+  it('opens edit form with volunteer checkboxes when Edit is clicked', async () => {
+    (api.listVolunteers as jest.Mock).mockResolvedValue([
+      { id: 5, name: 'Alice', active: true, operational_roles: 'Dog Shelter Volunteer', is_trainee: false },
+      { id: 6, name: 'Bob', active: true, operational_roles: 'Behaviour Team', is_trainee: false },
+    ]);
     renderAt('/schedules');
     await waitFor(() => expect(screen.getByText('Edit')).toBeInTheDocument());
     fireEvent.click(screen.getByText('Edit'));
+    await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument());
+    expect(screen.getByText('Bob')).toBeInTheDocument();
     expect(screen.getByText(/Edit Shift/)).toBeInTheDocument();
   });
 
diff --git a/web/src/pages/Schedules.tsx b/web/src/pages/Schedules.tsx
index 5233a06..c857751 100644
--- a/web/src/pages/Schedules.tsx
+++ b/web/src/pages/Schedules.tsx
@@ -4,6 +4,7 @@ import {
   api,
   ShiftTemplate,
   ShiftInstance,
+  Volunteer,
   CreateShiftTemplateInput,
   TemplateRole,
   OPERATIONAL_ROLES,
@@ -142,6 +143,171 @@ function TemplateForm({ initial, onSave, onCancel, title }: TemplateFormProps) {
   );
 }
 
+// ---------------------------------------------------------------------------
+// Shift instance edit form
+// ---------------------------------------------------------------------------
+
+interface ShiftEditFormProps {
+  instance: ShiftInstance;
+  templateId: number | undefined;
+  onSave: (inst: ShiftInstance) => void;
+  onCancel: () => void;
+}
+
+function ShiftEditForm({ instance, templateId, onSave, onCancel }: ShiftEditFormProps) {
+  const [volunteers, setVolunteers] = useState<Volunteer[]>([]);
+  const [templateRoles, setTemplateRoles] = useState<TemplateRole[]>([]);
+  const [selectedIds, setSelectedIds] = useState<Set<number>>(
+    new Set(instance.volunteers.map(v => v.volunteer_id))
+  );
+  const [minCap, setMinCap] = useState(instance.min_capacity);
+  const [maxCap, setMaxCap] = useState(instance.max_capacity);
+  const [error, setError] = useState('');
+  const [saving, setSaving] = useState(false);
+  const [loading, setLoading] = useState(true);
+
+  useEffect(() => {
+    const loads: Promise<void>[] = [
+      api.listVolunteers().then((vols) => setVolunteers(vols as Volunteer[])),
+    ];
+    if (templateId) {
+      loads.push(
+        api.listShiftTemplates().then((tmpls) => {
+          const tmpl = tmpls.find(t => t.id === templateId);
+          if (tmpl) setTemplateRoles(tmpl.roles ?? []);
+        })
+      );
+    }
+    Promise.all(loads).finally(() => setLoading(false));
+  }, [templateId]);
+
+  function toggleVolunteer(id: number) {
+    setSelectedIds(prev => {
+      const next = new Set(prev);
+      if (next.has(id)) next.delete(id);
+      else next.add(id);
+      return next;
+    });
+  }
+
+  async function handleSubmit(e: FormEvent) {
+    e.preventDefault();
+    setError('');
+    setSaving(true);
+    try {
+      const updated = await api.updateShift(instance.id, {
+        volunteer_ids: Array.from(selectedIds),
+        min_capacity: minCap,
+        max_capacity: maxCap,
+      });
+      onSave(updated);
+    } catch (err: any) {
+      setError(err.message);
+    } finally {
+      setSaving(false);
+    }
+  }
+
+  // Group active volunteers by their operational roles for display
+  const activeVolunteers = volunteers.filter(v => v.active);
+
+  // Build a map of role → volunteers who hold that role
+  const byRole = new Map<string, Volunteer[]>();
+  for (const v of activeVolunteers) {
+    const roles = v.operational_roles
+      ? v.operational_roles.split(',').map(r => r.trim()).filter(Boolean)
+      : [];
+    if (roles.length === 0) {
+      const list = byRole.get('Unassigned') ?? [];
+      list.push(v);
+      byRole.set('Unassigned', list);
+    } else {
+      for (const r of roles) {
+        const list = byRole.get(r) ?? [];
+        list.push(v);
+        byRole.set(r, list);
+      }
+    }
+  }
+
+  // Order sections: template roles first, then any remaining
+  const roleOrder: string[] = [];
+  for (const tr of templateRoles) {
+    if (!roleOrder.includes(tr.role_name)) roleOrder.push(tr.role_name);
+  }
+  byRole.forEach((_, key) => {
+    if (!roleOrder.includes(key)) roleOrder.push(key);
+  });
+
+  if (loading) return <div className="card"><p>Loading volunteers…</p></div>;
+
+  return (
+    <form className="card" onSubmit={handleSubmit}>
+      <h3>Edit Shift: {instance.name} — {instance.date}</h3>
+      {error && <p className="error">{error}</p>}
+
+      {templateRoles.length > 0 && (
+        <p style={{ marginBottom: '0.5rem', color: '#666' }}>
+          <strong>Required:</strong>{' '}
+          {templateRoles.map(r => `${r.count}× ${r.role_name}`).join(', ')}
+        </p>
+      )}
+
+      <fieldset>
+        <legend>Volunteers ({selectedIds.size} selected)</legend>
+        {roleOrder.map(roleName => {
+          const vols = byRole.get(roleName);
+          if (!vols || vols.length === 0) return null;
+          const required = templateRoles.find(r => r.role_name === roleName);
+          const selectedInRole = vols.filter(v => selectedIds.has(v.id)).length;
+          return (
+            <div key={roleName} style={{ marginBottom: '0.75rem' }}>
+              <strong>
+                {roleName}
+                {required && (
+                  <span style={{ fontWeight: 'normal', color: selectedInRole >= required.count ? '#2a7' : '#c55' }}>
+                    {' '}({selectedInRole}/{required.count})
+                  </span>
+                )}
+              </strong>
+              <div style={{ display: 'grid', gridTemplateColumns: 'repeat(auto-fill, minmax(200px, 1fr))', gap: '0.25rem', marginTop: '0.25rem' }}>
+                {vols.map(v => (
+                  <label key={v.id} style={{ display: 'flex', alignItems: 'center', gap: '0.5rem', cursor: 'pointer' }}>
+                    <input
+                      type="checkbox"
+                      checked={selectedIds.has(v.id)}
+                      onChange={() => toggleVolunteer(v.id)}
+                    />
+                    <span>{v.name}{v.is_trainee ? ' (trainee)' : ''}</span>
+                  </label>
+                ))}
+              </div>
+            </div>
+          );
+        })}
+      </fieldset>
+
+      <div style={{ display: 'flex', gap: '1rem', marginTop: '0.5rem' }}>
+        <label>
+          Min Capacity
+          <input type="number" min={1} value={minCap} style={{ width: '5rem' }}
+            onChange={e => setMinCap(Number(e.target.value))} />
+        </label>
+        <label>
+          Max Capacity
+          <input type="number" min={1} value={maxCap} style={{ width: '5rem' }}
+            onChange={e => setMaxCap(Number(e.target.value))} />
+        </label>
+      </div>
+
+      <div style={{ display: 'flex', gap: '0.5rem', marginTop: '1rem' }}>
+        <button type="submit" disabled={saving}>{saving ? 'Saving…' : 'Save'}</button>
+        <button type="button" onClick={onCancel}>Cancel</button>
+      </div>
+    </form>
+  );
+}
+
 // ---------------------------------------------------------------------------
 // Main page
 // ---------------------------------------------------------------------------
@@ -169,9 +335,6 @@ export default function Schedules() {
   const [showTemplateForm, setShowTemplateForm] = useState(false);
   const [editingTemplate, setEditingTemplate] = useState<ShiftTemplate | null>(null);
   const [editingInstance, setEditingInstance] = useState<ShiftInstance | null>(null);
-  const [editVolIds, setEditVolIds] = useState('');
-  const [editMinCap, setEditMinCap] = useState('');
-  const [editMaxCap, setEditMaxCap] = useState('');
 
   // Navigation helpers
   const goToShifts = useCallback((y: number, m: number) => {
@@ -274,31 +437,9 @@ export default function Schedules() {
     }
   }
 
-  function openEditInstance(inst: ShiftInstance) {
-    setEditingInstance(inst);
-    setEditVolIds(inst.volunteers.map(v => v.volunteer_id).join(','));
-    setEditMinCap(String(inst.min_capacity));
-    setEditMaxCap(String(inst.max_capacity));
-  }
-
-  async function handleUpdateInstance(e: FormEvent) {
-    e.preventDefault();
-    if (!editingInstance) return;
-    setError('');
-    try {
-      const volIds = editVolIds.trim()
-        ? editVolIds.split(',').map(s => Number(s.trim())).filter(Boolean)
-        : [];
-      const updated = await api.updateShift(editingInstance.id, {
-        volunteer_ids: volIds,
-        min_capacity: Number(editMinCap),
-        max_capacity: Number(editMaxCap),
-      });
-      setInstances(prev => prev.map(i => (i.id === updated.id ? updated : i)));
-      setEditingInstance(null);
-    } catch (err: any) {
-      setError(err.message);
-    }
+  function handleInstanceSaved(updated: ShiftInstance) {
+    setInstances(prev => prev.map(i => (i.id === updated.id ? updated : i)));
+    setEditingInstance(null);
   }
 
   const allPublished = instances.length > 0 && instances.every(i => i.status === 'published');
@@ -403,27 +544,12 @@ export default function Schedules() {
 
           {/* Instance edit form */}
           {editingInstance && (
-            <form className="card" onSubmit={handleUpdateInstance}>
-              <h3>Edit Shift: {editingInstance.name} — {editingInstance.date}</h3>
-              <label>
-                Volunteer IDs (comma-separated)
-                <input value={editVolIds} onChange={e => setEditVolIds(e.target.value)} />
-              </label>
-              <label>
-                Min Capacity
-                <input type="number" min={1} value={editMinCap}
-                  onChange={e => setEditMinCap(e.target.value)} />
-              </label>
-              <label>
-                Max Capacity
-                <input type="number" min={1} value={editMaxCap}
-                  onChange={e => setEditMaxCap(e.target.value)} />
-              </label>
-              <div style={{ display: 'flex', gap: '0.5rem', marginTop: '1rem' }}>
-                <button type="submit">Save</button>
-                <button type="button" onClick={() => setEditingInstance(null)}>Cancel</button>
-              </div>
-            </form>
+            <ShiftEditForm
+              instance={editingInstance}
+              templateId={editingInstance.template_id}
+              onSave={handleInstanceSaved}
+              onCancel={() => setEditingInstance(null)}
+            />
           )}
 
           {instances.length === 0 ? (
@@ -465,7 +591,7 @@ export default function Schedules() {
                       <td>{inst.volunteers.length}/{inst.max_capacity}</td>
                       {role === 'admin' && (
                         <td>
-                          <button className="btn-small" onClick={() => openEditInstance(inst)}>Edit</button>
+                          <button className="btn-small" onClick={() => setEditingInstance(inst)}>Edit</button>
                         </td>
                       )}
                       {role !== 'admin' && inst.status === 'published' && !confirmed && (