CodeSOD: Unique Emails

Once upon a time, a company started a large React application. Like so many such projects, the requirements were vague and poorly understood, the management didn’t have any sort of plan, and the developers didn’t understand the framework. Once this inevitably started to go off the rails, all the developers were fired and then contractors were brought in, because that would be “cheaper”.

Spoilers: it was not. So all the contractors were fired, and a new round of hires were brought in. This time, though, they’d correct their mistakes by strictly enforcing Scrum practices to be “agile”.

This is where Gretchen entered the story.

The code was a mess, and every feature was fragile if not outright broken. The project was overbudget and behind schedule before Gretchen even started. In other words, things were going great.

Gretchen started with what appeared to be a simple task- fixing the fact that many text fields overflowed their bounding box and were thus unreadable. She dug in, and started going through the component code, which is where she found this:

const WorkspaceListItem = ({
address,
client,
primaryContact,
phoneNumber,
}) => {
/** README: since from bulk invite primary contact proceed to the process even primary email address is empty,
so I put in temporary email address ‘hideme.ricblyz+@gmail.com’, this temporary email should not display in user interface */
const clientName = primaryContact ? (
primaryContact.username.match(/hideme.ricblyz/g) ? (
<em>n/a</em>
) : (
primaryContact.username
)
) : (
<em>n/a</em>
)

return (
<tr>
{/* Lots of table columns constructed in a weird way, but that’s not TRWTF */}
</tr>
)
}

Now, let’s start with the comment. This component is used to handle bulk updates, and those bulk updates might be missing their email address. Now, the first problem here: email address is the username, and thus, is absolutely required. The idea that you can simply skip past this missing field by using a temporary value is just plain wrong. This is a hard requirement, and there absolutely is no requirement to accept entries without valid email addresses.

Worse, not pictured here, the temporary email address is hideme.ricblyz+UNIXTIMESTAMP@gmail.com- the Unix timestamp in seconds. The bulk operation, as you can imagine, does more than one row per second, meaning the “temporary” email addresses (that we don’t need) are not unique.

At this point, does it even matter that the regex is technically wrong? The . matches any character, and while it’s unlikely that someone has hidemeFricblyz as their email address, it would certainly cause unexpected behavior here.

Now, as it turns out, this code is only “fixing” the UI layer display of the missing email address. When the data is sent to the database, the username field is left as null, which causes the database to throw out errors- which is the correct behavior, despite what the UI thinks it’s doing.

So, unnecessary code which doesn’t do the right thing, and also contains a subtle bug, but the database schema ends up doing the right thing by accident anyway. And, for fun, this anti-pattern is spammed through 40-or-so React components.

As for Gretchen:

I’m not going to fix this now, after all it has the desired behavior in it’s own way. And anyways, the sprint is already started and we’re not allowed to change its scope.

Source: Read More