feat(audit): Add the ability to specify a custom audit log serializer… #1746
Conversation
daviande
force-pushed
the
audit-serializers
branch
2 times, most recently
from
fdf4e59
to
443374d
Compare
lib/plugins/audit.js
Outdated
| if (!res) { | ||
| return false; | ||
| } | ||
| var reqSerializer = function auditRequestSerializer(req) { |
There was a problem hiding this comment.
Now that you've broken out the default serializers, it looks like these could probably be lifted to the top as constants/defaults (or maybe even a DEFAULT_SERIALIZERS obj). Is that possible, or do we need the closure still?
There was a problem hiding this comment.
I think we still need the closure because of the " body: opts.body === true ? req.body : undefined,". We could do something like create a function that returns the DEFAULT_SERIALIZERS and pass it the opts/opts.body param. It might just be enough to define a DEFAULTS_SERIALIZERS within the clousure, though. What do you think is more readable?
lib/plugins/audit.js
Outdated
| }; | ||
| }; | ||
|
|
||
| if (opts.serializers && opts.serializers.res) { |
There was a problem hiding this comment.
Style nit: might we be able to collapse these three if checks into a single assignment? Something like:
var serializers = Object.assign({}, DEFAULT_SERIALIZERS, opts.serializers);There was a problem hiding this comment.
This is a good idea.
There was a problem hiding this comment.
Made the change, what do you think now @DonutEspresso?
… (for err, req and res)
daviande
force-pushed
the
audit-serializers
branch
from
443374d
to
8380209
Compare
… (for err, req and res)
Pre-Submission Checklist
make prepushIssues
Closes:
Changes
Adds the ability to specify a custom audit log serializer for err, req and res