Details

      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.

        Gliffy Diagrams

          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
            Hide
            Rajesh Taneja added a comment -

            I have unassigned myself from this issue, as I won't be able to get to this soon.

            Show
            Rajesh Taneja added a comment - I have unassigned myself from this issue, as I won't be able to get to this soon.

              People

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

                Dates

                • Created:
                  Updated: