code review: timestamps, validation, responses, bugs

lib/model/migrations/20240312-01-add-dataset-create-verb.js

@@ -1,4 +1,4 @@
-// Copyright 2023 ODK Central Developers
+// Copyright 2024 ODK Central Developers
 // See the NOTICE file at the top-level directory of this distribution and at
 // https://github.com/getodk/central-backend/blob/master/NOTICE.
 // This file is part of ODK Central. It is subject to the license terms in

lib/model/query/datasets.js

@@ -8,7 +8,7 @@
 // except according to the terms contained in the LICENSE file.
 
 const { sql } = require('slonik');
-const { extender, insert, markPublished, QueryOptions, equals, updater } = require('../../util/db');
+const { extender, insert, QueryOptions, equals, updater } = require('../../util/db');
 const { Dataset, Form, Audit } = require('../frames');
 const { validatePropertyName } = require('../../data/dataset');
 const { isEmpty, isNil, either, reduceBy, groupBy, uniqWith, equals: rEquals, map } = require('ramda');
@@ -125,22 +125,23 @@ const createOrMerge = (parsedDataset, form, fields) => async ({ one, Actees, Dat
 // Insert a Dataset when there is no associated form and immediately publish
 const createPublishedDataset = (dataset, project) => async ({ one, Actees }) => {
   const actee = await Actees.provision('dataset', project);
-  const unpublished = await one(insert(dataset.with({ acteeId: actee.id })))
-    .then((d) => new Dataset(d));
-  return one(sql`${markPublished(unpublished)} returning *`)
-    .then((d) => new Dataset(d));
+  const dsWithId = await one(insert(dataset.with({ acteeId: actee.id })));
+  return new Dataset(await one(sql`UPDATE datasets SET "publishedAt" = "createdAt" where id = ${dsWithId.id} RETURNING *`));
 };
 
 createPublishedDataset.audit = (dataset) => (log) =>
   log('dataset.create', dataset, { properties: [] });
 createPublishedDataset.audit.withResult = true;
 
 // Insert a Dataset Property when there is no associated form and immediately publish
-// eslint-disable-next-line no-unused-vars
-const createPublishedProperty = (property, dataset) => ({ one, run }) =>
-  one(insert(property))
-    .then((p) => run(markPublished(new Dataset.Property(p))));
+const _createPublishedProperty = (property) => sql`
+INSERT INTO ds_properties ("name", "datasetId", "publishedAt")
+VALUES (${property.name}, ${property.datasetId}, clock_timestamp())
+returning *`;
 
+// eslint-disable-next-line no-unused-vars
+const createPublishedProperty = (property, dataset) => ({ one }) =>
+  one(_createPublishedProperty(property));
 
 createPublishedProperty.audit = (property, dataset) => (log) =>
   log('dataset.update', dataset, { properties: [property.name] });

lib/resources/datasets.js

@@ -11,7 +11,7 @@ const sanitize = require('sanitize-filename');
 const { getOrNotFound } = require('../util/promise');
 const { streamEntityCsv } = require('../data/entity');
 const { validateDatasetName, validatePropertyName } = require('../data/dataset');
-const { contentDisposition, withEtag } = require('../util/http');
+const { contentDisposition, success, withEtag } = require('../util/http');
 const { md5sum } = require('../util/crypto');
 const { Dataset } = require('../model/frames');
 const Problem = require('../util/problem');
@@ -57,21 +57,24 @@ module.exports = (service, endpoint) => {
     await auth.canOrReject('dataset.create', project);
 
     const { name } = body;
-    if (!validateDatasetName(name))
+    if (name == null || !validateDatasetName(name))
       throw Problem.user.unexpectedValue({ field: 'name', value: name, reason: 'This is not a valid dataset name.' });
 
-    return Datasets.createPublishedDataset(new Dataset({ name, projectId: project.id }), project);
+    const dataset = Dataset.fromApi(body).with({ name, projectId: project.id });
+
+    return Datasets.getMetadata(await Datasets.createPublishedDataset(dataset, project));
   }));
 
   service.post('/projects/:projectId/datasets/:name/properties', endpoint(async ({ Datasets }, { params, body, auth }) => {
     const dataset = await Datasets.get(params.projectId, params.name, true).then(getOrNotFound);
     await auth.canOrReject('dataset.update', dataset);
 
     const { name } = body;
-    if (!validatePropertyName(name))
+    if (name == null || !validatePropertyName(name))
       throw Problem.user.unexpectedValue({ field: 'name', value: name, reason: 'This is not a valid property name.' });
 
-    return Datasets.createPublishedProperty(new Dataset.Property({ name, datasetId: dataset.id }, dataset));
+    await Datasets.createPublishedProperty(new Dataset.Property({ name, datasetId: dataset.id }), dataset);
+    return success;
   }));
 
   service.get('/projects/:projectId/datasets/:name/entities.csv', endpoint(async ({ Datasets, Entities, Projects }, { params, auth, query }, request, response) => {

lib/util/db.js

@@ -195,9 +195,6 @@ where ${sql.identifier([ whereKey ])}=${obj[whereKey]}
 returning *`;
 };
 
-const markPublished = (obj) =>
-  sql`update ${raw(obj.constructor.table)} set "publishedAt"=now() where id=${obj.id}`;
-
 // generic del utility
 const markDeleted = (obj) =>
   sql`update ${raw(obj.constructor.table)} set "deletedAt"=now() where id=${obj.id}`;
@@ -570,7 +567,7 @@ const postgresErrorToProblem = (x) => {
 module.exports = {
   connectionString, connectionObject,
   unjoiner, extender, equals, page, queryFuncs,
-  insert, insertMany, updater, markDeleted, markUndeleted, markPublished,
+  insert, insertMany, updater, markDeleted, markUndeleted,
   QueryOptions,
   postgresErrorToProblem
 };

test/integration/api/datasets.js

@@ -28,14 +28,27 @@ describe('datasets and entities', () => {
           .expect(403);
       }));
 
-      it('should reject dataset name is not provided', testService(async (service) => {
+      it('should reject if dataset name is not provided', testService(async (service) => {
         const asAlice = await service.login('alice');
 
         await asAlice.post('/v1/projects/1/datasets')
           .send({})
           .expect(400)
           .then(({ body }) => {
-            body.message.should.equal('Required parameter name missing.');
+            body.message.should.equal('Unexpected name value undefined; This is not a valid dataset name.');
+          });
+      }));
+
+      it('should reject if dataset name is null', testService(async (service) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/datasets')
+          .send({
+            name: null
+          })
+          .expect(400)
+          .then(({ body }) => {
+            body.message.should.equal('Unexpected name value null; This is not a valid dataset name.');
           });
       }));
 
@@ -52,19 +65,36 @@ describe('datasets and entities', () => {
           });
       }));
 
-      it('should create a new dataset', testService(async (service) => {
+      it('should create a new dataset and return dataset', testService(async (service) => {
         const asAlice = await service.login('alice');
 
         await asAlice.post('/v1/projects/1/datasets')
           .send({
             name: 'trees'
           })
-          .expect(200);
+          .expect(200)
+          .then(({ body }) => {
+            body.should.be.a.Dataset();
+            body.name.should.equal('trees');
+            body.properties.should.eql([]);
+            body.approvalRequired.should.eql(false);
+          });
+      }));
 
-        await asAlice.get('/v1/projects/1/datasets')
+      it('should allow approvalRequired to be set on new dataset', testService(async (service) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/datasets')
+          .send({
+            name: 'trees',
+            approvalRequired: true,
+          })
+          .expect(200)
           .then(({ body }) => {
-            body[0].name.should.equal('trees');
-            body[0].createdAt.should.be.an.isoDate();
+            body.should.be.a.Dataset();
+            body.name.should.equal('trees');
+            body.properties.should.eql([]);
+            body.approvalRequired.should.eql(true);
           });
       }));
 
@@ -92,7 +122,10 @@ describe('datasets and entities', () => {
           .send({
             name: 'trees'
           })
-          .expect(200);
+          .expect(200)
+          .then(({ body }) => {
+            console.log('just made new dataset', body);
+          });
 
         await asAlice.post('/v1/projects/1/datasets/trees/entities')
           .send({
@@ -132,9 +165,12 @@ describe('datasets and entities', () => {
           .expect(200);
 
         await asAlice.get('/v1/audits')
+          .set('X-Extended-Metadata', 'true')
           .then(({ body: logs }) => {
             logs[0].action.should.equal('dataset.create');
             logs[0].actorId.should.equal(5);
+            logs[0].actee.should.be.a.Dataset();
+            logs[0].actee.name.should.equal('trees');
             logs[0].details.properties.should.eql([]);
           });
       }));
@@ -150,7 +186,26 @@ describe('datasets and entities', () => {
 
       }));
 
-      it('should add properties to a dataset', testService(async (service) => {
+      it('should add property to dataset and return success', testService(async (service) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/datasets')
+          .send({
+            name: 'trees'
+          })
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/datasets/trees/properties')
+          .send({
+            name: 'height'
+          })
+          .expect(200)
+          .then(({ body }) => {
+            body.success.should.be.true();
+          });
+      }));
+
+      it('should accept entities with properties after properties have been added', testService(async (service) => {
         const asAlice = await service.login('alice');
 
         await asAlice.post('/v1/projects/1/datasets')
@@ -227,6 +282,38 @@ describe('datasets and entities', () => {
           });
       }));
 
+      it('should reject if property name is not provided', testService(async (service) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/datasets')
+          .send({ name: 'trees' })
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/datasets/trees/properties')
+          .send({})
+          .expect(400)
+          .then(({ body }) => {
+            body.message.should.equal('Unexpected name value undefined; This is not a valid property name.');
+          });
+      }));
+
+      it('should reject if property name is null', testService(async (service) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/datasets')
+          .send({ name: 'trees' })
+          .expect(200);
+
+        await asAlice.post('/v1/projects/1/datasets/trees/properties')
+          .send({
+            name: null
+          })
+          .expect(400)
+          .then(({ body }) => {
+            body.message.should.equal('Unexpected name value null; This is not a valid property name.');
+          });
+      }));
+
       it('should log an event for creating a new dataset property', testService(async (service) => {
         const asAlice = await service.login('alice');
 
@@ -243,9 +330,12 @@ describe('datasets and entities', () => {
           .expect(200);
 
         await asAlice.get('/v1/audits')
+          .set('X-Extended-Metadata', 'true')
           .then(({ body: logs }) => {
             logs[0].action.should.equal('dataset.update');
             logs[0].actorId.should.equal(5);
+            logs[0].actee.should.be.a.Dataset();
+            logs[0].actee.name.should.equal('trees');
             logs[0].details.properties.should.eql([ 'circumference' ]);
           });
       }));