diff --git a/app/models/legacy-base.model.js b/app/models/legacy-base.model.js index 64853a6c94..3cd69674e4 100644 --- a/app/models/legacy-base.model.js +++ b/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 { diff --git a/app/models/water/billing-batch.model.js b/app/models/water/billing-batch.model.js index 8ca92c907f..8702ae1ac0 100644 --- a/app/models/water/billing-batch.model.js +++ b/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: { diff --git a/app/models/water/billing-charge-category.model.js b/app/models/water/billing-charge-category.model.js index 40a245fc1b..10236923a9 100644 --- a/app/models/water/billing-charge-category.model.js +++ b/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: { diff --git a/app/models/water/charge-element.model.js b/app/models/water/charge-element.model.js index 0605eaa618..ef4a7c51d7 100644 --- a/app/models/water/charge-element.model.js +++ b/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: { diff --git a/app/models/water/charge-purpose.model.js b/app/models/water/charge-purpose.model.js index 850e55525a..fdc6de58fb 100644 --- a/app/models/water/charge-purpose.model.js +++ b/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: { diff --git a/app/models/water/charge-version.model.js b/app/models/water/charge-version.model.js index d2698e4cff..924f827f37 100644 --- a/app/models/water/charge-version.model.js +++ b/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: { diff --git a/app/models/water/event.model.js b/app/models/water/event.model.js index 6161743e68..f5a90df302 100644 --- a/app/models/water/event.model.js +++ b/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 diff --git a/app/models/water/licence.model.js b/app/models/water/licence.model.js index 4413ea6837..dd16970a5c 100644 --- a/app/models/water/licence.model.js +++ b/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: { diff --git a/app/models/water/region.model.js b/app/models/water/region.model.js index a6727d453a..8756dda26b 100644 --- a/app/models/water/region.model.js +++ b/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: { diff --git a/app/presenters/supplementary-billing/create-billing-batch-event.presenter.js b/app/presenters/supplementary-billing/create-billing-batch-event.presenter.js index 3f5bed4078..4b01026824 100644 --- a/app/presenters/supplementary-billing/create-billing-batch-event.presenter.js +++ b/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, diff --git a/test/models/legacy-base.model.test.js b/test/models/legacy-base.model.test.js index 7125a00d94..032857efc6 100644 --- a/test/models/legacy-base.model.test.js +++ b/test/models/legacy-base.model.test.js @@ -7,13 +7,21 @@ const Code = require('@hapi/code') const { describe, it } = exports.lab = Lab.script() const { expect } = Code +// Test helpers +const { DBError } = require('objection') +const EventModel = require('../../app/models/water/event.model.js') + // Thing under test const LegacyBaseModel = require('../../app/models/legacy-base.model.js') describe('Legacy Base model', () => { - describe('.schema()', () => { + describe('.schema', () => { describe('when the getter is not overridden', () => { - class BadModel extends LegacyBaseModel {} + class BadModel extends LegacyBaseModel { + static get translations () { + return [] + } + } it('throws an error when called', () => { expect(() => BadModel.query()).to.throw() @@ -25,6 +33,10 @@ describe('Legacy Base model', () => { static get schema () { return 'water' } + + static get translations () { + return [] + } } it('does not throw an error when called', () => { @@ -32,4 +44,116 @@ describe('Legacy Base model', () => { }) }) }) + + describe('.translations', () => { + describe('when the getter is not overridden', () => { + class BadModel extends LegacyBaseModel { + static get schema () { + return 'water' + } + } + + it('throws an error when called', () => { + const instance = new BadModel() + expect(() => instance.$toJson()).not.to.throw() + }) + }) + + describe('when the getter is overridden', () => { + class GoodModel extends LegacyBaseModel { + static get schema () { + return 'water' + } + + static get translations () { + return [] + } + } + + it('does not throw an error when called', () => { + const instance = new GoodModel() + expect(() => instance.$toJson()).not.to.throw() + }) + }) + }) + + describe('when working with Legacy model instances', () => { + describe('and the legacy table a model is based on has non-standard columns', () => { + class YeOldeBillingModel extends LegacyBaseModel { + static get translations () { + return [ + { database: 'yehAlrightLove', model: 'greeting' }, + { database: 'doesNotExist', model: 'notHere' } + ] + } + } + + const dummyDatabaseJson = { + billingId: '76353750-5ace-411a-b00c-4e4a9ac934fb', + yehAlrightLove: 'Hello' + } + + const dummyModelJson = { + billingId: '76353750-5ace-411a-b00c-4e4a9ac934fb', + greeting: 'Hello' + } + + describe('when translating from the database table', () => { + it('translates them to standard model properties', () => { + const yeOldeBilling = new YeOldeBillingModel() + + // You would never call this directly. $parseDatabaseJson() is an objection method we're overloading in + // LegacyBaseModel. But it gives us the JSON Objection.js will then use to build the model so it works for this + // test. The JSON it expects as a param is generated by Objection based on the table in the DB. + // Note: The spread operator is used to create a shallow clone as $parseDatabaseJson() amends the value passed + // in. That would break subsequent tests + const result = yeOldeBilling.$parseDatabaseJson({ ...dummyDatabaseJson }) + + expect(result).to.equal(dummyModelJson) + }) + + it('ignores any translations that do not exist', () => { + const yeOldeBilling = new YeOldeBillingModel() + + const result = yeOldeBilling.$parseDatabaseJson({ ...dummyDatabaseJson }) + + expect(result.notHere).not.to.exist() + }) + }) + + describe('when translating back from the model instance', () => { + it('translates the model properties back to non-standard columns', () => { + const yeOldeBilling = new YeOldeBillingModel() + + // You would never call this directly. $formatDatabaseJson() is an objection method we're overloading in + // LegacyBaseModel. But it gives us the JSON Objection.js will then use to build to build the query so it works + // for this test. The JSON it expects as a param is generated by Objection based on the model instance. + // Note: The spread operator is used to create a shallow clone as $formatDatabaseJson() amends the value passed + // in. That would break subsequent tests + const result = yeOldeBilling.$formatDatabaseJson({ ...dummyModelJson }) + + expect(result).to.equal(dummyDatabaseJson) + }) + + it('ignores any translations that do not exist', () => { + const yeOldeBilling = new YeOldeBillingModel() + + const result = yeOldeBilling.$formatDatabaseJson({ ...dummyModelJson }) + + expect(result.doesNotExist).not.to.exist() + }) + }) + }) + }) + + describe('when working with Legacy model classes', () => { + describe('and the legacy table a model is based on has non-standard columns', () => { + it('throws an error when a query uses the standard model property name', async () => { + const timeNow = new Date().toISOString() + + const error = await expect(EventModel.query().where('createdAt', '<', timeNow)).to.reject() + expect(error).to.be.an.instanceOf(DBError) + }) + }) + }) }) diff --git a/test/presenters/supplementary-billing/create-billing-batch-event.presenter.test.js b/test/presenters/supplementary-billing/create-billing-batch-event.presenter.test.js index 7b7a0416e4..224938cf2b 100644 --- a/test/presenters/supplementary-billing/create-billing-batch-event.presenter.test.js +++ b/test/presenters/supplementary-billing/create-billing-batch-event.presenter.test.js @@ -44,8 +44,8 @@ describe('Create Billing Batch Event presenter', () => { expect(result.batch.isSummer).to.equal(billingBatch.isSummer) expect(result.batch.netTotal).to.equal(billingBatch.netTotal) - expect(result.batch.dateCreated).to.equal(billingBatch.dateCreated) - expect(result.batch.dateUpdated).to.equal(billingBatch.dateUpdated) + expect(result.batch.dateCreated).to.equal(billingBatch.createdAt) + expect(result.batch.dateUpdated).to.equal(billingBatch.updatedAt) expect(result.batch.invoiceCount).to.equal(billingBatch.invoiceCount) expect(result.batch.invoiceValue).to.equal(billingBatch.invoiceValue) expect(result.batch.creditNoteCount).to.equal(billingBatch.creditNoteCount) diff --git a/test/support/helpers/water/billing-charge-category.helper.js b/test/support/helpers/water/billing-charge-category.helper.js index dd8067b899..da72445f80 100644 --- a/test/support/helpers/water/billing-charge-category.helper.js +++ b/test/support/helpers/water/billing-charge-category.helper.js @@ -61,7 +61,7 @@ function defaults (data = {}) { // epoch in milliseconds, whereas a PostgreSQL timestamp field can only hold the seconds since the epoch. Pass it // an ISO string though ('2023-01-05T08:37:05.575Z') and PostgreSQL can do the conversion // https://stackoverflow.com/a/61912776/6117745 - dateCreated: new Date().toISOString() + createdAt: new Date().toISOString() } return {