Onfido’s MissingNos. or How to Stop Infinite Master Balls in Identity Verification
In Pokémon, a master ball allows you to catch any wild Pokémon. When I was a kid, there was this moment in my class in which somebody got hold of an awesome new trick. They could generate infinite master balls.
A series of computer events in the game engine, involving data sharing and safe defaults in the error handling mechanism, lead to the appearance of a placeholder Pokémon, number 000. All hail MissingNo. After an encounter with it, the sixth item in your item menu increases by a large amount.
My name is Daniel Serrano and I’m a back end software engineer at Onfido. I’m going to talk to you about the development cycle and what we care about at Onfido, and in particular I’m going to delve into how the team that I am a part of works.
Whenever we develop a feature, the first thing we do is assign a JIRA ticket to ourselves (if one hasn’t already been assigned). We pick it up and move it to In Progress. We then open a branch with the JIRA code. I’m part of the Identify team. When we’re working on ticket, e.g., ID-23, which has the title “Change ID Global microservice to cache database-stored configurations”, we will create a branch named something in the lines of feature/ID-23-cache-db-configs.
We do this because it provides traceability. With this naming, we can be sure we know what JIRA ticket lead to the changes introduced by this feature branch. We can also get a glimpse of what those changes are by reading the simple title on the branch. That’s nice for whoever is trying to, for example, debug failed tests in Jenkins (which shouldn’t happen that often outside the scope of each team). Then, while developing the feature, we will end each commit message with the JIRA code. E.g.:
Change countries in test files to France
To make it more realistic and according to the use case of this
microservice, we choose to change this to non-GBR, e.g., FRA.
JIRA ID-45
You can leverage a simple git hook to help with the task of extracting the JIRA code from the branch name. Bitbucket and JIRA are then smart enough to interpret it.
Again, traceability. Particularly, when git blaming, we will be able to quickly understand what the motivation was for the change. By looking at the ticket number in the commit message, we can go to Bitbucket and find the PR that lead to these changes — surrounding commits, for instance.
Next time you’re git blaming, think about how useful it would be to almost immediately know which JIRA ticket (which often translates to a business rule) is responsible for the bug you’re trying to fix.
Another thing we care about deeply at Onfido is the content of each commit message. It is well known that code is more often read than written. This is even more true for commit messages. If you’re writing commit messages like “wip” and “fixes minor issues” you aren’t providing much value for a future reader. You should instead explain the motivation behind your decisions, why you took the path that you did, why certain alternatives weren’t followed, and so on. As the awesome Raoul once told me, it should be looked at as a “micro blog post”. The seminal blog post on this was written by the great Tim Pope.
Being explicit and descriptive might help that one guy that will be fixing your code one day. He will know and understand why he shouldn’t even think about a certain specific way of (apparently) achieving the same result, because you’ve warned him in that useful commit message that you wrote. He will thank you, in the middle of a Friday morning, with the deploy cut-off time fast approaching. He will also remember you cared.
After we’ve finished developing the initial version of the feature, we open a Pull Request and assign three reviewers to the PR — one or two within the team and one or two outside the team. At least two people must Approve the PR before we can merge it to the development branch (which will be deployed and tested in Staging).
The rationale behind having people outside the team review our PR is two fold. Firstly, we are sure that we get a fresh, unbiased look at a given problem. Secondly, we broaden the knowledge of that issue or existence of that feature (and corresponding solution) by having someone outside the team revise the decisions made and the intricacies of our specific implementation.
In this process of review, there is a single golden rule we follow at Onfido: “Don’t be a jerk.” Trust me when I say this isn’t written somewhere, other than here. It isn’t handed out as part of our onboarding kit, or expected to be passed around between teammates. It’s just part of who we are, and the values we embody. I must say I’m quite proud of that. So, when reviewing a pull request, instead of snarkily suggesting that Hash access might yield a null value, or rudely asking why the hell that huge array isn’t being lazily loaded, we ponder. We put ourselves in the shoes of the other person and, instead of screaming, we suggest. Instead of taking the opportunity to be sarcastically funny, we calmly ask why.
We also avoid unnecessary cold golfing, bike shedding or yak shaving. If there is an inherent performance concern in the development of a given feature, we want to know about ways to improve the efficiency of our solution. If we’re discussing another (maybe not even better, but different) way of iterating a 10-element array, maybe we shouldn’t spend that much time on it. This is not to say we don’t want to hear about it, but would it be worth changing or halting the development cycle to have a discussion about it? This should be weighed with pragmatism, while trying to keep a sane code base to the fullest extent.
On a side-note about PR reviews, no review is complete without emojis or gifs. They do exist, so you might as well make the most of them. 😛
We have a set of rules enforced on Bitbucket: PRs to the development branch should be approved by at least two people, the tests for the branch have to be green, etc. When all conditions are met we merge our work to development, which will trigger a deploy to Staging, and test it there. Aside from testing the specific feature, we have end to end tests running as part of our deployment pipelines (Staging and Production), which gives us extra confidence.
If any problem is spotted, we go back to the development cycle. Otherwise, if all goes smoothly, we move the story to Closed in JIRA. We’re done.
For me, it’s hard to think of Pokémon without thinking about MissingNo. How different would the Pokémon experience have been without it? As engineers, serendipity is often associated with errors, unwanted events. We create rules, follow best practices and read blog posts about software development precisely to avoid creating a MissingNo.
At Onfido, we sometimes allow the generation of infinite master balls of our own. What allows us to make it so that these do not happen often (or that at least we quickly recover from them) are the processes we have put in place — some read about in blog posts like this one, others the result of trying different approaches to any issue we’re faced with. It’s not to say these will work for you. The dynamics of each team can vary according to many different aspects: the nature of the work (development, ops, testing, security, etc.), or the area of expertise (front-end, back-end development), among other factors.
So far, this has worked for us. But we want to improve, so if you have any suggestions please let us know about it in the comments section.
The work described in this blog post is the result of joint efforts of many teams. Provisionment is handled by our awesome DevOps team. End-to-end testing is at the care of our incredible Testing team. And all of our best practices and processes are collectively shared and iterated upon by Onfido Engineering as a whole. ❤️
