Making payroll code more unit-testable

[jniles]

We recently had a bunch of code written to test the payroll code, and many of the tests call the expressjs controllers. This is necessary because the code wasn't written to be tested with unit tests, so we only have the expressjs controllers as the best wrapper. I think we should fix that.

Any logic that is in controllers should be broken out to distinct functions that do one thing and one thing only. The function, calculateIRPTaxRate is a good example - it does this already.

Example: Seniority & Raises

I'll give a concrete example for calculating seniority bonuses in the payroll code. First, take this function in the server code that calculates seniority:

bhima/server/controllers/payroll/configuration/index.js

Lines 234 to 253 in b5dd5b2

	newEmployees.forEach(employee => {
	employee.hiring_date = moment(employee.hiring_date).format('YYYY-MM-DD');
	const yearOfSeniority = parseInt(moment(dateTo).diff(employee.hiring_date, 'years'), 10);
	// Here we increment the base index based on the number of years
	for (let i = 0; i < yearOfSeniority; i++) {
	employee.value += (employee.value * (baseIndexGrowthRate / 100));
	}
	const dataStaffingIndice = {
	uuid : db.uuid(),
	employee_uuid : employee.uuid,
	grade_uuid : employee.grade_uuid,
	fonction_id : employee.fonction_id,
	grade_indice : util.roundDecimal(employee.value, 0),
	function_indice : employee.function_indice_value || 0,
	date : new Date(),
	};
	transaction.addQuery('INSERT INTO staffing_indice SET ?', dataStaffingIndice);
	});

Move the inner function into a separate file with a function called calculateEmployeeSenority(employee). It can either modify the employee object, or return yearsOfSeniority that could be attached to the employee object.

Then make a function called calculateSeniorityBonus(employee, baseIndexGrowthRate) in the same file to compute the salary change based on the years of experience. Again, one can either modify the employee object passed in, or return just the number and attach it to the employee object.

This would allow us to test the logic in that file, and skip testing the await config() since it wouldn't contain any real logic. One could then also test corner cases - what happens if the person was hired in the future? What happens if the baseIndexGrowth is 0?

Similarly, the same could be done with this function:

bhima/server/controllers/payroll/configuration/index.js

Lines 255 to 286 in b5dd5b2

	oldEmployees.forEach(employee => {
	employee.hiring_date = moment(employee.hiring_date).format('YYYY-MM-DD');
	employee.lastDateIncrease = moment(employee.lastDateIncrease).format('YYYY-MM-DD');
	// For employees who have already been configured, we will compare the number of years of seniority
	// and the difference in years between the date of the last increment of the base index,
	// if this difference is greater than zero, the we will have to increment
	// the base index in relation to this difference
	const yearOfSeniority = parseInt(moment(dateTo).diff(employee.hiring_date, 'years'), 10);
	const yearLastIncrementation = parseInt(moment(employee.lastDateIncrease).diff(employee.hiring_date, 'years'),
	10);
	const diffSeniorityIncrementation = yearOfSeniority - yearLastIncrementation;
	if ((diffSeniorityIncrementation > 0) && (baseIndexGrowthRate > 0)) {
	for (let i = 0; i < diffSeniorityIncrementation; i++) {
	employee.grade_indice += (employee.grade_indice * (baseIndexGrowthRate / 100));
	}
	const dataStaffingIndice = {
	uuid : db.uuid(),
	employee_uuid : employee.uuid,
	grade_uuid : employee.grade_uuid,
	fonction_id : employee.fonction_id,
	grade_indice : util.roundDecimal(employee.grade_indice, 0),
	function_indice : employee.function_indice_value || 0,
	date : new Date(),
	};
	transaction.addQuery('INSERT INTO staffing_indice SET ?', dataStaffingIndice);
	}
	});

Something like calculateSeniorityForOldEmployees(employee).

Finally, the config() controller code would then look something like this:

  const [newEmployees, oldEmployees, dataEnterprise] = await Promise.all([
    db.exec(sqlFindNewEmployees, idPeriod),
    db.exec(sqlFindOldEmployees, idPeriod),
    db.exec(sqlGetBaseIndexGrowthRate),
  ]);

  const transaction = db.transaction();

  const baseIndexGrowthRate = dataEnterprise[0].base_index_growth_rate;

  newEmployees.forEach(employee => {
     const seniority = calculateSeniorityBonus(employee);
      
     const dataStaffingIndice = {
       uuid : db.uuid(),
       employee_uuid : employee.uuid,
       grade_uuid : employee.grade_uuid,
       fonction_id : employee.fonction_id,
       grade_indice : util.roundDecimal(seniority.value, 0),
       function_indice : employee.function_indice_value || 0,
       date : new Date(),
    };

    transaction.addQuery('INSERT INTO staffing_indice SET ?', dataStaffingIndice);
  });

This would make me feel better that the calculation logic was actually being tested. The db.exec() calls are less important to test, since we know the integration tests will fail if those are broken. But testing that the calculations for bonuses and seniority:

1. Produces the correct result when good data is provided.
2. Throws an error when bad data is provided (e.g. the hiring_date is not a date, the employee was hired in the future, etc.),

...is the most important part of these unit tests.

I think the code for payroll (and stock, etc) should be reviewed with these ideas in mind.