Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31671

Unable to delete all events in a recurring series after the first event from that recurring series had already been deleted.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      Test1

      1. Add a repeated event to calendar (Site pages>calendar>new event)
      2. Goto the day in which the first event of the series appears
      3. Click on delete for the first event
      4. Make sure you are able to delete the event without any issue
      5. Goto the database table events and make sure the "repeatid" column for rest of the entries of the series points to next event of the series.
      6. Now visit any other event of the series and try deleting it. Make sure you can delete it without any issue.
      7. Try deleting the complete series and make sure you dont get any errors and events are deleted.
      8. Test it in more than one database.
        Test2
      9. add following line to your config.php after the line in which $CFG->dirroot is defined

        $CFG->calendar = 'cal';;

      10. create a folder 'cal' in your calendar dir.
      11. copy the attached file to the just created folder.
      12. follow step 1-3 above
      13. make sure the script dies echoing the id of the next event in the series (next to the event just deleted)

      Note:- you might notice other un-related errors. See linked issues for further details.

      Show
      Test1 Add a repeated event to calendar (Site pages>calendar>new event) Goto the day in which the first event of the series appears Click on delete for the first event Make sure you are able to delete the event without any issue Goto the database table events and make sure the "repeatid" column for rest of the entries of the series points to next event of the series. Now visit any other event of the series and try deleting it. Make sure you can delete it without any issue. Try deleting the complete series and make sure you dont get any errors and events are deleted. Test it in more than one database. Test2 add following line to your config.php after the line in which $CFG->dirroot is defined $CFG->calendar = 'cal';; create a folder 'cal' in your calendar dir. copy the attached file to the just created folder. follow step 1-3 above make sure the script dies echoing the id of the next event in the series (next to the event just deleted) Note:- you might notice other un-related errors. See linked issues for further details.
    • Workaround:
      Hide

      Moodle should check if the first recurring event exists before providing the option "Delete all" to the user.

      By modifying:

      file calendar/lib.php, function count_repeats()

      ...
          /**
           * Return the number of repeat events there are in this events series
           *
           * @return int
           */
          public function count_repeats() {
              global $DB;
              if (!empty($this->properties->repeatid)) {
       
                  // verify if the event exists
                  $event = $DB->get_record('event', array('id' => $this->properties->repeatid));
                  if($event) {
       
                      $this->properties->eventrepeats = $DB->count_records('event', array('repeatid'=>$this->properties->repeatid));
                      // We don't want to count ourselves
                      $this->properties->eventrepeats--;
       
                  }
              }
              return $this->properties->eventrepeats;
          }

      OR
      calendar/delete.php, the section that check repeated events

      // If there are repeated events then add a Delete Repeated button
      $repeatspan = '';
      if (!empty($event->eventrepeats) && $event->eventrepeats > 0) {
       
       
          // verify if the event exists
          $repeatedevent = $DB->get_record('event', array('id' => $event->repeatid));
          if($repeatedevent) {
       
              $url = new moodle_url(CALENDAR_URL.'delete.php', array('id'=>$event->repeatid, 'confirm'=>true, 'repeats'=>true));
              $buttons .= $OUTPUT->single_button($url, get_string('deleteall'));
              $repeatspan = '<br /><br /><span>'.get_string('youcandeleteallrepeats', 'calendar').'</span>';
       
          }
      }

      Show
      Moodle should check if the first recurring event exists before providing the option "Delete all" to the user. By modifying: file calendar/lib.php, function count_repeats() ... /** * Return the number of repeat events there are in this events series * * @return int */ public function count_repeats() { global $DB; if (!empty($this->properties->repeatid)) {   // verify if the event exists $event = $DB->get_record('event', array('id' => $this->properties->repeatid)); if($event) {   $this->properties->eventrepeats = $DB->count_records('event', array('repeatid'=>$this->properties->repeatid)); // We don't want to count ourselves $this->properties->eventrepeats--;   } } return $this->properties->eventrepeats; } OR calendar/delete.php, the section that check repeated events // If there are repeated events then add a Delete Repeated button $repeatspan = ''; if (!empty($event->eventrepeats) && $event->eventrepeats > 0) {     // verify if the event exists $repeatedevent = $DB->get_record('event', array('id' => $event->repeatid)); if($repeatedevent) {   $url = new moodle_url(CALENDAR_URL.'delete.php', array('id'=>$event->repeatid, 'confirm'=>true, 'repeats'=>true)); $buttons .= $OUTPUT->single_button($url, get_string('deleteall')); $repeatspan = '<br /><br /><span>'.get_string('youcandeleteallrepeats', 'calendar').'</span>';   } }
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-31671-master2

      Description

      Deleted single event from a recurring series, first event, then attempted to 'Delete all' from the same recurring series and received error message "cannot find data record in database table event'.Unable to delete this series of events using 'Delete all' function.

      Moodle should not display "Delete all" when the first event is deleted to avoid this error.

      Replication steps:

      1. Create a recurring event, where "Repeat weekly, creating altogether" should have 3 or more
      2. Select first recurring event from calendar
      3. Click 'Delete' icon from event listing
      4. Delete event screen is displayed
      5. Click 'delete' button to delete single event.
      6. Screen displays all events from the recurring series except the first deleted event
      7. Select next recurring event from calendar
      8. Click 'Delete' icon from event listing
      9. Delete event screen is displayed with the option to "Delete All"
      10. Click 'Delete all' button to delete all recurring events
      11. Screen displays error "cannot find data record in database table event"

        Gliffy Diagrams

        1. lib.php
          0.1 kB
          Ankit Agarwal

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for spotting that and providing a solution.

            Show
            salvetore Michael de Raadt added a comment - Thanks for spotting that and providing a solution.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Martha,
            IMHO, it makes more sense if we do not allow deleting of the first event of a repeated series.
            That is do not display the "delete" button for the first element.(delete all will be shown)
            This way we do not end up with events having a parentid (repeatid) which is non-existent
            And all existing orphan entries can be cleaned up once MDL-31571 is implemented

            Will try to get some more feedback on this.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Martha, IMHO, it makes more sense if we do not allow deleting of the first event of a repeated series. That is do not display the "delete" button for the first element.(delete all will be shown) This way we do not end up with events having a parentid (repeatid) which is non-existent And all existing orphan entries can be cleaned up once MDL-31571 is implemented Will try to get some more feedback on this. Thanks
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Sending for peer-review
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Sending for peer-review Thanks
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Ankit,

            Patch looks good, except for line 105 for printing out $event->id and $event->repeatid.

            Other solutions regarding the fix:
            1. reset the repeatid to the next event occurrence instead of prohibiting it to delete the first event.
            2. if the first event is deleted, set the rest of the event to stand-alone (setting repeatid to 0)

            My preference is for #1.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Ankit, Patch looks good, except for line 105 for printing out $event->id and $event->repeatid. Other solutions regarding the fix: 1. reset the repeatid to the next event occurrence instead of prohibiting it to delete the first event. 2. if the first event is deleted, set the rest of the event to stand-alone (setting repeatid to 0) My preference is for #1.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Rosie for the review.
            Here is another solution as you suggested (#1)
            https://github.com/ankitagarwal/moodle/compare/MDL-31671-master2

            Both solutions looks good to me.
            I will let integrator choose the most appropriate one.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Rosie for the review. Here is another solution as you suggested (#1) https://github.com/ankitagarwal/moodle/compare/MDL-31671-master2 Both solutions looks good to me. I will let integrator choose the most appropriate one. Thanks
            Hide
            rwijaya Rossiani Wijaya added a comment -

            The second patch looks good Ankit.

            Thanks.

            Show
            rwijaya Rossiani Wijaya added a comment - The second patch looks good Ankit. Thanks.
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            samhemelryk Sam Hemelryk added a comment -

            Hi Ankit,

            I was about to comment on this issue suggesting altering the repeatid would be a better option when I found you had created MDL-31671-master2.
            I do think that is the better option and will be the branch I look to integrate.

            Before I do however this appears to be a bug and thus requires fixing on the stable branches as well.
            There's no separate branches, no comments about cherry-picking, and nothing to suggest this has been looked at.
            Could you please confirm the branches this needs to be applied to and that they have all been tested.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Ankit, I was about to comment on this issue suggesting altering the repeatid would be a better option when I found you had created MDL-31671 -master2. I do think that is the better option and will be the branch I look to integrate. Before I do however this appears to be a bug and thus requires fixing on the stable branches as well. There's no separate branches, no comments about cherry-picking, and nothing to suggest this has been looked at. Could you please confirm the branches this needs to be applied to and that they have all been tested. Cheers Sam
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Sam,
            I didnt create branches since I was not sure which solution would be picked up.
            Anyways created the branches now.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Sam, I didnt create branches since I was not sure which solution would be picked up. Anyways created the branches now. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hmm, started looking at this again.
            I've just realised that this new code isn't calling calling the calendar event hooks for updating an event.

            Sorry Ankit that needs to be addressed before this can get in.
            It may require a bit of research to work out how those hooks were meant to be operating, of track down and test I suppose.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hmm, started looking at this again. I've just realised that this new code isn't calling calling the calendar event hooks for updating an event. Sorry Ankit that needs to be addressed before this can get in. It may require a bit of research to work out how those hooks were meant to be operating, of track down and test I suppose. Cheers Sam
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Ankit,

            I think it would be better to use $event instead of $event=>id for update_event hook().

            other than that, the patch looks good.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Ankit, I think it would be better to use $event instead of $event=>id for update_event hook(). other than that, the patch looks good.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Rosie for the review,
            yeah $event would be consistent with the current code.
            Made changes sending for integration.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Rosie for the review, yeah $event would be consistent with the current code. Made changes sending for integration. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Sorry forgot to move it into integration

            Show
            samhemelryk Sam Hemelryk added a comment - Sorry forgot to move it into integration
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Ankit,

            Works Great

            FYI:
            Tested on Mysql, oracle, Mssql and postgres.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Ankit, Works Great FYI: Tested on Mysql, oracle, Mssql and postgres.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

            Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/12