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
    • Rank:
      39855

      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.

        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: