Enhances #664: add logs only when changes are published #931
Conversation
There was a problem hiding this comment.
I think I want to stare at this a little bit longer, but here are some comments.
- I'm looking forward to this change!
- I left suggestions to update some code comments because they are out of date or this part of the code is still complex (but less so now)
- Is this (property list) what we want to log in the audit details for these events? Anything else?
lib/model/query/datasets.js
Outdated
| @@ -21,20 +21,14 @@ const { sanitizeOdataIdentifier } = require('../../util/util'); | |||
| // DATASET CREATE AND UPDATE | |||
|
|
|||
| // createOrUpdate is called from Forms.createNew and Forms.createVersion | |||
There was a problem hiding this comment.
This comment seems out of place and i think the name changed to createOrMerge.
| UPDATE datasets SET "publishedAt" = clock_timestamp() | ||
| WHERE name = ${dataset.name} AND "projectId" = ${dataset.projectId} AND "publishedAt" IS NULL | ||
| ), | ||
| ` : sql``} | ||
| ds AS ( |
There was a problem hiding this comment.
Just wanted to double-check my understanding that with the query being simplified a bit, the rest of this should still be here. It's how you determine whether the dataset was freshly inserted (created) or already there (not inserted in ds_ins but found in datasets) to get the create or update action, right?
There was a problem hiding this comment.
This clause was updating dataset only if publish was true. The ds CTE just below (line 38-41 left side) tells us whether dataset is newly inserted or it was already there - it has "action" column with 'created' or 'updated' value
| const _createOrMerge = (dataset, fields, acteeId) => sql` | ||
| ${_insertDatasetDef(dataset, acteeId)} | ||
| ${isNilOrEmpty(fields) ? sql`` : _insertProperties(fields)} |
There was a problem hiding this comment.
I like how all these publish flags are going away!
| ? log('dataset.update.publish', dataset, { properties: dataset.properties.map(p => p.name) }) | ||
| : null)); | ||
| createOrMerge.audit.withResult = true; | ||
|
|
||
| //////////////////////////////////////////////////////////////////////////// | ||
| // DATASET (AND DATASET PROPERTY) PUBLISH | ||
|
|
There was a problem hiding this comment.
Comment near here (publishIfExists) that says "createOrMerge may also publish datasets" can change. Instead I'd love a comment reminding me why there's the "ifExists" part.
| @@ -2079,13 +2080,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']]); | |||
There was a problem hiding this comment.
I noticed that the details of the audit log changed from fields to properties. Ah I guess this is because dataset.update.publish used to return those while the other dataset events returned something else. And now every event is closer in meaning to a publish.
…#931) * Enhances getodk#664: add logs only when changes are published * updated API doc * updated comments
Closes #664 and #811
What has been done to verify that this works as intended?
Integration tests
Why is this the best possible solution? Were any other approaches considered?
NA
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
None
Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.
Yes
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes