Enhances #664: add logs only when changes are published (#931)

* Enhances #664: add logs only when changes are published

* updated API doc

* updated comments

docs/api.md

@@ -1259,9 +1259,9 @@ You will get following workflow warnings while creating a new form or uploading
 
 ### Creating Datasets with Forms
 
-Starting from Version 2022.3, a Form can also create a Dataset by defining a Dataset schema in the Form definition (XForms XML or XLSForm). When a Form with a Dataset schema is uploaded, a Dataset and its Properties are created and a `dataset.create` event is logged in the Audit logs. The state of the Dataset is dependent on the state of the Form; you will need to publish the Form to publish the Dataset. Datasets in the Draft state are not returned in [Dataset APIs](#reference/datasets), however the [Related Datasets](#reference/forms/related-datasets/draft-form-dataset-diff) API for the Form can be called to get the Dataset and its Properties.
+Starting from Version 2022.3, a Form can also create a Dataset by defining a Dataset schema in the Form definition (XForms XML or XLSForm). When a Form with a Dataset schema is uploaded, a Dataset and its Properties are created. The state of the Dataset is dependent on the state of the Form; you will need to publish the Form to publish the Dataset. Datasets in the Draft state are not returned in [Dataset APIs](#reference/datasets), however the [Related Datasets](#reference/forms/related-datasets/draft-form-dataset-diff) API for the Form can be called to get the Dataset and its Properties.
 
-It is possible to define the schema of a Dataset in multiple Forms. Such Forms can be created and published in any order. The creation of the first Form will generate a `dataset.create` event in Audit logs and subsequent Form creation will generate `dataset.update` events. Publishing any of the Forms will also publish the Dataset and will generate a `dataset.update.publish` event. The state of a Property of a Dataset is also dependent on the state of the Form that FIRST defines that Property, which means if a Form is in the Draft state then the Properties defined by that Form will not appear in the [.csv file](#reference/datasets/download-dataset/download-dataset) of the Dataset.
+It is possible to define the schema of a Dataset in multiple Forms. Such Forms can be created and published in any order. Publishing any of the Forms will also publish the Dataset and will generate a `dataset.create` event; `dataset.update` events are generated in Audit logs when a Form adds a new property in the Dataset. The state of a Property of a Dataset is also dependent on the state of the Form that FIRST defines that Property, which means if a Form is in the Draft state then the Properties defined by that Form will not appear in the [.csv file](#reference/datasets/download-dataset/download-dataset) of the Dataset.
 
 + Parameters
     + ignoreWarnings: `false` (boolean, optional) - Defaults to `false`. Set to `true` if you want the Form to be created even if the XLSForm conversion results in warnings.
@@ -1776,7 +1776,7 @@ If you wish for the `version` to be set on your behalf as part of the publish op
 
 Once the Draft is published, there will no longer be a Draft version of the form.
 
-Starting with Version 2022.3, publishing a Draft Form that defines a Dataset schema will also publish the Dataset. It will generate `dataset.update.publish` event in Audit logs and make the Dataset available in [Datasets APIs](#reference/datasets)
+Starting with Version 2022.3, publishing a Draft Form that defines a Dataset schema will also publish the Dataset. It will generate `dataset.create` event in Audit logs and make the Dataset available in [Datasets APIs](#reference/datasets). If the Dataset is already published and the Form adds new properties then `dataset.update` event will be generated.
 
 + Parameters
     + version: `newVersion` (string, optional) - The `version` to be associated with the Draft once it's published.
@@ -4499,7 +4499,6 @@ Server Audit Logs entries are created for the following `action`s:
 * `submission.attachment.update` when a Submission Attachment binary is set or cleared, but _only via the REST API_. Attachments created alongside the submission over the OpenRosa `/submission` API (including submissions from Collect) do not generate audit log entries.
 * `dataset.create` when a Dataset is created.
 * `dataset.update` when a Dataset is updated.
-* `dataset.update.publish` when a Dataset is published.
 * `entity.create` when an Entity is created.
 * `entity.create.error` when there is an error during entity creation process.
 * `config.set` when a system configuration is set.

lib/model/query/audits.js

@@ -50,7 +50,7 @@ const actionCondition = (action) => {
   else if (action === 'submission')
     return sql`action in ('submission.create', 'submission.update', 'submission.update.version', 'submission.attachment.update')`;
   else if (action === 'dataset')
-    return sql`action in ('dataset.create', 'dataset.update', 'dataset.update.publish')`;
+    return sql`action in ('dataset.create', 'dataset.update')`;
   else if (action === 'entity')
     return sql`action in ('entity.create', 'entity.create.error', 'entity.update.version', 'entity.delete')`;
 

lib/model/query/datasets.js

@@ -20,21 +20,14 @@ const { sanitizeOdataIdentifier } = require('../../util/util');
 ////////////////////////////////////////////////////////////////////////////
 // DATASET CREATE AND UPDATE
 
-// createOrUpdate is called from Forms.createNew and Forms.createVersion
-const _insertDatasetDef = (dataset, acteeId, withDsDefsCTE, publish) => sql`
+const _insertDatasetDef = (dataset, acteeId) => sql`
   WITH ds_ins AS (
-    INSERT INTO datasets (id, name, "projectId", "createdAt", "acteeId", "publishedAt")
-    VALUES (nextval(pg_get_serial_sequence('datasets', 'id')), ${dataset.name}, ${dataset.projectId}, clock_timestamp(), ${acteeId}, ${(publish === true) ? sql`clock_timestamp()` : null})
+    INSERT INTO datasets (id, name, "projectId", "createdAt", "acteeId")
+    VALUES (nextval(pg_get_serial_sequence('datasets', 'id')), ${dataset.name}, ${dataset.projectId}, clock_timestamp(), ${acteeId})
     ON CONFLICT ON CONSTRAINT datasets_name_projectid_unique 
     DO NOTHING
     RETURNING *
   ),
-  ${publish ? sql`
-  update_ds AS (
-    UPDATE datasets SET "publishedAt" = clock_timestamp()
-    WHERE name = ${dataset.name} AND "projectId" = ${dataset.projectId} AND "publishedAt" IS NULL
-  ),
-  ` : sql``}
   ds AS (
     SELECT *, 'created' "action" FROM ds_ins
     UNION ALL
@@ -48,13 +41,13 @@ const _insertDatasetDef = (dataset, acteeId, withDsDefsCTE, publish) => sql`
   )
 `;
 
-const _insertProperties = (fields, publish) => sql`
+const _insertProperties = (fields) => sql`
 ,fields("propertyName", "formDefId", "schemaId", path) AS (VALUES      
   ${sql.join(fields.map(p => sql`( ${sql.join([p.aux.propertyName, p.aux.formDefId, p.schemaId, p.path], sql`,`)} )`), sql`,`)}
 ),
 insert_properties AS (
-    INSERT INTO ds_properties (id, name, "datasetId", "publishedAt")
-    SELECT nextval(pg_get_serial_sequence('ds_properties', 'id')), fields."propertyName", ds.id, ${(publish === true) ? sql`clock_timestamp()` : null} FROM fields, ds
+    INSERT INTO ds_properties (id, name, "datasetId")
+    SELECT nextval(pg_get_serial_sequence('ds_properties', 'id')), fields."propertyName", ds.id FROM fields, ds
     ON CONFLICT  ON CONSTRAINT ds_properties_name_datasetid_unique
     DO NOTHING 
     RETURNING ds_properties.id, ds_properties.name, ds_properties."datasetId"
@@ -71,22 +64,14 @@ insert_property_fields AS (
   INSERT INTO ds_property_fields ("dsPropertyId", "formDefId", "schemaId", "path")
   SELECT "dsPropertyId", "formDefId"::integer, "schemaId"::integer, path FROM all_properties
 )
-${publish ? sql`
-,
-update_ds_properties AS (
-  UPDATE ds_properties SET "publishedAt" = clock_timestamp()
-  FROM all_properties
-  WHERE ds_properties.id = all_properties."dsPropertyId" AND ds_properties."publishedAt" IS NULL
-)
-` : sql``}
 `;
 
 // should be moved to util.js or we already have similar func somewhere?
 const isNilOrEmpty = either(isNil, isEmpty);
 
-const _createOrMerge = (dataset, fields, acteeId, publish) => sql`
-${_insertDatasetDef(dataset, acteeId, true, publish)}
-${isNilOrEmpty(fields) ? sql`` : _insertProperties(fields, publish)}
+const _createOrMerge = (dataset, fields, acteeId) => sql`
+${_insertDatasetDef(dataset, acteeId)}
+${isNilOrEmpty(fields) ? sql`` : _insertProperties(fields)}
 SELECT "action", "id" FROM ds
 `;
 
@@ -96,8 +81,7 @@ SELECT "action", "id" FROM ds
 // * dataset name
 // * form (a Form frame or object containing projectId, formDefId, and schemaId)
 // * array of field objects and property names that were parsed from the form xml
-// * publish flag saying whether or not to immediately publish the dataset
-const createOrMerge = (name, form, fields, publish) => async ({ one, Actees, Datasets, Projects }) => {
+const createOrMerge = (name, form, fields) => async ({ one, Actees, Datasets, Projects }) => {
   const { projectId } = form;
   const { id: formDefId, schemaId } = form.def;
 
@@ -123,39 +107,26 @@ const createOrMerge = (name, form, fields, publish) => async ({ one, Actees, Dat
 
   // Insert or update: update dataset, dataset properties, and links to fields and current form
   // result contains action (created or updated) and the id of the dataset.
-  const result = await one(_createOrMerge(partial, dsPropertyFields, acteeId, publish))
+  const result = await one(_createOrMerge(partial, dsPropertyFields, acteeId))
     .catch(error => {
       if (error.constraint === 'ds_property_fields_dspropertyid_formdefid_unique') {
         throw Problem.user.invalidEntityForm({ reason: 'Multiple Form Fields cannot be saved to a single Dataset Property.' });
       }
       throw error;
     });
 
-  // Fetch all published properties for this dataset, even beyond what is defined in this form.
-  const publishedProperties = ((publish === true)
-    ? await Datasets.getProperties(result.id)
-    : null);
-
   // Partial contains acteeId which is needed for auditing.
   // Additional form fields and properties needed for audit logging as well.
-  return partial.with({ action: result.action, fields: dsPropertyFields, properties: publishedProperties });
+  return partial.with({ action: result.action, fields: dsPropertyFields });
 };
 
-createOrMerge.audit = (dataset, name, form, fields, publish) => (log) =>
-  ((dataset.action === 'created')
-    ? log('dataset.create', dataset, { fields: dataset.fields.map((f) => [f.path, f.propertyName]) })
-    : log('dataset.update', dataset, { fields: dataset.fields.map((f) => [f.path, f.propertyName]) }))
-    .then(() => ((publish === true)
-      ? log('dataset.update.publish', dataset, { properties: dataset.properties.map(p => p.name) })
-      : null));
-createOrMerge.audit.withResult = true;
-
 ////////////////////////////////////////////////////////////////////////////
 // DATASET (AND DATASET PROPERTY) PUBLISH
 
-// Called by Forms.publish.
-// createOrMerge may also publish datasets and properties within datasets.
-
+// Publishes the Dataset if it exists, noop if it doesn't.
+// `IfExists` part is particularly helpful for `Forms.publish`,
+// with that it doesn't need to ensure the existence of the Dataset
+// and it can call this function blindly
 const publishIfExists = (formDefId, publishedAt) => ({ all }) => all(sql`
 WITH properties_update as (
   UPDATE ds_properties dp SET "publishedAt" = ${publishedAt}
@@ -178,29 +149,37 @@ WITH properties_update as (
 -- we are returning first row for dataset
 -- because we want to log it even if no
 -- property is published.
-SELECT '' "name", ds.id, ds."acteeId"
+SELECT -1 "propertyId", '' "name", ds.id, ds."acteeId", 'datasetCreated' action
 FROM datasets_update ds
 UNION
-SELECT p.name, ds.id, ds."acteeId"
+SELECT p.id "propertyId", p.name, ds.id, ds."acteeId", 'propertyAdded' action
 FROM properties_update p
 JOIN datasets ds on ds.id = p."datasetId"
 UNION
-SELECT p.name, ds.id, ds."acteeId"
+SELECT p.id "propertyId", p.name, ds.id, ds."acteeId", 'noop' action
 FROM ds_properties p
 JOIN datasets ds on ds.id = p."datasetId"
 JOIN dataset_form_defs dfd on dfd."datasetId" = ds.id
 WHERE dfd."formDefId" = ${formDefId}
 AND p."publishedAt" IS NOT NULL
+ORDER BY "propertyId"
 `);
 
 publishIfExists.audit = (properties) => (log) => {
   const promiseArr = [];
   const datasets = groupBy(c => c.acteeId, properties);
 
   for (const acteeId of Object.keys(datasets)) {
-    promiseArr.push(log('dataset.update.publish', { acteeId }, {
-      properties: datasets[acteeId].filter(p => p.name).map(p => p.name).sort()
-    }));
+
+    const event = datasets[acteeId].some(p => p.action === 'datasetCreated') ? 'dataset.create' : 'dataset.update';
+
+    // In case of update, we don't want to log anything if no new property is published
+    if (event === 'dataset.create' || (event === 'dataset.update' && datasets[acteeId].some(p => p.action === 'propertyAdded'))) {
+      promiseArr.push(log(event, { acteeId }, {
+        properties: datasets[acteeId].filter(p => p.name).map(p => p.name)
+      }));
+    }
+
   }
 
   return Promise.all(promiseArr);

lib/model/query/forms.js

@@ -96,7 +96,11 @@ const createNew = (partial, project, publish = false) => async ({ run, Datasets,
 
   // Update datasets and properties if defined
   if (datasetName.isDefined()) {
-    await Datasets.createOrMerge(datasetName.get(), savedForm, fields, publish);
+    await Datasets.createOrMerge(datasetName.get(), savedForm, fields);
+
+    if (publish) {
+      await Datasets.publishIfExists(savedForm.def.id, savedForm.def.publishedAt.toISOString());
+    }
   }
 
   // Return the final saved form
@@ -229,8 +233,12 @@ const createVersion = (partial, form, publish, duplicating = false) => async ({
   ]);
 
   // Update datasets and properties if defined
-  if (datasetName.isDefined())
-    await Datasets.createOrMerge(datasetName.get(), new Form(form, { def: savedDef }), fieldsForInsert, publish);
+  if (datasetName.isDefined()) {
+    await Datasets.createOrMerge(datasetName.get(), new Form(form, { def: savedDef }), fieldsForInsert);
+    if (publish) {
+      await Datasets.publishIfExists(savedDef.id, savedDef.publishedAt.toISOString());
+    }
+  }
   return savedDef;
 };
 

test/integration/api/audits.js

@@ -314,9 +314,8 @@ describe('/audits', () => {
       await asAlice.get('/v1/audits?action=dataset')
         .expect(200)
         .then(({ body }) => {
-          body.length.should.equal(2);
+          body.length.should.equal(1);
           body.map(a => a.action).should.eql([
-            'dataset.update.publish',
             'dataset.create'
           ]);
         });
@@ -566,11 +565,10 @@ describe('/audits', () => {
       await asAlice.get('/v1/audits?action=nonverbose')
         .expect(200)
         .then(({ body }) => {
-          body.length.should.equal(5);
+          body.length.should.equal(4);
           body.map(a => a.action).should.eql([
             'form.update.publish',
             'form.create',
-            'dataset.update.publish',
             'dataset.create',
             'user.session.create'
           ]);

test/integration/api/datasets.js

@@ -11,6 +11,7 @@ const { QueryOptions } = require('../../../lib/util/db');
 
 /* eslint-disable import/no-dynamic-require */
 const { exhaust } = require(appRoot + '/lib/worker/worker');
+const Option = require(appRoot + '/lib/util/option');
 /* eslint-enable import/no-dynamic-require */
 
 describe('datasets and entities', () => {
@@ -2111,20 +2112,20 @@ describe('datasets and entities', () => {
     });
 
     describe('dataset audit logging at /projects/:id/forms POST', () => {
-      it('should log dataset creation in audit log', testService(async (service, { Audits }) => {
+      it('should not log dataset creation when form is not published', testService(async (service, { Audits }) => {
         await service.login('alice', (asAlice) =>
           asAlice.post('/v1/projects/1/forms')
             .send(testData.forms.simpleEntity)
             .set('Content-Type', 'text/xml')
             .expect(200));
 
-        const audit = await Audits.getLatestByAction('dataset.create').then((o) => o.get());
-        audit.details.fields.should.eql([['/name', 'first_name'], ['/age', 'age']]);
+        const audit = await Audits.getLatestByAction('dataset.create');
+        audit.should.equal(Option.none());
       }));
 
-      it('should log dataset modification in audit log', testService(async (service, { Audits }) => {
+      it('should not log dataset modification when form is not published', testService(async (service, { Audits }) => {
         await service.login('alice', (asAlice) =>
-          asAlice.post('/v1/projects/1/forms')
+          asAlice.post('/v1/projects/1/forms?publish=true')
             .send(testData.forms.simpleEntity)
             .set('Content-Type', 'text/xml')
             .expect(200)
@@ -2135,13 +2136,28 @@ describe('datasets and entities', () => {
               .set('Content-Type', 'text/xml')
               .expect(200)));
 
-        const audit = await Audits.getLatestByAction('dataset.create').then((o) => o.get());
-        audit.details.fields.should.eql([['/name', 'first_name'], ['/age', 'age']]);
+        const audit = await Audits.getLatestByAction('dataset.update');
+
+        audit.should.equal(Option.none());
+      }));
 
-        const audit2 = await Audits.getLatestByAction('dataset.update').then((o) => o.get());
-        audit2.details.fields.should.eql([['/name', 'color_name'], ['/age', 'age']]);
+      it('should not log dataset modification when no new property is added', testService(async (service, { Audits }) => {
+        const asAlice = await service.login('alice');
 
-        audit.acteeId.should.equal(audit2.acteeId);
+        await asAlice.post('/v1/projects/1/forms?publish=true')
+          .send(testData.forms.simpleEntity)
+          .set('Content-Type', 'text/xml')
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/forms/simpleEntity/draft')
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/forms/simpleEntity/draft/publish?version=v2')
+          .expect(200);
+
+        const audit = await Audits.getLatestByAction('dataset.update');
+
+        audit.should.equal(Option.none());
       }));
 
       it('should log dataset publishing in audit log', testService(async (service, { Audits }) => {
@@ -2153,23 +2169,20 @@ describe('datasets and entities', () => {
           .set('Content-Type', 'text/xml')
           .expect(200);
 
-        await Audits.getLatestByAction('dataset.update.publish')
+        await Audits.getLatestByAction('dataset.create')
           .then(o => o.get())
           .then(audit => audit.details.should.eql({ properties: ['first_name', 'age'] }));
 
-        await asAlice.post('/v1/projects/1/forms')
+        await asAlice.post('/v1/projects/1/forms?publish=true')
           .send(testData.forms.simpleEntity
             .replace('simpleEntity', 'simpleEntity2')
             .replace('first_name', 'color_name'))
           .set('Content-Type', 'text/xml')
           .expect(200);
 
-        await asAlice.post('/v1/projects/1/forms/simpleEntity2/draft/publish')
-          .expect(200);
-
-        await Audits.getLatestByAction('dataset.update.publish')
+        await Audits.getLatestByAction('dataset.update')
           .then(o => o.get())
-          .then(audit => audit.details.should.eql({ properties: ['age', 'color_name', 'first_name'] }));
+          .then(audit => audit.details.should.eql({ properties: ['first_name', 'age', 'color_name', ] }));
 
       }));