You are viewing a read-only archive of the Blogs.Harvard network. Learn more.

Shame on Me: the missing code review (or is it unit testing?)

So a bug was reported not long ago. Let’s say “sensitive data” was available where it shouldn’t have been. I had an API view, a view that was returning json only, or should have been. Apparently early on in the project, I had added a comment to that view, to make sure it was returning the appropriate data. So I had added an html comment to a view that was supposed to be returning json. And I had it outputting everything under the sun it could output.

<!--
"secure data" => "stuff I don't want the user to see",
... 30-40 lines of this ...
-->
{"success": true}

So there’s 2 problems with that. The most important of which is that with DOM inspection tools, like chrome developer tools or firebug, you can inspect the return value of that request and see the data I don’t really want you to see. The other problem is that I’ve created invalid json, and whatever is checking that success is clearly not using that value.

Why did I do this?

This was early on in development and I was debugging the easiest way I could.

What could have prevented this?

A code review of any kind. One look at that file would have been a red flag to anyone. But a view file that was only outputting true or false never seemed like something I ever needed to look at once I got it working.

Feature Branches with Code Reviews

We have been trying to find a process for doing code reviews for a little while. Because most of our current projects are in github, it made sense to try and use some of the github features to help in this process.

I have been using issues for stories and milestones for sprints for a while. And it works out nicely because I get a pretty neat gamificationy results.

So I started using a feature branch in order to have the ability to make pull requests on my own project and get a discussion thread before code is merged into the trunk (production).

https://github.com/Harvard-ATG/Quizmo/pull/119

This discussion is a great way to get a reasonable code review before merging code. Right now we’ve decided to only do this for non-trivial changes, but with a very high bar on “trivial”.

Posted in ATG, Development. Tags: , , , , . Comments Off on Feature Branches with Code Reviews »