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

          Attachments

            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