Moodle
  1. Moodle
  2. MDL-28329

qeupgradehelper cron functionality is not implemented.

    Details

    • Testing Instructions:
      Hide

      1. Start with a Moodle 2.0 site with quiz attempts.
      2. qeupgradehelper from https://github.com/timhunt/moodle-local_qeupgradehelper and follow the instructions in partialupgrade-example.php to ensure the upgrade does not upgrade all the quiz attempts.
      3. Upgrade to 2.1
      4. Go to qeupgradehelper and configure cron to complete the upgrade.
      5. Run cron, and check the mtrace output to confirm that it does upgrade (some of) the remaining quiz attempts.

      Show
      1. Start with a Moodle 2.0 site with quiz attempts. 2. qeupgradehelper from https://github.com/timhunt/moodle-local_qeupgradehelper and follow the instructions in partialupgrade-example.php to ensure the upgrade does not upgrade all the quiz attempts. 3. Upgrade to 2.1 4. Go to qeupgradehelper and configure cron to complete the upgrade. 5. Run cron, and check the mtrace output to confirm that it does upgrade (some of) the remaining quiz attempts.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18046

      Description

      It seems the code for the qeupgradehelper cron functionality has been left out of the release.

      I have verified this with Tim Hunt.

      I receive the following message in crontab output:

      qeupgradehelper: local_qeupgradehelper_cron() started at 16:49:27
      qeupgradehelper: processing ...
      qeupgradehelper: sorry, not implemented yet.
      qeupgradehelper: local_qeupgradehelper_cron() finished at 16:49:27

      In qeupgradehelper's lib.php I see this:

      while (time() < $stoptime) {
      mtrace('qeupgradehelper: processing ...');

      // TODO
      mtrace('qeupgradehelper: sorry, not implemented yet.');
      return;
      }

        Activity

        Hide
        Elan Hasson added a comment -

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

        Show
        Elan Hasson added a comment - We are implementing a fix and will be releasing it to the community in the next few days after our QA resources validate it.
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Elan Hasson added a comment -

        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.

        Show
        Elan Hasson added a comment - 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.
        Hide
        Tim Hunt added a comment -

        Will try to find time to look soon.

        Show
        Tim Hunt added a comment - Will try to find time to look soon.
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Tim Hunt added a comment -

        Just updating the status

        Show
        Tim Hunt added a comment - Just updating the status
        Hide
        Tim Hunt added a comment -

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

        Show
        Tim Hunt added a comment - Elan, are you planning to do any more work on this, or do I have to take it on?
        Hide
        Elan Hasson added a comment -

        Tim,

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

        Show
        Elan Hasson added a comment - Tim, Apologies for the delay. The resource that initial made the modifications will be taking this over and making the above changes.
        Hide
        Tim Hunt added a comment -

        Great! Thanks.

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

        Show
        Tim Hunt added a comment - Great! Thanks. (Although if I was the developer you are referring to, I would probably be offended by being called a 'resource' )
        Hide
        Deepa Narayanan added a comment -

        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.

        Show
        Deepa Narayanan added a comment - 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.
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Deepa Narayanan added a comment -
        Show
        Deepa Narayanan added a comment - Yes Tim, the code to review is https://github.com/ElanHasson/moodle-local_qeupgradehelper/commits/master
        Hide
        jai gupta added a comment -

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

        Do we really require to use

        {question_sessions} table? This query will run many times (every cron) and {question_sessions}

        table is huge for many production sites.

        Show
        jai gupta added a comment - function local_qeupgradehelper_get_quiz_for_upgrade in file locallib.php uses mdl hard coded prefix. Do we really require to use {question_sessions} table? This query will run many times (every cron) and {question_sessions} table is huge for many production sites.
        Hide
        Deepa Narayanan added a comment -

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

        {question_sessions}

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

        Show
        Deepa Narayanan added a comment - sorry for the late response. removed the hard coded mdl prefix from function local_qeupgradehelper_get_quiz_for_upgrade() and use {question_sessions} table is not required as we need only the quiz ids which needs to upgrade. I have committed the changes.
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Elan Hasson added a comment -

        Tim,

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

        Show
        Elan Hasson added a comment - Tim, That is correct, KNDeepa should get the credit for this patch.
        Hide
        Deepa Narayanan added a comment -

        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

        Show
        Deepa Narayanan added a comment - 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
        Hide
        Tim Hunt added a comment -

        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.

        Show
        Tim Hunt added a comment - 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.
        Hide
        Sam Hemelryk added a comment -

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

        Show
        Sam Hemelryk added a comment - Thanks for all the work on this guys - it has now been integrated and awaits testing
        Hide
        Eloy Lafuente (stronk7) added a comment -

        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

        Show
        Eloy Lafuente (stronk7) added a comment - 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
        Hide
        Eloy Lafuente (stronk7) added a comment -

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

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao

          People

          • Votes:
            3 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: