From aa708b550e97fbdf1b4fd35c0bc3238de890a1d5 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 21 Apr 2017 12:39:53 +0800 Subject: [PATCH] MDL-58665 mod_feedback: avoid extra db queries on dashboard --- mod/feedback/classes/completion.php | 18 ++++++------- mod/feedback/classes/structure.php | 36 +++++++++++++++++++++----- mod/feedback/lib.php | 38 +++++++++++++++------------ mod/feedback/tests/lib_test.php | 40 +++++++++++++++++++++++++---- 4 files changed, 94 insertions(+), 38 deletions(-) diff --git a/mod/feedback/classes/completion.php b/mod/feedback/classes/completion.php index fd413c06703..36d8b434d70 100644 --- a/mod/feedback/classes/completion.php +++ b/mod/feedback/classes/completion.php @@ -53,9 +53,9 @@ class mod_feedback_completion extends mod_feedback_structure { /** * Constructor * - * @param stdClass $feedback feedback object, in case of the template - * this is the current feedback the template is accessed from + * @param stdClass $feedback feedback object * @param cm_info $cm course module object corresponding to the $feedback + * (at least one of $feedback or $cm is required) * @param int $courseid current course (for site feedbacks only) * @param bool $iscompleted has feedback been already completed? If yes either completedid or userid must be specified. * @param int $completedid id in the table feedback_completed, may be omitted if userid is specified @@ -66,17 +66,15 @@ class mod_feedback_completion extends mod_feedback_structure { */ public function __construct($feedback, $cm, $courseid, $iscompleted = false, $completedid = null, $userid = null) { global $DB; - // Make sure courseid is always set for site feedback and never for course feedback. - if ($feedback->course == SITEID) { - $courseid = $courseid ?: SITEID; - } else { - $courseid = 0; - } parent::__construct($feedback, $cm, $courseid, 0); + // Make sure courseid is always set for site feedback. + if ($this->feedback->course == SITEID && !$this->courseid) { + $this->courseid = SITEID; + } if ($iscompleted) { // Retrieve information about the completion. $this->iscompleted = true; - $params = array('feedback' => $feedback->id); + $params = array('feedback' => $this->feedback->id); if (!$userid && !$completedid) { throw new coding_exception('Either $completedid or $userid must be specified for completed feedbacks'); } @@ -713,7 +711,7 @@ class mod_feedback_completion extends mod_feedback_structure { $this->jumpto = $nextpage; } else { $this->save_response(); - if (!$this->feedback->page_after_submit) { + if (!$this->get_feedback()->page_after_submit) { \core\notification::success(get_string('entries_saved', 'feedback')); } $this->justcompleted = true; diff --git a/mod/feedback/classes/structure.php b/mod/feedback/classes/structure.php index 9c3597df316..2dedb67a0df 100644 --- a/mod/feedback/classes/structure.php +++ b/mod/feedback/classes/structure.php @@ -32,7 +32,10 @@ defined('MOODLE_INTERNAL') || die(); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class mod_feedback_structure { - /** @var stdClass */ + /** @var stdClass record from 'feedback' table. + * Reliably has fields: id, course, timeopen, timeclose, anonymous, completionsubmit. + * For full object or to access any other field use $this->get_feedback() + */ protected $feedback; /** @var cm_info */ protected $cm; @@ -50,15 +53,31 @@ class mod_feedback_structure { * * @param stdClass $feedback feedback object, in case of the template * this is the current feedback the template is accessed from - * @param cm_info $cm course module object corresponding to the $feedback + * @param stdClass|cm_info $cm course module object corresponding to the $feedback + * (at least one of $feedback or $cm is required) * @param int $courseid current course (for site feedbacks only) * @param int $templateid template id if this class represents the template structure */ public function __construct($feedback, $cm, $courseid = 0, $templateid = null) { - $this->feedback = $feedback; - $this->cm = $cm; - $this->courseid = ($feedback->course == SITEID) ? $courseid : 0; + if ((empty($feedback->id) || empty($feedback->course)) && (empty($cm->instance) || empty($cm->course))) { + throw new coding_exception('Either $feedback or $cm must be passed to constructor'); + } + $this->feedback = $feedback ?: (object)['id' => $cm->instance, 'course' => $cm->course]; + $this->cm = ($cm && $cm instanceof cm_info) ? $cm : + get_fast_modinfo($this->feedback->course)->instances['feedback'][$this->feedback->id]; $this->templateid = $templateid; + $this->courseid = ($this->feedback->course == SITEID) ? $courseid : 0; + + if (!$feedback) { + // If feedback object was not specified, populate object with fields required for the most of methods. + // These fields were added to course module cache in feedback_get_coursemodule_info(). + // Full instance record can be retrieved by calling mod_feedback_structure::get_feedback(). + $customdata = ($this->cm->customdata ?: []) + ['timeopen' => 0, 'timeclose' => 0, 'anonymous' => 0]; + $this->feedback->timeopen = $customdata['timeopen']; + $this->feedback->timeclose = $customdata['timeclose']; + $this->feedback->anonymous = $customdata['anonymous']; + $this->feedback->completionsubmit = empty($this->cm->customdata['customcompletionrules']['completionsubmit']) ? 0 : 1; + } } /** @@ -66,6 +85,11 @@ class mod_feedback_structure { * @return stdClass */ public function get_feedback() { + global $DB; + if (!isset($this->feedback->publish_stats) || !isset($this->feedback->name)) { + // Make sure the full object is retrieved. + $this->feedback = $DB->get_record('feedback', ['id' => $this->feedback->id], '*', MUST_EXIST); + } return $this->feedback; } @@ -182,7 +206,7 @@ class mod_feedback_structure { return true; } - if (intval($this->feedback->publish_stats) != 1 || + if (intval($this->get_feedback()->publish_stats) != 1 || !has_capability('mod/feedback:viewanalysepage', $context)) { return false; } diff --git a/mod/feedback/lib.php b/mod/feedback/lib.php index ede74ff5780..5fdca5a874a 100644 --- a/mod/feedback/lib.php +++ b/mod/feedback/lib.php @@ -3428,8 +3428,7 @@ function mod_feedback_core_calendar_is_event_visible(calendar_event $event) { global $DB; $cm = get_fast_modinfo($event->courseid)->instances['feedback'][$event->instance]; - $feedback = $DB->get_record('feedback', ['id' => $event->instance]); - $feedbackcompletion = new mod_feedback_completion($feedback, $cm, 0); + $feedbackcompletion = new mod_feedback_completion(null, $cm, 0); // The event is only visible if the user can submit it. return $feedbackcompletion->can_complete(); @@ -3447,26 +3446,21 @@ function mod_feedback_core_calendar_is_event_visible(calendar_event $event) { */ function mod_feedback_core_calendar_provide_event_action(calendar_event $event, \core_calendar\action_factory $factory) { - global $DB; $cm = get_fast_modinfo($event->courseid)->instances['feedback'][$event->instance]; - $feedback = $DB->get_record('feedback', ['id' => $event->instance]); - $feedbackcompletion = new mod_feedback_completion($feedback, $cm, 0); + $feedbackcompletion = new mod_feedback_completion(null, $cm, 0); - if ($feedbackcompletion->is_already_submitted()) { - // There is no action if the user has already submitted the feedback. + if (!empty($cm->customdata['timeclose']) && $cm->customdata['timeclose'] < time()) { + // Feedback is already closed, do not display it even if it was never submitted. return null; } - $now = time(); - if ($feedback->timeopen && $feedback->timeclose) { - $actionable = ($now >= $feedback->timeopen) && ($now <= $feedback->timeclose); - } else if ($feedback->timeclose) { - $actionable = $now < $feedback->timeclose; - } else if ($feedback->timeopen) { - $actionable = $now >= $feedback->timeopen; - } else { - $actionable = true; + // The feedback is actionable if it does not have timeopen or timeopen is in the past. + $actionable = $feedbackcompletion->is_open(); + + if ($actionable && $feedbackcompletion->is_already_submitted()) { + // There is no need to display anything if the user has already submitted the feedback. + return null; } return $factory->create_instance( @@ -3492,7 +3486,7 @@ function feedback_get_coursemodule_info($coursemodule) { global $DB; $dbparams = ['id' => $coursemodule->instance]; - $fields = 'id, name, intro, introformat, completionsubmit'; + $fields = 'id, name, intro, introformat, completionsubmit, timeopen, timeclose, anonymous'; if (!$feedback = $DB->get_record('feedback', $dbparams, $fields)) { return false; } @@ -3509,6 +3503,16 @@ function feedback_get_coursemodule_info($coursemodule) { if ($coursemodule->completion == COMPLETION_TRACKING_AUTOMATIC) { $result->customdata['customcompletionrules']['completionsubmit'] = $feedback->completionsubmit; } + // Populate some other values that can be used in calendar or on dashboard. + if ($feedback->timeopen) { + $result->customdata['timeopen'] = $feedback->timeopen; + } + if ($feedback->timeclose) { + $result->customdata['timeclose'] = $feedback->timeclose; + } + if ($feedback->anonymous) { + $result->customdata['anonymous'] = $feedback->anonymous; + } return $result; } diff --git a/mod/feedback/tests/lib_test.php b/mod/feedback/tests/lib_test.php index 90e83e3049d..99e024f61e6 100644 --- a/mod/feedback/tests/lib_test.php +++ b/mod/feedback/tests/lib_test.php @@ -32,6 +32,39 @@ require_once($CFG->dirroot . '/mod/feedback/lib.php'); */ class mod_feedback_lib_testcase extends advanced_testcase { + public function test_feedback_initialise() { + $this->resetAfterTest(); + $this->setAdminUser(); + + $course = $this->getDataGenerator()->create_course(); + $params['course'] = $course->id; + $params['timeopen'] = time() - 5 * MINSECS; + $params['timeclose'] = time() + DAYSECS; + $params['anonymous'] = 1; + $params['intro'] = 'Some introduction text'; + $feedback = $this->getDataGenerator()->create_module('feedback', $params); + + // Test different ways to construct the structure object. + $pseudocm = get_coursemodule_from_instance('feedback', $feedback->id); // Object similar to cm_info. + $cm = get_fast_modinfo($course)->instances['feedback'][$feedback->id]; // Instance of cm_info. + + $constructorparams = [ + [$feedback, null], + [null, $pseudocm], + [null, $cm], + [$feedback, $pseudocm], + [$feedback, $cm], + ]; + + foreach ($constructorparams as $params) { + $structure = new mod_feedback_completion($params[0], $params[1], 0); + $this->assertTrue($structure->is_open()); + $this->assertTrue($structure->get_cm() instanceof cm_info); + $this->assertEquals($feedback->cmid, $structure->get_cm()->id); + $this->assertEquals($feedback->intro, $structure->get_feedback()->intro); + } + } + /** * Tests for mod_feedback_refresh_events. */ @@ -175,11 +208,8 @@ class mod_feedback_lib_testcase extends advanced_testcase { $factory = new \core_calendar\action_factory(); $actionevent = mod_feedback_core_calendar_provide_event_action($event, $factory); - $this->assertInstanceOf('\core_calendar\local\event\value_objects\action', $actionevent); - $this->assertEquals(get_string('answerquestions', 'feedback'), $actionevent->get_name()); - $this->assertInstanceOf('moodle_url', $actionevent->get_url()); - $this->assertEquals(1, $actionevent->get_item_count()); - $this->assertFalse($actionevent->is_actionable()); + // No event on the dashboard if feedback is closed. + $this->assertNull($actionevent); } /**