Still code reviewing

package.json

@@ -49,7 +49,6 @@
     "json-diff": "^1.0.6",
     "json-stringify-safe": "^5.0.1",
     "json2csv": "^4.5.4",
-    "lodash": "^4.17.21",
     "mime-types": "^2.1.35",
     "minimist": "^1.2.8",
     "mkdirp": "^3.0.1",

src/fn/merge-contacts.js

@@ -24,22 +24,22 @@ module.exports = {
 const parseExtraArgs = (projectDir, extraArgs = []) => {
   const args = minimist(extraArgs, { boolean: true });
 
-  const sourceIds = (args.remove || '')
+  const sourceIds = (args.sources || args.source || '')
     .split(',')
     .filter(Boolean);
 
-  if (!args.keep) {
+  if (!args.destination) {
     usage();
-    throw Error(`Action "merge-contacts" is missing required contact ID ${bold('--keep')}. Other contacts will be merged into this contact.`);
+    throw Error(`Action "merge-contacts" is missing required contact ID ${bold('--destination')}. Other contacts will be merged into this contact.`);
   }
 
   if (sourceIds.length === 0) {
     usage();
-    throw Error(`Action "merge-contacts" is missing required contact ID(s) ${bold('--remove')}. These contacts will be merged into the contact specified by ${bold('--keep')}`);
+    throw Error(`Action "merge-contacts" is missing required contact ID(s) ${bold('--sources')}. These contacts will be merged into the contact specified by ${bold('--destination')}`);
   }
 
   return {
-    destinationId: args.keep,
+    destinationId: args.destination,
     sourceIds,
     docDirectoryPath: path.resolve(projectDir, args.docDirectoryPath || 'json_docs'),
     force: !!args.force,
@@ -50,17 +50,18 @@ const bold = text => `\x1b[1m${text}\x1b[0m`;
 const usage = () => {
   info(`
 ${bold('cht-conf\'s merge-contacts action')}
-When combined with 'upload-docs' this action merges multiple contacts and all their associated data into one.
+When combined with 'upload-docs' this action moves all of the contacts and reports under ${bold('sources')} to be under ${bold('destination')}.
+The top-level contact(s) ${bold('at source')} are deleted and no data in this document is merged or preserved.
 
 ${bold('USAGE')}
-cht --local merge-contacts -- --keep=<keep_id> --remove=<remove_id1>,<remove_id2>
+cht --local merge-contacts -- --destination=<destination_id> --sources=<source_id1>,<source_id2>
 
 ${bold('OPTIONS')}
---keep=<keep_id>
-  Specifies the ID of the contact that should have all other contact data merged into it.
+--destination=<destination_id>
+  Specifies the ID of the contact that should receive the moving contacts and reports.
 
---remove=<remove_id1>,<remove_id2>
-  A comma delimited list of IDs of contacts which will be deleted and all of their data will be merged into the keep contact.
+--sources=<source_id1>,<source_id2>
+  A comma delimited list of IDs of contacts which will be deleted. The hierarchy of contacts and reports under it will be moved to be under the destination contact.
 
 --docDirectoryPath=<path to stage docs>
   Specifies the folder used to store the documents representing the changes in hierarchy.

src/lib/hierarchy-operations/hierarchy-data-source.js

@@ -1,4 +1,3 @@
-const _ = require('lodash');
 const lineageManipulation = require('./lineage-manipulation');
 
 const HIERARCHY_ROOT = 'root';
@@ -73,7 +72,8 @@ async function getReportsForContacts(db, createdByIds, createdAtId, skip) {
     skip,
   });
 
-  return _.uniqBy(reports.rows.map(row => row.doc), '_id');
+  const docsWithId = reports.rows.map(({ doc }) => [doc._id, doc]);
+  return Array.from(new Map(docsWithId).values());
 }
 
 async function getAncestorsOf(db, contactDoc) {

src/lib/hierarchy-operations/index.js

@@ -70,11 +70,9 @@ async function moveReports(db, options, moveContext) {
     const createdAtId = options.merge && moveContext.sourceId;
     reportDocsBatch = await DataSource.getReportsForContacts(db, descendantIds, createdAtId, skip);
 
-    const updatedReports = replaceLineageInReports(options, reportDocsBatch, moveContext);
-
-    if (options.merge) {
-      reassignReports(reportDocsBatch, moveContext, updatedReports);
-    }
+    const lineageUpdates = replaceLineageInReports(options, reportDocsBatch, moveContext);
+    const reassignUpdates = reassignReports(options, reportDocsBatch, moveContext);
+    const updatedReports = reportDocsBatch.filter(doc => lineageUpdates.has(doc._id) || reassignUpdates.has(doc._id));
 
     minifyLineageAndWriteToDisk(options, updatedReports);
 
@@ -84,25 +82,22 @@ async function moveReports(db, options, moveContext) {
   return skip;
 }
 
-function reassignReports(reports, { sourceId, destinationId }, updatedReports) {
+function reassignReports(options, reports, { sourceId, destinationId }) {
   function reassignReportWithSubject(report, subjectId) {
-    let updated = false;
     if (report[subjectId] === sourceId) {
       report[subjectId] = destinationId;
-      updated = true;
+      updated.add(report._id);
     }
 
     if (report.fields[subjectId] === sourceId) {
       report.fields[subjectId] = destinationId;
-      updated = true;
+      updated.add(report._id);
     }
+  }
 
-    if (updated) {
-      const isAlreadyUpdated = !!updatedReports.find(updated => updated._id === report._id);
-      if (!isAlreadyUpdated) {
-        updatedReports.push(report);
-      }
-    }
+  const updated = new Set();
+  if (!options.merge) {
+    return updated;
   }
 
   for (const report of reports) {
@@ -111,6 +106,8 @@ function reassignReports(reports, { sourceId, destinationId }, updatedReports) {
       reassignReportWithSubject(report, subjectId);
     }
   }
+
+  return updated;
 }
 
 function minifyLineageAndWriteToDisk(options, docs) {
@@ -120,19 +117,21 @@ function minifyLineageAndWriteToDisk(options, docs) {
   });
 }
 
-function replaceLineageInReports(options, reportsCreatedByDescendants, moveContext) {
-  return reportsCreatedByDescendants.reduce((agg, doc) => {
-    const replaceLineageOptions = {
-      replaceWith: moveContext.replacementLineage,
-      startingFromId: moveContext.sourceId,
-      merge: options.merge,
+function replaceLineageInReports(options, reports, moveContext) {
+  const replaceLineageOptions = {
+    replaceWith: moveContext.replacementLineage,
+    startingFromId: moveContext.sourceId,
+    merge: options.merge,
     };
-
+
+  const updates = new Set();
+  reports.forEach(doc => {
     if (lineageManipulation.replaceContactLineage(doc, replaceLineageOptions)) {
-      agg.push(doc);
+      updates.add(doc._id);
     }
-    return agg;
-  }, []);
+  });
+
+  return updates;
 }
 
 function replaceLineageInAncestors(descendantsAndSelf, ancestors) {

src/lib/hierarchy-operations/lineage-constraints.js

@@ -1,4 +1,5 @@
 const log = require('../log');
+const { HIERARCHY_ROOT } = require('./hierarchy-data-source');
 const { trace } = log;
 
 const lineageManipulation = require('./lineage-manipulation');
@@ -103,6 +104,10 @@ const getMergeViolations = (sourceDoc, destinationDoc) => {
     return commonViolations;
   }
 
+  if ([sourceDoc._id, destinationDoc._id].includes(HIERARCHY_ROOT)) {
+    return `cannot merge using id: "${HIERARCHY_ROOT}"`;
+  }
+
   const sourceContactType = getContactType(sourceDoc);
   const destinationContactType = getContactType(destinationDoc);
   if (sourceContactType !== destinationContactType) {

src/lib/hierarchy-operations/lineage-manipulation.js

@@ -13,7 +13,7 @@ function replaceLineage(doc, lineageAttributeName, params) {
 
   // Replace the full lineage
   if (!startingFromId) {
-    return replaceWithinLineage(doc, lineageAttributeName, replaceWith);
+    return replaceEntireLineage(doc, lineageAttributeName, replaceWith);
   }
 
   function getInitialState() {
@@ -33,7 +33,7 @@ function replaceLineage(doc, lineageAttributeName, params) {
   function traverseOne() {
     const compare = merge ? state.element[state.attributeName] : state.element;
     if (compare?._id === startingFromId) {
-      return replaceWithinLineage(state.element, state.attributeName, replaceWith);
+      return replaceEntireLineage(state.element, state.attributeName, replaceWith);
     }
 
     state.element = state.element[state.attributeName];
@@ -59,14 +59,11 @@ function replaceContactLineage(doc, params) {
   return replaceLineage(doc, 'contact', params);
 }
 
-const replaceWithinLineage = (replaceInDoc, lineageAttributeName, replaceWith) => {
+const replaceEntireLineage = (replaceInDoc, lineageAttributeName, replaceWith) => {
   if (!replaceWith) {
     const lineageWasDeleted = !!replaceInDoc[lineageAttributeName];
     replaceInDoc[lineageAttributeName] = undefined;
     return lineageWasDeleted;
-  } else if (replaceInDoc[lineageAttributeName]) {
-    replaceInDoc[lineageAttributeName]._id = replaceWith._id;
-    replaceInDoc[lineageAttributeName].parent = replaceWith.parent;
   } else {
     replaceInDoc[lineageAttributeName] = replaceWith;
   }

test/fn/merge-contacts.spec.js

@@ -14,7 +14,7 @@ describe('merge-contacts', () => {
     it('remove only', () => expect(() => parseExtraArgs(__dirname, ['--remove=a'])).to.throw('required contact'));
 
     it('remove and keeps', () => {
-      const args = ['--remove=food,is,tasty', '--keep=bar', '--docDirectoryPath=/', '--force=hi'];
+      const args = ['--sources=food,is,tasty', '--destination=bar', '--docDirectoryPath=/', '--force=hi'];
       expect(parseExtraArgs(__dirname, args)).to.deep.eq({
         sourceIds: ['food', 'is', 'tasty'],
         destinationId: 'bar',

test/lib/hierarchy-operations/lineage-constraints.spec.js

@@ -63,6 +63,13 @@ describe('lineage constriants', () => {
       await expect(runScenario([], 'a', 'a', true)).to.eventually.rejectedWith('self');
     });
 
+    it('cannot merge with id: "root"', async () => {
+      const mockDb = { get: () => ({ settings: { contact_types: [] } }) };
+      const { assertNoHierarchyErrors } = await lineageConstraints(mockDb, { merge: true });
+      const actual = () => assertNoHierarchyErrors({ _id: 'root', type: 'dne' }, { _id: 'foo', type: 'clinic' });
+      expect(actual).to.throw('root');
+    });
+
     describe('default schema', () => {
       it('no defined rules enforces defaults schema', async () => await expect(runScenario(undefined, 'district_hospital', 'health_center')).to.eventually.rejectedWith('cannot have parent'));