From 8ddf37d8934c5963475701a5077e9c89827d8117 Mon Sep 17 00:00:00 2001 From: Jonathan Niles Date: Fri, 13 Oct 2017 16:39:50 +0100 Subject: [PATCH] feat: decouple transaction checks This commit decouples the transaction checks from transaction controller so that they can be shared between multiple controllers. This ensures safety in case a developers accidentally writes code that uses the delete interface to delete transactions w/o ensuring the transactions are properly dereferenced. --- client/src/i18n/fr/form.json | 4 +- client/src/modules/cash/payments/registry.js | 4 +- .../src/modules/invoices/registry/registry.js | 8 +- .../modules/vouchers/voucher-registry.ctrl.js | 4 +- server/controllers/finance/cash.js | 23 +-- server/controllers/finance/patientInvoice.js | 21 +-- server/controllers/finance/shared.js | 134 ++++++++++++++++++ server/controllers/finance/transactions.js | 102 +------------ server/controllers/finance/vouchers.js | 22 +-- test/integration/patientInvoice.js | 3 +- 10 files changed, 193 insertions(+), 132 deletions(-) create mode 100644 server/controllers/finance/shared.js diff --git a/client/src/i18n/fr/form.json b/client/src/i18n/fr/form.json index f414eb14b0..685c186ac7 100644 --- a/client/src/i18n/fr/form.json +++ b/client/src/i18n/fr/form.json @@ -67,7 +67,7 @@ }, "DIALOGS": { "CONFIRM_ACTION": "Confirmez vous cette action ?", - "CONFIRMATION_NEEDED" : "Confirmation Nécessaire", + "CONFIRMATION_NEEDED" : "Confirmation Requise", "CONFIRM_ACTIVATION": "Voulez vous vraiment activer les droits d'accès de cet utilisateur", "CONFIRM_CREDIT_NOTE": "La facture dont vous voulez annuler a déjà été payée, voulez-vous vraiment annuler cette facture ", "CONFIRM_DEACTIVATION": "Voulez vous vraiment blocker les droits d'accès de cet utilisateur", @@ -157,7 +157,7 @@ "UNPOSTED_TRANSACTION": "Transactions non-posté ne sont pas affichées", "TRANSACTIONS": "Transactions", "ROWS": "enregistrements", - "DELETE_RECORD_SUCCESS": "Supprimèe la transaction avec succès." + "DELETE_RECORD_SUCCESS": "Suppression de la transaction avec succès." }, "LABELS": { "ABBREVIATION": "Abbreviation", diff --git a/client/src/modules/cash/payments/registry.js b/client/src/modules/cash/payments/registry.js index 2ff937ce6b..f53c11e072 100644 --- a/client/src/modules/cash/payments/registry.js +++ b/client/src/modules/cash/payments/registry.js @@ -44,7 +44,7 @@ function CashPaymentRegistryController( vm.cancelCash = cancelCash; vm.openColumnConfigModal = openColumnConfigModal; vm.clearGridState = clearGridState; - vm.deleteCashPayment = deleteCashPayment; + vm.deleteCashPayment = deleteCashPaymentWithConfirmation; vm.download = Cash.download; columnDefs = [{ @@ -217,7 +217,7 @@ function CashPaymentRegistryController( // this function deletes the cash payment and associated transactions from // the database - function deleteCashPayment(entity) { + function deleteCashPaymentWithConfirmation(entity) { Modals.confirm('FORM.DIALOGS.CONFIRM_DELETE') .then(function (isOk) { if (isOk) { remove(entity); } diff --git a/client/src/modules/invoices/registry/registry.js b/client/src/modules/invoices/registry/registry.js index c15b757823..b5e1520182 100644 --- a/client/src/modules/invoices/registry/registry.js +++ b/client/src/modules/invoices/registry/registry.js @@ -35,7 +35,7 @@ function InvoiceRegistryController( vm.creditNote = creditNote; vm.bhConstants = bhConstants; vm.download = Invoices.download; - vm.deleteInvoice = deleteInvoice; + vm.deleteInvoice = deleteInvoiceWithConfirmation; // track if module is making a HTTP request for invoices vm.loading = false; @@ -190,13 +190,13 @@ function InvoiceRegistryController( $state.reload(); }; - // Call the opening of Modal + // Call the opening of Modal function openModal(invoice) { Invoices.openCreditNoteModal(invoice) .then(function (success) { if (success) { Notify.success('FORM.INFO.TRANSACTION_REVER_SUCCESS'); - return load(vm.filters); + load(vm.filters); } }) .catch(Notify.handleError); @@ -234,7 +234,7 @@ function InvoiceRegistryController( } // check if it is okay to remove the entity. - function deleteInvoice(entity) { + function deleteInvoiceWithConfirmation(entity) { Modals.confirm('FORM.DIALOGS.CONFIRM_DELETE') .then(function (isOk) { if (isOk) { remove(entity); } diff --git a/client/src/modules/vouchers/voucher-registry.ctrl.js b/client/src/modules/vouchers/voucher-registry.ctrl.js index 878717fbd3..8370a13841 100644 --- a/client/src/modules/vouchers/voucher-registry.ctrl.js +++ b/client/src/modules/vouchers/voucher-registry.ctrl.js @@ -40,7 +40,7 @@ function VoucherController( vm.openColumnConfigModal = openColumnConfigModal; vm.clearGridState = clearGridState; vm.download = Vouchers.download; - vm.deleteVoucher = deleteVoucher; + vm.deleteVoucher = deleteVoucherWithConfirmation; vm.loading = false; @@ -248,7 +248,7 @@ function VoucherController( } // this function deletes the voucher from the database - function deleteVoucher(entity) { + function deleteVoucherWithConfirmation(entity) { Modals.confirm('FORM.DIALOGS.CONFIRM_DELETE') .then(function (isOk) { if (isOk) { remove(entity); } diff --git a/server/controllers/finance/cash.js b/server/controllers/finance/cash.js index 8132b48253..e2b95b7431 100644 --- a/server/controllers/finance/cash.js +++ b/server/controllers/finance/cash.js @@ -29,6 +29,10 @@ const { BadRequest, NotFound } = require('../../lib/errors'); const identifiers = require('../../config/identifiers'); const cashCreate = require('./cash.create'); +// shared transaction methods +// TODO(@jniles) - find a better name +const shared = require('./shared'); + exports.detail = detail; exports.read = read; exports.create = cashCreate; @@ -282,14 +286,17 @@ function safelyDeleteCashPayment(uuid) { DELETE FROM document_map WHERE uuid = ?; `; - const binaryUuid = db.bid(uuid); - const transaction = db.transaction(); + return shared.isRemovableTransaction(uuid) + .then(() => { + const binaryUuid = db.bid(uuid); + const transaction = db.transaction(); - transaction - .addQuery(DELETE_TRANSACTION, binaryUuid) - .addQuery(DELETE_TRANSACTION_HISTORY, binaryUuid) - .addQuery(DELETE_CASH_PAYMENT, binaryUuid) - .addQuery(DELETE_DOCUMENT_MAP, binaryUuid); + transaction + .addQuery(DELETE_TRANSACTION, binaryUuid) + .addQuery(DELETE_TRANSACTION_HISTORY, binaryUuid) + .addQuery(DELETE_CASH_PAYMENT, binaryUuid) + .addQuery(DELETE_DOCUMENT_MAP, binaryUuid); - return transaction.execute(); + return transaction.execute(); + }); } diff --git a/server/controllers/finance/patientInvoice.js b/server/controllers/finance/patientInvoice.js index 4b573097aa..8ba051ff95 100644 --- a/server/controllers/finance/patientInvoice.js +++ b/server/controllers/finance/patientInvoice.js @@ -16,6 +16,8 @@ const BadRequest = require('../../lib/errors/BadRequest'); const createInvoice = require('./invoice/patientInvoice.create'); const Debtors = require('./debtors'); +const shared = require('./shared'); + const entityIdentifier = identifiers.INVOICE.key; const CREDIT_NOTE_ID = 10; @@ -303,14 +305,17 @@ function safelyDeleteInvoice(guid) { DELETE FROM document_map WHERE uuid = ?; `; - const binaryUuid = db.bid(guid); - const transaction = db.transaction(); + return shared.isRemovableTransaction(guid) + .then(() => { + const binaryUuid = db.bid(guid); + const transaction = db.transaction(); - transaction - .addQuery(DELETE_TRANSACTION, binaryUuid) - .addQuery(DELETE_TRANSACTION_HISTORY, binaryUuid) - .addQuery(DELETE_INVOICE, binaryUuid) - .addQuery(DELETE_DOCUMENT_MAP, binaryUuid); + transaction + .addQuery(DELETE_TRANSACTION, binaryUuid) + .addQuery(DELETE_TRANSACTION_HISTORY, binaryUuid) + .addQuery(DELETE_INVOICE, binaryUuid) + .addQuery(DELETE_DOCUMENT_MAP, binaryUuid); - return transaction.execute(); + return transaction.execute(); + }); } diff --git a/server/controllers/finance/shared.js b/server/controllers/finance/shared.js new file mode 100644 index 0000000000..7807387367 --- /dev/null +++ b/server/controllers/finance/shared.js @@ -0,0 +1,134 @@ +/** + * @overview shared.js + * + * @description + * This module contains helper functions for operating on transactions. These + * helper functions do things like like + * + * @requires lib/db + * @requires lib/errors/BadRequest + */ + +const db = require('../../lib/db'); +const BadRequest = require('../../lib/errors/BadRequest'); + +exports.getTransactionReferences = getTransactionReferences; +exports.getTransactionRecords = getTransactionRecords; +exports.isRemovableTransaction = isRemovableTransaction; + +exports.getRecordTextByUuid = getRecordTextByUuid; +exports.getEntityTextByUuid = getEntityTextByUuid; + + +/** + * @function getRecordTextByUuid + * + * @description + * This function returns a record's human readable text string by its uuid. + */ +function getRecordTextByUuid(uuid) { + const sql = ` + SELECT text FROM document_map WHERE uuid = ?; + `; + + return db.one(sql, db.bid(uuid)); +} + +/** + * @function getEntityTextByUuid + * + * @description + * This function returns an entity's human readable text from the entity_map + * table. + */ +function getEntityTextByUuid(uuid) { + const sql = ` + SELECT text FROM entity_map WHERE uuid = ?; + `; + + return db.one(sql, db.bid(uuid)); +} + +/** + * @function getTransactionReferences + * + * @description + * This function will find the uuids of any transactions that reference the + * provided transaction's uuid. + * + * @param {String} transactionUuid - the record_uuid of the transaction + */ +function getTransactionReferences(transactionUuid) { + const sql = ` + SELECT DISTINCT uuid, text FROM ( + SELECT dm.uuid, dm.text + FROM posting_journal AS j JOIN document_map AS dm ON + j.reference_uuid = dm.uuid + WHERE j.reference_uuid = ? + + UNION ALL + + SELECT dm.uuid, dm.text + FROM general_ledger AS g JOIN document_map AS dm ON + g.reference_uuid = dm.uuid + WHERE g.reference_uuid = ? + )c; + `; + + const buid = db.bid(transactionUuid); + + return db.exec(sql, [buid, buid]); +} + +/** + * @function getTransactionRecords + * + * @description + * Returns the transaction from the posting journal and general_ledger. + */ +function getTransactionRecords(uuid) { + const sql = ` + SELECT BUID(j.uuid) AS uuid, trans_id, BUID(record_uuid) AS record_uuid, + trans_date, debit_equiv, credit_equiv, currency_id, + BUID(reference_uuid) AS reference_uuid, + BUID(entity_uuid) AS entity_uuid, 0 AS posted, + document_map.text AS identifier + FROM posting_journal AS j JOIN document_map ON + j.record_uuid = document_map.uuid + WHERE record_uuid = ? + + UNION ALL + + SELECT BUID(j.uuid) AS uuid, trans_id, BUID(record_uuid) AS record_uuid, + trans_date, debit_equiv, credit_equiv, currency_id, + BUID(reference_uuid) AS reference_uuid, + BUID(entity_uuid) AS entity_uuid, 1 AS posted, + document_map.text AS identifier + FROM general_ledger AS j JOIN document_map ON + j.record_uuid = document_map.uuid + WHERE record_uuid = ? + `; + + return db.exec(sql, [db.bid(uuid), db.bid(uuid)]); +} + + +function isRemovableTransaction(uuid) { + // get all the rows of the transaction + return getTransactionRecords(uuid) + .then(rows => { + const isPosted = rows[0].posted; + if (isPosted) { + throw new BadRequest('This transaction is already posted.', 'TRANSACTIONS.ERRORS.TRANSACTION_POSTED'); + } + + // check if the transaction has references elsewhere + return getTransactionReferences(uuid); + }) + .then(references => { + const isReferenced = references.length > 0; + if (isReferenced) { + throw new BadRequest('This transaction is referenced.', 'TRANSACTIONS.ERRORS.TRANSACTION_REFERENCED'); + } + }); +} diff --git a/server/controllers/finance/transactions.js b/server/controllers/finance/transactions.js index 0db41b8f86..89d722c499 100644 --- a/server/controllers/finance/transactions.js +++ b/server/controllers/finance/transactions.js @@ -2,19 +2,15 @@ * @overview transactions.js * * @description - * This module contains helper functions for operating on transactions. These - * helper functions - * - * @requires lib/db - * @requires lib/errors/BadRequest + * This module contains the DELETE route for transactions. * + * @requires controllers/finance/shared * @requires controllers/finance/cash * @requires controllers/finance/vouchers * @requires controllers/finance/patientInvoice */ -const db = require('../../lib/db'); -const BadRequest = require('../../lib/errors/BadRequest'); +const shared = require('./shared'); const Cash = require('./cash'); const Invoices = require('./patientInvoice'); @@ -30,37 +26,6 @@ const safeDeletionMethods = { exports.deleteTransaction = deleteTransaction; -/** - * @function getTransactionReferences - * - * @description - * This function will find the uuids of any transactions that reference the - * provided transaction's uuid. - * - * @param {String} transactionUuid - the record_uuid of the transaction - */ -function getTransactionReferences(transactionUuid) { - const sql = ` - SELECT DISTINCT uuid, text FROM ( - SELECT dm.uuid, dm.text - FROM posting_journal AS j JOIN document_map AS dm ON - j.reference_uuid = dm.uuid - WHERE j.reference_uuid = ? - - UNION ALL - - SELECT dm.uuid, dm.text - FROM general_ledger AS g JOIN document_map AS dm ON - g.reference_uuid = dm.uuid - WHERE g.reference_uuid = ? - )c; - `; - - const buid = db.bid(transactionUuid); - - return db.exec(sql, [buid, buid]); -} - /** * @function parseDocumentMapString * @@ -77,39 +42,6 @@ function parseDocumentMapString(text) { return safeDeletionMethods[key]; } -/** - * @function getTransactionRecords - * - * @description - * Returns the transaction from the posting journal and general_ledger. - */ -function getTransactionRecords(uuid) { - const sql = ` - SELECT BUID(j.uuid) AS uuid, trans_id, BUID(record_uuid) AS record_uuid, - trans_date, debit_equiv, credit_equiv, currency_id, - BUID(reference_uuid) AS reference_uuid, - BUID(entity_uuid) AS entity_uuid, 0 AS posted, - document_map.text AS identifier - FROM posting_journal AS j JOIN document_map ON - j.record_uuid = document_map.uuid - WHERE record_uuid = ? - - UNION ALL - - SELECT BUID(j.uuid) AS uuid, trans_id, BUID(record_uuid) AS record_uuid, - trans_date, debit_equiv, credit_equiv, currency_id, - BUID(reference_uuid) AS reference_uuid, - BUID(entity_uuid) AS entity_uuid, 0 AS posted, - document_map.text AS identifier - FROM general_ledger AS j JOIN document_map ON - j.record_uuid = document_map.uuid - WHERE record_uuid = ? - `; - - return db.exec(sql, [db.bid(uuid), db.bid(uuid)]); -} - - /** * @function deleteTransation * @@ -120,33 +52,11 @@ function getTransactionRecords(uuid) { */ function deleteTransaction(req, res, next) { const { uuid } = req.params; - let transaction; - - // get all the rows of the transaction - getTransactionRecords(uuid) - .then(rows => { - transaction = rows; - - // TODO(@jniles) - i18n - const isPosted = transaction[0].posted; - if (isPosted) { - throw new BadRequest('This transaction is already posted.', 'TRANSACTIONS.ERRORS.TRANSACTION_POSTED'); - } - - // check if the transaction has references elsewhere - return getTransactionReferences(uuid); - }) - .then(references => { - // TODO(@jniles) - i18n - const isReferenced = references.length > 0; - if (isReferenced) { - throw new BadRequest('This transaction is referenced.', 'TRANSACTIONS.ERRORS.TRANSACTION_REFERENCED'); - } - - const documentMapText = transaction[0].identifier; + shared.getRecordTextByUuid(uuid) + .then(documentMap => { // route to do the correct safe deletion method. - const safeDeleteFn = parseDocumentMapString(documentMapText); + const safeDeleteFn = parseDocumentMapString(documentMap.text); // run the safe deletion method return safeDeleteFn(uuid); diff --git a/server/controllers/finance/vouchers.js b/server/controllers/finance/vouchers.js index bb46316b77..7de6c61a42 100644 --- a/server/controllers/finance/vouchers.js +++ b/server/controllers/finance/vouchers.js @@ -15,7 +15,6 @@ * @requires lib/errors/BadRequest */ - const _ = require('lodash'); const uuid = require('node-uuid'); @@ -26,6 +25,8 @@ const BadRequest = require('../../lib/errors/BadRequest'); const identifiers = require('../../config/identifiers'); const FilterParser = require('../../lib/filter'); +const shared = require('./shared'); + const entityIdentifier = identifiers.VOUCHER.key; /** Get list of vouchers */ @@ -260,14 +261,17 @@ function safelyDeleteVoucher(guid) { DELETE FROM document_map WHERE uuid = ?; `; - const binaryUuid = db.bid(guid); - const transaction = db.transaction(); + return shared.isRemovableTransaction(guid) + .then(() => { + const binaryUuid = db.bid(guid); + const transaction = db.transaction(); - transaction - .addQuery(DELETE_TRANSACTION, binaryUuid) - .addQuery(DELETE_TRANSACTION_HISTORY, binaryUuid) - .addQuery(DELETE_VOUCHER, binaryUuid) - .addQuery(DELETE_DOCUMENT_MAP, binaryUuid); + transaction + .addQuery(DELETE_TRANSACTION, binaryUuid) + .addQuery(DELETE_TRANSACTION_HISTORY, binaryUuid) + .addQuery(DELETE_VOUCHER, binaryUuid) + .addQuery(DELETE_DOCUMENT_MAP, binaryUuid); - return transaction.execute(); + return transaction.execute(); + }); } diff --git a/test/integration/patientInvoice.js b/test/integration/patientInvoice.js index 49cf53fe37..9b216bafe9 100644 --- a/test/integration/patientInvoice.js +++ b/test/integration/patientInvoice.js @@ -11,6 +11,7 @@ describe('(/invoices) Patient Invoices', function () { /* total number of invoices in the database */ const numInvoices = 3; const numCreatedInvoices = 3; + const numDeletedInvoices = 1; const fetchableInvoiceUuid = '957e4e79-a6bb-4b4d-a8f7-c42152b2c2f6'; const debtorUuid = '3be232f9-a4b9-4af6-984c-5d3f87d5c107'; const patientUuid = '274c51ae-efcc-4238-98c6-f402bfb39866'; @@ -68,7 +69,7 @@ describe('(/invoices) Patient Invoices', function () { it('GET /invoices/ should return all invoices if no query string provided', function () { return agent.get('/invoices') .then((res) => { - helpers.api.listed(res, numInvoices + numCreatedInvoices - 1); + helpers.api.listed(res, numInvoices + numCreatedInvoices - numDeletedInvoices); }) .catch(helpers.handler); });