Thursday, March 29, 2012

Fixing a bug in Moodle core: the mechanics

Several people at work have asked me about this, so I thought I would write it out as a blog post. In this post, I want to focus on the mechanics of using git and the Moodle tracker to prepare a bug-fix and submit it. Therefore I need a really simple bug. Fortunately one was reported recently: MDL-32039. If you go and look at that, you can see that the mistake is that I failed to write coherent English when working on the code to upgrade from Moodle 2.0 to 2.1.

What we need to do

This bug was introduced in Moodle 2.1, and affects every version since then. Since it is a bug, it needs to be fixed in all supported versions, which means on the 2.1 and 2.2 stable branches, and on the master branch.

My development set-up

I need to fix then test code on three branches. The way I handle this is to have a separate install of Moodle for each stable branch, and one for master.

I use Eclipse as my IDE, so these three copies of Moodle are separate projects in my Eclipse workspace .../workspace/moodle_head, .../workspace/moodle_22, and .../workspace/moodle_21. Each of these folders is a git repository. In each repositor, I have two remotes set up, for example:

In order to test the code, I have three different Moodle installs, each pointing at one of these copies of the code. Each install uses an different database prefix in config.php, so they can share one database.

Getting ready to fix the bug on the master branch

The way I normally work is to fix the bug on the master branch first, and then transfer the fix to previous branches.

The first thing I need to do is to make sure my master branch is up to date, which I do using git:

Other people use other conventions. For example they might call the branch MDL-32039_qeupgradehelper_typos. That is a much better name. It helps you see immediately what that branch is about, but I am too lazy to type long names like that.

To fix this bug it is just a matter of going into admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php and editing the two strings that were wrong.

Except that, if I screwed up those two strings, it is quite likely that I made other mistakes nearby. I therefore spent a bit of time proof-reading all the strings in that language file (it is not very long). That was worthwhile. I found and fixed two extra typos. This sort of thing is always worth doing. When you see one bug report, spend a bit of time thinking about and checking whether other similar things are also broken.

OK, so here is the bug fix:

timslaptop:moodle_head tim$ git diff -U1diff --git a/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php b/admin
index 3010666..7bd7c13 100644
--- a/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
+++ b/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php@@ -50,3 +50,3 @@ $string['gotoresetlink'] = 'Go to the list of quizzes that can
$string['includedintheupgrade'] = 'Included in the upgrade?';
-$string['invalidquizid'] = 'Invaid quiz id. Either the quiz does not exist, or +$string['invalidquizid'] = 'Invalid quiz id. Either the quiz does not exist, or
$string['listpreupgrade'] = 'List quizzes and attempts';
@@ -57,5 +57,5 @@ $string['listtodo_desc'] = 'This will show a report of all the
$string['listtodointro'] = 'These are all the quizzes with attempt data that st
-$string['listupgraded'] = 'List already upgrade quizzes that can be reset';+$string['listupgraded'] = 'List already upgraded quizzes that can be reset';
$string['listupgraded_desc'] = 'This will show a report of all the quizzes on t
-$string['listupgradedintro'] = 'These are all the quizzes that have attempts th+$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
$string['noquizattempts'] = 'Your site does not have any quiz attempts at all!'
@@ -82,2 +82,2 @@ $string['upgradedsitedetected'] = 'This appears to be a site t
$string['upgradedsiterequired'] = 'This script can only work after the site has
-$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never +$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never

(Note that:

I would not normally use the -U1 option. That just makes the output smaller, for the benefit of this blog post.

The diff is chopped off at 80 characters wide, which is the size of my terminal window.)

Now I need to test that the fix actually works. I go to Site administration ▶ Question engine upgrade helper in my web browser, and verify that the strings now look OK.

Notice that I followed the approved style for Moodle commit comments. First the issue id, then a brief indication of which part of the code is affected, then a colon, then a brief summary of what the fix was. This first line of the commit comment is meant to be less than about 70 characters long, which can be a challenge!

If this had been a more complex fix, I would probably have added some additional paragraphs to the commit comment to explain things (and so I would have typed the comment in my editor, rather than giving it on the command-line with the -m option). In this case, however, the one line commit comment says enough.

More to the point, I can go to the tracker issue, click Request peer review, and fill in the details of this git branch, including that compare URL.

If this was a complex fix, I would then want for someone else to review the changes and confirm that they are OK. In this case, however, the fix is simple and I will just carry on without waiting for a review.

Transferring the fix to the 2.2 stable branch

So, now I want to apply the same fix to my moodle_22 code. First I need to update that install. Since this is similar to what we did above to update master, I will not show the output of these commands, just what I typed:

Then I visit http://localhost/moodle_22/admin/index.php to complete the upgrade.

(This may seem a bit laborious, but look out for a future blog post where I intend to talk about how I automate some of this. I only typed out the commands in full this time because I was writing this blog post.)

I want to apply the bug-fix I did on master on top of MOODLE_22_STABLE, and fortunately the command git cherry-pick is designed to do exactly that. (Since we are back in new territory, I will start showing the output of commands again.)

Notice that the convention I use is to append _22 to the branch name to distinguish the branch for Moodle 2.2 stable from the branch for master. Other people use different conventions, but this one is simple and works for me.

Of course, in the middle of that, I checked that the fix actually worked in Moodle 2.2. In this case, there is not much to worry about, but with more complex changes, you really have to check. For example the fix you did on the master branch might have used a new API that is not available in Moodle 2.2. In that case, you would have had to redo the fix to work on the stable branch.

Transferring the fix to the 2.1 stable branch

Now I rinse and repeat for the 2.1 branch. (I will supress the command output again, until the last command, when something interesting happens.)

That may or may not make things clear. Fortunately, I know the history behind this. What is going on here is that in Moodle 2.1, this code was in local/qeupgradehelper, and in Moodle 2.2 it moved to admin/tool/qeupgradehelper, and this confuses git. Therefore, I will have to sort things out ourselves.

timslaptop:moodle_21 tim$ git diff -U1diff --git a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php b/local/qeu
index ac883b5..7bd7c13 100644
--- a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
+++ b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php@@ -19,3 +19,3 @@
*
- * @package local+ * @package tool
* @subpackage qeupgradehelper
@@ -50,3 +50,3 @@ $string['gotoresetlink'] = 'Go to the list of quizzes that can
$string['includedintheupgrade'] = 'Included in the upgrade?';
-$string['invalidquizid'] = 'Invaid quiz id. Either the quiz does not exist, or +$string['invalidquizid'] = 'Invalid quiz id. Either the quiz does not exist, or
$string['listpreupgrade'] = 'List quizzes and attempts';
@@ -57,5 +57,5 @@ $string['listtodo_desc'] = 'This will show a report of all the
$string['listtodointro'] = 'These are all the quizzes with attempt data that st
-$string['listupgraded'] = 'List already upgrade quizzes that can be reset';+$string['listupgraded'] = 'List already upgraded quizzes that can be reset';
$string['listupgraded_desc'] = 'This will show a report of all the quizzes on t
-$string['listupgradedintro'] = 'These are all the quizzes that have attempts th+$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
$string['noquizattempts'] = 'Your site does not have any quiz attempts at all!'
@@ -82,2 +82,2 @@ $string['upgradedsitedetected'] = 'This appears to be a site t
$string['upgradedsiterequired'] = 'This script can only work after the site has
-$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never +$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never

Actually, you can see that there is one wrong change there (the change to @package, so I need to undo that. The easy way to undo that would be to edit the file in Eclipse, but I want to show off another git trick:

Submitting the fix for integration

Now I have tested versions of the fix on all three of the branches where the bug needed to be fixed. So, I can go back to the Tracker issue and submit it for integration.

When I get to the bug, I see that Jim Tittsler, who reported the bug, has seen my request for peer review, and added a comment:

Fantastic! It really makes a difference when people follow-up on the bugs they report, and supply extra information, or just say thank you. In this case, although I intended to carry on without a peer reveiw, I got one.

Now I press the Submit for integration... button, and fill in the fix version, the details of the other branches, and most important, we write some testing instructions, so that someone else can test the bug next Wednesday as part of the integration process.

Finally, we are done. Now we sit back and wait for next Monday, when the weekly integration cycle starts. Our change will be reviewed, tested, and, all being well, included in the next weekly build of Moodle.

Reflection

Is this an overly laborious process? Well, it is if you try to describe every detail in a blog post! In normal circumstances, however, it really does not take long. In normal circumstances I could probably have done this fix in ten to fifteen minutes.

What usually takes the time is thinking about the problem, and writing and testing the code. This takes much longer than typing the git commands and completing the tracker issue. Writing the testing instructions can be laborious, particularly if it is a complex issue, but that is normally time well spent. It forces you to think carefully about the changes you have made, and what needs to be done to verify that they fix the bug that was reported without breaking anything else. As I said in my previous blog post, I think testing instructions are a really good discipline.

I hope this rather long blog post was interesting, or at least useful to somebody.

8 comments:

"I hope this rather long blog post was interesting, or at least useful to somebody."

It's fascinating...I only know the very, very basics of git, and as a sole user at that, so seeing a walkthrough of a real issue is really valuable ito suggesting to me some of the things I need to learn about. Thanks:-)

An interesting discussion is worth comment. I think you should write more on this topic, it might not be a taboo subject but generally people are not enough to speak on such topics. To the next. Cheers

Michael, there are various levels of permission in the tracker, depending on your role. I get the Submit for integration button because I am component owner for the quiz and questions components.

'Normal' developers just get the Submit for peer review button.

Users who are not in the developers group cannot do that, I think. You can just attach your patch, or put a link to the github compare URL in the comments.

I don't think they have yet formalised what you need to do to earn the higher levels of access. There is an emerging patter that Michel de Raadt tends to notice the first time someone's patch gets submitted for integration, and gives them developer access if they don't already have it. That seems a reasonable heuristic to me. Perhaps it should be formalised?

What you need to do to be allowed direct access to submit for integration is less clear, but it is basically fix a number of bugs without causing too much hassle for other people, and then ask nicely.

Well, this is not the place to discuss that of moodle.org policy question. http://moodle.org/mod/forum/view.php?id=7135 is the right place.