diff --git a/package.json b/package.json index f3a59408bc..abbc5020b0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "open-foris-arena", - "version": "2.5.9", + "version": "2.5.10", "license": "MIT", "engines": { "node": "^24.11.1" @@ -237,4 +237,4 @@ "{common,core,server,test,webapp}/**/*.{js,jsx,ts,tsx}": "eslint --cache --fix" }, "packageManager": "yarn@4.17.1" -} +} \ No newline at end of file diff --git a/server/modules/dataExport/api/dataExportApi.js b/server/modules/dataExport/api/dataExportApi.js index 1662148006..769cad525e 100644 --- a/server/modules/dataExport/api/dataExportApi.js +++ b/server/modules/dataExport/api/dataExportApi.js @@ -7,6 +7,7 @@ import * as Request from '@server/utils/request' import * as AuthMiddleware from '@server/modules/auth/authApiMiddleware' import { sendTempFileToResponse } from '@server/modules/fileDownload/api/fileDownloadApi' +import { requireRecordsMatchUserGroupQualifiers } from '@server/modules/record/api/recordQualifierMiddleware' import * as SurveyService from '@server/modules/survey/service/surveyService' import * as DataExportService from '../service/dataExportService' @@ -18,25 +19,30 @@ const checkExportUuid = (exportUuid) => { } export const init = (app) => { - app.post('/survey/:surveyId/data-export', AuthMiddleware.requireRecordsExportPermission, async (req, res, next) => { - try { - const { surveyId, cycle, recordUuids, search, options } = Request.getParams(req) - - const user = Request.getUser(req) - - const job = DataExportService.startCsvDataExportJob({ - user, - surveyId, - cycle, - recordUuids, - search, - options, - }) - res.json({ job: JobUtils.jobToJSON(job) }) - } catch (error) { - next(error) + app.post( + '/survey/:surveyId/data-export', + AuthMiddleware.requireRecordsExportPermission, + requireRecordsMatchUserGroupQualifiers, + async (req, res, next) => { + try { + const { surveyId, cycle, recordUuids, search, options } = Request.getParams(req) + + const user = Request.getUser(req) + + const job = DataExportService.startCsvDataExportJob({ + user, + surveyId, + cycle, + recordUuids, + search, + options, + }) + res.json({ job: JobUtils.jobToJSON(job) }) + } catch (error) { + next(error) + } } - }) + ) app.post( '/survey/:surveyId/data-summary-export', diff --git a/server/modules/record/api/recordApi.js b/server/modules/record/api/recordApi.js index 4936c9d008..d5d855e6a9 100644 --- a/server/modules/record/api/recordApi.js +++ b/server/modules/record/api/recordApi.js @@ -239,8 +239,9 @@ export const init = (app) => { app.get('/survey/:surveyId/records/summary/export', requireRecordListExportPermission, async (req, res, next) => { try { const { surveyId, cycle, fileFormat = FileFormats.xlsx } = Request.getParams(req) + const user = Request.getUser(req) - await RecordService.exportRecordsSummary({ res, surveyId, cycle, fileFormat }) + await RecordService.exportRecordsSummary({ res, surveyId, cycle, fileFormat, user }) } catch (error) { next(error) } diff --git a/server/modules/record/api/recordQualifierMiddleware.js b/server/modules/record/api/recordQualifierMiddleware.js index 1905015445..440d847156 100644 --- a/server/modules/record/api/recordQualifierMiddleware.js +++ b/server/modules/record/api/recordQualifierMiddleware.js @@ -38,14 +38,13 @@ const _recordMatchesUserGroupQualifiers = async ({ user, surveyId, recordUuid, p const survey = await fetchSurveyByCycle(Record.getCycle(record)) - const qualifierFilters = await RecordManager.fetchUserQualifierFilters({ user, survey }) + const qualifierFilters = await SurveyManager.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 }) } /** diff --git a/server/modules/record/manager/_recordManager/recordQualifierMatcher.js b/server/modules/record/manager/_recordManager/recordQualifierMatcher.js index 5250bb7f4b..344781748f 100644 --- a/server/modules/record/manager/_recordManager/recordQualifierMatcher.js +++ b/server/modules/record/manager/_recordManager/recordQualifierMatcher.js @@ -1,96 +1,57 @@ -import { Objects, ServiceRegistry } from '@openforis/arena-core' -import { ServerServiceType } from '@openforis/arena-server' +import { Objects } from '@openforis/arena-core' -import * as User from '@core/user/user' -import * as UserGroup from '@core/user/userGroup/userGroup' -import * as UserGroupQualifier from '@core/user/userGroup/userGroupQualifier' -import * as Survey from '@core/survey/survey' import * as NodeDef from '@core/survey/nodeDef' -import * as CategoryItem from '@core/survey/categoryItem' import * as Record from '@core/record/record' import * as Node from '@core/record/node' import { NodeValues } from '@core/record/nodeValues' -import { db } from '@server/db/db' -import { CategoryItemProviderDefault } from '@server/modules/category/manager/categoryItemProviderDefault' - -/** - * Determines the qualifier attribute/value pairs that the given user's group restricts them to, if any. - * Mirrors the lookup used to auto-fill qualifier attributes when a new record is created - * (see _applyGroupQualifierValues in recordUpdateManager.js), so write-time and read-time - * restrictions stay consistent: only the user's first UserGroup in the survey is considered. - * For code attributes, the qualifier's code is resolved to its category item via a direct DB lookup - * (works regardless of category size, unlike the survey's category items ref data index, which - * excludes "big" categories), so the returned value can be compared against a record's node value by - * item uuid rather than relying on the ref data index being loaded. - * @param {object} params - The function parameters. - * @param {object} params.user - The current user. - * @param {object} params.survey - The survey, with node defs loaded. - * @param {pgPromise.IDatabase} [client] - The db client. - * @returns {Promise>} - An empty array when the user is - * unrestricted (no group, or group with no matching qualifiers), otherwise the list of qualifier - * filters to apply. - */ -export const fetchUserQualifierFilters = async ({ user, survey }, client = db) => { - const qualifierNodeDefs = Survey.getQualifierNodeDefs(survey) - if (qualifierNodeDefs.length === 0) return [] - - const userGroupService = ServiceRegistry.getInstance().getService(ServerServiceType.userGroup) - const userGroups = await userGroupService.getManyByUser( - { userUuid: User.getUuid(user), surveyUuid: Survey.getUuid(survey) }, - client - ) - const userGroup = userGroups[0] - if (!userGroup) return [] - - const qualifiers = UserGroup.getQualifiers(userGroup) - if (qualifiers.length === 0) return [] - - const filters = [] - for (const nodeDef of qualifierNodeDefs) { - const qualifier = qualifiers.find((q) => UserGroupQualifier.getName(q) === NodeDef.getName(nodeDef)) - const qualifierValue = qualifier ? UserGroupQualifier.getValue(qualifier) : null - if (Objects.isEmpty(qualifierValue)) continue - - if (NodeDef.isCode(nodeDef)) { - const categoryUuid = NodeDef.getCategoryUuid(nodeDef) - const item = await CategoryItemProviderDefault.getItemByCode( - { survey, categoryUuid, code: qualifierValue }, - client - ) - if (!item) continue - filters.push({ - nodeDef, - value: Node.newNodeValueCode({ itemUuid: CategoryItem.getUuid(item), code: qualifierValue }), - }) - } else { - filters.push({ nodeDef, value: qualifierValue }) - } - } - return filters -} - /** * Checks whether a record's qualifier attribute values match the given qualifier filters. * @param {object} params - The function parameters. * @param {object} params.survey - The survey, with node defs and categories loaded. * @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 {Array<{nodeDef: object, value: *}>} params.qualifierFilters - The qualifier filters, + * as returned by SurveyManager.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 when a qualifier attribute has an actual value that + * differs from the expected one, or when `pendingNode` explicitly targets a qualifier attribute with + * an empty value: the UI never lets a user edit an already-applied qualifier node (see + * `isQualifierValueApplied` in nodeDefSwitch.js), so a request clearing it can only be a deliberate + * attempt to drop the restriction, not an in-progress record still being initialized. */ -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 isPendingNode = pendingNode && Node.getNodeDefUuid(pendingNode) === nodeDefUuid + const node = isPendingNode ? pendingNode : Record.getNodeChildByDefUuid(rootNode, nodeDefUuid)(record) + + if (!node) return true + + if (Objects.isEmpty(Node.getValue(node))) { + // a persisted node without a value yet: record is still being initialized (the qualifier + // auto-fill is one of the last steps of record creation), not (yet) evidence of a mismatch; + // but pendingNode explicitly clearing the value is a deliberate attempt to bypass the + // restriction, so it must be rejected rather than given the same benefit of the doubt + return !isPendingNode + } return NodeValues.isValueEqual({ survey, nodeDef, value: Node.getValue(node), valueSearch: value }) }) } diff --git a/server/modules/record/manager/_recordManager/recordUpdateManager.js b/server/modules/record/manager/_recordManager/recordUpdateManager.js index 607806076b..e5d792353f 100644 --- a/server/modules/record/manager/_recordManager/recordUpdateManager.js +++ b/server/modules/record/manager/_recordManager/recordUpdateManager.js @@ -20,8 +20,8 @@ import * as RecordFileManager from '@server/modules/record/manager/recordFileMan import * as NodeDefRepository from '@server/modules/nodeDef/repository/nodeDefRepository' import * as DataTableUpdateRepository from '@server/modules/surveyRdb/repository/dataTableUpdateRepository' import * as DataTableReadRepository from '@server/modules/surveyRdb/repository/dataTableReadRepository' +import * as SurveyManager from '@server/modules/survey/manager/surveyManager' -import * as RecordQualifierMatcher from './recordQualifierMatcher' import * as RecordValidationManager from './recordValidationManager' import * as NodeCreationManager from './nodeCreationManager' import * as NodeUpdateManager from './nodeUpdateManager' @@ -79,7 +79,7 @@ const _applyGroupQualifierValues = async ( { user, survey, record, timezoneOffset, nodesUpdateListener, nodesValidationListener }, client ) => { - const qualifierFilters = await RecordQualifierMatcher.fetchUserQualifierFilters({ user, survey }, client) + const qualifierFilters = await SurveyManager.fetchUserQualifierFilters({ user, survey }, client) if (qualifierFilters.length === 0) return record const rootNode = Record.getRootNode(record) diff --git a/server/modules/record/manager/recordManager.js b/server/modules/record/manager/recordManager.js index b5e549d928..f1a9335ff3 100644 --- a/server/modules/record/manager/recordManager.js +++ b/server/modules/record/manager/recordManager.js @@ -27,7 +27,6 @@ import * as UserManager from '@server/modules/user/manager/userManager' import * as RecordRepository from '../repository/recordRepository' import * as FileRepository from '../repository/fileRepository' import * as NodeRepository from '../repository/nodeRepository' -import * as RecordQualifierMatcher from './_recordManager/recordQualifierMatcher' import * as RecordUpdateManager from './_recordManager/recordUpdateManager' import { NodeRdbManager } from './_recordManager/nodeRDBManager' @@ -78,7 +77,7 @@ export const fetchRecordsSummaryBySurveyId = async ( summaryDefs = Survey.getRootSummaryDefs({ cycle })(survey) nodeDefKeys = Survey.getNodeDefRootKeysSorted({ cycle })(survey) if (user) { - qualifierNodeDefFilters = await RecordQualifierMatcher.fetchUserQualifierFilters({ user, survey }, client) + qualifierNodeDefFilters = await SurveyManager.fetchUserQualifierFilters({ user, survey }, client) } } @@ -160,9 +159,7 @@ export const countRecordsBySurveyId = async ( const survey = await SurveyManager.fetchSurveyAndNodeDefsBySurveyId({ surveyId, cycle, draft: nodeDefsDraft }, client) const nodeDefKeys = Survey.getNodeDefRootKeys(survey) const summaryDefs = Survey.getRootSummaryDefs({ cycle })(survey) - const qualifierNodeDefFilters = user - ? await RecordQualifierMatcher.fetchUserQualifierFilters({ user, survey }, client) - : [] + const qualifierNodeDefFilters = user ? await SurveyManager.fetchUserQualifierFilters({ user, survey }, client) : [] return RecordRepository.countRecordsBySurveyId( { surveyId, cycle, search, nodeDefKeys, summaryDefs, nodeDefRoot, ownerUuid, qualifierNodeDefFilters }, @@ -262,7 +259,7 @@ export const fetchRecordAndNodesByUuid = async ( export { fetchNodeByUuid, fetchChildNodesByNodeDefUuids } from '../repository/nodeRepository' -export { fetchUserQualifierFilters, recordMatchesQualifierFilters } from './_recordManager/recordQualifierMatcher' +export { recordMatchesQualifierFilters } from './_recordManager/recordQualifierMatcher' const fetchNodeRefData = async ({ survey, node, isCode }, client) => { const surveyId = Survey.getId(survey) diff --git a/server/modules/record/repository/recordRepository.js b/server/modules/record/repository/recordRepository.js index 18e192c97f..6269e02b25 100644 --- a/server/modules/record/repository/recordRepository.js +++ b/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 ') diff --git a/server/modules/record/service/recordService.js b/server/modules/record/service/recordService.js index 848a100fa5..91f582d3bd 100644 --- a/server/modules/record/service/recordService.js +++ b/server/modules/record/service/recordService.js @@ -85,11 +85,12 @@ export const { updateRecordOwner, } = RecordManager -export const exportRecordsSummary = async ({ res, surveyId, cycle, fileFormat }) => { +export const exportRecordsSummary = async ({ res, surveyId, cycle, fileFormat, user }) => { const { list, nodeDefKeys } = await RecordManager.fetchRecordsSummaryBySurveyId({ surveyId, cycle, includeCounts: true, + user, }) const valueFormattersByType = { diff --git a/server/modules/survey/manager/surveyManager.js b/server/modules/survey/manager/surveyManager.js index 8def61f024..cb70c50097 100644 --- a/server/modules/survey/manager/surveyManager.js +++ b/server/modules/survey/manager/surveyManager.js @@ -599,3 +599,5 @@ export const deleteAllActivityLog = async ({ surveyId }, client = db) => ActivityLogRepository.deleteAll({ surveyId }, client) export const { dropSurveySchema } = SurveyRepository + +export { fetchUserQualifierFilters } from './surveyUserGroupQualifierFilters' diff --git a/server/modules/survey/manager/surveyUserGroupQualifierFilters.js b/server/modules/survey/manager/surveyUserGroupQualifierFilters.js new file mode 100644 index 0000000000..0bc18c1a62 --- /dev/null +++ b/server/modules/survey/manager/surveyUserGroupQualifierFilters.js @@ -0,0 +1,69 @@ +import { Objects, ServiceRegistry } from '@openforis/arena-core' +import { ServerServiceType } from '@openforis/arena-server' + +import * as User from '@core/user/user' +import * as UserGroup from '@core/user/userGroup/userGroup' +import * as UserGroupQualifier from '@core/user/userGroup/userGroupQualifier' +import * as Survey from '@core/survey/survey' +import * as NodeDef from '@core/survey/nodeDef' +import * as CategoryItem from '@core/survey/categoryItem' +import * as Node from '@core/record/node' + +import { db } from '@server/db/db' +import { CategoryItemProviderDefault } from '@server/modules/category/manager/categoryItemProviderDefault' + +/** + * Determines the qualifier attribute/value pairs that the given user's group restricts them to, if any. + * Mirrors the lookup used to auto-fill qualifier attributes when a new record is created + * (see _applyGroupQualifierValues in recordUpdateManager.js), so write-time and read-time + * restrictions stay consistent: only the user's first UserGroup in the survey is considered. + * For code attributes, the qualifier's code is resolved to its category item via a direct DB lookup + * (works regardless of category size, unlike the survey's category items ref data index, which + * excludes "big" categories), so the returned value can be compared against a record's node value by + * item uuid rather than relying on the ref data index being loaded. + * @param {object} params - The function parameters. + * @param {object} params.user - The current user. + * @param {object} params.survey - The survey, with node defs loaded. + * @param {pgPromise.IDatabase} [client] - The db client. + * @returns {Promise>} - An empty array when the user is + * unrestricted (no group, or group with no matching qualifiers), otherwise the list of qualifier + * filters to apply. + */ +export const fetchUserQualifierFilters = async ({ user, survey }, client = db) => { + const qualifierNodeDefs = Survey.getQualifierNodeDefs(survey) + if (qualifierNodeDefs.length === 0) return [] + + const userGroupService = ServiceRegistry.getInstance().getService(ServerServiceType.userGroup) + const userGroups = await userGroupService.getManyByUser( + { userUuid: User.getUuid(user), surveyUuid: Survey.getUuid(survey) }, + client + ) + const userGroup = userGroups[0] + if (!userGroup) return [] + + const qualifiers = UserGroup.getQualifiers(userGroup) + if (qualifiers.length === 0) return [] + + const filters = [] + for (const nodeDef of qualifierNodeDefs) { + const qualifier = qualifiers.find((q) => UserGroupQualifier.getName(q) === NodeDef.getName(nodeDef)) + const qualifierValue = qualifier ? UserGroupQualifier.getValue(qualifier) : null + if (Objects.isEmpty(qualifierValue)) continue + + if (NodeDef.isCode(nodeDef)) { + const categoryUuid = NodeDef.getCategoryUuid(nodeDef) + const item = await CategoryItemProviderDefault.getItemByCode( + { survey, categoryUuid, code: qualifierValue }, + client + ) + if (!item) continue + filters.push({ + nodeDef, + value: Node.newNodeValueCode({ itemUuid: CategoryItem.getUuid(item), code: qualifierValue }), + }) + } else { + filters.push({ nodeDef, value: qualifierValue }) + } + } + return filters +} diff --git a/server/modules/surveyRdb/manager/surveyRdbManager.js b/server/modules/surveyRdb/manager/surveyRdbManager.js index 672b919f8a..780afbd05c 100644 --- a/server/modules/surveyRdb/manager/surveyRdbManager.js +++ b/server/modules/surveyRdb/manager/surveyRdbManager.js @@ -16,6 +16,7 @@ import * as NodeDefTable from '@common/surveyRdb/nodeDefTable' import { db } from '@server/db/db' import * as DbUtils from '@server/db/dbUtils' import * as RecordRepository from '@server/modules/record/repository/recordRepository' +import * as SurveyManager from '@server/modules/survey/manager/surveyManager' import * as FileUtils from '@server/utils/file/fileUtils' import * as FlatDataWriter from '@server/utils/file/flatDataWriter' import { StreamUtils } from '@server/utils/streamUtils' @@ -58,21 +59,19 @@ const maxExcelCellsLimit = 1000000 /** * Executes a select query on an entity definition data view. - * * @param {!object} params - The query parameters. * @param {!Survey} [params.survey] - The survey. * @param {!string} [params.cycle] - The survey cycle. * @param {!Query} [params.query] - The Query to execute. - * @param {boolean} [params.columnNodeDefs=false] - Whether to select only columnNodes. - * @param {boolean} [params.includeFileAttributeDefs=true] - Whether to include file attribute column node defs. + * @param {boolean} [params.columnNodeDefs] - Whether to select only columnNodes. + * @param {boolean} [params.includeFileAttributeDefs] - Whether to include file attribute column node defs. * @param {Array} [params.recordSteps] - The record steps used to filter data. If null or empty, data in all steps will be fetched. * @param {string} [params.recordOwnerUuid] - The record owner UUID. If null, data from all records will be fetched, otherwise only the ones owned by the specified user. - * @param {number} [params.offset=null] - The query offset. - * @param {number} [params.limit=null] - The query limit. - * @param {boolean|object} [params.outputStream=null] - The output to be used to stream the data (if specified). - * @param {string} [params.fileFormat=null] - The format of the output file (csv or xlsx). - * - * @param {pgPromise.IDatabase} [client=db] - The database client. + * @param {number} [params.offset] - The query offset. + * @param {number} [params.limit] - The query limit. + * @param {boolean|object} [params.outputStream] - The output to be used to stream the data (if specified). + * @param {string} [params.fileFormat] - The format of the output file (csv or xlsx). + * @param {pgPromise.IDatabase} [client] - The database client. * @returns {Promise} - An object with fetched rows and selected fields. */ export const fetchViewData = async (params, client = db) => { @@ -154,16 +153,15 @@ export const fetchViewData = async (params, client = db) => { /** * Runs a select query on a data view associated to an entity node definition, * aggregating the rows by the given measures aggregate functions and grouping by the given dimensions. - * * @param {!object} params - The query parameters. * @param {!Survey} [params.survey] - The survey. * @param {!string} [params.cycle] - The survey cycle. * @param {!Query} [params.query] - The query object. - * @param {number} [params.offset=null] - The query offset. + * @param {number} [params.offset] - The query offset. * @param {string} [params.recordOwnerUuid] - The record owner UUID. If null, data from all records will be fetched, otherwise only the ones owned by the specified user. - * @param {number} [params.limit=null] - The query limit. - * @param {boolean} [params.outputStream=null] - The output to be used to stream the data (if specified). - * @param {object} [params.options=null] - Export options object (e.g. {fileFormat: 'csv'}). + * @param {number} [params.limit] - The query limit. + * @param {boolean} [params.outputStream] - The output to be used to stream the data (if specified). + * @param {object} [params.options] - Export options object (e.g. {fileFormat: 'csv'}). * @param {object} client - DB client. * @returns {Promise} - An object with fetched rows and selected fields. */ @@ -189,10 +187,14 @@ export const fetchViewDataAgg = async (params, client = db) => { return result } -const _determineRecordUuidsFilter = async ({ survey, cycle, recordsModifiedAfter, recordUuids, search }) => { +const _determineRecordUuidsFilter = async ({ survey, cycle, recordsModifiedAfter, recordUuids, search, user }) => { + // recordUuids explicitly selected by the user are already checked against qualifiers by + // requireRecordsMatchUserGroupQualifiers, at the API layer if (recordUuids) return recordUuids - if (Objects.isEmpty(search) && !recordsModifiedAfter) return null + const qualifierNodeDefFilters = user ? await SurveyManager.fetchUserQualifierFilters({ user, survey }) : [] + + if (Objects.isEmpty(search) && !recordsModifiedAfter && qualifierNodeDefFilters.length === 0) return null const surveyId = Survey.getId(survey) const nodeDefRoot = Survey.getNodeDefRoot(survey) @@ -203,6 +205,7 @@ const _determineRecordUuidsFilter = async ({ survey, cycle, recordsModifiedAfter nodeDefKeys, cycle, search, + qualifierNodeDefFilters, }) if (recordsModifiedAfter) { recordsSummaries = recordsSummaries.filter((recordSummary) => @@ -340,6 +343,7 @@ const checkCanExportEntitiesData = async ({ fileFormat, nodeDefs, fetchParamsByN export const fetchEntitiesDataToCsvFiles = async ( { + user = null, survey, cycle, search = null, @@ -361,6 +365,7 @@ export const fetchEntitiesDataToCsvFiles = async ( recordsModifiedAfter, recordUuids: recordUuidsParam, search, + user, }) if (filterRecordUuids?.length === 0) { @@ -451,7 +456,6 @@ export const fetchEntitiesFileUuidsByCycle = async ( /** * Counts the number of rows in the data view related to the specified query object. - * * @param {!object} params - The query parameters. * @param {!Survey} [params.survey] - The survey. * @param {!string} [params.cycle] - The survey cycle. @@ -470,7 +474,6 @@ export const countTable = async ({ survey, cycle, recordOwnerUuid, query }, clie /** * Returns the count of rows in the data views of the specified entity defs, indexed by entity def UUID. * If entityDefUuids is not specified, all the entity defs will be considered. - * * @param {!object} params - The parameters. * @param {!Survey} [params.survey] - The survey. * @param {!string} [params.cycle] - The survey cycle. diff --git a/server/modules/surveyRdb/service/surveyRdbService.js b/server/modules/surveyRdb/service/surveyRdbService.js index 5d691add86..c657e132d9 100644 --- a/server/modules/surveyRdb/service/surveyRdbService.js +++ b/server/modules/surveyRdb/service/surveyRdbService.js @@ -182,6 +182,7 @@ export const fetchEntitiesDataToCsvFiles = async ({ } return SurveyRdbManager.fetchEntitiesDataToCsvFiles({ + user, survey, cycle, outputDir, diff --git a/test/unit/tests/035recordQualifierMatcher.test.js b/test/unit/tests/035recordQualifierMatcher.test.js index 8d9ca55f4b..d0ab79d47c 100644 --- a/test/unit/tests/035recordQualifierMatcher.test.js +++ b/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,92 @@ 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 false when pendingNode clears an already matching qualifier attribute value', 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, null) + + 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) + }) }) diff --git a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserCard.tsx b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserCard.tsx index e67cc11ff0..05025e1c69 100644 --- a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserCard.tsx +++ b/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 ( -
  • +
  • {User.getName(user)}
    diff --git a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupColumn.tsx b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupColumn.tsx index 3a5c2af4ac..12f0f1831b 100644 --- a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupColumn.tsx +++ b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupColumn.tsx @@ -16,6 +16,7 @@ type Props = { groupKey: string members: SurveyUserType[] draggable: boolean + pendingUserUuids: ReadonlySet 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 && (
  • {i18n.t('usersView:userGroup.noMembers')}
  • )} - {members.map((user) => ( - - ))} + {members.map((user) => { + const userUuid = User.getUuid(user) as string + const pending = pendingUserUuids.has(userUuid) + return + })} ) diff --git a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.scss b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.scss index c2d5f8df4e..643adb847d 100644 --- a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.scss +++ b/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; } diff --git a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.tsx b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.tsx index 0d98155d2b..d0d55f6745 100644 --- a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/UserGroupsSummary.tsx +++ b/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)} /> ))} diff --git a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsKanbanDnd.ts b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsKanbanDnd.ts index 57fa8802c9..b2cdb3ab0e 100644 --- a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsKanbanDnd.ts +++ b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsKanbanDnd.ts @@ -8,7 +8,6 @@ interface UseUserGroupsKanbanDndParams { enabled: boolean columnKeys: string[] onChangeUserGroup: OnChangeUserGroup - reload: () => Promise 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>(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('|')]) diff --git a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsSummary.ts b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsSummary.ts index 9ee0853f45..d481df9526 100644 --- a/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsSummary.ts +++ b/webapp/views/App/views/Users/UserGroupsOverview/UserGroupsSummary/useUserGroupsSummary.ts @@ -9,7 +9,7 @@ import * as UserGroup from '@core/user/userGroup/userGroup' import * as API from '@webapp/service/api' import { useSurveyId } from '@webapp/store/survey' -import { LoaderActions } from '@webapp/store/ui' +import { NotificationActions } from '@webapp/store/ui' export type SurveyUserType = Record @@ -28,22 +28,22 @@ interface UseUserGroupsSummaryResult { users: SurveyUserType[] groupUuidByUserUuid: Record onChangeUserGroup: (userUuid: string, groupUuidNew: string | null) => Promise - reload: () => Promise + pendingUserUuids: ReadonlySet } /** * Loads every user group defined in the current survey together with the survey's full user * list and, for every user already assigned to a group, that group's uuid (building a * userUuid -> groupUuid map used to drive the Kanban board's columns and cards). Exposes a - * handler to move a user to a different group (or unassign them) by removing/adding group - * membership and reloading. + * handler to move a user to a different group (or unassign them) by optimistically updating that + * map and then removing/adding group membership on the server. * * @returns {UseUserGroupsSummaryResult} The groups, survey users, the userUuid -> groupUuid map, * and the handler to change a user's group. */ export const useUserGroupsSummary = (): UseUserGroupsSummaryResult => { - // LoaderActions dispatches thunks (functions), not plain actions, but the untyped store JS - // modules make useDispatch() infer the plain redux `Dispatch` type; type it + // NotificationActions dispatches a thunk (function), not a plain action, but the untyped store + // JS modules make useDispatch() infer the plain redux `Dispatch` type; type it // explicitly as a thunk dispatch here, following the precedent in useEditUserGroup.ts / // useUserGroupMembersEditor.ts. const dispatch = useDispatch>() @@ -54,10 +54,12 @@ export const useUserGroupsSummary = (): UseUserGroupsSummaryResult => { const [groups, setGroups] = useState([]) const [users, setUsers] = useState([]) const [groupUuidByUserUuid, setGroupUuidByUserUuid] = useState>({}) + // Users with a group change currently in flight; used to keep their cards from being dragged + // again until the pending request settles, so overlapping requests for the same user can't race. + const [pendingUserUuids, setPendingUserUuids] = useState>(new Set()) - // Pure data fetcher: never sets state itself, so it's safe to call both from the reactive effect - // below (via a staleness-guarded `.then()`) and from the imperative `reload` used by - // onChangeUserGroup, following the useUserGroupMembersEditor.ts precedent. + // Pure data fetcher: never sets state itself, so it's safe to call from the reactive effect below + // via a staleness-guarded `.then()`, following the useUserGroupMembersEditor.ts precedent. const fetchData = useCallback(async (): Promise => { const [groupsList, { data: usersRes }] = await Promise.all([ API.fetchUserGroups({ surveyId }), @@ -79,9 +81,7 @@ export const useUserGroupsSummary = (): UseUserGroupsSummaryResult => { // a closure-local staleness guard (not `await` directly in the effect body) so that if `surveyId` // changes again before this fetch resolves, the stale response is discarded instead of // overwriting newer state - same pattern established in UserGroupQualifiersEditor.tsx / Task 11, - // reused in useUserGroupMembersEditor.ts / Task 12. No loader dispatch here (matching the - // useUserGroupMembersEditor.ts precedent): the loader is only shown around the imperative - // add/remove-driven reload below, not the initial reactive load. + // reused in useUserGroupMembersEditor.ts / Task 12. useEffect(() => { let ignore = false @@ -98,34 +98,78 @@ export const useUserGroupsSummary = (): UseUserGroupsSummaryResult => { } }, [fetchData]) - // Reload used by the imperative onChangeUserGroup handler below: called from an event handler - // (not directly from an effect body), so there's no risk of setting state after a superseded - // effect run - matches the reload precedent in useUserGroupMembersEditor.ts. - const reload = useCallback(async (): Promise => { - const data = await fetchData() - setGroups(data.groups) - setUsers(data.users) - setGroupUuidByUserUuid(data.groupUuidByUserUuid) - }, [fetchData]) - + // Applies (or reverts) a userUuid -> groupUuid mapping change without touching any other entry; + // shared by the optimistic update below and its error rollback. + const applyGroupUuidChange = useCallback((userUuid: string, groupUuid: string | null) => { + setGroupUuidByUserUuid((prev) => { + const next = { ...prev } + if (groupUuid) { + next[userUuid] = groupUuid + } else { + delete next[userUuid] + } + return next + }) + }, []) + + const setUserPending = useCallback((userUuid: string, pending: boolean) => { + setPendingUserUuids((prev) => { + const next = new Set(prev) + if (pending) { + next.add(userUuid) + } else { + next.delete(userUuid) + } + return next + }) + }, []) + + // Updates groupUuidByUserUuid optimistically, before the remove/add requests even start, so the + // dragged card shows in its destination column immediately instead of sitting back in the origin + // column (see useUserGroupsKanbanDnd's reconcileCrossColumnDrop) for as long as the requests take + // - which, on a slow connection, used to be very noticeable since it previously waited for a full + // reload (groups + users + every group's members) on top of the two mutation requests. On failure, + // the mapping is rolled back to groupUuidOld so the card returns to its original column. Also + // marks the user pending for the duration of the request (cleared in `finally`, so it's cleared + // on both success and failure): UserGroupsSummary uses pendingUserUuids to keep the card from + // being dragged again until this call settles, since a second concurrent call for the same user + // would race its remove/add requests against this one and could leave the two calls' optimistic + // updates/rollbacks stomping on each other. const onChangeUserGroup = useCallback( async (userUuid: string, groupUuidNew: string | null): Promise => { - const groupUuidOld = groupUuidByUserUuid[userUuid] + const groupUuidOld = groupUuidByUserUuid[userUuid] ?? null + if (groupUuidOld === groupUuidNew) return + + applyGroupUuidChange(userUuid, groupUuidNew) + setUserPending(userUuid, true) + + // Tracks whether the old membership was actually removed on the server, so that if the + // subsequent add fails, the rollback below reflects the user's real server-side state + // (unassigned) instead of incorrectly restoring them to the old group in the UI. + let removedOld = false + try { - dispatch(LoaderActions.showLoader()) if (groupUuidOld) { await API.removeUserGroupMember({ surveyId, groupUuid: groupUuidOld, userUuid }) + removedOld = true } if (groupUuidNew) { await API.addUserGroupMember({ surveyId, groupUuid: groupUuidNew, userUuid }) } - await reload() + } catch (error) { + applyGroupUuidChange(userUuid, removedOld ? null : groupUuidOld) + // NotificationActions.notifyError's untyped implementation destructures `params` with no + // default, so TS infers it as a required property; pass an empty object explicitly + // (equivalent to the action creator's own runtime default), following the + // useEditUserGroup.ts precedent. + dispatch(NotificationActions.notifyError({ key: 'appErrors:networkError', params: {} })) + throw error } finally { - dispatch(LoaderActions.hideLoader()) + setUserPending(userUuid, false) } }, - [dispatch, groupUuidByUserUuid, reload, surveyId] + [applyGroupUuidChange, dispatch, groupUuidByUserUuid, setUserPending, surveyId] ) - return { groups, users, groupUuidByUserUuid, onChangeUserGroup, reload } + return { groups, users, groupUuidByUserUuid, onChangeUserGroup, pendingUserUuids } }