Skip to content

Commit

Permalink
MDL-60958 calendar: stop db query in repeat collection constructor
Browse files Browse the repository at this point in the history 
Stop loading the parent record in the constructor because it is an
unnecessary DB query for each event instantiation.

Return null on get_repeats() for events that don't have repeats.
  • Loading branch information
@ryanwyllie
ryanwyllie committed Feb 12, 2018
1 parent 315a0a3 commit 2885299
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 65 deletions.
2 changes: 1 addition & 1 deletion calendar/classes/local/event/entities/event.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public function __construct(
proxy_interface $course = null,
proxy_interface $group = null,
proxy_interface $user = null,
event_collection_interface $repeats,
event_collection_interface $repeats = null,
proxy_interface $coursemodule = null,
$type,
times_interface $times,
Expand Down
5 changes: 3 additions & 2 deletions calendar/classes/local/event/entities/event_interface.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,10 @@ public function get_type();
public function get_times();

/**
* Get repeats of this event.
* Get repeats of this event or null if the event has no
* repeats.
*
* @return event_collection_interface
* @return event_collection_interface|null
*/
public function get_repeats();

Expand Down
32 changes: 20 additions & 12 deletions calendar/classes/local/event/entities/repeat_event_collection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
defined('MOODLE_INTERNAL') || die();

use core_calendar\local\event\factories\event_factory_interface;
use core_calendar\local\event\exceptions\no_repeat_parent_exception;

/**
* Class representing a collection of repeat events.
Expand Down Expand Up @@ -64,18 +63,27 @@ class repeat_event_collection implements event_collection_interface {
/**
* Constructor.
*
* @param int $parentid ID of the parent event.
* @param int $repeatid If non-zero this will be used as the parent id.
* @param stdClass $dbrow The event dbrow that is being repeated.
* @param event_factory_interface $factory Event factory.
* @throws no_repeat_parent_exception If the parent record can't be loaded.
*/
public function __construct($parentid, $repeatid, event_factory_interface $factory) {
$this->parentid = $repeatid ? $repeatid : $parentid;
$this->factory = $factory;
public function __construct($dbrow, event_factory_interface $factory) {
$eventid = $dbrow->id;
$repeatid = $dbrow->repeatid;

if (empty($repeatid)) {
$this->parentrecord = $dbrow;
$this->parentid = $eventid;
} else {
$this->parentid = $repeatid;
}

if (!$this->get_parent_record()) {
throw new no_repeat_parent_exception(sprintf('No record found for id %d', $parentid));
if ($eventid === $repeatid) {
// This means the record we've been given is the parent
// record.
$this->parentrecord = $dbrow;
}

$this->factory = $factory;
}

public function get_id() {
Expand Down Expand Up @@ -109,11 +117,11 @@ public function getIterator() {
protected function get_parent_record() {
global $DB;

if (isset($this->parentrecord)) {
return $this->parentrecord;
if (!isset($this->parentrecord)) {
$this->parentrecord = $DB->get_record('event', ['id' => $this->parentid]);
}

return $DB->get_record('event', ['id' => $this->parentid]);
return $this->parentrecord;
}

/**
Expand Down
36 changes: 0 additions & 36 deletions calendar/classes/local/event/exceptions/no_repeat_parent_exception.php
View file

This file was deleted.

8 changes: 7 additions & 1 deletion calendar/classes/local/event/factories/event_abstract_factory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,12 @@ public function create_instance(\stdClass $dbrow) {
});
}

if (!empty($dbrow->repeatid)) {
$repeatcollection = new repeat_event_collection($dbrow, $this);
} else {
$repeatcollection = null;
}

$event = new event(
$dbrow->id,
$dbrow->name,
Expand All @@ -173,7 +179,7 @@ public function create_instance(\stdClass $dbrow) {
$course,
$group,
$user,
new repeat_event_collection($dbrow->id, $dbrow->repeatid, $this),
$repeatcollection,
$module,
$dbrow->eventtype,
new event_times(
Expand Down
3 changes: 2 additions & 1 deletion calendar/classes/local/event/mappers/event_mapper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public function from_event_to_legacy_event(event_interface $event) {
$properties->userid = empty($properties->userid) ? 0 : $properties->userid;
$properties->modulename = empty($properties->modulename) ? 0 : $properties->modulename;
$properties->instance = empty($properties->instance) ? 0 : $properties->instance;
$properties->repeatid = empty($properties->repeatid) ? 0 : $properties->repeatid;

return new \calendar_event($properties);
}
Expand All @@ -122,7 +123,7 @@ public function from_event_to_assoc_array(event_interface $event) {
'categoryid' => $event->get_category() ? $event->get_category()->get('id') : null,
'groupid' => $event->get_group() ? $event->get_group()->get('id') : null,
'userid' => $event->get_user() ? $event->get_user()->get('id') : null,
'repeatid' => $event->get_repeats()->get_id(),
'repeatid' => $event->get_repeats() ? $event->get_repeats()->get_id() : null,
'modulename' => $event->get_course_module() ? $event->get_course_module()->get('modname') : null,
'instance' => $event->get_course_module() ? $event->get_course_module()->get('instance') : null,
'eventtype' => $event->get_type(),
Expand Down
2 changes: 1 addition & 1 deletion calendar/tests/container_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public function test_event_factory_create_instance($dbrow) {
}

$this->assertEquals($dbrow->userid, $event->get_user()->get('id'));
$this->assertEquals($legacyevent->id, $event->get_repeats()->get_id());
$this->assertEquals(null, $event->get_repeats());
$this->assertEquals($dbrow->modulename, $event->get_course_module()->get('modname'));
$this->assertEquals($dbrow->instance, $event->get_course_module()->get('instance'));
$this->assertEquals($dbrow->timestart, $event->get_times()->get_start_time()->getTimestamp());
Expand Down
49 changes: 38 additions & 11 deletions calendar/tests/repeat_event_collection_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,35 @@
*/
class core_calendar_repeat_event_collection_testcase extends advanced_testcase {
/**
* Test that creating a repeat collection for a parent that doesn't
* exist throws an exception.
* Test that the collection id is set to the parent id if the repeat id
* is falsey.
*/
public function test_no_parent_collection() {
public function test_parent_id_no_repeat_id() {
$this->resetAfterTest(true);
$parentid = 123122131;
$dbrow = (object) [
'id' => 123122131,
'repeatid' => null
];
$factory = new core_calendar_repeat_event_collection_event_test_factory();
$this->expectException('\core_calendar\local\event\exceptions\no_repeat_parent_exception');
$collection = new repeat_event_collection($parentid, null, $factory);
$collection = new repeat_event_collection($dbrow, $factory);

$this->assertEquals($dbrow->id, $collection->get_id());
}

/**
* Test that the repeat id is set to the parent id if the repeat id
* is not falsey (even if the parent id is provided).
*/
public function test_parent_id_and_repeat_id() {
$this->resetAfterTest(true);
$dbrow = (object) [
'id' => 123122131,
'repeatid' => 5647839
];
$factory = new core_calendar_repeat_event_collection_event_test_factory();
$collection = new repeat_event_collection($dbrow, $factory);

$this->assertEquals($dbrow->repeatid, $collection->get_id());
}

/**
Expand All @@ -68,13 +88,16 @@ public function test_empty_collection() {
'repeat' => 1,
'repeats' => 0
]);
$parentid = $event->id;
$dbrow = (object) [
'id' => $event->id,
'repeatid' => null
];
$factory = new core_calendar_repeat_event_collection_event_test_factory();

// Event collection with no repeats.
$collection = new repeat_event_collection($parentid, null, $factory);
$collection = new repeat_event_collection($dbrow, $factory);

$this->assertEquals($parentid, $collection->get_id());
$this->assertEquals($event->id, $collection->get_id());
$this->assertEquals(0, $collection->get_num());
$this->assertNull($collection->getIterator()->next());
}
Expand All @@ -94,6 +117,10 @@ public function test_values_collection() {
'repeats' => 0
]);
$parentid = $event->id;
$dbrow = (object) [
'id' => $parentid,
'repeatid' => null
];
$repeats = [];

for ($i = 1; $i < 4; $i++) {
Expand All @@ -108,7 +135,7 @@ public function test_values_collection() {
}

// Event collection with no repeats.
$collection = new repeat_event_collection($parentid, null, $factory);
$collection = new repeat_event_collection($dbrow, $factory);

$this->assertEquals($parentid, $collection->get_id());
$this->assertEquals(count($repeats), $collection->get_num());
Expand Down Expand Up @@ -167,7 +194,7 @@ public function create_instance(\stdClass $dbrow) {
new std_proxy($dbrow->courseid, $identity),
new std_proxy($dbrow->groupid, $identity),
new std_proxy($dbrow->userid, $identity),
new repeat_event_collection($dbrow->id, null, $this),
$dbrow->repeatid ? new repeat_event_collection($dbrow, $this) : null,
new std_proxy($dbrow->instance, $identity),
$dbrow->type,
new event_times(
Expand Down

0 comments on commit 2885299

Please sign in to comment.