Moodle
  1. Moodle
  2. MDL-32895

Assignment Upgrade tool improvements for large numbers of assignments

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide

      Use the local plugin "generator" to generate a lot of instances of mod_assignment(at least 200). (/admin/tool/generator/index.php)

      View the assignment upgrade tool. You should be able to change the number of assignments that are displayed at once. The number of pagination links should be correct.

      Upgrade all assignments. The upgrade process should be updated once per assignment by the browser in real time (instead of seeing a blank page until the entire batch upgrade is complete).

      The upgrade process should complete and should not hit the php memory or time limits.

      Show
      Use the local plugin "generator" to generate a lot of instances of mod_assignment(at least 200). (/admin/tool/generator/index.php) View the assignment upgrade tool. You should be able to change the number of assignments that are displayed at once. The number of pagination links should be correct. Upgrade all assignments. The upgrade process should be updated once per assignment by the browser in real time (instead of seeing a blank page until the entire batch upgrade is complete). The upgrade process should complete and should not hit the php memory or time limits.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
      git@github.com:netspotau/moodle-mod_assign.git
    • Pull Master Branch:
    • Rank:
      39966

      Description

      I did some testing of the assignment upgrade tool with 2800 assignments from a customer database and found some areas needing improvement. Specifically:

      • The pagination is hardcoded to 5 - should be a drop down list defaulting to 100
      • Needs set timeout and session close for batch upgrades
      • Needs output buffer reworking so the progress is displayed in realtime during the upgrade process
      • Display more verbose error when an exception is thrown during an upgrade
      • Display the correct number of pagination links for the assignment upgrade table

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi Damyon,

        Looks like these changes need rebasing on current master. lib.php has been editing in these changes but has been removed from master.
        I've looked over the rest of these changes and made the following notes.

        1. upgradableassignmentstable.php the $countfrom variable probably doesn't need to exist. Putting that SQL directly into the set_count_sql is probably going to be clearer.
        2. paginationform.php Argument 5 for the perpage select is $autosubmit which doesn't really make sense as argument 5 should be an optional array of attributes to give the select element. I think $autosubmit just needs to be removed by the looks.
        3. locallib.php
          • tool_assignmentupgrade_upgrade_assignment phpdocs need to be fixed up.
          • unused $upgrades variable.
        4. batchupgrade.php
          • In general calling set_time_limit(0) is a really bad idea. A better solution would be to add a set_time_limit call with a reasonable value within the while that is processing each assignment, so that before each assignment is upgraded the timeout is reset. This way if things do go wrong the script still can't run indefinitely.
          • The while loop can (and really should be improved) presently the count in the while condition is evil, the array is counted for every iteration despite it never changing as you're working on with a incremented variable for the current element. To be truthful it looks like this while could be written much more simply as a foreach, and the count (which also happens internally) separate out of the iteration into a variable once.
        5. renderer.php assignment_list_page phpdocs need updating

        The rest looks fine

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Damyon, Looks like these changes need rebasing on current master. lib.php has been editing in these changes but has been removed from master. I've looked over the rest of these changes and made the following notes. upgradableassignmentstable.php the $countfrom variable probably doesn't need to exist. Putting that SQL directly into the set_count_sql is probably going to be clearer. paginationform.php Argument 5 for the perpage select is $autosubmit which doesn't really make sense as argument 5 should be an optional array of attributes to give the select element. I think $autosubmit just needs to be removed by the looks. locallib.php tool_assignmentupgrade_upgrade_assignment phpdocs need to be fixed up. unused $upgrades variable. batchupgrade.php In general calling set_time_limit(0) is a really bad idea. A better solution would be to add a set_time_limit call with a reasonable value within the while that is processing each assignment, so that before each assignment is upgraded the timeout is reset. This way if things do go wrong the script still can't run indefinitely. The while loop can (and really should be improved) presently the count in the while condition is evil, the array is counted for every iteration despite it never changing as you're working on with a incremented variable for the current element. To be truthful it looks like this while could be written much more simply as a foreach, and the count (which also happens internally) separate out of the iteration into a variable once. renderer.php assignment_list_page phpdocs need updating The rest looks fine Cheers Sam
        Hide
        Damyon Wiese added a comment -

        Hi Sam,

        I have made changes for all of your comments. I have also rebased this on top of the latest weekly master.

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Hi Sam, I have made changes for all of your comments. I have also rebased this on top of the latest weekly master. Regards, Damyon
        Hide
        Dan Poltawski added a comment -

        ping.

        Show
        Dan Poltawski added a comment - ping.
        Hide
        Sam Hemelryk added a comment -

        Thanks for the ping Dan, everything looks good now thanks Damyon, putting this up for integration

        Show
        Sam Hemelryk added a comment - Thanks for the ping Dan, everything looks good now thanks Damyon, putting this up for integration
        Hide
        Dan Poltawski added a comment -

        BTW, its great that you have managed to test this on some real data

        Show
        Dan Poltawski added a comment - BTW, its great that you have managed to test this on some real data
        Hide
        Dan Poltawski added a comment -

        Thanks - i've integrated this now

        Show
        Dan Poltawski added a comment - Thanks - i've integrated this now
        Hide
        Frédéric Massart added a comment -

        A notice appears when updating all assignments.

        Notice: Undefined property: stdClass::$sendlatenotifications in /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php on line 422 
        

        Followed by this exception:

        Error writing to database
        
        Debug info: Column 'sendlatenotifications' cannot be null
        INSERT INTO mdl_assign (name,timemodified,course,intro,introformat,alwaysshowdescription,preventlatesubmissions,submissiondrafts,sendnotifications,sendlatenotifications,duedate,allowsubmissionsfromdate,grade) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?)
        [array (
        0 => 'Assignment 0',
        1 => 1337751381,
        2 => '18',
        3 => 'This assignment has been randomly generated by a very useful script, for the purpose of testing the boundaries of Moodle in various contexts. Moodle should be able to scale to any size without its speed and ease of use being affected dramatically.',
        4 => '0',
        5 => 1,
        6 => '0',
        7 => '0',
        8 => '0',
        9 => NULL,
        10 => '1427238041',
        11 => '0',
        12 => '62',
        )]
        Error code: dmlwriteexception
        Stack trace:
        
            line 416 of /lib/dml/moodle_database.php: dml_write_exception thrown
            line 952 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
            line 994 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
            line 426 of /mod/assign/locallib.php: call to mysqli_native_moodle_database->insert_record()
            line 95 of /mod/assign/upgradelib.php: call to assign->add_instance()
            line 190 of /admin/tool/assignmentupgrade/locallib.php: call to assign_upgrade_manager->upgrade_assignment()
            line 63 of /admin/tool/assignmentupgrade/batchupgrade.php: call to tool_assignmentupgrade_upgrade_assignment()
        
        Show
        Frédéric Massart added a comment - A notice appears when updating all assignments. Notice: Undefined property: stdClass::$sendlatenotifications in /home/fred/www/repositories/testing_master/moodle/mod/assign/locallib.php on line 422 Followed by this exception: Error writing to database Debug info: Column 'sendlatenotifications' cannot be null INSERT INTO mdl_assign (name,timemodified,course,intro,introformat,alwaysshowdescription,preventlatesubmissions,submissiondrafts,sendnotifications,sendlatenotifications,duedate,allowsubmissionsfromdate,grade) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?) [array ( 0 => 'Assignment 0', 1 => 1337751381, 2 => '18', 3 => 'This assignment has been randomly generated by a very useful script, for the purpose of testing the boundaries of Moodle in various contexts. Moodle should be able to scale to any size without its speed and ease of use being affected dramatically.', 4 => '0', 5 => 1, 6 => '0', 7 => '0', 8 => '0', 9 => NULL, 10 => '1427238041', 11 => '0', 12 => '62', )] Error code: dmlwriteexception Stack trace: line 416 of /lib/dml/moodle_database.php: dml_write_exception thrown line 952 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 994 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw() line 426 of /mod/assign/locallib.php: call to mysqli_native_moodle_database->insert_record() line 95 of /mod/assign/upgradelib.php: call to assign->add_instance() line 190 of /admin/tool/assignmentupgrade/locallib.php: call to assign_upgrade_manager->upgrade_assignment() line 63 of /admin/tool/assignmentupgrade/batchupgrade.php: call to tool_assignmentupgrade_upgrade_assignment()
        Hide
        Damyon Wiese added a comment -

        Hi Frederic,

        That error would not be triggered by this change - it would have been caused by MDL-31414 - I'll post a fix on that issue.

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Hi Frederic, That error would not be triggered by this change - it would have been caused by MDL-31414 - I'll post a fix on that issue. Regards, Damyon
        Hide
        Dan Poltawski added a comment -

        I've pulled in the fix for that. Thanks fred/damyon

        Show
        Dan Poltawski added a comment - I've pulled in the fix for that. Thanks fred/damyon
        Hide
        Frédéric Massart added a comment -

        Success on master!

        Show
        Frédéric Massart added a comment - Success on master!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

        Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: