Improve behavior with blank data in entity updates

lib/data/entity.js

@@ -38,9 +38,26 @@ const odataToColumnMap = new Map([
 const validateEntity = (entity) => {
   const { label, id, update, baseVersion } = entity.system;
 
-  if (!label || label.trim() === '')
+  // Trying to describe this logic.
+  // if coming from a submission, create and/or update will hopefully be defined.
+  // if coming from json, create and update will not be available.
+  // in most cases, we need label to exist and not be blank...
+  // - creating an entity with a label
+  // - updating an entity's label
+  // but in one case, if its an update and label is not even specified,
+  // that is ok -- the label just wont be updated on the entity.
+  // TODO: do something different with validateEntity because it has too
+  // many special, subtle cases.
+  if (!(update === '1' || update === 'true') && (!label || label.trim() === ''))
     throw Problem.user.missingParameter({ field: 'label' });
 
+  // on update, it is okay for label to not be provided. but if it is,
+  // it cannot be an empty string.
+  if ((update === '1' || update === 'true') && (label != null && label.trim() === '')) {
+  //  console.log('deciding to throw missing label param', label);
+    throw Problem.user.missingParameter({ field: 'label' });
+  }
+
   if (!id || id.trim() === '')
     throw Problem.user.missingParameter({ field: 'uuid' });
 
@@ -78,7 +95,7 @@ const validateEntity = (entity) => {
 //    entity node attributes like dataset name.
 const parseSubmissionXml = (entityFields, xml) => new Promise((resolve, reject) => {
   const entity = { system: Object.create(null), data: Object.create(null) };
-  const stream = submissionXmlToFieldStream(entityFields, xml, true);
+  const stream = submissionXmlToFieldStream(entityFields, xml, true, true);
   stream.on('error', reject);
   stream.on('data', ({ field, text }) => {
     if (field.name === 'entity') {

lib/data/submission.js

@@ -27,7 +27,14 @@ const { union, last, pluck } = require('ramda');
 // if you are reading this thinking to use it elsewhere, you'll almost certainly
 // need to work out a sensible way to flag the SchemaStack allowEmptyNavigation boolean
 // to false for whatever you are doing.
-const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false) => {
+//
+// actually this is also used to parse entity data out of submissions, a scenario
+// in which we want to capture potentially empty nodes like <age></age> that might
+// mean the property should be set to blank. includeEmptyNodes allows that instead
+// of skipping over outputting empty nodes. i think this is different from what the
+// note above is talking about...
+// allowEmptyNavigation and includeStructuralAttrs are already true.
+const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, includeEmptyNodes = false) => {
   const outStream = new Readable({ objectMode: true, read: noop });
 
   const stack = new SchemaStack(fields, true);
@@ -50,7 +57,7 @@ const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false)
     onclosetag: () => {
       const field = stack.pop();
 
-      if (textBuffer !== '') {
+      if (textBuffer !== '' || includeEmptyNodes) {
         if ((field != null) && !field.isStructural()) // don't output useless whitespace
           outStream.push({ field, text: textBuffer });
         textBuffer = '';

lib/model/query/entities.js

@@ -207,7 +207,7 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss
 
   // merge data
   const mergedData = mergeRight(serverEntity.aux.currentVersion.data, clientEntity.def.data);
-  const mergedLabel = clientEntity.def.label || serverEntity.aux.currentVersion.label;
+  const mergedLabel = clientEntity.def.label ?? serverEntity.aux.currentVersion.label;
 
   // make some kind of source object
   const sourceDetails = {

test/integration/api/entities.js

@@ -1592,13 +1592,12 @@ describe('Entities API', () => {
         });
     }));
 
-    it.skip('should set label to blank', testEntityUpdates(async (service, container) => {
-      // TODO: fix the entity label update logic to make this test pass.
+    it('should not update label if not included', testEntityUpdates(async (service, container) => {
       const asAlice = await service.login('alice');
 
       await asAlice.post('/v1/projects/1/forms/updateEntity/submissions')
         .send(testData.instances.updateEntity.one
-          .replace('<label>Alicia (85)</label>', '<label></label>'))
+          .replace('<label>Alicia (85)</label>', ''))
         .expect(200);
 
       await exhaust(container);
@@ -1608,32 +1607,35 @@ describe('Entities API', () => {
         .then(({ body: person }) => {
           person.currentVersion.dataReceived.should.eql({ age: '85', first_name: 'Alicia' });
           person.currentVersion.data.should.eql({ age: '85', first_name: 'Alicia' });
-          person.currentVersion.label.should.equal('');
+          person.currentVersion.label.should.equal('Johnny Doe');
         });
     }));
 
-    it.skip('should not update label if not included', testEntityUpdates(async (service, container) => {
-      // TODO: fix the entity label update logic to make this test pass.
+    it('should log error and not update if trying to set label to blank', testEntityUpdates(async (service, container) => {
       const asAlice = await service.login('alice');
 
       await asAlice.post('/v1/projects/1/forms/updateEntity/submissions')
         .send(testData.instances.updateEntity.one
-          .replace('<label>Alicia (85)</label>', ''))
+          .replace('<label>Alicia (85)</label>', '<label></label>'))
         .expect(200);
 
       await exhaust(container);
 
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
         .then(({ body: person }) => {
-          person.currentVersion.dataReceived.should.eql({ age: '85', first_name: 'Alicia' });
-          person.currentVersion.data.should.eql({ age: '85', first_name: 'Alicia' });
-          person.currentVersion.label.should.equal('Johnny Doe');
+          should(person.currentVersion.baseVersion).be.null();
+        });
+
+      await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits')
+        .expect(200)
+        .then(({ body: logs }) => {
+          logs[0].should.be.an.Audit();
+          logs[0].action.should.be.eql('entity.error');
         });
     }));
 
-    it.skip('should set field to blank', testEntityUpdates(async (service, container) => {
-      // TODO: fix update logic to make this test pass
+    it('should set field to blank', testEntityUpdates(async (service, container) => {
       const asAlice = await service.login('alice');
 
       await asAlice.post('/v1/projects/1/forms/updateEntity/submissions')
@@ -1646,7 +1648,7 @@ describe('Entities API', () => {
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
         .then(({ body: person }) => {
-          person.currentVersion.dataReceived.should.eql({ label: 'Alicia (85)', first_name: 'Alicia' });
+          person.currentVersion.dataReceived.should.eql({ label: 'Alicia (85)', first_name: 'Alicia', age: '' });
           person.currentVersion.data.first_name.should.eql('Alicia');
           person.currentVersion.data.age.should.eql('');
         });