This past week I continued work on the Firefox Marketplace and Add-ons projects by proposing fixes for a couple of existing bugs. As I am becoming more familiar with the codebase, choosing new bugs to work on seems to be getting increasingly easier. This is a great feeling since near the beginning of my term I was finding that to be by far the biggest challenge to contributing.
Collection Add-on Comments Won’t SaveAfter completing my usual local code update cycle, I ventured over to Bugzilla to look for a good bug to take on. I generally find myself sorting the available bug list by the report date so that the chances are higher that the bug is reproducible and has not already been fixed. I came across this bug which detailed that when you add comments to specific add-ons within your own collections, they do not get saved. This seemed like something good to look into since it was a pretty big piece of functionality and a lot of different things could be going wrong.
Each reported bug usually includes some "Steps to reproduce" which makes my job as a developer much easier. I fired up my development server for the Add-ons project and added a new test collection with a single add-on in it. Next I went to edit the collection and attempted to add a comment and save the changes. Sure enough, the comment that I had added did not seem to be persisted anywhere. When I started working on the bug, I had figured that there must be some error that was happening prior to the save being called. Oddly, there was no traceback or error message present upon saving the comment changes.
I started to trace each line of code that was being executed after the save button was pressed. This starts with some form processing but also jumps around into a lot of different helper and decorator methods. This tends to pinpoint one of the main challenges of working with a large codebase. There is a fine line between modularity and maintainability. Splitting the code into a variety of well-defined functions is almost always a good idea but inevitably it will also make the code much more difficult to follow, especially for someone who doesn’t know all of its inner workings.
After tracing the code through and nearly chalking up the error to some strange database behaviour, I decided to break down the main comment saving code in the Django shell. Django’s shell is an interpreter that you can launch within your project using:
python manage.py shell
It allows you to type Python code having each line executed individually and ties into your project so you can access all of your defined models. It also shows you the individual SQL statements that are being executed whenever you manipulate or search for anything using the ORM. I started by replicating the exact lines of problematic code. I manually added a new comment and called save, just as in the method, and sure enough I saw the same behaviour.
I noticed that in the code they were trying to retrieve a single instance of the
CollectionAddon object but they were using
.filter which returned any results in a lazily evaluated array. Since we only expected a single instance to be returned, I changed the way that it was fetched by calling
.get and I found that in this case I was able to save the comments properly. I was thrilled that the comments finally got saved and I quickly made the fix but I still was not entirely sure why it worked. After a little more reading, I learned that since Django’s returned querysets from
.filter are lazily evaluated, each time a comment was set and saved a different instance of the object was being used. In essence one instance had a comment set but was never saved while the other instance had save called but the comment was never set!
I added a unit test for my fix to ensure proper behaviour and submitted my pull request for review. I received only a single piece of feedback about how to better structure my code and I made that change quickly.
Hiding Links for Deleted VersionsAfter submitting my pull request, I ventured to the bug list for the Marketplace project. I came across a bug from early 2013 that described how if app reviewers tried to perform validation or check the contents of a deleted version of a packaged application they would hit a 500 error. The reporter of the bug suggested that those links be hidden for any deleted versions and one of the developers chimed in to mention that we should also handle the error that was happening in the off chance that someone still hit the 500 page.
I was able to reproduce this bug quite easily and identified the spot in the templates where I could perform the hiding of the links. I also added a new
try block to handle the error that was being thrown and I opted to throw a 404 in that case instead (since the error was happening because the files being searched for did not exist). I added a unit test to ensure that a 404 did in fact occur and then I submitted my pull request here.
One of the core developers responded and asked me to reformat my commit message so that it included the bug number. I noticed that a few others had done this and it does make sense given that it will make it much easier to identify the bug in order to revert it (if the need arises). I amended the commit and after a bit of clarification and some tweaks, the developer merged the pull request!
Shortly after, the same developer who merged my code sent me a message on IRC informing me that my code broke the build! Mozilla uses a continuous integration system in order to constantly run all of the tests and ensure that everything that is being merged works well. He sent me a link to the error that occurred. I acted quickly on this by ensuring that the 500 error doesn’t happen in all cases. I submitted another small pull request to address this and the developer merged it immediately which rectified the problem, whew!
That same developer actually reached out to me again on IRC later that day to recommend a bug for me to fix which is related to the one I just closed (its actually an edge case). It is great to be in a position where the developers feel that they can contact me. Perhaps that will be a good one to tackle next week!