Small Bugs, Big Value

Published in Open Source

The fourth week of working on the Firefox Marketplace and Add-ons project has now come to a close. I spent the time working on fixing or reproducing a variety of different small bugs.

To get started, I did the usual task of pulling in any changes that were made from the upstream Zamboni repository. There are almost always changes in it each time I attempt to sync because of the sheer volume of code change in a given day. After running my development server I found that I was getting some generic errors about certain classes not containing the appropriate methods. Through some investigation I found that some of the upstream changes included a modification of the dependencies in the product. To ensure that my dependencies were up to date, I re-ran the pip install command to get all recent versions. I also updated all of the project’s submodules just to be on the safe side.

pip install --no-deps -r requirements/dev.txt

git submodule update --init --recursive

Next, I was experiencing some unknown column errors when database operations were supposed to be executing. It turns out that part of the changes that were made included some modification of the database structure. The Zamboni project uses a very simple tool called Schematic in order to version the database schema and make changes. The idea is that you can make incremental changes to database fields and types in a bunch of different files. An extra table in the database will keep track of which files have been applied and which have not. In order to get my database up to date I simply had to run:

schematic migrations

I am now making a point of installing all new dependencies and running migrations each time I update the codebase. This will likely save some headache down the road.

Removing Own Access to Marketplace Apps

With my environment setup and running smoothly, the first bug that I decided to tackle was on the Firefox Marketplace. The bug occurred whenever a developer would remove their own access from an application that they were currently an owner on. The typical use case is that someone can add new users or change user permissions on an application within that application’s settings. In this special case, when removing their own access to the application the user was presented with a 403 forbidden error after the page reloaded. This makes sense since they have forfeited their rights to the application but on production this resulted in a blank page and likely some user confusion. I was able to find the spot in the code that should be changed and I added a conditional that would check if the user being removed was the current user that was performing the modification. If so, I redirect that user back to their My Submission page rather than the typical application settings page. From my manual testing, everything seemed to be working properly.

I decided that it would be a good idea to write a unit test for this case as well so that if anything changes in the future we will know that a regression of this bug is occurring. Upon running the existing test suite, some of the tests were already failing. I was certain that the developers of the project wouldn’t let these broken tests exist so I knew that it had to be a problem with my local setup. The first thing that I tried was to explicitly delete my test database and recreate it with a handy provided make target:

make test_force_db

This worked nicely and made all of the unit tests pass again as they should. I imagine that this error happened due to the database schema changes that I ran into earlier (I believe schematic does not handle updating the test database). I added my test to ensure that the redirection worked properly. Django view testing is quite neat because they provide a fake http client that you can use to make requests to certain URLs in your application. From there, you can get the actual response and determine if your views and URLs are interacting in a proper manner.

I submitted a pull request for this bug fix here. There were a few small changes after review but it got quickly merged into master!

Preserving Review Formatting

I continued searching through the available list of bugs and kept finding some that were plausible but had been fixed and some that I just was simply unable to reproduce. Finally, I found a bug that seemed to be getting a lot of attention and would be relatively easy to fix. The bug itself was opened and confirmed in 2010 and had never been addressed!

On the Add-ons site, users can leave reviews for extensions and Add-ons to let other users know how good or bad they think they are. Upon opting to edit your review, if you had multiple paragraphs in your initial review all formatting would be lost. As you can imagine this would be quite maddening if all you wanted to do was fix a grammar or punctuation error in your review and you were forced to reformat. The error stemmed from the fact that the reviews were saved with standard new line characters
but had to be translated to HTML line breaks <br> to get the same behaviour in the code. Whenever someone would opt to edit the review, only the text from the review was being added to the text box and the HTML line breaks were being thrown out. The fix was fairly simple. When editing the review, convert HTML line breaks to new line characters and do the opposite conversion when displaying the review. I don’t have super in depth Javascript knowledge (I prefer working with backend code) but I knew just enough to be able to fix this bug effectively. My pull request for this bug can be found here.

Overall, I am becoming much more comfortable with the process of fixing bugs, which is a great feeling. I am starting to feel like I am less of a burden to the existing developers and that I am actually making worthwhile contributions. My fixes are not breakthroughs by any means, but these are true user facing bugs that can be a source of frustration for the many millions of daily users of the Marketplace and Add-ons websites. Frankly, the fact that I am able to make that experience a little bit smoother is pretty awesome.

Written By

Andrew Halligan

Published January 31, 2014