chore(payroll): clean up comments and variable naming

Cleans up a few comments and inconsistent variable names noticed during
the review period.

server/controllers/payroll/multiplePayroll/calculateEmployeePension.js

@@ -8,7 +8,10 @@ const common = require('./common');
   *
   * @description
   * Filters rubrics for pension rubrics and creates the vouchers and transactions that apply to those
-  * kinds of rubrics. Returns an array of transactions.
+  * kinds of rubrics. Returns an array of transactions.description
+  *
+  * Requires the `txnTypeId` parameter since the transaction type for pension funds is user-configurable.
+  * TODO(@jniles) - review this decision.  Maybe our default transaction types should account for this.
   *
   * The options parameter should contain "lang", "sharedI18nProps" and "sharedVoucherProps".
   */
@@ -20,7 +23,7 @@ function calculateEmployeePension(employee, rubrics, txnTypeId, options = {}) {
   // hold the growing list of transactions elements
   const transactions = [];
 
-  // break early if no withholding rubrics apply.
+  // break early if no pension rubrics apply.
   if (employeePension.length === 0) { return transactions; }
 
   // get the grand total value.

server/controllers/payroll/multiplePayroll/calculateEmployeeWithholdings.js

@@ -29,8 +29,6 @@ function calculateEmployeeWithholdings(employee, rubrics, options = {}) {
   // get the grand total value.
   const totalEmployeeWithholding = common.sumRubricValues(employeeRubricsWithholdings);
 
-  // eslint-disable-next-line
-  // "Payroll withholdings of {{amount}} for {{employee.displayname}} ({{employee.reference}}) for \"{{rubric.label}}\" in payment period {{paymentPeriod}}.",
   const descriptionWithholding = common.fmtI18nDescription(options.lang, 'PAYROLL_RUBRIC.WITHHOLDING_DESCRIPTION', {
     ...options.sharedI18nProps,
     amount : totalEmployeeWithholding,

server/controllers/payroll/multiplePayroll/commitment.js

@@ -142,23 +142,23 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
   const totalWithholdings = common.sumRubricTotals(rubricsWithholdings);
 
   debug(`Computed total value of associated rubrics:`);
-  debug(`Enterprise Charge on Remuneration : ${totalPayrollTaxes}.`);
+  debug(`Enterprise payroll taxes : ${totalPayrollTaxes}.`);
   debug(`Pension Fund : ${totalPensionFunds} .`);
   debug(`Withholdings : ${totalWithholdings} .`);
 
   debug(`Running dataCommitment() function`);
   const dataCommitment = commitmentFunction.dataCommitment(
     employees,
-    exchangeRates,
     rubrics,
+    exchangeRates,
     identificationCommitment,
   );
 
   const {
     transactions,
     employeesBenefitsItem,
     employeesWithholdingItem,
-    employeesPensionFundsItem,
+    pensionFundVoucherItems,
   } = dataCommitment;
 
   // accumulates the total commitments for all employees.
@@ -167,7 +167,7 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
   debug(`Computed total commitments for employees: ${totalCommitments}.`);
   debug(`Computed total basic salaries: ${totalBasicSalaries}.`);
 
-  // helper function to make a clean voucher
+  // shared helper object to contain common voucher metadata
   const sharedVoucherProps = {
     date : datePeriodTo,
     project_id : projectId,
@@ -210,7 +210,7 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
 
   // deal with payroll taxes
   let voucherPayrollTax = {};
-  const enterprisePayrollTaxes = [];
+  const payrollTaxVoucherItems = [];
 
   if (payrollTaxes.length) {
     voucherPayrollTax = {
@@ -221,23 +221,23 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
       amount : util.roundDecimal(totalPayrollTaxes, 2),
     };
 
-    payrollTaxes.forEach(chargeRemuneration => {
-      enterprisePayrollTaxes.push([
+    payrollTaxes.forEach(rubric => {
+      payrollTaxVoucherItems.push([
         db.uuid(),
-        chargeRemuneration.debtor_account_id,
+        rubric.debtor_account_id,
         0,
-        chargeRemuneration.totals,
+        rubric.totals,
         voucherPayrollTax.uuid,
         null,
         null,
       ], [
         db.uuid(),
-        chargeRemuneration.expense_account_id,
-        chargeRemuneration.totals,
+        rubric.expense_account_id,
+        rubric.totals,
         0,
         voucherPayrollTax.uuid,
         null,
-        chargeRemuneration.cost_center_id,
+        rubric.cost_center_id,
       ]);
     });
   }
@@ -277,7 +277,7 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
     };
 
     pensionFunds.forEach(pensionFund => {
-      employeesPensionFundsItem.push([
+      pensionFundVoucherItems.push([
         db.uuid(),
         pensionFund.expense_account_id,
         util.roundDecimal(totalPensionFunds, 2),
@@ -312,7 +312,7 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
     }, {
       query : `INSERT INTO voucher_item (
         uuid, account_id, debit, credit, voucher_uuid, entity_uuid, cost_center_id) VALUES ?`,
-      params : [enterprisePayrollTaxes],
+      params : [payrollTaxVoucherItems],
     }, {
       query : 'CALL PostVoucher(?);',
       params : [voucherPayrollTax.uuid],
@@ -344,7 +344,7 @@ function commitments(employees, rubrics, rubricsConfig, configuration,
         uuid, account_id, debit, credit, voucher_uuid, entity_uuid,
         description, cost_center_id
       ) VALUES ?`,
-      params : [employeesPensionFundsItem],
+      params : [pensionFundVoucherItems],
     }, {
       query : 'CALL PostVoucher(?);',
       params : [voucherPensionFunds.uuid],

server/controllers/payroll/multiplePayroll/commitmentFunction.js

@@ -10,16 +10,28 @@ const common = require('./common');
 /**
  * @method dataCommitment
  *
- * This function prepares the data required to process payment encumbrance transactions.
- * It takes as input a list of employees and calculates the following:
+ * This function loops through all employees and accumulates their salaries, benefits, pension, and withholdings
+ * in arrays that are handed back to the caller function.  It does similar work to the calculate*()
+ * (e.g. calculateEmployeePension) payroll functions, but those functions directly return executable SQL statements,
+ * whereas this one returns an array that can be passed through to a `params` call.
+ *
+ * Additionally, this function keeps track of the running totals of all employee transactions, whereas the
+ * calculate*() functions concern themselves with dealing with only a single employee at a time.
+ *
+ * TODO(@jniles) - this functionality can be merged with the calculate*() functions so that we are not doing the work
+ * twice.  Ideally, we would process each employee, call a calculate() function on them, then do a sum() to get there
+ * totals.  This allows us to reuse the same code to compute withholdings/benefits/pensions/etc while still creating
+ * aggregate transactions.
+ *
+ * This function takes as input a list of employees and calculates the following:
  * - Total base salaries
  * - Total benefits per employee
  * - Total deductions (retentions) from payments for each employee
  *
- * It returns a list of transaction to be executed, the calcualted benefits, the calcualed deductions,
+ * It returns a list of transaction to be executed, the calculated benefits, the calculated deductions,
  * and the pension calculations.
 */
-function dataCommitment(employees, exchangeRates, rubrics, identificationCommitment) {
+function dataCommitment(employees, rubrics, exchangeRates, identificationCommitment) {
   const transactions = [];
   let totalCommitments = 0;
   let totalBasicSalaries = 0;
@@ -81,11 +93,10 @@ function dataCommitment(employees, exchangeRates, rubrics, identificationCommitm
     const employeeRubrics = rubrics.filter(rubric => (employee.employee_uuid === rubric.employee_uuid));
 
     if (employeeRubrics.length) {
-      // Get Expenses borne by the employee
-      const employeeWithholdings = employeeRubrics.filter(rubric => (rubric.is_discount && rubric.is_employee));
 
-      // FIXME(@jniles) - why are we rounding on each loop?  Why not round the whole thing?
-      // We might be under or overcharging because of the repeated rounding!
+      // get expenses borne by the employee
+      const employeeWithholdings = employeeRubrics.filter(common.isWithholdingRubric);
+
       const totalEmployeeWithholdings = common.sumRubricValues(employeeWithholdings);
 
       employeesWithholdingItem.push([
@@ -99,6 +110,8 @@ function dataCommitment(employees, exchangeRates, rubrics, identificationCommitm
         null,
       ]);
 
+      // TODO(@jniles) - why filter these withholdings again?
+      // What does the is_associated_employee do?
       employeeWithholdings
         .filter(rubric => (rubric.is_associated_employee === 1))
         .forEach(rubric => {
@@ -114,8 +127,10 @@ function dataCommitment(employees, exchangeRates, rubrics, identificationCommitm
           ]);
         });
 
-      // PENSION FUNDS
+      // pension funds
+      // FIXME(@jniles) - why is this definition of "pension fund" different from common.isPensionFundRubric()?
       const employeePensionFunds = employeeRubrics.filter(rubric => (rubric.is_linked_pension_fund));
+
       employeePensionFunds.forEach(pensionFund => {
         employeesPensionFundsItem.push([
           db.uuid(),

server/controllers/payroll/multiplePayroll/groupedCommitment.js

@@ -147,8 +147,8 @@ function groupedCommitments(employees, rubrics, rubricsConfig, configuration,
   // get the base data for commitment
   const dataCommitment = commitmentFunction.dataCommitment(
     employees,
-    exchangeRates,
     rubrics,
+    exchangeRates,
     identificationCommitment,
   );
 

server/controllers/payroll/multiplePayroll/setMultiConfiguration.js

@@ -1,10 +1,9 @@
 /**
- *
- * @description
  * @description
  * This controller initializes the payment configuration for multiple employees simultaneously. It calculates
  * the necessary data, including the values of the rubrics defined individually for each employee.
  *
+ * @requires debug
  * @requires db
  * @requires Exchange
  * @requires payrollSettings
@@ -24,7 +23,7 @@ async function config(req, res, next) {
 
   debug(`Creating a configuration for ${employees.length} employees with payroll id (${payrollConfigurationId}).`);
 
-  // retrieves the date and currencty information for the payment period
+  // retrieves the date and currency information for the payment period
   const getPeriodData = `
     SELECT payroll_configuration.id, payroll_configuration.dateFrom, payroll_configuration.dateTo,
       payroll_configuration.config_ipr_id, taxe_ipr.currency_id
@@ -54,6 +53,7 @@ async function config(req, res, next) {
     const { dateFrom, dateTo } = periodData;
     debug(`Discovered payroll configuration spans from ${dateFrom} to ${dateTo}.`);
 
+    // TODO(@jniles): rename setConfig() to a clearer name.  It is not evident what it does.
     // retrieves a list of queries that should be executed in the same transaction.
     const payrollTxns = await payrollSettings.setConfig(
       employees,
@@ -68,7 +68,7 @@ async function config(req, res, next) {
 
     const txn = db.transaction();
 
-    // flatten from list of lists and add each query to the db.transaaction query
+    // flatten from list of lists and add each query to the db.transaction query
     payrollTxns
       .flat()
       .forEach(({ query, params }) => { txn.addQuery(query, params); });

server/controllers/payroll/multiplePayrollIndice/index.js

@@ -1,9 +1,6 @@
 /**
- * @method find
- *
  * @description
- * This method will apply filters from the options object passed in to
- * filter.
+ * HTTP controller for multiple payroll indice.
  */
 const db = require('../../../lib/db');
 
@@ -20,48 +17,58 @@ function read(req, res, next) {
   }).catch(next);
 }
 
-// specfiying indice's value for an employee
-function create(req, res, next) {
+// specifying indice's value for an employee
+async function create(req, res, next) {
   const currencyId = req.body.currency_id;
   const payrollConfigurationId = req.body.payroll_configuration_id;
   const employeeUuid = req.body.employee_uuid;
   const { rubrics } = req.body;
   const minMonentaryUnit = req.session.enterprise.min_monentary_unit;
 
-  const monetaryRubrics = rubrics.filter(r => r.is_monetary === 1);
+  try {
 
-  const transaction = db.transaction();
-  transaction.addQuery(`DELETE FROM employee_advantage WHERE employee_uuid = ?`, [db.bid(employeeUuid)]);
+    // TODO(@jniles) - migrate this to "common.isMonetary()"
+    const monetaryRubrics = rubrics.filter(r => r.is_monetary === 1);
 
-  monetaryRubrics.forEach(r => {
-    r.value = minMonentaryUnit * Math.round(r.value / minMonentaryUnit);
+    const transaction = db.transaction();
 
-    transaction.addQuery('INSERT INTO employee_advantage SET ?', {
-      employee_uuid : db.bid(employeeUuid),
-      rubric_payroll_id : r.id,
-      value : r.value,
+    transaction.addQuery(`DELETE FROM employee_advantage WHERE employee_uuid = ?`, [db.bid(employeeUuid)]);
+
+    // TODO(@jniles) - This might be a bug - the minMonentaryUnit is related to the enterprise currency,
+    // yet we don't check the currency of rubrics we are converting. We should probably do that.
+    monetaryRubrics.forEach(r => {
+      r.value = minMonentaryUnit * Math.round(r.value / minMonentaryUnit);
+
+      transaction.addQuery('INSERT INTO employee_advantage SET ?', {
+        employee_uuid : db.bid(employeeUuid),
+        rubric_payroll_id : r.id,
+        value : r.value,
+      });
     });
-  });
 
-  rubrics.forEach(r => {
-    transaction.addQuery(`
-      DELETE FROM stage_payment_indice
-      WHERE employee_uuid = ? AND payroll_configuration_id=? AND rubric_id = ?`, [
-      db.bid(employeeUuid), payrollConfigurationId, r.id]);
-
-    transaction.addQuery('INSERT INTO stage_payment_indice SET ?', {
-      uuid : db.uuid(),
-      employee_uuid : db.bid(employeeUuid),
-      payroll_configuration_id : payrollConfigurationId,
-      rubric_id : r.id,
-      currency_id : currencyId,
-      rubric_value : r.value,
+    rubrics.forEach(r => {
+      transaction.addQuery(
+        `DELETE FROM stage_payment_indice WHERE employee_uuid = ? AND payroll_configuration_id = ? AND rubric_id = ?`,
+        [db.bid(employeeUuid), payrollConfigurationId, r.id],
+      );
+
+      transaction.addQuery('INSERT INTO stage_payment_indice SET ?', {
+        uuid : db.uuid(),
+        employee_uuid : db.bid(employeeUuid),
+        payroll_configuration_id : payrollConfigurationId,
+        rubric_id : r.id,
+        currency_id : currencyId,
+        rubric_value : r.value,
+      });
     });
-  });
 
-  transaction.execute().then(() => {
+    await transaction.execute();
+
     res.sendStatus(201);
-  }).catch(next);
+  } catch (err) {
+    next(err);
+  }
+
 }
 
 // retrieve indice's value for employee(s)

test/server-unit/payroll/common.spec.js

@@ -167,6 +167,8 @@ describe('test/server-unit/payroll/common', () => {
     expect(common.sumRubricTotals(rubrics)).to.equal(205);
   });
 
+  // TODO(@jniles) - Note, we should diversify our test rubrics to include an example of each kind of rubric.
+
   it('#isBenefitRubric() detects the benefits rubrics', () => {
     const firstRubric = rubrics[0];
     expect(common.isBenefitRubric(firstRubric)).to.equal(true);