Make timestamps consistent at model layer (#85)

DEFRA/water-abstraction-team#69

We identified that there are inconsistencies in the legacy database, timestamps in the tables being one of them. We don't want these issues to make a mess of our code. So, until we're ready for some full-scale data migration we're going to deal with them in our model layer.

Timestamps being different across tables is the main inconsistency we noted and this change deals with it.

We use [Objection.js](https://vincit.github.io/objection.js/api/model/instance-methods.html#parsedatabasejson) `$parseDatabaseJson()` and `$formatDatabaseJson()` to present a consistent API in our models whilst still working with the DB. This is built into the `LegacyBaseModel` with every child model expected to include a `translations()` getter that will be used by both. There is an argument to move `translations()` into `WaterBaseModel` and then just override it for exceptions like `EventModel`. But we have an eye to also use this logic to make the ID property for each model _actually_ `.id`. And that will be different in each which means every model is going to need to set `translations ()`. So, it'd be better to keep the translations together in one place for each model, rather than splitting them.

> WARNING! Queries cannot use the 'code' version of a column name. They must use the database version. We've included a unit test to demonstrate this and documentation to explain the reasons why.

app/models/legacy-base.model.js

@@ -65,6 +65,159 @@ class LegacyBaseModel extends BaseModel {
   static get schema () {
     throw new Error('defaultSchema() not implemented in child class')
   }
+
+  /**
+   * Translations for non-standard column names to our standardised ones
+   *
+   * The primary example of non-standard column names in the legacy data is timestamps. For example, we have found the
+   * column which has the timestamp for when a record is created has been named `date_created`, 'created', 'modified',
+   * and 'created_at`.
+   *
+   * Within our code we want to just be using `createdAt` (which would be `created_at` in the DB), as this is what the
+   * field would have been called if Knex had been used to generate the migrations and build the tables.
+   *
+   * All models that extend `LegacyBaseModel` are expected to implement this getter. It should always return an array.
+   * If there are no translations an empty array is valid.
+   *
+   * @example
+   * ```javascript
+   * static get translations () {
+   *  return [
+   *    { database: 'created', model: 'createdAt' },
+   *    { database: 'modified', model: 'updatedAt' }
+   *  ]
+   * }
+   * ```
+   *
+   */
+  static get translations () {
+    throw new Error('translations() not implemented in child class')
+  }
+
+  /**
+   * Called when a Model is converted to database format.
+   *
+   * This is an Objection.js instance method which we are overriding. We use it to convert our standardised property
+   * names into the legacy ones, for example `createdAt` to `dateCreated`. Timestamps is a primary example
+   * where the legacy tables do not use a consistent naming convention. Various tables use different names for the same
+   * thing. But we want few surprises and lots of consistency in our code. So, this allows us to translate the names of
+   * properties before Objection.js finalises the database version.
+   *
+   * Each Model that extends `LegacyBaseModel` is expected to implement `static get translations` which defines the
+   * database to model translations needed. Any fields listed in `translations` not found will be ignored.
+   *
+   * For more details see
+   * {@link http://vincit.github.io/objection.js/api/model/instance-methods.html#parsedatabasejson $parseDatabaseJson()}
+   * and the
+   * {@link https://vincit.github.io/objection.js/api/model/overview.html#model-data-lifecycle Model data lifecycle}
+   *
+   * -------------------------------------------------------------------------------------------------------------------
+   *
+   * **WARNING!** This translation method _only_ works for instances. We do not yet have a solution that will also
+   * work with static queries. Running the following will cause an exception because `createdAt` (automatically
+   * converted to `created_at` in the final query) does not exist in the `water.events` table.
+   *
+   * ```javascript
+   * EventModel.query().where('createdAt', '<', new Date().toISOString())
+   * ```
+   *
+   * Overriding
+   * {@link https://vincit.github.io/objection.js/api/model/static-properties.html#static-columnnamemappers columnNameMappers}
+   * had the same result as what we're doing. We also looked into
+   * {@link https://vincit.github.io/objection.js/api/objection/#knexidentifiermapping knexIdentifierMapping}. This
+   * would do the mapping at a lower level, in the same way we've used
+   * {@link https://vincit.github.io/objection.js/api/objection/#knexsnakecasemappers knexSnakeCaseMappers} to
+   * make changes before Objection.js takes over.
+   *
+   * The first problem we hit was that you can only use one of these; you can't apply both to the config object based to
+   * knex (see `knexfile.application.js` and `db/db.js` for how we currently do it). This is because both methods
+   * return an object which defines `wrapIdentifier()` and `postProcessResponse()`. So, whichever is added last
+   * overwrites what the previous one set.
+   *
+   * You can resolve this by importing `objection/lib/utils/identifierMapping.js` directly and doing something like this
+   *
+   * ```javascript
+   * // https://gitter.im/Vincit/objection.js?at=5e25f065a259cb0f060754ee
+   * const og = require('objection/lib/utils/identifierMapping');
+   * const SNAKE_CASE_OVERRIDES = { col12: 'col_1_2' };
+   *
+   * function snakeCase(str, { upperCase = false, underscoreBeforeDigits = false } = {}) {
+   *   // if this column has a manual override, return the snake-cased column name from the override
+   *   if (str.length && str in SNAKE_CASE_OVERRIDES) {
+   *     return SNAKE_CASE_OVERRIDES[str];
+   *   }
+   *
+   *   // otherwise simply call the original snakeCase() function
+   *   return og.snakeCase(str, { upperCase: upperCase, underscoreBeforeDigits: underscoreBeforeDigits});
+   * }
+   *
+   * function knexSnakeCaseMappers(opt = {}) {
+   *   return og.knexIdentifierMappers({
+   *     parse: str => og.camelCase(str, opt),
+   *     format: str => snakeCase(str, opt), // call overridden snakeCase() function
+   *   });
+   * }
+   * ```
+   *
+   * But that leads us to the second problem; at this level translations are global. So, any reference to `date_created`
+   * would get translated to `created_at`, irrespective of the table referenced. If we only had to worry about database
+   * to model it would be okay. The problem is to make queries work we need to be able to translate back from our names
+   * to the database version. That is impossible where `created_at` might need to become `date_created`, `created`,
+   * or `modified`. With no access to the table name for context we cannot make that decision.
+   *
+   * This is why we have accepted we can only support translations when dealing with instances of a model for now.
+   *
+   * @param {Object} json The JSON POJO in internal format
+   * @returns {Object} The JSON POJO in database format
+   */
+  $formatDatabaseJson (json) {
+    json = super.$formatDatabaseJson(json)
+
+    for (const translation of this.constructor.translations) {
+      if (translation.model in json) {
+        json[translation.database] = json[translation.model]
+
+        delete json[translation.model]
+      }
+    }
+
+    return json
+  }
+
+  /**
+   * Called when a Model instance is created from a database JSON object. This method converts the JSON object from the
+   * database format to the internal format.
+   *
+   * This is an Objection.js instance method which we are overriding. We use it to convert unconventional database
+   * column names into standardised ones, for example `dateCreated` to `createdAt`. Timestamps is a primary example
+   * where the legacy tables do not use a consistent naming convention. Various tables use different names for the same
+   * thing. But we want few surprises and lots of consistency in our code. So, this allows us to translate the names of
+   * columns before Objection.js finalises the model instance.
+   *
+   * Each Model that extends `LegacyBaseModel` is expected to implement `static get translations` which defines the
+   * database to model translations needed. Any fields listed in `translations` not found will be ignored.
+   *
+   * For more details see
+   * {@link http://vincit.github.io/objection.js/api/model/instance-methods.html#parsedatabasejson $parseDatabaseJson()}
+   * and the
+   * {@link https://vincit.github.io/objection.js/api/model/overview.html#model-data-lifecycle Model data lifecycle}
+   *
+   * @param {Object} json The JSON POJO in database format
+   * @returns {Object} The JSON POJO in internal format
+   */
+  $parseDatabaseJson (json) {
+    json = super.$parseDatabaseJson(json)
+
+    for (const translation of this.constructor.translations) {
+      if (translation.database in json) {
+        json[translation.model] = json[translation.database]
+
+        delete json[translation.database]
+      }
+    }
+
+    return json
+  }
 }
 
 class SchemaQueryBuilder extends QueryBuilder {

app/models/water/billing-batch.model.js

@@ -18,6 +18,13 @@ class BillingBatchModel extends WaterBaseModel {
     return 'billingBatchId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       region: {

app/models/water/billing-charge-category.model.js

@@ -18,6 +18,13 @@ class BillingChargeCategoryModel extends WaterBaseModel {
     return 'billingChargeCategoryId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       chargeElements: {

app/models/water/charge-element.model.js

@@ -18,6 +18,13 @@ class ChargeElementModel extends WaterBaseModel {
     return 'chargeElementId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       chargeVersion: {

app/models/water/charge-purpose.model.js

@@ -18,6 +18,13 @@ class ChargePurposeModel extends WaterBaseModel {
     return 'chargePurposeId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       chargeElement: {

app/models/water/charge-version.model.js

@@ -18,6 +18,13 @@ class ChargeVersionModel extends WaterBaseModel {
     return 'chargeVersionId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       licence: {

app/models/water/event.model.js

@@ -15,6 +15,13 @@ class EventModel extends WaterBaseModel {
   static get idColumn () {
     return 'eventId'
   }
+
+  static get translations () {
+    return [
+      { database: 'created', model: 'createdAt' },
+      { database: 'modified', model: 'updatedAt' }
+    ]
+  }
 }
 
 module.exports = EventModel

app/models/water/licence.model.js

@@ -18,6 +18,13 @@ class LicenceModel extends WaterBaseModel {
     return 'licenceId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       chargeVersions: {

app/models/water/region.model.js

@@ -18,6 +18,13 @@ class RegionModel extends WaterBaseModel {
     return 'regionId'
   }
 
+  static get translations () {
+    return [
+      { database: 'dateCreated', model: 'createdAt' },
+      { database: 'dateUpdated', model: 'updatedAt' }
+    ]
+  }
+
   static get relationMappings () {
     return {
       licences: {

app/presenters/supplementary-billing/create-billing-batch-event.presenter.js

@@ -11,9 +11,7 @@ function go (billingBatch) {
     billingBatchId,
     creditNoteCount,
     creditNoteValue,
-    dateCreated,
     createdAt,
-    dateUpdated,
     updatedAt,
     fromFinancialYearEnding,
     invoiceCount,
@@ -44,12 +42,8 @@ function go (billingBatch) {
       isSummer,
       netTotal,
       startYear: { yearEnding: fromFinancialYearEnding },
-      // NOTE: In the 'real' schema timestamp fields are dateCreated & dateUpdated. If you follow the standard
-      // convention of using a trigger as seen in db/migrations/[*]_create_update_timestamp_trigger.js you get
-      // createdAt & updatedAt. In our testing schema we use the later. To ensure unit tests pass we need to account
-      // for both.
-      dateCreated: dateCreated ?? createdAt,
-      dateUpdated: dateUpdated ?? updatedAt,
+      dateCreated: createdAt,
+      dateUpdated: updatedAt,
       invoiceCount,
       invoiceValue,
       creditNoteCount,