Details

    • Rank:
      39174

      Description

      In order to support the calendar event hooks

      http://docs.moodle.org/dev/Calendar_API#Event_hook

      it looks like there are about a dozen spots where core files need to change
      from accessing the calendar database tables directly to using the calendar
      API instead.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Brian.

          Thanks for reporting that.

          Any help with fixing that would be appreciated. Even if you could list the places where changes need to be made, that would be useful.

          Show
          Michael de Raadt added a comment - Hi, Brian. Thanks for reporting that. Any help with fixing that would be appreciated. Even if you could list the places where changes need to be made, that would be useful.
          Hide
          Brian Jorgensen added a comment -

          While porting our 2.1.3 Calendar API fixes to the two stable branches and master, I came upon a question that I would appreciate input on: should restoring an event use the Calendar API, and thereby cause calendar event_hooks to fire, or should restore access the db tables directly to avoid hooks being fired?

          When restoring a course from a previous semester, and then modifying assignment dates etc for this semester, the new events from the restore would trigger add_event with the "wrong" (old/previous) date and then when those assignments are updated for the new semester, the update_event would provide the correct date.

          Perhaps this is not really a special case at all since it is basically the same as creating an assignment with the wrong date and then fixing the date, which I would certainly expect to call the add_event and update_event hooks.

          Thoughts?

          Show
          Brian Jorgensen added a comment - While porting our 2.1.3 Calendar API fixes to the two stable branches and master, I came upon a question that I would appreciate input on: should restoring an event use the Calendar API, and thereby cause calendar event_hooks to fire, or should restore access the db tables directly to avoid hooks being fired? When restoring a course from a previous semester, and then modifying assignment dates etc for this semester, the new events from the restore would trigger add_event with the "wrong" (old/previous) date and then when those assignments are updated for the new semester, the update_event would provide the correct date. Perhaps this is not really a special case at all since it is basically the same as creating an assignment with the wrong date and then fixing the date, which I would certainly expect to call the add_event and update_event hooks. Thoughts?
          Hide
          Rajesh Taneja added a comment -

          Thanks for spot-on patch Brian,

          Pushing it for peer-review

          Show
          Rajesh Taneja added a comment - Thanks for spot-on patch Brian, Pushing it for peer-review
          Hide
          Andrew Davis added a comment -

          It doesn't look like theres any way to delete multiple events through the API. Maybe we should add one.

          We currently use 1 query.
          $DB->delete_records('event', array('courseid'=>$data->courseid));

          Now it looks like we'll be using 1 + 2n queries, n = the number of events. 1 query to retrieve all the events, 1 to load each event individually and 1 to delete each event individually.

          $events = $DB->get_records('event', array('courseid'=>$data->courseid));
          foreach ($events as $event) {
              $cal = calendar_event::load($event);
              $cal->delete(false);
          }
          

          It would be better if we could come up with a way to ensure that the event hooks fire but which doesn't impose an obvious performance hit. Something like calendar_event::delete($conditions) that can be made to work with all of the cases that appear in the current diff.

          Show
          Andrew Davis added a comment - It doesn't look like theres any way to delete multiple events through the API. Maybe we should add one. We currently use 1 query. $DB->delete_records('event', array('courseid'=>$data->courseid)); Now it looks like we'll be using 1 + 2n queries, n = the number of events. 1 query to retrieve all the events, 1 to load each event individually and 1 to delete each event individually. $events = $DB->get_records('event', array('courseid'=>$data->courseid)); foreach ($events as $event) { $cal = calendar_event::load($event); $cal->delete( false ); } It would be better if we could come up with a way to ensure that the event hooks fire but which doesn't impose an obvious performance hit. Something like calendar_event::delete($conditions) that can be made to work with all of the cases that appear in the current diff.
          Hide
          Brian Jorgensen added a comment -

          Thanks, Andrew.

          Should I create another ticket for the new multi-delete method or leave it under this ticket?

          And does the "Development in progress" status mean that an HQ developer is taking on that work? If not, I will take it on.

          Show
          Brian Jorgensen added a comment - Thanks, Andrew. Should I create another ticket for the new multi-delete method or leave it under this ticket? And does the "Development in progress" status mean that an HQ developer is taking on that work? If not, I will take it on.
          Hide
          Andrew Davis added a comment -

          Leaving it under this ticket is preferable unless theres a particularly pressing reason to do them separately. It would be nice to avoid putting in detrimental to performance.

          "development in progress" is the status it goes back to after peer review. Theoretically it means that the assignee will immediately return to it to act on the feedback from the peer review however that may or may not be accurate.

          Show
          Andrew Davis added a comment - Leaving it under this ticket is preferable unless theres a particularly pressing reason to do them separately. It would be nice to avoid putting in detrimental to performance. "development in progress" is the status it goes back to after peer review. Theoretically it means that the assignee will immediately return to it to act on the feedback from the peer review however that may or may not be accurate.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks Brian and Andrew,

          Brian please feel free to work on this, if you want. Else, I will try get this in next sprint and work on it

          Show
          Rajesh Taneja added a comment - - edited Thanks Brian and Andrew, Brian please feel free to work on this, if you want. Else, I will try get this in next sprint and work on it
          Hide
          Brian Jorgensen added a comment - - edited

          It looks fairly straightforward to flatten the db calls for all cases except deleting course group events in group/lib.php, so I have committed two new static methods to master on github: delete_events(), which achieves the desired flattening, and delete_course_group_events(), for the special case. Adding these static methods also requires that the calculate_context() method be made static.

          The main assumptions I have made are:

          1. selecting rows from mdl_events by courseid, groupid or userid will also return all "child" (repeat) events in that series; with the testing I've done, this looks like a reasonable assumption;

          2. deleting a group of parent events while keeping their children (and therefore having to elevate the "first" child) is not particularly useful at this time; this also seems reasonable based on the diffs we are working with.

          If this is heading in the right direction, I will backport it to 2.1.x and 2.2.x. Otherwise, comments and advice welcome!

          Show
          Brian Jorgensen added a comment - - edited It looks fairly straightforward to flatten the db calls for all cases except deleting course group events in group/lib.php, so I have committed two new static methods to master on github: delete_events(), which achieves the desired flattening, and delete_course_group_events(), for the special case. Adding these static methods also requires that the calculate_context() method be made static. The main assumptions I have made are: 1. selecting rows from mdl_events by courseid, groupid or userid will also return all "child" (repeat) events in that series; with the testing I've done, this looks like a reasonable assumption; 2. deleting a group of parent events while keeping their children (and therefore having to elevate the "first" child) is not particularly useful at this time; this also seems reasonable based on the diffs we are working with. If this is heading in the right direction, I will backport it to 2.1.x and 2.2.x. Otherwise, comments and advice welcome!
          Hide
          Rajesh Taneja added a comment -

          Thanks for working on this Brian,

          I gave it a quick glimpse and it seems we never used to delete event files https://github.com/ualberta-eclass/moodle/commit/f3b862014ff3ce51a5696829b84feb971916de49#L1R2349
          So, not sure if can be backported.
          I will go though the calendar code to see why these files were not handled, the way you are doing now and will get back to you.

          FYI:
          Comments are not inline with moodle coding style. Comment should start with capital letter and end with dot(.) http://docs.moodle.org/dev/Coding_style#Inline_comments

          Show
          Rajesh Taneja added a comment - Thanks for working on this Brian, I gave it a quick glimpse and it seems we never used to delete event files https://github.com/ualberta-eclass/moodle/commit/f3b862014ff3ce51a5696829b84feb971916de49#L1R2349 So, not sure if can be backported. I will go though the calendar code to see why these files were not handled, the way you are doing now and will get back to you. FYI: Comments are not inline with moodle coding style. Comment should start with capital letter and end with dot(.) http://docs.moodle.org/dev/Coding_style#Inline_comments

            People

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

              Dates

              • Created:
                Updated: