From bff097f216427a32c3de59062776fbc6d6bb48da Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Tue, 11 Feb 2020 17:06:55 +0800 Subject: [PATCH] MDL-66301 forumreport_summary: Refactors and fixes to course report --- .../classes/event/report_downloaded.php | 9 +++- .../summary/classes/event/report_viewed.php | 9 +++- .../report/summary/classes/output/filters.php | 8 +++ .../report/summary/classes/summary_table.php | 24 +++++---- mod/forum/report/summary/index.php | 51 +++++++++---------- 5 files changed, 61 insertions(+), 40 deletions(-) diff --git a/mod/forum/report/summary/classes/event/report_downloaded.php b/mod/forum/report/summary/classes/event/report_downloaded.php index 1dd8f2671d3..9374eb5309d 100644 --- a/mod/forum/report/summary/classes/event/report_downloaded.php +++ b/mod/forum/report/summary/classes/event/report_downloaded.php @@ -75,8 +75,13 @@ class report_downloaded extends \core\event\base { * @return \moodle_url */ public function get_url() { - return new \moodle_url('/mod/forum/report/summary/index.php', - ['courseid' => $this->courseid, 'forumid' => $this->other['forumid']]); + $params = ['courseid' => $this->courseid]; + + if (!empty($this->other['forumid'])) { + $params['forumid'] = $this->other['forumid']; + } + + return new \moodle_url('/mod/forum/report/summary/index.php', $params); } /** diff --git a/mod/forum/report/summary/classes/event/report_viewed.php b/mod/forum/report/summary/classes/event/report_viewed.php index a3d068db828..991dd326437 100644 --- a/mod/forum/report/summary/classes/event/report_viewed.php +++ b/mod/forum/report/summary/classes/event/report_viewed.php @@ -76,8 +76,13 @@ class report_viewed extends \core\event\base { * @return \moodle_url */ public function get_url() { - return new \moodle_url('/mod/forum/report/summary/index.php', - ['courseid' => $this->courseid, 'forumid' => $this->other['forumid']]); + $params = ['courseid' => $this->courseid]; + + if (!empty($this->other['forumid'])) { + $params['forumid'] = $this->other['forumid']; + } + + return new \moodle_url('/mod/forum/report/summary/index.php', $params); } /** diff --git a/mod/forum/report/summary/classes/output/filters.php b/mod/forum/report/summary/classes/output/filters.php index 66aa974f0bf..ad5558fd44f 100644 --- a/mod/forum/report/summary/classes/output/filters.php +++ b/mod/forum/report/summary/classes/output/filters.php @@ -142,6 +142,7 @@ class filters implements renderable, templatable { $coursegroups = groups_get_all_groups($this->courseid); $forumids = []; $allgroups = false; + $hasgroups = false; // Check if any forum gives the user access to all groups and no groups. foreach ($this->cms as $cm) { @@ -156,6 +157,8 @@ class filters implements renderable, templatable { continue; } + $hasgroups = true; + // Fetch for the current cm's forum. $context = \context_module::instance($cm->id); $aag = has_capability('moodle/site:accessallgroups', $context); @@ -166,6 +169,11 @@ class filters implements renderable, templatable { } } + // If no groups mode enabled, nothing to prepare. + if (!$hasgroups) { + return; + } + // Any groups, and no groups. if ($allgroups) { $nogroups = new stdClass(); diff --git a/mod/forum/report/summary/classes/summary_table.php b/mod/forum/report/summary/classes/summary_table.php index fb4db7b3de7..2e5e7a68a88 100644 --- a/mod/forum/report/summary/classes/summary_table.php +++ b/mod/forum/report/summary/classes/summary_table.php @@ -65,7 +65,10 @@ class summary_table extends table_sql { /** @var int The course ID containing the forum(s) being reported on. */ protected $courseid; - /** @var bool True if reporting on all forums in course, false if reporting on specific forum(s) */ + /** @var bool True if reporting on all forums in course user has access to, false if reporting on a single forum */ + protected $iscoursereport = false; + + /** @var bool True if user has access to all forums in the course (and is running course report), otherwise false. */ protected $accessallforums = false; /** @var \stdClass The course module object(s) of the forum(s) being reported on. */ @@ -119,16 +122,18 @@ class summary_table extends table_sql { * @param bool $canseeprivatereplies Whether the user can see all private replies or not. * @param int $perpage The number of rows to display per page. * @param bool $canexport Is the user allowed to export records? + * @param bool $iscoursereport Whether the user is running a course level report * @param bool $accessallforums If user is running a course level report, do they have access to all forums in the course? */ public function __construct(int $courseid, array $filters, bool $allowbulkoperations, - bool $canseeprivatereplies, int $perpage, bool $canexport, bool $accessallforums) { - global $USER, $OUTPUT; + bool $canseeprivatereplies, int $perpage, bool $canexport, bool $iscoursereport, bool $accessallforums) { + global $OUTPUT; - $uniqueid = $courseid . (empty($filters['forums']) ? '' : '_' . $filters['forums'][0]); + $uniqueid = $courseid . ($iscoursereport ? '' : '_' . $filters['forums'][0]); parent::__construct("summaryreport_{$uniqueid}"); $this->courseid = $courseid; + $this->iscoursereport = $iscoursereport; $this->accessallforums = $accessallforums; $this->allowbulkoperations = $allowbulkoperations; $this->canseeprivatereplies = $canseeprivatereplies; @@ -197,8 +202,8 @@ class summary_table extends table_sql { protected function set_forum_properties(array $forumids): void { global $USER; - // If no forum IDs filtered, reporting on all forums in the course the user has access to. - if ($this->accessallforums) { + // Course context if reporting on all forums in the course the user has access to. + if ($this->iscoursereport) { $this->userfieldscontext = \context_course::instance($this->courseid); } @@ -269,7 +274,7 @@ class summary_table extends table_sql { } global $OUTPUT; - return $OUTPUT->user_picture($data, array('size' => 35, 'courseid' => $this->courseid, 'includefullname' => true)); + return $OUTPUT->user_picture($data, array('courseid' => $this->courseid, 'includefullname' => true)); } /** @@ -336,8 +341,7 @@ class summary_table extends table_sql { global $OUTPUT; // If no posts, nothing to export. - // If reporting on more than one forum (eg a course), unable to export (export only handles a single forum). - if (empty($data->earliestpost) || count($this->cms) > 1) { + if (empty($data->earliestpost)) { return ''; } @@ -682,7 +686,7 @@ class summary_table extends table_sql { * @return void. */ protected function apply_filters(array $filters): void { - // Apply the forums filter if not reporting on whole course. + // Apply the forums filter if not reporting on every forum in a course. if (!$this->accessallforums) { $this->add_filter(self::FILTER_FORUM, $filters['forums']); } diff --git a/mod/forum/report/summary/index.php b/mod/forum/report/summary/index.php index 283f95e0c66..64d39c90302 100644 --- a/mod/forum/report/summary/index.php +++ b/mod/forum/report/summary/index.php @@ -49,65 +49,66 @@ $courseforums = $modinfo->instances['forum']; $cms = []; // Determine which forums the user has access to in the course. +$accessallforums = false; $allforumidsincourse = array_keys($courseforums); $forumsvisibletouser = []; +$forumselectoptions = [0 => get_string('forumselectcourseoption', 'forumreport_summary')]; foreach ($courseforums as $courseforumid => $courseforum) { if ($courseforum->uservisible) { - $forumsvisibletouser[$courseforumid] = $courseforum->name; + $forumsvisibletouser[$courseforumid] = $courseforum; + $forumselectoptions[$courseforumid] = $courseforum->name; } } if ($forumid) { - $filters['forums'] = [$forumid]; - - if (!isset($courseforums[$forumid])) { - throw new \moodle_exception("A valid forum ID is required to generate a summary report."); + if (!isset($forumsvisibletouser[$forumid])) { + throw new \moodle_exception('A valid forum ID is required to generate a summary report.'); } - $foruminfo = $courseforums[$forumid]; - $title = $foruminfo->name; - $forumcm = $foruminfo->get_course_module_record(); + $filters['forums'] = [$forumid]; + $title = $forumsvisibletouser[$forumid]->name; + $forumcm = $forumsvisibletouser[$forumid]->get_course_module_record(); $cms[] = $forumcm; require_login($courseid, false, $forumcm); $context = \context_module::instance($forumcm->id); $canexport = !$download && has_capability('mod/forum:exportforum', $context); - $redirecturl = new moodle_url("/mod/forum/view.php"); - $redirecturl->param('id', $forumid); + $redirecturl = new moodle_url('/mod/forum/view.php', ['id' => $forumid]); + $numforums = 1; $pageurlparams['forumid'] = $forumid; - $accessallforums = false; + $iscoursereport = false; } else { - // Course level report + // Course level report. require_login($courseid, false); $filters['forums'] = array_keys($forumsvisibletouser); - // Fetch the forum cms for the course. - foreach ($courseforums as $courseforum) { - $cms[] = $courseforum->get_course_module_record(); + // Fetch the forum CMs for the course. + foreach ($forumsvisibletouser as $visibleforum) { + $cms[] = $visibleforum->get_course_module_record(); } $context = \context_course::instance($courseid); $title = $course->fullname; // Export currently only supports single forum exports. $canexport = false; - $redirecturl = new moodle_url("/course/view.php"); - $redirecturl->param('id', $courseid); + $redirecturl = new moodle_url('/course/view.php', ['id' => $courseid]); + $numforums = count($forumsvisibletouser); + $iscoursereport = true; - // Determine whether user has access to all forums in the course. + // Specify whether user has access to all forums in the course. $accessallforums = empty(array_diff($allforumidsincourse, $filters['forums'])); } -$pageurl = new moodle_url("/mod/forum/report/summary/index.php", $pageurlparams); +$pageurl = new moodle_url('/mod/forum/report/summary/index.php', $pageurlparams); $PAGE->set_url($pageurl); $PAGE->set_pagelayout('report'); $PAGE->set_title($title); $PAGE->set_heading($course->fullname); -$PAGE->navbar->add(get_string('nodetitle', "forumreport_summary")); +$PAGE->navbar->add(get_string('nodetitle', 'forumreport_summary')); -$numforums = count($filters['forums']); $allowbulkoperations = !$download && !empty($CFG->messaging) && has_capability('moodle/course:bulkmessaging', $context); $canseeprivatereplies = false; $hasviewall = false; @@ -119,7 +120,7 @@ foreach ($cms as $cm) { $forumcontext = \context_module::instance($cm->id); // This capability is required in at least one of the given contexts to view any version of the report. - if (has_capability("forumreport/summary:view", $forumcontext)) { + if (has_capability('forumreport/summary:view', $forumcontext)) { $canview = true; } @@ -148,7 +149,7 @@ if ($numforums === $viewallcount) { // Prepare and display the report. $table = new \forumreport_summary\summary_table($courseid, $filters, $allowbulkoperations, - $canseeprivatereplies, $perpage, $canexport, $accessallforums); + $canseeprivatereplies, $perpage, $canexport, $iscoursereport, $accessallforums); $table->baseurl = $pageurl; $eventparams = [ @@ -173,9 +174,7 @@ if ($download) { } // Allow switching to course report (or other forum user has access to). - $forumselectoptions = [0 => get_string('forumselectcourseoption', 'forumreport_summary')] + $forumsvisibletouser; - $reporturl = new moodle_url("/mod/forum/report/summary/index.php"); - $reporturl->param('courseid', $courseid); + $reporturl = new moodle_url('/mod/forum/report/summary/index.php', ['courseid' => $courseid]); $forumselect = new single_select($reporturl, 'forumid', $forumselectoptions, $forumid); $forumselect->set_label(get_string('forumselectlabel', 'forumreport_summary')); echo $OUTPUT->render($forumselect);