Moodle
  1. Moodle
  2. MDL-32827

Fix calendar events with empty "eventtype" field during upgrade.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide
      • This needs to be tested on all supported databases
      • This needs to be tested on all patched branches
      • The testing needs, changes both before and after the patch is applied.

      Testing instructions

      1. Create calender event of type user, group, site, due(Assignment) on a site without the patch
        • site events (sitepages->calendar->new event->type>site)
        • user events (sitepages->calendar->new event->type->user)
        • group events(sitepages->calendar->new event->type->group) (you must have multiple groups)
        • due events (Create an assign module with a future due date)
        • due events (Create an assignment module with a future due date)
      2. Goto database and make a note of id and their associated "Eventtype" in the table "Event"
      3. Delete the values in the column 'eventtype'
      4. Apply the patch
      5. it should prompt you for an upgrade, run the upgrade.
      6. Goto database again and make sure all events have "Eventtype" as noted on step 2
      7. visit the calendar and make sure no errors are shown
      8. Repeat with atleast one more database
        TEST 2
      9. Backup a course with calendar event
      10. check the box "include calendar events" while backing up
      11. open the backup file and edit the calendar.xml to remove event type
      12. restore the new backup file
      13. make sure the new course has calendar events with appropriate event types
      Show
      This needs to be tested on all supported databases This needs to be tested on all patched branches The testing needs, changes both before and after the patch is applied. Testing instructions Create calender event of type user, group, site, due(Assignment) on a site without the patch site events (sitepages->calendar->new event->type>site) user events (sitepages->calendar->new event->type->user) group events(sitepages->calendar->new event->type->group) (you must have multiple groups) due events (Create an assign module with a future due date) due events (Create an assignment module with a future due date) Goto database and make a note of id and their associated "Eventtype" in the table "Event" Delete the values in the column 'eventtype' Apply the patch it should prompt you for an upgrade, run the upgrade. Goto database again and make sure all events have "Eventtype" as noted on step 2 visit the calendar and make sure no errors are shown Repeat with atleast one more database TEST 2 Backup a course with calendar event check the box "include calendar events" while backing up open the backup file and edit the calendar.xml to remove event type restore the new backup file make sure the new course has calendar events with appropriate event types
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-32827-master

      Description

      Due to MDL-32826 there can be events without any "Eventtype" value. This should be fixed during upgrade based on the values of courseid and userid.
      Please refer the linked issues to understand the problems it is creating.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Ankit Agarwal added a comment -

            Requesting review.
            Will put up the stable branches once review is done.
            Thanks

            Show
            Ankit Agarwal added a comment - Requesting review. Will put up the stable branches once review is done. Thanks
            Hide
            Rossiani Wijaya added a comment -

            patch looks good ankit.

            just removed some of the spaces for the comment on line 1050.

            Show
            Rossiani Wijaya added a comment - patch looks good ankit. just removed some of the spaces for the comment on line 1050.
            Hide
            Ankit Agarwal added a comment -

            Thanks Rosie for the review,
            Sending this for integration.

            Show
            Ankit Agarwal added a comment - Thanks Rosie for the review, Sending this for integration.
            Hide
            Sam Hemelryk added a comment -

            Hi Ankit,

            Sending this back presently sorry just so a couple of things can be looked at.

            1. Backup and restore Moodle 2+
            I see for course + group events we will have an eventtype, however for module events we're not guaranteed to have one, and as you are explicitly looking for assignment events without an event type I imagine they may end up on a backup.
            As such I think certainly there needs to be some catch/logic within restore code for events without an event type.

            2. Restore of Moodle 1.9.
            Can you tell me do calendar events get converted by the moodle1 converter before being restored, or are they lost?
            Obviously if they are being converted and then restored then certainly we either need to add a catch/logic in the converter or make sure the logic being added to the restore code is robust enough to handle anything thrown at it from a 1.9 restore.

            3. Is there a check now to make sure eventtype is != ''?

            All points above really relate to the fact that we don't want to have to keep running upgrade code to fix these things. We need to ensure that we deal with them in every aspect.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit, Sending this back presently sorry just so a couple of things can be looked at. 1. Backup and restore Moodle 2+ I see for course + group events we will have an eventtype, however for module events we're not guaranteed to have one, and as you are explicitly looking for assignment events without an event type I imagine they may end up on a backup. As such I think certainly there needs to be some catch/logic within restore code for events without an event type. 2. Restore of Moodle 1.9. Can you tell me do calendar events get converted by the moodle1 converter before being restored, or are they lost? Obviously if they are being converted and then restored then certainly we either need to add a catch/logic in the converter or make sure the logic being added to the restore code is robust enough to handle anything thrown at it from a 1.9 restore. 3. Is there a check now to make sure eventtype is != ''? All points above really relate to the fact that we don't want to have to keep running upgrade code to fix these things. We need to ensure that we deal with them in every aspect. Cheers Sam
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Ankit Agarwal added a comment -

            Hi Sam,
            Thanks for the feedback.
            1. It was decided as a part of MDL-22895, that events with no event types should be ignored by the restore controller. If you think this needs to be changed, I will file an improvment bug for that.
            2. I am not sure if am missing something, but I cannot see any calendar events in 1.9 backups.
            3. Yes the original issue as reported in MDL-32826, is fixed for MOODLE 2.0 +

            Thanks

            Show
            Ankit Agarwal added a comment - Hi Sam, Thanks for the feedback. 1. It was decided as a part of MDL-22895 , that events with no event types should be ignored by the restore controller. If you think this needs to be changed, I will file an improvment bug for that. 2. I am not sure if am missing something, but I cannot see any calendar events in 1.9 backups. 3. Yes the original issue as reported in MDL-32826 , is fixed for MOODLE 2.0 + Thanks
            Hide
            Aparup Banerjee added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Sam Hemelryk added a comment -

            Hi Ankit,

            Something occured to me right before I integrated this.
            We could really improve the upgrade code I think.
            I don't think it the select is necessary, we could easily just move the conditions to the and UPDATE statement.

            Looks like we'd need:

            UPDATE event SET eventtype = 'site' WHERE eventtype = '' AND courseid = $SITE->id
            UPDATE event SET eventtype = 'due' WHERE eventtype = '' AND courseid != 0 AND groupid == 0 AND (modulename = 'assignment' OR modulename = 'assign')
            UPDATE event SET eventtype = 'course' WHERE eventtype = '' AND courseid != 0 AND groupid = 0;
            UPDATE event SET eventtype = 'group' WHERE eventtype = '' AND groupid != 0;
            UPDATE event SET eventtype = 'user' WHERE eventtype = '' AND userid != 0;

            In which case you could do the following:

            $DB->set_field('event', 'eventtype', 'site', array('eventtype' => '', 'courseid' => $SITE->id));
            $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND courseid != 0 AND groupid == 0 AND (modulename = 'assignment' OR modulename = 'assign')");
            $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND courseid != 0 AND groupid = 0");
            $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND groupid != 0");
            $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND userid != 0");
            

            If thats possible then certainly that is an improvement as on a large site with many activities you are probably talking hundreds of thousands of events if not millions. We'd be running a single update for each condition, rather than a update call for each event.

            Anyway could you please have a think about that.
            I'll leave this in integration presently, if you have time and agree feel free to make the change, test it, and ping me, if its good I'll integrate it.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Ankit, Something occured to me right before I integrated this. We could really improve the upgrade code I think. I don't think it the select is necessary, we could easily just move the conditions to the and UPDATE statement. Looks like we'd need: UPDATE event SET eventtype = 'site' WHERE eventtype = '' AND courseid = $SITE->id UPDATE event SET eventtype = 'due' WHERE eventtype = '' AND courseid != 0 AND groupid == 0 AND (modulename = 'assignment' OR modulename = 'assign') UPDATE event SET eventtype = 'course' WHERE eventtype = '' AND courseid != 0 AND groupid = 0; UPDATE event SET eventtype = 'group' WHERE eventtype = '' AND groupid != 0; UPDATE event SET eventtype = 'user' WHERE eventtype = '' AND userid != 0; In which case you could do the following: $DB->set_field('event', 'eventtype', 'site', array('eventtype' => '', 'courseid' => $SITE->id)); $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND courseid != 0 AND groupid == 0 AND (modulename = 'assignment' OR modulename = 'assign')"); $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND courseid != 0 AND groupid = 0"); $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND groupid != 0"); $DB->set_field_select('event', 'eventtype', 'due', "eventtype = '' AND userid != 0"); If thats possible then certainly that is an improvement as on a large site with many activities you are probably talking hundreds of thousands of events if not millions. We'd be running a single update for each condition, rather than a update call for each event. Anyway could you please have a think about that. I'll leave this in integration presently, if you have time and agree feel free to make the change, test it, and ping me, if its good I'll integrate it. Cheers Sam
            Hide
            Ankit Agarwal added a comment -

            Hi Sam,
            Your suggestions make sense.
            I have made the changes accordingly and tested on postgres and mysql. Everything is working fine.
            Have a look and let me know if anything needs further polishing.
            Thanks

            Show
            Ankit Agarwal added a comment - Hi Sam, Your suggestions make sense. I have made the changes accordingly and tested on postgres and mysql. Everything is working fine. Have a look and let me know if anything needs further polishing. Thanks
            Hide
            Sam Hemelryk added a comment -

            Thanks Ankit, I've integrated this now

            Show
            Sam Hemelryk added a comment - Thanks Ankit, I've integrated this now
            Hide
            Frédéric Massart added a comment -

            Hi guys,

            just had this notice while updating my 2.2.

            PHP Notice:  Undefined variable: SITE in /home/fred/www/repositories/testing_22/moodle/lib/db/upgrade.php on line 7142
            PHP Stack trace:
            PHP   1. {main}() /home/fred/www/repositories/testing_22/moodle/admin/cli/upgrade.php:0
            PHP   2. upgrade_core() /home/fred/www/repositories/testing_22/moodle/admin/cli/upgrade.php:150
            PHP   3. xmldb_main_upgrade() /home/fred/www/repositories/testing_22/moodle/lib/upgradelib.php:1394
            

            Show
            Frédéric Massart added a comment - Hi guys, just had this notice while updating my 2.2. PHP Notice: Undefined variable: SITE in /home/fred/www/repositories/testing_22/moodle/lib/db/upgrade.php on line 7142 PHP Stack trace: PHP 1. {main}() /home/fred/www/repositories/testing_22/moodle/admin/cli/upgrade.php:0 PHP 2. upgrade_core() /home/fred/www/repositories/testing_22/moodle/admin/cli/upgrade.php:150 PHP 3. xmldb_main_upgrade() /home/fred/www/repositories/testing_22/moodle/lib/upgradelib.php:1394
            Hide
            Ankit Agarwal added a comment -

            Thanks Fred for noticing that.
            @sam
            I have made one more commit on 22 , to include the missing global. Can you please pick it up?
            Thanks

            Show
            Ankit Agarwal added a comment - Thanks Fred for noticing that. @sam I have made one more commit on 22 , to include the missing global. Can you please pick it up? Thanks
            Hide
            Sam Hemelryk added a comment -

            Aha thanks for spotting and posting. I've cheery-picked your commit now thanks Ankit.

            Ready for testing once more.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Aha thanks for spotting and posting. I've cheery-picked your commit now thanks Ankit. Ready for testing once more. Cheers Sam
            Hide
            Michael de Raadt added a comment -

            Test result: Success

            I tested this on a continuous upgrade from 1.9 to 2.2 then 2.3 and master. At each stage (after 1.9) I remove the eventtype fields and they were re-added.

            I also tested this by downgrading the version on PostgreSQL, MS SQL and MySQL in master. The upgrade re-added removed event types. I wasn't able to test this in Oracle as it wouldn't let me add empty values for the eventtype field.

            I also tested this by manipulating a course backup. I added the eventtype field for some modified and introduced calendar events.

            Show
            Michael de Raadt added a comment - Test result: Success I tested this on a continuous upgrade from 1.9 to 2.2 then 2.3 and master. At each stage (after 1.9) I remove the eventtype fields and they were re-added. I also tested this by downgrading the version on PostgreSQL, MS SQL and MySQL in master. The upgrade re-added removed event types. I wasn't able to test this in Oracle as it wouldn't let me add empty values for the eventtype field. I also tested this by manipulating a course backup. I added the eventtype field for some modified and introduced calendar events.
            Hide
            Dan Poltawski added a comment -

            asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

            Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

            Show
            Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: