Users grouping improvements

server/modules/record/api/recordQualifierMiddleware.js

@@ -40,12 +40,11 @@ const _recordMatchesUserGroupQualifiers = async ({ user, surveyId, recordUuid, p
 
   const qualifierFilters = await RecordManager.fetchUserQualifierFilters({ user, survey })
 
-  // apply the node value carried by the request (if any) before checking, so an edit that sets
-  // a qualifier attribute to a value outside the user's group qualifiers is rejected too, not just
-  // records that already mismatch before the edit is applied
-  const recordWithPendingNode = pendingNode ? Record.assocNode(pendingNode)(record) : record
-
-  return RecordManager.recordMatchesQualifierFilters({ survey, record: recordWithPendingNode, qualifierFilters })
+  // pendingNode is passed through rather than merged into the record via Record.assocNode: the record
+  // was fetched with fetchForUpdate: false, so its _nodesIndex hasn't been built, and assocNode would
+  // initialize one containing only pendingNode, shadowing every other already-persisted node (see
+  // recordMatchesQualifierFilters' jsdoc for details)
+  return RecordManager.recordMatchesQualifierFilters({ survey, record, qualifierFilters, pendingNode })
 }
 
 /**

server/modules/record/manager/_recordManager/recordQualifierMatcher.js

@@ -77,20 +77,39 @@ export const fetchUserQualifierFilters = async ({ user, survey }, client = db) =
  * @param {object} params.record - The record (with nodes loaded) to check.
  * @param {Array<{nodeDef: object, value: string}>} params.qualifierFilters - The qualifier filters,
  * as returned by fetchUserQualifierFilters.
+ * @param {object} [params.pendingNode] - A node carried by the current request but not yet persisted
+ * (e.g. a create/update of some attribute), applied on top of the record's own nodes when checking, so
+ * an edit that sets a qualifier attribute to a value outside the user's group qualifiers is rejected
+ * too. Deliberately not merged into `record` itself via `Record.assocNode`: doing so on a record whose
+ * `_nodesIndex` hasn't been built (see `fetchForUpdate` in `fetchRecordAndNodesByUuid`) makes
+ * `assocNode` initialize a new index containing only this one node, which then shadows the full node
+ * list for every other lookup (`Record.getNodeChildrenByDefUuid` and friends trust a present index
+ * instead of falling back to a full scan), making every other already-persisted node - including the
+ * qualifier attribute - invisible.
  * @returns {boolean} - True if qualifierFilters is empty (unrestricted), if the record has not been
- * initialized yet (no nodes: checkin will auto-fill qualifier attributes to match, see
- * _applyGroupQualifierValues in recordUpdateManager.js), or every filter matches the record's
- * corresponding attribute value; false otherwise.
+ * initialized yet (no nodes), if a qualifier attribute node is missing or not yet given a value
+ * (record creation will auto-fill qualifier attributes to match, see _applyGroupQualifierValues in
+ * recordUpdateManager.js, but that happens as one of the last steps of record creation, so a
+ * just-created record may briefly have the node without its value yet), or every filter matches the
+ * record's corresponding attribute value; false only when a qualifier attribute has an actual value
+ * that differs from the expected one.
  */
-export const recordMatchesQualifierFilters = ({ survey, record, qualifierFilters }) => {
+export const recordMatchesQualifierFilters = ({ survey, record, qualifierFilters, pendingNode = null }) => {
   if (qualifierFilters.length === 0) return true
   if (Record.getNodesArray(record).length === 0) return true
 
   const rootNode = Record.getRootNode(record)
 
   return qualifierFilters.every(({ nodeDef, value }) => {
-    const node = Record.getNodeChildrenByDefUuid(rootNode, NodeDef.getUuid(nodeDef))(record)[0]
-    if (!node) return false
+    const nodeDefUuid = NodeDef.getUuid(nodeDef)
+    const node =
+      pendingNode && Node.getNodeDefUuid(pendingNode) === nodeDefUuid
+        ? pendingNode
+        : Record.getNodeChildByDefUuid(rootNode, nodeDefUuid)(record)
+    // node missing or not yet given a value: record is still being initialized (the qualifier auto-fill
+    // is one of the last steps of record creation), so this is not (yet) evidence of a cross-group
+    // mismatch; only a value that has actually been set and differs should be rejected
+    if (!node || Objects.isEmpty(Node.getValue(node))) return true
     return NodeValues.isValueEqual({ survey, nodeDef, value: Node.getValue(node), valueSearch: value })
   })
 }

server/modules/record/repository/recordRepository.js

@@ -341,7 +341,8 @@ export const fetchRecordsSummaryBySurveyId = async (
     const paramName = `qualifierValue${index}`
     const colName = NodeDefTable.getColumnName(nodeDef)
     recordsSelectWhereConditions.push(`"${colName}" = $/${paramName}/`)
-    qualifierFilterParams[paramName] = value
+    const columnValue = NodeDef.isCode(nodeDef) ? value?.code : value
+    qualifierFilterParams[paramName] = columnValue
   })
 
   const whereConditionsJoint = recordsSelectWhereConditions.map((condition) => `(${condition})`).join(' AND ')

test/unit/tests/035recordQualifierMatcher.test.js

@@ -1,5 +1,6 @@
 import * as Survey from '@core/survey/survey'
 import * as NodeDef from '@core/survey/nodeDef'
+import * as Record from '@core/record/record'
 import * as Node from '@core/record/node'
 
 import { recordMatchesQualifierFilters } from '@server/modules/record/manager/_recordManager/recordQualifierMatcher'
@@ -77,13 +78,75 @@ describe('recordMatchesQualifierFilters', () => {
     ).toBe(false)
   })
 
-  it('is false when the qualifier attribute node is missing from the record', async () => {
+  it('is true when the qualifier attribute node is missing from the record (not yet auto-filled)', async () => {
     const survey = await buildSurvey()
     const teamDef = Survey.getNodeDefByName('team')(survey)
     const record = RB.record(user, survey, RB.entity('plot')).build()
 
     expect(
       recordMatchesQualifierFilters({ survey, record, qualifierFilters: [{ nodeDef: teamDef, value: 'north' }] })
+    ).toBe(true)
+  })
+
+  it('is true when the qualifier attribute node exists but has no value yet (not yet auto-filled)', async () => {
+    const survey = await buildSurvey()
+    const teamDef = Survey.getNodeDefByName('team')(survey)
+    const record = RB.record(user, survey, RB.entity('plot', RB.attribute('team'))).build()
+
+    expect(
+      recordMatchesQualifierFilters({ survey, record, qualifierFilters: [{ nodeDef: teamDef, value: 'north' }] })
+    ).toBe(true)
+  })
+
+  it('is false when pendingNode sets the qualifier attribute to a value outside the qualifier filters', async () => {
+    const survey = await buildSurvey()
+    const teamDef = Survey.getNodeDefByName('team')(survey)
+    const record = RB.record(user, survey, RB.entity('plot', RB.attribute('team', 'north'))).build()
+    const rootNode = Record.getRootNode(record)
+    const pendingNode = Node.newNode(NodeDef.getUuid(teamDef), Record.getUuid(record), rootNode, 'south')
+
+    expect(
+      recordMatchesQualifierFilters({
+        survey,
+        record,
+        qualifierFilters: [{ nodeDef: teamDef, value: 'north' }],
+        pendingNode,
+      })
     ).toBe(false)
   })
+
+  it('is true when pendingNode sets the qualifier attribute to a value matching the qualifier filters', async () => {
+    const survey = await buildSurvey()
+    const teamDef = Survey.getNodeDefByName('team')(survey)
+    const record = RB.record(user, survey, RB.entity('plot')).build()
+    const rootNode = Record.getRootNode(record)
+    const pendingNode = Node.newNode(NodeDef.getUuid(teamDef), Record.getUuid(record), rootNode, 'north')
+
+    expect(
+      recordMatchesQualifierFilters({
+        survey,
+        record,
+        qualifierFilters: [{ nodeDef: teamDef, value: 'north' }],
+        pendingNode,
+      })
+    ).toBe(true)
+  })
+
+  it('is true when pendingNode is unrelated to the qualifier attribute and the record already matches', async () => {
+    const survey = await buildSurvey()
+    const teamDef = Survey.getNodeDefByName('team')(survey)
+    const statusDef = Survey.getNodeDefByName('status')(survey)
+    const record = RB.record(user, survey, RB.entity('plot', RB.attribute('team', 'north'))).build()
+    const rootNode = Record.getRootNode(record)
+    const pendingNode = Node.newNode(NodeDef.getUuid(statusDef), Record.getUuid(record), rootNode, null)
+
+    expect(
+      recordMatchesQualifierFilters({
+        survey,
+        record,
+        qualifierFilters: [{ nodeDef: teamDef, value: 'north' }],
+        pendingNode,
+      })
+    ).toBe(true)
+  })
 })

webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserCard.tsx

@@ -9,6 +9,7 @@ import type { SurveyUserType } from './useUserGroupsSummary'
 type Props = {
   user: SurveyUserType
   draggable: boolean
+  pending?: boolean
 }
 
 /**
@@ -19,14 +20,19 @@ type Props = {
  * @param props0 - The component props.
  * @param props0.user - The survey user to render.
  * @param props0.draggable - Whether the card can be dragged; false renders a static, non-interactive card.
+ * @param props0.pending - Whether a group change for this user is currently being saved; renders a dimmed, non-interactive card regardless of `draggable`, so it can't be dragged again until the change settles.
  * @returns {React.ReactElement} - The UserCard component.
  */
 const UserCard = (props: Props): React.ReactElement => {
-  const { user, draggable } = props
+  const { user, draggable, pending = false } = props
   const userUuid = User.getUuid(user) as string
 
+  const className = ['user-card', draggable && 'user-card--draggable', pending && 'user-card--pending']
+    .filter(Boolean)
+    .join(' ')
+
   return (
-    <li className={`user-card${draggable ? ' user-card--draggable' : ''}`} data-user-uuid={userUuid}>
+    <li className={className} data-user-uuid={userUuid}>
       <ProfilePicture userUuid={userUuid} thumbnail />
       <div className="user-card__details">
         <div className="user-card__name">{User.getName(user)}</div>

webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupColumn.tsx

@@ -16,6 +16,7 @@ type Props = {
   groupKey: string
   members: SurveyUserType[]
   draggable: boolean
+  pendingUserUuids: Set<string>
   containerRef: (el: HTMLUListElement | null) => void
 }
 
@@ -28,11 +29,12 @@ type Props = {
  * @param props0.groupKey - The group's uuid, or the UNASSIGNED_GROUP_KEY sentinel; set as the column's data-group-uuid attribute.
  * @param props0.members - The survey users currently assigned to this column.
  * @param props0.draggable - Whether the cards in this column can be dragged.
+ * @param props0.pendingUserUuids - Uuids of users with a group change currently in flight; their cards render as non-draggable until it settles.
  * @param props0.containerRef - Callback ref registering this column's drop-target list element with the drag-and-drop hook.
  * @returns {React.ReactElement} - The UserGroupColumn component.
  */
 const UserGroupColumn = (props: Props): React.ReactElement => {
-  const { group, groupKey, members, draggable, containerRef } = props
+  const { group, groupKey, members, draggable, pendingUserUuids, containerRef } = props
   const i18n = useI18n()
   const preferredLang = useSurveyPreferredLang() as string
 
@@ -50,9 +52,11 @@ const UserGroupColumn = (props: Props): React.ReactElement => {
         {members.length === 0 && (
           <li className="user-group-column__empty">{i18n.t('usersView:userGroup.noMembers')}</li>
         )}
-        {members.map((user) => (
-          <UserCard key={User.getUuid(user) as string} user={user} draggable={draggable} />
-        ))}
+        {members.map((user) => {
+          const userUuid = User.getUuid(user) as string
+          const pending = pendingUserUuids.has(userUuid)
+          return <UserCard key={userUuid} user={user} draggable={draggable && !pending} pending={pending} />
+        })}
       </ul>
     </div>
   )

webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.scss

@@ -84,6 +84,11 @@
     cursor: grab;
   }
 
+  &--pending {
+    opacity: 0.6;
+    cursor: wait;
+  }
+
   &.draggable-source--is-dragging {
     opacity: 0.4;
   }

webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.tsx

@@ -30,7 +30,7 @@ interface ColumnData {
  */
 const UserGroupsSummary = (): React.ReactElement => {
   const canManage = useAuthCanManageUserGroups()
-  const { groups, users, groupUuidByUserUuid, onChangeUserGroup, reload } = useUserGroupsSummary()
+  const { groups, users, groupUuidByUserUuid, onChangeUserGroup, pendingUserUuids } = useUserGroupsSummary()
 
   const columns: ColumnData[] = useMemo(() => {
     const unassignedMembers = users.filter((user) => !groupUuidByUserUuid[User.getUuid(user) as string])
@@ -49,7 +49,6 @@ const UserGroupsSummary = (): React.ReactElement => {
     enabled: canManage,
     columnKeys: columns.map((column) => column.key),
     onChangeUserGroup,
-    reload,
     unassignedGroupKey: UNASSIGNED_GROUP_KEY,
   })
 
@@ -62,6 +61,7 @@ const UserGroupsSummary = (): React.ReactElement => {
           group={column.group}
           members={column.members}
           draggable={canManage}
+          pendingUserUuids={pendingUserUuids}
           containerRef={registerColumnRef(column.key)}
         />
       ))}

webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsKanbanDnd.ts

@@ -8,7 +8,6 @@ interface UseUserGroupsKanbanDndParams {
   enabled: boolean
   columnKeys: string[]
   onChangeUserGroup: OnChangeUserGroup
-  reload: () => Promise<void>
   unassignedGroupKey: string
 }
 
@@ -29,37 +28,35 @@ interface UseUserGroupsKanbanDndResult {
  * cross-column handling below is therefore deferred to a microtask, so it runs once that
  * synchronous continuation has completed and the element has actually been relocated; only then
  * is it safe to move it back to its original column so the DOM still matches what React currently
- * believes is true, letting the subsequent state-driven re-render (whether onChangeUserGroup
- * succeeds or, via the reload fallback, if it fails) reconcile the real move safely instead of
- * crashing on a stale DOM parent reference.
+ * believes is true, letting the subsequent state-driven re-render - triggered by onChangeUserGroup
+ * optimistically updating group membership before it even calls the server, and again if it later
+ * rolls that back on failure - reconcile the real move safely instead of crashing on a stale DOM
+ * parent reference.
  *
  * @param params0 - The hook parameters.
  * @param params0.enabled - Whether drag-and-drop should be active; when false, Sortable is never instantiated and cards render inert.
  * @param params0.columnKeys - The current ordered list of column keys (group uuids plus the unassigned sentinel); Sortable is reinitialized whenever this list changes shape.
- * @param params0.onChangeUserGroup - Called with the dragged user's uuid and the destination column's group uuid (or null) on a cross-column drop.
- * @param params0.reload - Re-fetches board data; called if onChangeUserGroup rejects, to force a state-driven re-render that reconciles the DOM.
+ * @param params0.onChangeUserGroup - Called with the dragged user's uuid and the destination column's group uuid (or null) on a cross-column drop; expected to update membership state optimistically and roll it back itself on failure.
  * @param params0.unassignedGroupKey - The sentinel value used as the Unassigned column's data-group-uuid, mapped to null before calling onChangeUserGroup.
  * @returns {UseUserGroupsKanbanDndResult} An object exposing registerColumnRef, used to obtain a stable callback ref for each column's drop-target element.
  */
 export const useUserGroupsKanbanDnd = (params: UseUserGroupsKanbanDndParams): UseUserGroupsKanbanDndResult => {
-  const { enabled, columnKeys, onChangeUserGroup, reload, unassignedGroupKey } = params
+  const { enabled, columnKeys, onChangeUserGroup, unassignedGroupKey } = params
 
   const containersByKeyRef = useRef<Map<string, HTMLUListElement>>(new Map())
 
   // Sortable's `sortable:stop` listener is registered once, when the Sortable instance is
   // constructed (see the effect below) - it does NOT re-run on every render, only when the set
   // of columns changes shape. onChangeUserGroup's identity, however, changes on every successful
-  // drag (it depends on groupUuidByUserUuid). Reading onChangeUserGroup/reload through refs kept
-  // current on every render means the listener always calls the latest version, instead of a
-  // closure captured once and frozen until the next column-shape change - without this, a second
-  // drag of the same card in one session would use a stale membership snapshot and silently leave
-  // the user in both their old and new group.
+  // drag (it depends on groupUuidByUserUuid). Reading it through a ref kept current on every render
+  // means the listener always calls the latest version, instead of a closure captured once and
+  // frozen until the next column-shape change - without this, a second drag of the same card in one
+  // session would use a stale membership snapshot and silently leave the user in both their old and
+  // new group.
   const onChangeUserGroupRef = useRef(onChangeUserGroup)
-  const reloadRef = useRef(reload)
   useLayoutEffect(() => {
     onChangeUserGroupRef.current = onChangeUserGroup
-    reloadRef.current = reload
-  }, [onChangeUserGroup, reload])
+  }, [onChangeUserGroup])
 
   const registerColumnRef = useCallback(
     (groupKey: string) =>
@@ -73,20 +70,16 @@ export const useUserGroupsKanbanDnd = (params: UseUserGroupsKanbanDndParams): Us
     []
   )
 
-  // If onChangeUserGroup rejects, force a state-driven re-render (via reload) to reconcile the
-  // DOM; a failed reload is swallowed since there's nothing further to fall back to.
-  const handleChangeUserGroupError = (): void => {
-    reloadRef.current().catch(() => {})
-  }
-
   // Sortable has now physically moved cardElement into newContainer as part of finishing the drag
   // gesture, independent of React. React still believes the card belongs to oldContainer (nothing
-  // has re-rendered yet), so its next reconciliation - triggered by the reload() that
-  // onChangeUserGroup runs internally on success, or by handleChangeUserGroupError's reload() on
-  // failure - would try to remove the card from oldContainer by calling oldContainer.removeChild()
-  // on a node whose actual DOM parent is now newContainer, which throws. Moving the card back to
-  // oldContainer here, before React gets a chance to reconcile, keeps the DOM consistent with what
-  // React last rendered; the resulting state update then drives the real move.
+  // has re-rendered yet), so its next reconciliation - triggered by onChangeUserGroup's optimistic
+  // state update, fired synchronously below before the server request even starts - would try to
+  // remove the card from oldContainer by calling oldContainer.removeChild() on a node whose actual
+  // DOM parent is now newContainer, which throws. Moving the card back to oldContainer here, before
+  // React gets a chance to reconcile, keeps the DOM consistent with what React last rendered; the
+  // resulting state update then drives the real move. onChangeUserGroup rolls its own state update
+  // back on failure (and reports the error), so no further handling is needed here - the rejection
+  // is caught only to avoid an unhandled promise rejection.
   const reconcileCrossColumnDrop = (params: {
     oldContainer: HTMLElement
     cardElement: HTMLElement
@@ -98,7 +91,7 @@ export const useUserGroupsKanbanDnd = (params: UseUserGroupsKanbanDndParams): Us
 
     const groupUuidNew = groupKeyNew === unassignedGroupKey ? null : groupKeyNew
 
-    onChangeUserGroupRef.current(userUuid, groupUuidNew).catch(handleChangeUserGroupError)
+    onChangeUserGroupRef.current(userUuid, groupUuidNew).catch(() => {})
   }
 
   const handleSortableStop = (event: SortableStopEvent): void => {
@@ -142,8 +135,8 @@ export const useUserGroupsKanbanDnd = (params: UseUserGroupsKanbanDndParams): Us
     }
     // columnKeys is read via its joined form so the effect only reinitializes Sortable when the
     // set of columns actually changes shape, not on every unrelated re-render. handleSortableStop
-    // reads onChangeUserGroup/reload through refs kept current every render (see
-    // onChangeUserGroupRef/reloadRef above), so it's intentionally excluded too.
+    // reads onChangeUserGroup through a ref kept current every render (see onChangeUserGroupRef
+    // above), so it's intentionally excluded too.
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [enabled, columnKeys.join('|')])