diff --git a/docs/api.md b/docs/api.md index 63e522958..31592b3ae 100644 --- a/docs/api.md +++ b/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. diff --git a/lib/model/query/audits.js b/lib/model/query/audits.js index d81304ce8..d99662e38 100644 --- a/lib/model/query/audits.js +++ b/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')`; diff --git a/lib/model/query/datasets.js b/lib/model/query/datasets.js index 03954bd30..988e49768 100644 --- a/lib/model/query/datasets.js +++ b/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,7 +107,7 @@ 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.' }); @@ -131,31 +115,18 @@ const createOrMerge = (name, form, fields, publish) => async ({ one, Actees, Dat 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,19 +149,20 @@ 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) => { @@ -198,9 +170,16 @@ publishIfExists.audit = (properties) => (log) => { 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); diff --git a/lib/model/query/forms.js b/lib/model/query/forms.js index 728f0985f..d01a19296 100644 --- a/lib/model/query/forms.js +++ b/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; }; diff --git a/test/integration/api/audits.js b/test/integration/api/audits.js index dc5351dd0..5f0b5772f 100644 --- a/test/integration/api/audits.js +++ b/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' ]); diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 397d2de40..dbea89bf9 100644 --- a/test/integration/api/datasets.js +++ b/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', ] })); }));