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 Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38245

      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"
      1. lib.php
        0.1 kB
        Ankit Agarwal

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a solution.
          Hide
          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 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 Agarwal added a comment -

          Sending for peer-review
          Thanks

          Show
          Ankit Agarwal added a comment - Sending for peer-review Thanks
          Hide
          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
          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 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 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
          Rossiani Wijaya added a comment -

          The second patch looks good Ankit.

          Thanks.

          Show
          Rossiani Wijaya added a comment - The second patch looks good Ankit. Thanks.
          Hide
          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
          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
          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
          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 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 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
          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
          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
          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
          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 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 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
          Sam Hemelryk added a comment -

          Sorry forgot to move it into integration

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

          Thanks guys, this has been integrated now.

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

          Thanks Ankit,

          Works Great

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

          Show
          Rajesh Taneja added a comment - Thanks Ankit, Works Great FYI: Tested on Mysql, oracle, Mssql and postgres.
          Hide
          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
          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: