New audit_log table, and log edits to actions in the audit_log#415
New audit_log table, and log edits to actions in the audit_log#415yyi5708 wants to merge 1 commit into
Conversation
andymeneely
left a comment
andymeneely
left a comment
There was a problem hiding this comment.
Excellent! Ok the main things are:
- There might be some methods here that are either repeats of methods we've done elsewhere, so it might be worthwhile to either use them or move them to a more shared utility module.
- Since the AuditLogger primarily interacts with the database anyway, I think we should keep the parameter to the routes module as it is, and AuditLogger just uses the db object. Keeps the interface simpler I think.
| entity_type TEXT NOT NULL, | ||
| entity_id TEXT, | ||
| message TEXT NOT NULL, | ||
| details TEXT, |
There was a problem hiding this comment.
Let's rename it to details_json so future people know it's intended to be formatted using json.
| details TEXT, | |
| details_json TEXT, |
| try { | ||
| await db.query(insertQuery, params); | ||
| } catch (err) { | ||
| Logger.error(`Failed to write audit log entry: ${err.message}`); |
There was a problem hiding this comment.
Add more context info here so we can debug easier - like the username and action type. Don't need the ENTIRE audit info filling up the log, but making it easier to search for if it fails.
| @@ -9,4 +9,5 @@ | |||
| .read table_sql/users.sql | |||
| .read table_sql/sponsor_notes.sql | |||
| .read table_sql/page_html.sql | |||
There was a problem hiding this comment.
Make sure you also add test data to this PR as well. Might be good to capture some data you generate from testing things out, then putting it into the test data.
(nothing to do with create_all_tables.sql - probably goes into test_data)
| }; | ||
|
|
||
| module.exports = (db) => { | ||
| function getActor(req) { |
There was a problem hiding this comment.
Are getActor and actorLabel methods that we do elsewhere? We've had to look these up before for other features - might be good to reuse those if possible.
| } | ||
| } | ||
|
|
||
| function humanizeFieldName(field) { |
There was a problem hiding this comment.
Same thing with humanizeFieldName and displayValue as getActor - seem like methods that would be useful for all the modules to have, and maybe we had to do something like this before?
|
|
||
| // Routes | ||
| module.exports = (db) => { | ||
| module.exports = (db, AuditLog) => { |
There was a problem hiding this comment.
Are you sure we need AuditLog to be passed here? Don't we just import it and use it? Trying to figure out why we need the new parameter for this.
|
|
||
| db.query(updateQuery, params) | ||
| .then(() => { | ||
| let changedFields = {}; |
There was a problem hiding this comment.
Might be good to extract this whole chunk into its own method to show that it's all auditability stuff.
| const router = require("express").Router(); | ||
| const DBHandler = require("../database/db"); | ||
| let db = new DBHandler(); | ||
| const AuditLog = require("../audit/audit_logger")(db); |
There was a problem hiding this comment.
Since the audit log is being saved in the DB, it's using the DBHandler anyway. So I don't think AuditLog needs to be passed in as a parameter - just import it in the other files and use the db object from there. This is related to my above comment about (db, AuditLog)
github-project-automation
Bot
moved this from In Progress
to Under Review
in Portal Development
andymeneely
changed the title
andymeneely
changed the title
2 participants
No description provided.