Earlier this month, a round of updates were made to a client site and deployed to production. A critical bug came in saying that donut charts in visualizations were now showing the wrong numbers and that this was a top priority since it was visible to the public.
This Drupal 7 application fetches data from a server running a .NET application. The applications job is to ingest spreadsheet data and let the editor make modifications, then 'publish' that data version. From a custom module, we are using Guzzle to create a request to that server to get data related to the node we are currently viewing.
I personally had not worked on this project in about a year. Given that most of the team had gone on leave for the holidays, it was up to me to find and debug what the issue could be and resolve it post-haste. I had a list of items to check first to determine where the issue may be:
- The client team is new, and may have entered erroneous data. First, I will check the .NET application data to see if it is incorrect.
- Using the API URLs, check if the response we are getting back is transforming the data in ways that it should not be.
- Validate that we are not showing old cached data on the node.
- Check to see if our build array in the node has a new bug.
Given that there were no tickets related to amending or changing the functionality or display of a donut chart, this was the debugging priority I set in motion.
After going through steps 1-3, I determined that the data was properly formed and the API response was correct, and that no bad values were being cached and served. This really only leaves one last place to look, our theme, where the response data is put into preprocessing for a node template and loaded for ReactJS.
That's when I noticed the problem.
The Bug
I had noticed a line of code looked suspect and not how I remembered it being before. A quick diff illustrated the issue clearly:
$indicator['remaining_value'] = bcsub(100, $indicator['actual_value']);
was changed to:
$indicator['remaining_value'] = 100 + $indicator['actual_value'];
bcsub is a function in the bcmath family in PHP. Most developers would do one of two things here:
- Revert the change back to the way it was when it was working, or
- Change the + to a - sign instead
Simply reverting it is no good. The only reason I can gather that the function was removed was perhaps that the new hosting environment the site lives in does not contain a version of PHP built with bcmath enabled. That is entirely possible, and the commit message even states "Removing for compatibility". So we can't do this. Reverting it would just introduce a fatal error.
Changing the plus sign to a minus sign does fix the actual bug, but that is not a great fix in the long run.
The root of the issue here is that we should have a utility or service to perform this for us, without leaving it up to developer(s) at the theme level to figure it out. Changing just this line here does not guarantee we don't make this mistake later, perhaps on a new node type or area showing donut charts that don't currently have any. Lets create a more bulletproof solution that isolates the functionality.
Creating a DonutChart Class and Test
What we should do is have this functionality live in a single spot and covered by tests. This puts us on a much more stable path and provides an avenue for adding functionality later, should it be needed.
First we add a basic unit test:
<?php
/**
* Class DonutChartTest
*/
class DonutChartTest extends DrupalUnitTestCase {
/**
* Defines information about our test scenario.
*
* @return array
*/
public static function getInfo() {
return array(
'name' => 'Donut Chart Tests',
'description' => 'Unit tests that validate our Donut Chart class.',
'group' => 'PHCPI',
);
}
/**
* Test that we get the correct value remaining.
*/
public function testChartValues() {
module_load_include('inc', 'datacenter_api', 'includes/DonutChart');
$chart = new DonutChart(17);
$this->assertEqual(17, $chart->getActualValue());
$this->assertEqual(83, $chart->getRemainingValue());
$chart = new DonutChart(50);
$this->assertEqual(50, $chart->getActualValue());
$this->assertEqual(50, $chart->getRemainingValue());
$chart = new DonutChart(25);
$this->assertNotEqual(75, $chart->getActualValue());
$this->assertNotEqual(25, $chart->getRemainingValue());
}
}
The only value we receive from the .NET application is the raw value, which leaves it upon us to calculate the remaining value.
Now we supply a simple class:
<?php
declare(strict_types = 1);
class DonutChart {
const MAXVALUE = 100;
/**
* @var int
*/
protected $actual_value;
/**
* @var int
*/
protected $remaining_value;
public function __construct(int $raw_value) {
$this->actual_value = $raw_value;
$this->remaining_value = (self::MAXVALUE - $raw_value);
}
/**
* @return int
*/
public function getActualValue() : int {
return $this->actual_value;
}
/**
* @return int
*/
public function getRemainingValue() : int {
return $this->remaining_value;
}
}
Now that we have this in place and the tests pass, we can amend the template.php file to use this instead:
$donut_chart = new DonutChart($indicator["featuredDataPointRaw"]);
$indicator['actual_value'] = $donut_chart->getActualValue();
$indicator['remaining_value'] = $donut_chart->getRemainingValue();
This way, the developer(s) are not having to do math in the theme. Our new class does the work. If we wanted to show donut charts now in other places, this is now easy and less error prone than before. More importantly, the bug in question is resolved and can't ever happen again.
A few thoughts:
- The class constructor is not validating that the incoming value is greater than 0, and less than MAXVALUE. We could check and throw exceptions here, but that data validation responsibility should be performed in the .NET application in my opinion. We are just consumers of data, not enforcers of it - we should not be jumping through hoops and laying down guard clauses.
- The tests and class don't check for negative values or try to skirt by that by adding abs() around both values. The first bullet point explains why I don't do that.
- Closing this gap here, if the data is not displaying right in the future, we know that the issue is likely with the data itself now. Adding data validation and enforcement in the .NET app to prevent saving or publishing of values that are incorrect would eliminate this entirely. Both applications would be fairly bulletproof and run for years uninterrupted.
- It may seem silly to have a small class, but it lays the foundation down the road in case we need to add more functionality or create similar classes and interfaces for other chart types. It is also cleaner than having 1+ procedural functions, or a function that returns values in a named array.
Much of what I have learned from developing for years in Drupal 8, Symfony and other languages makes its way back when I have situations like these. Be thoughtful with your code.