From 0259109fb5b1cf75344f69efc8092d1750bcb4d2 Mon Sep 17 00:00:00 2001 From: Juan Leyva Date: Tue, 28 Mar 2017 10:58:52 +0200 Subject: [PATCH] MDL-58415 mod_lesson: Avoid API http redirections Deep in the lesson API there are some http redirections in special pages like clusters. We need to handle it via new parameters to avoid redirections. --- mod/lesson/classes/external.php | 82 ++++++++++++++++++----------------- mod/lesson/locallib.php | 16 +++++-- mod/lesson/pagetypes/cluster.php | 6 +-- mod/lesson/pagetypes/endofbranch.php | 14 +++--- mod/lesson/pagetypes/endofcluster.php | 13 +++--- mod/lesson/upgrade.txt | 5 +++ mod/lesson/view.php | 2 +- 7 files changed, 81 insertions(+), 57 deletions(-) diff --git a/mod/lesson/classes/external.php b/mod/lesson/classes/external.php index fb4c4392656..b3c16ebadee 100644 --- a/mod/lesson/classes/external.php +++ b/mod/lesson/classes/external.php @@ -934,7 +934,7 @@ class mod_lesson_external extends external_api { * @return external_single_structure * @since Moodle 3.3 */ - protected static function get_page_structure() { + protected static function get_page_structure($required = VALUE_REQUIRED) { return new external_single_structure( array( 'id' => new external_value(PARAM_INT, 'The id of this lesson page'), @@ -955,7 +955,7 @@ class mod_lesson_external extends external_api { 'typeid' => new external_value(PARAM_INT, 'The unique identifier for the page type'), 'typestring' => new external_value(PARAM_RAW, 'The string that describes this page type'), ), - 'Page fields' + 'Page fields', $required ); } @@ -1257,9 +1257,9 @@ class mod_lesson_external extends external_api { 'returncontents' => $returncontents); $params = self::validate_parameters(self::get_page_data_parameters(), $params); - $warnings = $contentfiles = $answerfiles = $responsefiles = array(); + $warnings = $contentfiles = $answerfiles = $responsefiles = $answers = array(); $pagecontent = $ongoingscore = ''; - $progress = null; + $progress = $pagedata = null; list($lesson, $course, $cm, $context, $lessonrecord) = self::validate_lesson($params['lessonid']); self::validate_attempt($lesson, $params); @@ -1274,42 +1274,44 @@ class mod_lesson_external extends external_api { if ($pageid != LESSON_EOL) { $reviewmode = $lesson->is_in_review_mode(); $lessonoutput = $PAGE->get_renderer('mod_lesson'); - list($page, $pagecontent) = $lesson->prepare_page_and_contents($pageid, $lessonoutput, $reviewmode); - // Page may have changed. - $pageid = $page->id; - - $pagedata = self::get_page_fields($page, true); - - // Files. - $contentfiles = external_util::get_area_files($context->id, 'mod_lesson', 'page_contents', $page->id); - - // Answers. - $answers = array(); - $pageanswers = $page->get_answers(); - foreach ($pageanswers as $a) { - $answer = array( - 'id' => $a->id, - 'answerfiles' => external_util::get_area_files($context->id, 'mod_lesson', 'page_answers', $a->id), - 'responsefiles' => external_util::get_area_files($context->id, 'mod_lesson', 'page_responses', $a->id), - ); - // For managers, return all the information (including correct answers, jumps). - // If the teacher enabled offline attempts, this information will be downloaded too. - if ($lesson->can_manage() || $lesson->allowofflineattempts) { - $extraproperties = array('jumpto', 'grade', 'score', 'flags', 'timecreated', 'timemodified'); - foreach ($extraproperties as $prop) { - $answer[$prop] = $a->{$prop}; + // Prepare page contents avoiding redirections. + list($pageid, $page, $pagecontent) = $lesson->prepare_page_and_contents($pageid, $lessonoutput, $reviewmode, false); + + if ($pageid > 0) { + + $pagedata = self::get_page_fields($page, true); + + // Files. + $contentfiles = external_util::get_area_files($context->id, 'mod_lesson', 'page_contents', $page->id); + + // Answers. + $answers = array(); + $pageanswers = $page->get_answers(); + foreach ($pageanswers as $a) { + $answer = array( + 'id' => $a->id, + 'answerfiles' => external_util::get_area_files($context->id, 'mod_lesson', 'page_answers', $a->id), + 'responsefiles' => external_util::get_area_files($context->id, 'mod_lesson', 'page_responses', $a->id), + ); + // For managers, return all the information (including correct answers, jumps). + // If the teacher enabled offline attempts, this information will be downloaded too. + if ($lesson->can_manage() || $lesson->allowofflineattempts) { + $extraproperties = array('jumpto', 'grade', 'score', 'flags', 'timecreated', 'timemodified'); + foreach ($extraproperties as $prop) { + $answer[$prop] = $a->{$prop}; + } } + $answers[] = $answer; } - $answers[] = $answer; - } - // Additional lesson information. - if (!$lesson->can_manage()) { - if ($lesson->ongoing && !$reviewmode) { - $ongoingscore = $lesson->get_ongoing_score_message(); - } - if ($lesson->progressbar) { - $progress = $lesson->calculate_progress(); + // Additional lesson information. + if (!$lesson->can_manage()) { + if ($lesson->ongoing && !$reviewmode) { + $ongoingscore = $lesson->get_ongoing_score_message(); + } + if ($lesson->progressbar) { + $progress = $lesson->calculate_progress(); + } } } } @@ -1317,7 +1319,6 @@ class mod_lesson_external extends external_api { $messages = self::format_lesson_messages($lesson); $result = array( - 'page' => $pagedata, 'newpageid' => $pageid, 'ongoingscore' => $ongoingscore, 'progress' => $progress, @@ -1328,6 +1329,9 @@ class mod_lesson_external extends external_api { 'displaymenu' => !empty(lesson_displayleftif($lesson)), ); + if (!empty($pagedata)) { + $result['page'] = $pagedata; + } if ($params['returncontents']) { $result['pagecontent'] = $pagecontent; // Return the complete page contents rendered. } @@ -1344,7 +1348,7 @@ class mod_lesson_external extends external_api { public static function get_page_data_returns() { return new external_single_structure( array( - 'page' => self::get_page_structure(), + 'page' => self::get_page_structure(VALUE_OPTIONAL), 'newpageid' => new external_value(PARAM_INT, 'New page id (if a jump was made)'), 'pagecontent' => new external_value(PARAM_RAW, 'Page html content', VALUE_OPTIONAL), 'ongoingscore' => new external_value(PARAM_TEXT, 'The ongoing score message'), diff --git a/mod/lesson/locallib.php b/mod/lesson/locallib.php index 3f22939853c..5a707a40806 100644 --- a/mod/lesson/locallib.php +++ b/mod/lesson/locallib.php @@ -3117,16 +3117,23 @@ class lesson extends lesson_base { * @param int $pageid the given page id * @param mod_lesson_renderer $lessonoutput the lesson output rendered * @param bool $reviewmode whether we are in review mode or not + * @param bool $redirect Optional, default to true. Set to false to avoid redirection and return the page to redirect. * @return array the page object and contents * @throws moodle_exception * @since Moodle 3.3 */ - public function prepare_page_and_contents($pageid, $lessonoutput, $reviewmode) { + public function prepare_page_and_contents($pageid, $lessonoutput, $reviewmode, $redirect = true) { global $USER, $CFG; $page = $this->load_page($pageid); // Check if the page is of a special type and if so take any nessecary action. - $newpageid = $page->callback_on_view($this->can_manage()); + $newpageid = $page->callback_on_view($this->can_manage(), $redirect); + + // Avoid redirections returning the jump to special page id. + if (!$redirect && is_numeric($newpageid) && $newpageid < 0) { + return array($newpageid, null, null); + } + if (is_numeric($newpageid)) { $page = $this->load_page($newpageid); } @@ -3167,7 +3174,7 @@ class lesson extends lesson_base { ob_end_clean(); } - return array($page, $lessoncontent); + return array($page->id, $page, $lessoncontent); } /** @@ -4155,9 +4162,10 @@ abstract class lesson_page extends lesson_base { * is viewed * * @param bool $canmanage True if the user has the manage cap + * @param bool $redirect Optional, default to true. Set to false to avoid redirection and return the page to redirect. * @return mixed */ - public function callback_on_view($canmanage) { + public function callback_on_view($canmanage, $redirect = true) { return true; } diff --git a/mod/lesson/pagetypes/cluster.php b/mod/lesson/pagetypes/cluster.php index c179913fd55..31c01b3ae59 100644 --- a/mod/lesson/pagetypes/cluster.php +++ b/mod/lesson/pagetypes/cluster.php @@ -55,14 +55,14 @@ class lesson_page_type_cluster extends lesson_page { public function get_grayout() { return 1; } - public function callback_on_view($canmanage) { + public function callback_on_view($canmanage, $redirect = true) { global $USER; if (!$canmanage) { // Get the next page in the lesson cluster jump - return $this->lesson->cluster_jump($this->properties->id); + return (int) $this->lesson->cluster_jump($this->properties->id); } else { // get the next page - return $this->properties->nextpageid; + return (int) $this->properties->nextpageid; } } public function override_next_page() { diff --git a/mod/lesson/pagetypes/endofbranch.php b/mod/lesson/pagetypes/endofbranch.php index 634da76eb26..97a64835331 100644 --- a/mod/lesson/pagetypes/endofbranch.php +++ b/mod/lesson/pagetypes/endofbranch.php @@ -51,12 +51,11 @@ class lesson_page_type_endofbranch extends lesson_page { public function get_idstring() { return $this->typeidstring; } - public function callback_on_view($canmanage) { - $this->redirect_to_first_answer($canmanage); - exit; + public function callback_on_view($canmanage, $redirect = true) { + return (int) $this->redirect_to_first_answer($canmanage, $redirect); } - public function redirect_to_first_answer($canmanage) { + public function redirect_to_first_answer($canmanage, $redirect) { global $USER, $PAGE; $answers = $this->get_answers(); $answer = array_shift($answers); @@ -94,7 +93,12 @@ class lesson_page_type_endofbranch extends lesson_page { $jumpto = $this->properties->prevpageid; } - redirect(new moodle_url('/mod/lesson/view.php', array('id'=>$PAGE->cm->id,'pageid'=>$jumpto))); + + if ($redirect) { + redirect(new moodle_url('/mod/lesson/view.php', array('id' => $PAGE->cm->id, 'pageid' => $jumpto))); + die; + } + return $jumpto; } public function get_grayout() { return 1; diff --git a/mod/lesson/pagetypes/endofcluster.php b/mod/lesson/pagetypes/endofcluster.php index f5543537053..8a0aaade21d 100644 --- a/mod/lesson/pagetypes/endofcluster.php +++ b/mod/lesson/pagetypes/endofcluster.php @@ -51,18 +51,21 @@ class lesson_page_type_endofcluster extends lesson_page { public function get_idstring() { return $this->typeidstring; } - public function callback_on_view($canmanage) { - $this->redirect_to_next_page($canmanage); - exit; + public function callback_on_view($canmanage, $redirect = true) { + return (int) $this->redirect_to_next_page($canmanage, $redirect); } - public function redirect_to_next_page() { + public function redirect_to_next_page($canmanage, $redirect) { global $PAGE; if ($this->properties->nextpageid == 0) { $nextpageid = LESSON_EOL; } else { $nextpageid = $this->properties->nextpageid; } - redirect(new moodle_url('/mod/lesson/view.php', array('id'=>$PAGE->cm->id,'pageid'=>$nextpageid))); + if ($redirect) { + redirect(new moodle_url('/mod/lesson/view.php', array('id' => $PAGE->cm->id, 'pageid' => $nextpageid))); + die; + } + return $nextpageid; } public function get_grayout() { return 1; diff --git a/mod/lesson/upgrade.txt b/mod/lesson/upgrade.txt index 31608793a83..79cc01a3578 100644 --- a/mod/lesson/upgrade.txt +++ b/mod/lesson/upgrade.txt @@ -1,5 +1,10 @@ This files describes API changes in the lesson code. +=== 3.3 === + +* lesson::callback_on_view() has an additional optional parameter $redirect default to true. + It can be set to false to avoid redirection and return the page to redirect. + === 3.1 === * Removed the unused file reformat.php * removedoublecr() and importmodifiedaikenstyle() have now been removed. diff --git a/mod/lesson/view.php b/mod/lesson/view.php index 0ca0495cc92..d74eda30693 100644 --- a/mod/lesson/view.php +++ b/mod/lesson/view.php @@ -212,7 +212,7 @@ if ($pageid != LESSON_EOL) { } } - list($page, $lessoncontent) = $lesson->prepare_page_and_contents($pageid, $lessonoutput, $reviewmode); + list($newpageid, $page, $lessoncontent) = $lesson->prepare_page_and_contents($pageid, $lessonoutput, $reviewmode); if (($edit != -1) && $PAGE->user_allowed_editing()) { $USER->editing = $edit; -- 2.11.4.GIT