Adding optional audit meta fields to track createdBy/updatedBy user

[estilles]

After reading the comments on #458 I remembered that I had implemented this several months ago, so I decided to share it with you all.

My implementation consists of an optional audit meta pattern that can be added to the List object (mirroring the existing standard meta pattern implementation).

The option is enabled as follows:

List.addPattern('audit meta');

This adds two fields to List (createdBy and updatedBy). I used these names to mirror the createdOn and updatedOn fields added by the standard pattern. Both fields are of type Relationship and they both refer to the model defined in the Keystone user model option.

This is how I implemented it.

lib/list.js

• Added the standard meta fields (if they dont' already exists)
• Added createdBy and updatedBy Relationship fields with ref pointing to keystone.get('user model')
• Mapped the new createdBy and updatedBy fields to the existing createdBy and modifiedBy mappings (I personally would have preferred using the same names as their respective fields, but I decided to use the two existing mappings ... which oddly appear to be unused elsewhere in Keystone).

NOTE: The idea of using the mappings is to give developers the option to implement their own createdBy and/or modifiedBy fields, while still using the original standard meta (if they so desire). Below is a somewhat silly example, but I'm sure it gets the point across.

var List = new keystone.List('Test', {
  map: { createdBy: 'userThatCreatedThis', modifiedBy: `userThatModifiedThis' }
});

List.add({
  ...
  userThatCreatedThis: { type: Types.Relationship, ref: 'User', noedit: true },
  userThatModifiedThis: { type: Types.Relationship, ref: 'User', noedit: true },
  ..
});

List.addPattern('standard meta');

lib/updateHandler.js

• Checked for an existing createdBymapping, and if one exists I set the corresponding mapped field to user._id (which was cached earlier using req.user in the UpdateHandler() constructor), but only when item.isNew.
• Checked for an existing modifiedBy mapping, and if one exists I set the corresponding mapped field to user._id

That's it! And ... as usual ... feel free to tweak away!

[estilles]

I decided to refactor this feature into a more testable code. I encapsulated all the functionality into a schemaPlugin. From the developer's perspective, the implementation remains identical. Just add List.addPattern('audit meta'); to enable the feature.

I also added a unit test for the feature. Unaware of any existing naming convention for Keystone unit tests, I simply named it auditMeta.test.js. My original test passed locally, but not on Travis-CI. I later realized that I was using a test model named Test, which was also being used on another test, causing a OverwriteModelError: Cannot overwrite 'Test' model once compiled error. I've since corrected the problem by renaming my test model to AuditMetaTest.

[SharadKumar]

+1

[JedWatson]

First off, big thanks to @JohnnyEstilles for wrapping this up and sharing it. It's an important thing to add to Keystone, and this is a good way to do it.

I wanted to raise some alternative ideas for discussion, because I think we should do this once and right so that when others build on it, it's our best foot forward.

Then I propose we either merge this, or implement an alternative solution quickly (which would also build on #514).

I'm not sure that addPattern is the best way of adding this functionality, and I'm tempted to deprecate it altogether in favor of List options (the way sortable is implemented).

I think a better way of doing it is:

var Test = new keystone.List('Test', {
  track: { createdBy: true, createdAt: true, modifiedBy: true, modifiedAt: true }
});

I think At is clearer than On for dates, because it's really "at a point in time" rather than "on a date", contrary to my original implementation. We could also go with createdTime and modifiedTime or something else.

I also haven't made it createdByUser because the name of the list Keystone understands as 'users' is variable. Having said that we've got _req_user so I realise it's not totally consistent, but this key would potentially end up user-facing.

We could specify alternative paths for each of the 4 fields this would add to the schema by setting the options to a String instead of true, like this:

var Test = new keystone.List('Test', {
track: { createdBy: 'userThatCreatedThis' /* etc */ }
});

This would allow backwards compatibility with the current addPattern('standard meta') feature, and we could make that function effectively implement this for a period of time (eventually it'd be removed).

Internally, we may use the map feature, or we may deprecate that too in favor of more specific functionality. I never really built out what I was expecting to behind this, and I'm not sure it's the best fit for what we really want to do.

For background: the idea behind mappings was to be able to have fields Keystone would understand the significance of (e.g. name, meta, etc) for specific usage in the UI, while allowing flexibility of how those fields are added to the schema. It's worked well for name but I think it breaks down for the meta use case.

Once this has been sorted out, we should add it into the Admin UI really nicely; it's currently completely missing (in part because I've been reluctant to build more momentum behind the current implementation, because of the ideas above).

[estilles]

@JedWatson, I completely agree. The one and only reason I used addPattern to create this feature was because it was what was used to implement the standard meta, so I incorrectly assumed this was the "Keystone way" to do this.

As a matter of fact, my original implementation uses options with names almost identical to the ones you suggested (except I called mine audit instead of track) ...

var Test = new keystone.List('Test', {
  audit: { createdBy: true, createdAt: true, modifiedBy: true, modifiedAt: true }
});

... and, the ability to specify user-defined fields ...

var Test = new keystone.List('Test', {
  audit: { createdBy: 'userThatCreatedThis' ... }
});

The reason I used mappings was because they already existed in lib\list.js, and I once again mistakenly assumed that was their intended purpose.

Now that I understand your intentions my suggested course of action is to rescind this PR and submit a new one implemented as follows:

• use track option instead of addPattern()
• createdBy, createdAt, modifiedBy, and modifiedAt will accept either Boolean (that when true indicate use of default field names) or String (which will allow developers to specify custom field names)
• Use a pre save hook to update createdBy, createdAt, modifiedBy, and modifiedAt (assuming you merge Adding current user available to pre/post save hooks #514 or I can just include that commit in the new PR and rescind Adding current user available to pre/post save hooks #514).
• Modify standard meta to detect the track option and gracefully defer to it if it's enabled (i.e. standard meta would only inject its fields/hook when track is not being used). Use mappings to maintain backwards compatibility.

Regarding this last point and the use of mappings. I understand that use cases like this was not your intended purpose. However, the existing mappings functionality allow us to develop new features (like this one) which alter field names in favor of more intelligible and less ambiguous ones (e.g. createdAt vs createdOn), while maintaining backwards compatibility. These mappings will exist on until the older features are deprecated. Doing away with mappings would eliminate a simple and somewhat "standard" way of doing this within Keystone. Personally, I vote for keeping it around.

Let me know what you think.

[SharadKumar]

I like the latest suggestions... any magic (of mappings standard meta) would probably not stand the test of time, and it will be much broadly appreciated to be able to flexibly decide the fields that make sense for implementers domain problem and conventions.

I'd say, rescind 514 too. :)

[estilles]

Just waiting on @JedWatson's green light to go ahead with the plan.

[JedWatson]

"I incorrectly assumed this was the 'Keystone way' to do this"

Actually, I incorrectly made this the "Keystone way" to do this, have been trying to work out how to roll it back without breaking compatibility since... sorry about that. You've been heading in the right direction all along, I just needed to get my thoughts clearer on it.

"Personally, I vote for keeping (mappings) around"

Yup, me too, with a clearer purpose though.

At the moment, it's designed to take a "conceptual" path that internal Keystone features (including the Admin UI) can understand, like name, or createdAt, and map them to any path in a schema.

The thing I think I got wrong, was it automaps any field added to the schema to any "mapped" path. Really, it should go the other way - plugins (sortable, or this new tracking / audit functionality) should define the fields they use, then we can use map to abstract the path used.

I think special (mapped) fields shouldn't show up in the main Item Details form in the Admin UI, they should be handled specifically (as name and sortOrder are). This means they don't get defined like normal fields, but are added as built-in features and are set as paths directly on the schema by the plugin that implements them.

To round this out, we should roll the handling of sortable into this as well... but one step at a time.

Green Light

I'll merge #514 now - this is a neat way of automating a common use of it, but I think it stands on its own as a feature, too.

Then:

• use track option instead of addPattern()
• createdBy, createdAt, modifiedBy, and modifiedAt will accept either Boolean (that when true indicate use of default field names) or String (which will allow developers to specify custom field names)
• use a pre save hook to update createdBy, createdAt, modifiedBy, and modifiedAt

re: standard meta - let's make that enable the track option (if it isn't already) and set the mappings to use the backwards-compatible names. So then everything is going through one implementation. We should also console.log() the deprecation warning to use the track option directly, rather than addPattern.

Then:

Tests and add the new options to the docs.

Let's do this :)

[estilles]

Sounds like a plan! I'm on it! :-)

[estilles]

Rescinding pull request.

[estilles]

I submitted a new PR (#523) adding the List track feature.