Feature getodk/central#1010: Added bulk delete and restore entpoints for the Entities#1580
Conversation
c86f9a4 to
f561efe
Compare
ktuite
left a comment
There was a problem hiding this comment.
Ideas about behavior if not all the ids provided can be deleted or restored:
- The audit log should probably just log the ones that have successfully been acted on, and not every ID included in a request.
- If a bulk delete or restore action has no impact because everything was already deleted or just doesn't exist, maybe no audit event should be logged.
- The API could return the number of entities affected so you knew if you sent 500 IDs that all 500 worked or if there was some other issue with the bulk operation.
| @@ -40,6 +40,12 @@ info: | |||
|
|
|||
There was a problem hiding this comment.
Add entity.bulk.delete and entity.bulk.restore and remove from nonverbose audit logs as in #1442
| // This index supports the query pattern: (audits.details->'entityUuids') @> '[entity_uuid]' | ||
| // and is filtered to only bulk delete and restore actions. | ||
| await db.raw(`CREATE INDEX audits_details_entityuuids_gin_index | ||
| ON audits USING gin ((details->'entityUuids')) | ||
| WHERE action IN ('entity.bulk.delete', 'entity.bulk.restore')`); |
There was a problem hiding this comment.
How does this index relate to this other index?
CREATE INDEX audits_details_entityUuids ON audits USING gin ((details -> 'entityUuids') jsonb_path_ops) WHERE ACTION = 'entity.purge';
There was a problem hiding this comment.
good catch! I have updated the existing index.
test/integration/api/entities.js
Outdated
| }) | ||
| .expect(200) | ||
| .then(({ body }) => { | ||
| body.success.should.be.true(); |
There was a problem hiding this comment.
Returning success here seems ok but it looks like this action will also get logged in the audit log with the 12345678-1234-4123-8234-nonexistent entity id.
I think it should only log the uuids of the entities it successfully deleted, if possible.
There was a problem hiding this comment.
Would it be possible to return the count of the actual # of entities deleted or restored?
|
One more note, in the frontend, a little bit of work needs to be done to use these new events. In this screenshot, the delete and restore events are weird. (I thought I changed something about this code so it would show the raw event action if it didn't know how to format it right.) 
|
ktuite
left a comment
There was a problem hiding this comment.
Looks good! Please update filename of migration before merging!
19d5248 to
deea88c
Compare
…for the Entities
* return count of affected entities * log only UUIDs of affected entities and nothing if no entity is affected * Use existing index just update its where clause * Update API docs with the new audit actions * Update nonverbose action list in Audit API
deea88c to
6137a1c
Compare
Closes getodk/central#1010
What has been done to verify that this works as intended?
Added tests and manual verification with the frontend.
Why is this the best possible solution? Were any other approaches considered?
Follows the existing pattern. One decision that I took is to return 200 even if provided UUIDs don't exist. My reasoning is that it makes the endpoint idempotent, returning 404 would be semantically wrong http status as endpoint does exists. In future, if there is a requirement then we can return the list of UUIDs that were actually deleted.
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?
I don't see any risk of regression.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
Yes
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes