We are implementing a fix and will be releasing it to the community in the next few days after our QA resources validate it.

The best way to share your code is as a git commit in a public git repository that I can pull from. You can either do that as a commit on top of the main MOODLE_21_STABLE branch, or you can fork https://github.com/timhunt/moodle-local_qeupgradehelper and issue a pull request through github.

I just got back from holiday, so will be able to look at this over the next few days, with any luck.

Tim,

I have checked in the changes to lib.php to https://github.com/ElanHasson/moodle-local_qeupgradehelper/blob/master/lib.php

Feel free to incorporate this into the main branch of QE Upgrade Helper.

Will try to find time to look soon.

Effectively right. Thank you very much for writing this code, but there are various problems I spotted when reviewing the code:

1. Numerous whitespace errors. You might like (hate, but it is a useful tool) https://github.com/timhunt/moodle-local_codechecker

2. You don't need to require_once config.php. That will already have ben included by whatever scripts requires lib.php. Similarly (I am pretty sure) adminlib.php.

3. There is a reason why we separate lib.php and locallib.php in plugins. The point is that lib.php gets required from lots of places, wherever part of core Moodle needs to find out something about the plugin. Therefore, we try to make lib.php as small as possible, and minimise the dependancies. As it happens, it does not really matter here, because lib.php is only used for cron. However, the general principle remains, and therefore require_once('locallib.php') is bad. If a few functions in lib need more libraries loaded, then you should put the require_once inside those functions. A non-trivial example where it is done right is mod/quiz/lib.php. So, anyway, the require_once(dirname(_FILE_) . '/locallib.php') should be moved inside local_qeupgradehelper_process.

Oh! I now see that I wrote this line of code! So I am wrong, but this should still be fixed.

3. We must not require_once(dirname(_FILE_) . '/afterupgradelib.php'); unconditionally. That will cause fatal errors when this is installed in Moodle 2.0. That should be required inside one of the functions, and it should probably be guarded by an local_qeupgradehelper_is_upgraded() test.

4. I was going to complain about the copy-and-paste SQL, but I see that I have done that a lot too in locallib.php. At least I would be inclined to move local_qeupgradehelper_get_quiz_for_upgrade into locallib, so that all the duplicated SQL is in one file. Refactoring to reduce duplication would be a bonus.

5. More importantly, you cannot just stick limit 1 on the end of the SQL because that does not work in all our supported databases. To do it right either pass IGNORE_MULTIPLE to get_record_sql, or use get_records_sql with $limitnum = 1.

6. The global $CFG; you have added does not seem to be used.

7. You have said $quizzes = local_qeupgradehelper_get_quiz_for_upgrade(); but that function returns a single $quiz, so that is a bad variable name.

You may feel that you have already done enough by writing a first draft of the code that I should have written ages ago, in which case feel free to say so, and I will polish it up and submit it. Alternatively, if you want to fix this code yourself, I will of course be even more grateful. Thanks.

Just updating the status

Elan, are you planning to do any more work on this, or do I have to take it on?

Tim,

Apologies for the delay.
The resource that initial made the modifications will be taking this over and making the above changes.

Great! Thanks.

(Although if I was the developer you are referring to, I would probably be offended by being called a 'resource' )

Hi Tim, the developer Elan was referring to its me. I have made the changes mentioned by you above and committed code. Please let us know your feedback on the same.

Just to be clear, the code to review is https://github.com/ElanHasson/moodle-local_qeupgradehelper/commits/master

I will look later, this is now on my todo list.

Yes Tim, the code to review is https://github.com/ElanHasson/moodle-local_qeupgradehelper/commits/master

function local_qeupgradehelper_get_quiz_for_upgrade in file locallib.php uses mdl hard coded prefix.

Do we really require to use

table is huge for many production sites.

sorry for the late response. removed the hard coded mdl prefix from function local_qeupgradehelper_get_quiz_for_upgrade() and use

table is not required as we need only the quiz ids which needs to upgrade. I have committed the changes.

More code-review comments:

1. Please sanitise your commits before you submit them for code-review. Your branch hand a lot of mess in it.

2. Don't make white-space errors. You have trailing white-space on some lines, and missing newlines at the ends of some files.

3. $DB->get_record_sql( ... , IGNORE_MULTIPLE); as a way to get just one record feels like a bit of a hack to me, but I suppose it does not do any actual harm so we can live with it.

4. The logic with the while loop was wrong. It is meant to upgrade quizzes until it runs out of time. I moved things around there, and added more mtrace output.

OK, so I think https://github.com/timhunt/moodle-local_qeupgradehelper/compare/master...MDL-28329 is now right, but I would appreciate a second opinion, particularly if you are able to test it.

I will probably squash this down further, to a single commit, before I add it to the master branch and submit it for integration into Moodle 2.1.x. Am I right in thinking that KNDeepa has done most of the work, and so he should get the credit?

Thanks again for your work on this.

Tim,

That is correct, KNDeepa should get the credit for this patch.

Tim and Elan,

Thanks for the Credit and sorry for the messy code. Seems like the editor problem or so since i had used codechecker to check the whitespace.

I found an issue with the your code.

1. in lib.php function local_qeupgradehelper_process(), line number 74 if ($quiz) { should be if(!quiz) as it should not process the quiz if there are no more quizzes. As of now if any quiz is there it will not process.

2. mtrace message should be "upgrade" instead of "upgrage". Just a spelling.

After making the if condtion to check for no quiz. It is working fine, I tested it.

I have made the changes mentioned above and committed the code in https://github.com/ElanHasson/moodle-local_qeupgradehelper/blob/master/lib.php

Change pushed to https://github.com/timhunt/moodle-local_qeupgradehelper, and I am about to submit this for integration into 2.1.

Thanks for all your hard work, and for catching my last few mistakes.

Thanks for all the work on this guys - it has now been integrated and awaits testing

Tried it with one site with 5 quizzes. Picked 1 for upgrade leaving the rest for cron.

After upgrade, the qehelper showed me properly the 1 (upgraded) and the 4 (to upgrade).

Ran CLI cron.php and got this:

Processing cron function for local_qeupgradehelper...

qeupgradehelper: local_qeupgradehelper_cron() started at 00:58:13

qeupgradehelper: processing ...

  starting upgrade of attempts at quiz 3

.  upgrade of quiz 3 complete.

  starting upgrade of attempts at quiz 2

...  upgrade of quiz 2 complete.

  starting upgrade of attempts at quiz 4

...  upgrade of quiz 4 complete.

  starting upgrade of attempts at quiz 5

...  upgrade of quiz 5 complete.

qeupgradehelper: No more quizzes to process. You should probably disable the qeupgradehelper cron settings now.

qeupgradehelper: Done.

qeupgradehelper: local_qeupgradehelper_cron() finished at 00:58:13

So sounds like a pass. Ciao

And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

Ciao