ISSUE #4641 address feedback

Author: carmenfan

backend/src/v4/models/group.js

@@ -100,22 +100,24 @@ function clean(groupData) {
 	cleanArray(groupData, "rules");
 	cleanArray(groupData, "transformation");
 
-	for (let i = 0; groupData.objects && i < groupData.objects.length; i++) {
-		Object.values(idTypes).forEach((idType) => {
-			if (groupData.objects[i][idType]) {
-				cleanArray(groupData.objects[i], idType);
+	if(groupData.objects) {
+		groupData.objects = groupData.objects.filter(({shared_ids, ...extIds}, i) => {
+			const shouldKeep =  shared_ids || getCommonElements(Object.keys(extIds), Object.values(idTypes)).length;
+
+			if(shouldKeep) {
+				Object.values(idTypes).forEach((idType) => {
+					if (groupData.objects[i][idType]) {
+						cleanArray(groupData.objects[i], idType);
+					}
+				});
+
+				if (groupData.objects[i].shared_ids) {
+					cleanArray(groupData.objects[i], "shared_ids");
+					groupData.objects[i].shared_ids = groupData.objects[i].shared_ids.map(x => utils.uuidToString(x));
+				}
 			}
-		});
 
-		if (groupData.objects[i].shared_ids) {
-			cleanArray(groupData.objects[i], "shared_ids");
-			groupData.objects[i].shared_ids = groupData.objects[i].shared_ids.map(x => utils.uuidToString(x));
-		}
-	}
-
-	if(groupData.objects) {
-		groupData.objects = groupData.objects.filter(({shared_ids, ...extIds}) => {
-			return shared_ids || Object.keys(extIds).some((v) => Object.values(idTypes).includes(v));
+			return shouldKeep;
 		});
 	}
 
@@ -132,8 +134,7 @@ async function getObjectIds(account, model, branch, revId, groupData, convertSha
 	if (groupData.rules && groupData.rules.length > 0) {
 		return Meta.findObjectIdsByRules(account, model, groupData.rules, branch, revId, convertSharedIDsToString, showIfcGuids);
 	} else {
-		const res = await getObjectsArray(model, branch, revId, groupData, convertSharedIDsToString, showIfcGuids);
-		return res;
+		return getObjectsArray(model, branch, revId, groupData, convertSharedIDsToString, showIfcGuids);
 	}
 }
 
@@ -142,10 +143,10 @@ async function getObjectsArray(model, branch, revId, groupData, convertSharedIDs
 	return Promise.all(groupData.objects.map(async({account, model:container, shared_ids, ...extIds}) => {
 
 		if (showIfcGuids) {
-			if (extIds[idTypes.IFC]) {
-				return {account, model: container, [idTypes.IFC]: extIds[idTypes.IFC]};
+			if (!shared_ids) {
+				// if we're storing other external Ids, just return empty array (not converting).
+				return {account, model: container, [idTypes.IFC]: extIds[idTypes.IFC] ?? []};
 			}
-			return {account, model: container, [idTypes.IFC]: []};
 		} else if(shared_ids) {
 			return {account, model: container, shared_ids: convertSharedIDsToString ? shared_ids.map(utils.uuidToString) : shared_ids.map(utils.stringToUUID)};
 		}
@@ -199,16 +200,16 @@ function getObjectsArrayAsExternalIds(account, model, branch, rId, data) {
 
 		// have to have shared_ids, IFC or RVT ids, but not more than one
 		if (
-			(!containerEntry[idTypes.IFC] && !containerEntry[idTypes.RVT] && !containerEntry.shared_ids) &&
-			!(containerEntry[idTypes.IFC] && !containerEntry[idTypes.RVT] && !containerEntry.shared_ids) &&
-			!(!containerEntry[idTypes.IFC] && containerEntry[idTypes.RVT] && !containerEntry.shared_ids) &&
+			(!containerEntry[idTypes.IFC] && !containerEntry[idTypes.RVT] && !containerEntry.shared_ids) ||
+			!(containerEntry[idTypes.IFC] && !containerEntry[idTypes.RVT] && !containerEntry.shared_ids) ||
+			!(!containerEntry[idTypes.IFC] && containerEntry[idTypes.RVT] && !containerEntry.shared_ids) ||
 			!(!containerEntry[idTypes.IFC] && !containerEntry[idTypes.RVT] && containerEntry.shared_ids)
 		) {
 			return Promise.reject(responseCodes.INVALID_GROUP);
 		}
 
 		if(!containerEntry.shared_ids) {
-			return data;
+			return containerEntry;
 		}
 
 		try {

backend/src/v5/processors/teamspaces/projects/models/commons/tickets.groups.js

@@ -69,11 +69,11 @@ const getObjectArrayFromRules = async (teamspace, project, model, revId, rules,
 
 	if (!matched.length) return res;
 
-	const wantedIds = await getExternalIdsFromMetadata(matched);
+	const wantedIds = getExternalIdsFromMetadata(matched);
 
 	if (wantedIds) {
 		if (unwanted.length) {
-			const unwantedIds = await getExternalIdsFromMetadata(unwanted, wantedIds.key);
+			const unwantedIds = getExternalIdsFromMetadata(unwanted, wantedIds.key);
 			if (unwantedIds) {
 				wantedIds.values = getArrayDifference(unwantedIds.values, wantedIds.values);
 			}
@@ -151,7 +151,7 @@ TicketGroups.addGroups = async (teamspace, project, model, ticket, groups) => {
 	const convertedGroups = await Promise.all(groups.map(
 		async (group) => {
 			if (group.objects) {
-				const objects = await convertToExternalIds(teamspace, project, group.objects, true);
+				const objects = await convertToExternalIds(teamspace, project, group.objects);
 				return { ...group, objects };
 			}
 
@@ -169,7 +169,7 @@ TicketGroups.updateTicketGroup = async (teamspace, project, model, ticket, group
 	const convertedData = { ...data };
 
 	if (data.objects) {
-		convertedData.objects = await convertToExternalIds(teamspace, project, data.objects, true);
+		convertedData.objects = await convertToExternalIds(teamspace, project, data.objects);
 	}
 
 	await updateGroup(teamspace, project, model, ticket, groupId, convertedData, author);

backend/tests/v5/unit/processors/teamspaces/projects/models/commons/scenes.test.js

@@ -166,9 +166,13 @@ const testSharedIdsToExternalIds = () => {
 			expect(fn).toHaveBeenCalledTimes(1);
 			expect(fn).toHaveBeenCalledWith(metaRes);
 
+			const externalIdKeys = Object.values(idTypesToKeys).flat();
+			const query = { parents: { $in: sharedIds }, 'metadata.key': { $in: externalIdKeys }, rev_id: revId };
+			const projection = { metadata: 1 };
+
 			expect(MetaModel.getMetadataByQuery).toHaveBeenCalledTimes(1);
 			expect(MetaModel.getMetadataByQuery).toHaveBeenCalledWith(
-				teamspace, container, expect.anything(), expect.anything());
+				teamspace, container, query, projection);
 		});
 	});
 };