From 8e8de15f6c685aed59017e51a7fcc146076a721b Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Tue, 15 Nov 2011 17:12:06 +1300 Subject: [PATCH 1/7] MDL-28967 navigation: Several navigation fixes * Fixed implementation of navigation settings navshowcategories, navshowallcourses, and navcourselimit * Improved performance associated with loading all, or a collection of courses * Fixed duplicate calls to loading functions when we had the information required to know it had already occurred --- lang/en/admin.php | 2 +- lib/navigationlib.php | 443 ++++++++++++++++++++++++++++++++---------- 2 files changed, 340 insertions(+), 105 deletions(-) diff --git a/lang/en/admin.php b/lang/en/admin.php index 6af4828d2be..98b79a0e171 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -256,7 +256,7 @@ $string['configmycoursesperpage'] = 'Maximum number of courses to display in any $string['configmymoodleredirect'] = 'This setting forces redirects to /my on login for non-admins and replaces the top level site navigation with /my'; $string['configmypagelocked'] = 'This setting prevents the default page from being edited by any non-admins'; $string['confignavcourselimit'] = 'Limits the number of courses shown to the user when they are either not logged in or are not enrolled in any courses.'; -$string['confignavshowallcourses'] = 'Setting this ensures that all courses on the site are shown in the navigation at all times.'; +$string['confignavshowallcourses'] = 'If enabled users will see courses they are enrolled in both within the My Courses branch and the course structure. When disabled users with enrolments will only see the My Courses branch of the navigaiton.'; $string['confignavshowcategories'] = 'Show course categories in the navigation bar and navigation blocks. This does not occur with courses the user is currently enrolled in, they will still be listed under mycourses without categories.'; $string['confignotifyloginfailures'] = 'If login failures have been recorded, email notifications can be sent out. Who should see these notifications?'; $string['confignotifyloginthreshold'] = 'If notifications about failed logins are active, how many failed login attempts by one user or one IP address is it worth notifying about?'; diff --git a/lib/navigationlib.php b/lib/navigationlib.php index f4b89df4288..c73276bd6fd 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -943,6 +943,8 @@ class global_navigation extends navigation_node { protected $cache; /** @var array An array of course ids that are present in the navigation */ protected $addedcourses = array(); + /** @var bool */ + protected $allcategoriesloaded = false; /** @var array An array of category ids that are included in the navigation */ protected $addedcategories = array(); /** @var int expansion limit */ @@ -950,6 +952,11 @@ class global_navigation extends navigation_node { /** @var int userid to allow parent to see child's profile page navigation */ protected $useridtouseforparentchecks = 0; + /** Used when loading categories to load all top level categories [parent = 0] **/ + const LOAD_ROOT_CATEGORIES = 0; + /** Used when loading categories to load all categories **/ + const LOAD_ALL_CATEGORIES = -1; + /** * Constructs a new global navigation * @@ -1050,18 +1057,15 @@ class global_navigation extends navigation_node { $this->rootnodes['users'] = $this->add(get_string('users'), null, self::TYPE_ROOTNODE, null, 'users'); // Fetch all of the users courses. - $limit = 20; - if (!empty($CFG->navcourselimit)) { - $limit = $CFG->navcourselimit; - } - - $mycourses = enrol_get_my_courses(NULL, 'visible DESC,sortorder ASC'); - $showallcourses = (count($mycourses) == 0 || !empty($CFG->navshowallcourses)); - // When checking if we are to show categories there is an additional override. - // If the user is viewing a category then we will load it regardless of settings. - // to ensure that the navigation is consistent. - $showcategories = $this->page->context->contextlevel == CONTEXT_COURSECAT || ($showallcourses && $this->show_categories()); + $mycourses = enrol_get_my_courses(); + // We need to show all courses if the user has selected to show all courses in the settings + // OR if the user is not enrolled in any courses and we're not showing categories + $showallcourses = (!empty($CFG->navshowallcourses) || (count($mycourses) === 0 && !$this->show_categories())); + // We need to show categories if we can show categories and the user isn't enrolled in any courses or we're not showing all courses + $showcategories = ($this->show_categories() && (count($mycourses) == 0 || !empty($CFG->navshowallcourses))); + // $issite gets set to true if the current pages course is the sites frontpage course $issite = ($this->page->course->id == SITEID); + // $ismycourse gets set to true if the user is enrolled in the current pages course. $ismycourse = (array_key_exists($this->page->course->id, $mycourses)); // Check if any courses were returned. @@ -1137,12 +1141,40 @@ class global_navigation extends navigation_node { } // Add all of the users courses to the navigation. + // First up we need to add to the mycourses section. foreach ($mycourses as $course) { $course->coursenode = $this->add_course($course, false, true); } - } - if ($showallcourses) { + if ($showallcourses) { + // Load all courses + $this->load_all_courses(); + } + + // Next if nasvshowallcourses is enabled then we need to add courses + // to the courses branch as well. + if (!empty($CFG->navshowallcourses)) { + foreach ($mycourses as $course) { + if (!empty($course->category) && !$this->can_add_more_courses_to_category($course->category)) { + continue; + } + $genericcoursenode = $this->add_course($course, true); + if ($genericcoursenode->isactive) { + // We don't want this node to be active because we want the + // node in the mycourses branch to be active. + $genericcoursenode->make_inactive(); + $genericcoursenode->collapse = true; + if ($genericcoursenode->parent && $genericcoursenode->parent->type == self::TYPE_CATEGORY) { + $parent = $genericcoursenode->parent; + while ($parent && $parent->type == self::TYPE_CATEGORY) { + $parent->collapse = true; + $parent = $parent->parent; + } + } + } + } + } + } else if ($showallcourses) { // Load all courses $this->load_all_courses(); } @@ -1160,14 +1192,15 @@ class global_navigation extends navigation_node { case CONTEXT_SYSTEM : // This has already been loaded we just need to map the variable $coursenode = $frontpagecourse; - $this->load_all_categories(null, $showcategories); + if ($this->show_categories()) { + $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, $showcategories); + } break; case CONTEXT_COURSECAT : // This has already been loaded we just need to map the variable $coursenode = $frontpagecourse; - $this->load_all_categories($this->page->context->instanceid, $showcategories); - if (array_key_exists($this->page->context->instanceid, $this->addedcategories)) { - $this->addedcategories[$this->page->context->instanceid]->make_active(); + if ($this->show_categories()) { + $this->load_all_categories($this->page->context->instanceid, $showcategories); } break; case CONTEXT_BLOCK : @@ -1179,7 +1212,7 @@ class global_navigation extends navigation_node { } // Load the course associated with the page into the navigation $course = $this->page->course; - if ($showcategories && !$ismycourse) { + if ($this->show_categories() && !$ismycourse) { $this->load_all_categories($course->category, $showcategories); } $coursenode = $this->load_course($course); @@ -1240,7 +1273,7 @@ class global_navigation extends navigation_node { $course = $this->page->course; $cm = $this->page->cm; - if ($showcategories && !$ismycourse) { + if ($this->show_categories() && !$ismycourse) { $this->load_all_categories($course->category, $showcategories); } @@ -1328,7 +1361,7 @@ class global_navigation extends navigation_node { break; } $course = $this->page->course; - if ($showcategories && !$ismycourse) { + if ($this->show_categories() && !$ismycourse) { $this->load_all_categories($course->category, $showcategories); } // Load the course associated with the user into the navigation @@ -1352,19 +1385,30 @@ class global_navigation extends navigation_node { break; } - $limit = 20; - if (!empty($CFG->navcourselimit)) { - $limit = $CFG->navcourselimit; - } if ($showcategories) { $categories = $this->find_all_of_type(self::TYPE_CATEGORY); - foreach ($categories as &$category) { - if ($category->children->count() >= $limit) { - $url = new moodle_url('/course/category.php', array('id'=>$category->key)); - $category->add(get_string('viewallcourses'), $url, self::TYPE_SETTING); + if (count($categories) !== 0) { + $categoryids = array(); + foreach ($categories as $category) { + $categoryids[] = $category->key; + } + list($categoriessql, $params) = $DB->get_in_or_equal($categoryids, SQL_PARAMS_NAMED); + $params['limit'] = (!empty($CFG->navcourselimit))?$CFG->navcourselimit:20; + $sql = "SELECT cc.id, COUNT(c.id) AS coursecount + FROM {course_categories} cc + JOIN {course} c ON c.category = cc.id + WHERE cc.id {$categoriessql} + GROUP BY cc.id + HAVING COUNT(c.id) > :limit"; + $excessivecategories = $DB->get_records_sql($sql, $params); + foreach ($categories as &$category) { + if (array_key_exists($category->key, $excessivecategories) && !$this->can_add_more_courses_to_category($category)) { + $url = new moodle_url('/course/category.php', array('id'=>$category->key)); + $category->add(get_string('viewallcourses'), $url, self::TYPE_SETTING); + } } } - } else if ($this->rootnodes['courses']->children->count() >= $limit) { + } else if ((!empty($CFG->navshowallcourses) || empty($mycourses)) && !$this->can_add_more_courses_to_category($this->rootnodes['courses'])) { $this->rootnodes['courses']->add(get_string('viewallcoursescategories'), new moodle_url('/course/index.php'), self::TYPE_SETTING); } @@ -1465,51 +1509,236 @@ class global_navigation extends navigation_node { /** * Loads the courses in Moodle into the navigation. * - * @param mixed $categoryids Either a string or array of category ids to load courses for - * @return array An array of navigation_node + * @global moodle_database $DB + * @param string|array $categoryids An array containing categories to load courses + * for, OR null to load courses for all categories. + * @return array An array of navigation_nodes one for each course */ - protected function load_all_courses($categoryids=null) { - global $CFG, $DB, $USER; + protected function load_all_courses($categoryids = null) { + global $CFG, $DB; - if ($categoryids !== null) { - if (is_array($categoryids)) { - list ($categoryselect, $params) = $DB->get_in_or_equal($categoryids, SQL_PARAMS_NAMED, 'catid'); - } else { - $categoryselect = '= :categoryid'; - $params = array('categoryid', $categoryids); - } - $params['siteid'] = SITEID; - $categoryselect = ' AND c.category '.$categoryselect; - } else { - $params = array('siteid' => SITEID); - $categoryselect = ''; - } - - $ccselect = context_helper::get_preload_record_columns_sql('ctx'); - $params['contextlevel'] = CONTEXT_COURSE; - list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); - $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category, cat.path AS categorypath, $ccselect - FROM {course} c - JOIN {context} ctx ON c.id = ctx.instanceid - LEFT JOIN {course_categories} cat ON cat.id=c.category - WHERE c.id {$courseids} AND - ctx.contextlevel = :contextlevel - {$categoryselect} - ORDER BY c.sortorder ASC"; + // Work out the limit of courses. $limit = 20; if (!empty($CFG->navcourselimit)) { $limit = $CFG->navcourselimit; } - $courses = $DB->get_records_sql($sql, $params + $courseparams, 0, $limit); + // Work out the key to use for caching. + if (is_array($categoryids)) { + $cachekey = sprintf('load_all_courses_%d_%s', $limit, md5(join('_', $categoryids))); + } else if (is_string($categoryids) || is_int($categoryids)) { + $cachekey = sprintf('load_all_courses_%d_%s', $limit, md5($categoryids)); + } else { + $cachekey = sprintf('load_all_courses_%d', $limit); + } + + $toload = (empty($CFG->navshowallcourses))?self::LOAD_ROOT_CATEGORIES:self::LOAD_ALL_CATEGORIES; + + // If we are going to show all courses AND we are showing categories then + // to save us repeated DB calls load all of the categories now + if ($this->show_categories()) { + $this->load_all_categories($toload); + } + + // Will be the return of our efforts $coursenodes = array(); - foreach ($courses as $course) { - context_helper::preload_from_record($course); - $coursenodes[$course->id] = $this->add_course($course); + + // Here we have a very important cache check. + // Loading all courses is a VERY costly buisness for two reasons. + // 1. We still have a limit per category of courses, however there is no + // great way to select a maximum per category in a single query. + // 2. Each course loaded is costly as we have permission checks, ajax tie + // in's and a great deal of code that will be executed. + if (!$this->cache->cached($cachekey)) { + // Check if we need to show categories. + if ($this->show_categories()) { + // Hmmm we need to show categories... this is going to be painful. + // We now need to fetch up to $limit courses for each category to + // be displayed. + if ($categoryids !== null) { + if (!is_array($categoryids)) { + $categoryids = array($categoryids); + } + list($categorywhere, $categoryparams) = $DB->get_in_or_equal($categoryids, SQL_PARAMS_NAMED, 'cc'); + $categorywhere = 'WHERE cc.id '.$categorywhere; + } else if ($toload == self::LOAD_ROOT_CATEGORIES) { + $categorywhere = 'WHERE cc.depth = 1 OR cc.depth = 2'; + $categoryparams = array(); + } else { + $categorywhere = ''; + $categoryparams = array(); + } + + // First up we are going to get the categories that we are going to + // need so that we can determine how best to load the courses from them. + $sql = "SELECT cc.id, COUNT(c.id) AS coursecount + FROM {course_categories} cc + LEFT JOIN {course} c ON c.category = cc.id + {$categorywhere} + GROUP BY cc.id"; + $categories = $DB->get_recordset_sql($sql, $categoryparams); + $fullfetch = array(); + $partfetch = array(); + foreach ($categories as $category) { + if (!$this->can_add_more_courses_to_category($category->id)) { + continue; + } + if ($category->coursecount > $limit * 5) { + $partfetch[] = $category->id; + } else if ($category->coursecount > 0) { + $fullfetch[] = $category->id; + } + } + $categories->close(); + + if (count($fullfetch)) { + // First up fetch all of the courses in categories where we know that we are going to + // need the majority of courses. + list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); + list($categoryids, $categoryparams) = $DB->get_in_or_equal($fullfetch, SQL_PARAMS_NAMED, 'lcategory'); + $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect + FROM {course} c + $ccjoin + WHERE c.category {$categoryids} AND + c.id {$courseids} + ORDER BY c.sortorder ASC"; + $coursesrs = $DB->get_recordset_sql($sql, $courseparams + $categoryparams); + foreach ($coursesrs as $course) { + if (!$this->can_add_more_courses_to_category($course->category)) { + continue; + } + context_instance_preload($course); + if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + continue; + } + $coursenodes[$course->id] = $this->add_course($course); + } + $coursesrs->close(); + } + + if (count($partfetch)) { + // Next we will work our way through the categories where we will likely only need a small + // proportion of the courses. + foreach ($partfetch as $categoryid) { + list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); + $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect + FROM {course} c + $ccjoin + WHERE c.category = :categoryid AND + c.id {$courseids} + ORDER BY c.sortorder ASC"; + $courseparams['categoryid'] = $categoryid; + $coursesrs = $DB->get_recordset_sql($sql, $courseparams, 0, $limit * 5); + foreach ($coursesrs as $course) { + if (!$this->can_add_more_courses_to_category($course->category)) { + break; + } + context_instance_preload($course); + if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + continue; + } + $coursenodes[$course->id] = $this->add_course($course); + } + $coursesrs->close(); + } + } + } else { + // Prepare the SQL to load the courses and their contexts + list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses), SQL_PARAMS_NAMED, 'lc', false); + $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect + FROM {course} c + $ccjoin + WHERE c.id {$courseids} + ORDER BY c.sortorder ASC"; + $coursesrs = $DB->get_recordset_sql($sql, $courseparams); + foreach ($coursesrs as $course) { + context_instance_preload($course); + if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + continue; + } + $coursenodes[$course->id] = $this->add_course($course); + if (count($coursenodes) >= $limit) { + break; + } + } + $coursesrs->close(); + } + + // Cache the course id's that we've had to load to save us running these queries again. + $this->cache->set($cachekey, array_keys($coursenodes)); + } else { + // YAY we've already cached this information + // First get the courses that we need to load from the navigation cache + $courseids = $this->cache->$cachekey; + + // Check to make sure we have some course nodes. + if (!count($courseids)) { + return $coursenodes; + } + + // Next check for courses that have already been loaded and remove them + // from the array we are about to load. + foreach (array_intersect($courseids, array_keys($this->addedcourses)) as $id) { + $key = array_search($id, $courseids); + unset($courseids[$key]); + } + + // Check that we still have course nodes. Any that have already been loaded + // will now have been removed. + if (!count($courseids)) { + return $coursenodes; + } + + // Prepare the SQL to load the courses and their contexts + list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); + list($courseids, $courseparams) = $DB->get_in_or_equal($courseids, SQL_PARAMS_NAMED, 'lc'); + $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect + FROM {course} c + $ccjoin + WHERE c.id {$courseids} + ORDER BY c.sortorder ASC"; + $coursesrs = $DB->get_recordset_sql($sql, $courseparams); + foreach ($coursesrs as $course) { + context_instance_preload($course); + $coursenodes[$course->id] = $this->add_course($course); + } + $coursesrs->close(); } return $coursenodes; } + /** + * Returns true if more courses can be added to the provided category. + * + * @global type $CFG + * @param type $categoryid + * @return type + */ + protected function can_add_more_courses_to_category($category) { + global $CFG; + $limit = 20; + if (!empty($CFG->navcourselimit)) { + $limit = (int)$CFG->navcourselimit; + } + if (is_numeric($category)) { + if (!array_key_exists($category, $this->addedcategories)) { + return true; + } + $coursecount = count($this->addedcategories[$category]->children->type(self::TYPE_COURSE)); + } else if ($category instanceof navigation_node) { + if ($category->type != self::TYPE_CATEGORY && $category->type != self::TYPE_ROOTNODE) { + return false; + } + $coursecount = count($category->children->type(self::TYPE_COURSE)); + } else if (is_object($category) && property_exists($category,'id')) { + $coursecount = count($this->addedcategories[$category->id]->children->type(self::TYPE_COURSE)); + } + return ($coursecount < $limit); + } + /** * Loads all categories (top level or if an id is specified for that category) * @@ -1518,16 +1747,18 @@ class global_navigation extends navigation_node { * as the requested category and any parent categories. * @return navigation_node|void returns a navigation node if a category has been loaded. */ - protected function load_all_categories($categoryid = null, $showbasecategories = false) { + protected function load_all_categories($categoryid = self::LOAD_ROOT_CATEGORIES, $showbasecategories = false) { global $DB; // Check if this category has already been loaded - if ($categoryid !== null && array_key_exists($categoryid, $this->addedcategories) && $this->addedcategories[$categoryid]->children->count() > 0) { - return $this->addedcategories[$categoryid]; + if ($this->allcategoriesloaded || ($categoryid < 1 && $this->is_category_fully_loaded($categoryid))) { + return true; } - $coursestoload = array(); - if (empty($categoryid)) { // can be 0 + $categoriestoload = array(); + if ($categoryid == self::LOAD_ALL_CATEGORIES) { + $categories = $DB->get_records('course_categories', null, 'sortorder ASC, id ASC'); + } else if ($categoryid == self::LOAD_ROOT_CATEGORIES) { // can be 0 // We are going to load all of the first level categories (categories without parents) $categories = $DB->get_records('course_categories', array('parent'=>'0'), 'sortorder ASC, id ASC'); } else if (array_key_exists($categoryid, $this->addedcategories)) { @@ -1558,11 +1789,11 @@ class global_navigation extends navigation_node { // This category hasn't been loaded yet so we need to fetch it, work out its category path // and load this category plus all its parents and subcategories $category = $DB->get_record('course_categories', array('id' => $categoryid), 'path', MUST_EXIST); - $coursestoload = explode('/', trim($category->path, '/')); - list($select, $params) = $DB->get_in_or_equal($coursestoload); + $categoriestoload = explode('/', trim($category->path, '/')); + list($select, $params) = $DB->get_in_or_equal($categoriestoload); $select = 'id '.$select.' OR parent '.$select; if ($showbasecategories) { - $select .= ' OR parent = 0'; + $select .= ' OR depth = 1'; } $params = array_merge($params, $params); $categories = $DB->get_records_select('course_categories', $select, $params, 'sortorder'); @@ -1605,9 +1836,20 @@ class global_navigation extends navigation_node { // Remove the category from the categories array now that we know it has been added. unset($categories[$category->id]); } + if ($categoryid === self::LOAD_ALL_CATEGORIES) { + $this->allcategoriesloaded = true; + } // Check if there are any categories to load. - if (count($coursestoload) > 0) { - $this->load_all_courses($coursestoload); + if (count($categoriestoload) > 0) { + $readytoloadcourses = array(); + foreach ($categoriestoload as $category) { + if ($this->can_add_more_courses_to_category($category)) { + $readytoloadcourses[] = $category; + } + } + if (count($readytoloadcourses)) { + $this->load_all_courses($readytoloadcourses); + } } } @@ -2259,8 +2501,8 @@ class global_navigation extends navigation_node { * Adds the given course to the navigation structure. * * @param stdClass $course - * @param bool $forcegeneric (optional) - * @param bool $ismycourse (optional) + * @param bool $forcegeneric + * @param bool $ismycourse * @return navigation_node */ public function add_course(stdClass $course, $forcegeneric = false, $ismycourse = false) { @@ -2282,7 +2524,6 @@ class global_navigation extends navigation_node { } $issite = ($course->id == SITEID); - $ismycourse = ($ismycourse && !$forcegeneric); $shortname = format_string($course->shortname, true, array('context' => $coursecontext)); if ($issite) { @@ -2291,7 +2532,7 @@ class global_navigation extends navigation_node { if (empty($CFG->usesitenameforsitepages)) { $shortname = get_string('sitepages'); } - } else if ($ismycourse) { + } else if ($ismycourse && !$forcegeneric) { if (!empty($CFG->navshowmycoursecategories) && ($parent = $this->rootnodes['mycourses']->find($course->category, self::TYPE_CATEGORY))) { // Nothing to do here the above statement set $parent to the category within mycourses. } else { @@ -2301,18 +2542,17 @@ class global_navigation extends navigation_node { } else { $parent = $this->rootnodes['courses']; $url = new moodle_url('/course/view.php', array('id'=>$course->id)); - } - - if (!$ismycourse && !$issite && !empty($course->category)) { - if ($this->show_categories()) { - // We need to load the category structure for this course - $this->load_all_categories($course->category); - } - if (array_key_exists($course->category, $this->addedcategories)) { - $parent = $this->addedcategories[$course->category]; - // This could lead to the course being created so we should check whether it is the case again - if (!$forcegeneric && array_key_exists($course->id, $this->addedcourses)) { - return $this->addedcourses[$course->id]; + if (!empty($course->category) && $this->show_categories()) { + if ($this->show_categories() && !$this->is_category_fully_loaded($course->category)) { + // We need to load the category structure for this course + $this->load_all_categories($course->category); + } + if (array_key_exists($course->category, $this->addedcategories)) { + $parent = $this->addedcategories[$course->category]; + // This could lead to the course being created so we should check whether it is the case again + if (!$forcegeneric && array_key_exists($course->id, $this->addedcourses)) { + return $this->addedcourses[$course->id]; + } } } } @@ -2324,25 +2564,20 @@ class global_navigation extends navigation_node { if (!$forcegeneric) { $this->addedcourses[$course->id] = &$coursenode; } - if ($ismycourse && !empty($CFG->navshowallcourses)) { - // We need to add this course to the general courses node as well as the - // my courses node, rerun the function with the kill param - $genericcourse = $this->add_course($course, true); - if ($genericcourse->isactive) { - $genericcourse->make_inactive(); - $genericcourse->collapse = true; - if ($genericcourse->parent && $genericcourse->parent->type == self::TYPE_CATEGORY) { - $parent = $genericcourse->parent; - while ($parent && $parent->type == self::TYPE_CATEGORY) { - $parent->collapse = true; - $parent = $parent->parent; - } - } - } - } return $coursenode; } + + /** + * Returns true if the category has already been loaded as have any child categories + * + * @param int $categoryid + * @return bool + */ + protected function is_category_fully_loaded($categoryid) { + return (array_key_exists($categoryid, $this->addedcategories) && ($this->allcategoriesloaded || $this->addedcategories[$categoryid]->children->count() > 0)); + } + /** * Adds essential course nodes to the navigation for the given course. * From ee03fe79eff18f2789df9c56b89e394863a11604 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Tue, 6 Dec 2011 12:04:51 +1300 Subject: [PATCH 2/7] MDL-28967 navigation: Fixed up front page generation when showing categories and no enrolled courses --- lib/navigationlib.php | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/navigationlib.php b/lib/navigationlib.php index c73276bd6fd..11d99d42091 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -1058,9 +1058,6 @@ class global_navigation extends navigation_node { // Fetch all of the users courses. $mycourses = enrol_get_my_courses(); - // We need to show all courses if the user has selected to show all courses in the settings - // OR if the user is not enrolled in any courses and we're not showing categories - $showallcourses = (!empty($CFG->navshowallcourses) || (count($mycourses) === 0 && !$this->show_categories())); // We need to show categories if we can show categories and the user isn't enrolled in any courses or we're not showing all courses $showcategories = ($this->show_categories() && (count($mycourses) == 0 || !empty($CFG->navshowallcourses))); // $issite gets set to true if the current pages course is the sites frontpage course @@ -1146,7 +1143,7 @@ class global_navigation extends navigation_node { $course->coursenode = $this->add_course($course, false, true); } - if ($showallcourses) { + if (!empty($CFG->navshowallcourses)) { // Load all courses $this->load_all_courses(); } @@ -1174,7 +1171,7 @@ class global_navigation extends navigation_node { } } } - } else if ($showallcourses) { + } else if (!empty($CFG->navshowallcourses) || !$this->show_categories()) { // Load all courses $this->load_all_courses(); } @@ -1191,14 +1188,12 @@ class global_navigation extends navigation_node { switch ($this->page->context->contextlevel) { case CONTEXT_SYSTEM : // This has already been loaded we just need to map the variable - $coursenode = $frontpagecourse; if ($this->show_categories()) { $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, $showcategories); } break; case CONTEXT_COURSECAT : // This has already been loaded we just need to map the variable - $coursenode = $frontpagecourse; if ($this->show_categories()) { $this->load_all_categories($this->page->context->instanceid, $showcategories); } @@ -1729,7 +1724,7 @@ class global_navigation extends navigation_node { } $coursecount = count($this->addedcategories[$category]->children->type(self::TYPE_COURSE)); } else if ($category instanceof navigation_node) { - if ($category->type != self::TYPE_CATEGORY && $category->type != self::TYPE_ROOTNODE) { + if ($category->type != self::TYPE_CATEGORY) { return false; } $coursecount = count($category->children->type(self::TYPE_COURSE)); From 176b75b59cd60d0bb32573b8bc91b4e57c995529 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Thu, 29 Mar 2012 12:29:40 +1300 Subject: [PATCH 3/7] MDL-28967 navigation: Optimised load_all_categories to load preload contexts to greatly reduce database queries --- lib/navigationlib.php | 59 +++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 11d99d42091..7f0c3a48ac9 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -1750,50 +1750,65 @@ class global_navigation extends navigation_node { return true; } + $catcontextsql = context_helper::get_preload_record_columns_sql('ctx'); + $sqlselect = "SELECT cc.*, $catcontextsql + FROM {course_categories} cc + JOIN {context} ctx ON cc.id = ctx.instanceid"; + $sqlwhere = "WHERE ctx.contextlevel = ".CONTEXT_COURSECAT; + $sqlorder = "ORDER BY depth ASC, sortorder ASC, id ASC"; + $params = array(); + $categoriestoload = array(); if ($categoryid == self::LOAD_ALL_CATEGORIES) { - $categories = $DB->get_records('course_categories', null, 'sortorder ASC, id ASC'); + // We are going to load all categories regardless... prepare to fire + // on the database server! } else if ($categoryid == self::LOAD_ROOT_CATEGORIES) { // can be 0 // We are going to load all of the first level categories (categories without parents) - $categories = $DB->get_records('course_categories', array('parent'=>'0'), 'sortorder ASC, id ASC'); + $sqlwhere .= " AND cc.parent = 0"; } else if (array_key_exists($categoryid, $this->addedcategories)) { // The category itself has been loaded already so we just need to ensure its subcategories // have been loaded list($sql, $params) = $DB->get_in_or_equal(array_keys($this->addedcategories), SQL_PARAMS_NAMED, 'parent', false); if ($showbasecategories) { // We need to include categories with parent = 0 as well - $sql = "SELECT * - FROM {course_categories} cc - WHERE (parent = :categoryid OR parent = 0) AND - parent {$sql} - ORDER BY depth DESC, sortorder ASC, id ASC"; + $sqlwhere .= " AND (parent = :categoryid OR parent = 0) AND parent {$sql}"; } else { - $sql = "SELECT * - FROM {course_categories} cc - WHERE parent = :categoryid AND - parent {$sql} - ORDER BY depth DESC, sortorder ASC, id ASC"; + // All we need is categories that match the parent + $sqlwhere .= " AND parent = :categoryid AND parent {$sql}"; } $params['categoryid'] = $categoryid; - $categories = $DB->get_records_sql($sql, $params); - if (count($categories) == 0) { - // There are no further categories that require loading. - return; - } } else { // This category hasn't been loaded yet so we need to fetch it, work out its category path // and load this category plus all its parents and subcategories $category = $DB->get_record('course_categories', array('id' => $categoryid), 'path', MUST_EXIST); $categoriestoload = explode('/', trim($category->path, '/')); list($select, $params) = $DB->get_in_or_equal($categoriestoload); - $select = 'id '.$select.' OR parent '.$select; - if ($showbasecategories) { - $select .= ' OR depth = 1'; - } + // We are going to use select twice so double the params $params = array_merge($params, $params); - $categories = $DB->get_records_select('course_categories', $select, $params, 'sortorder'); + $basecategorysql = ($showbasecategories)?' OR depth = 1':''; + $sqlwhere .= " AND (id {$select} OR parent {$select}{$basecategorysql})"; } + $categoriesrs = $DB->get_recordset_sql("$sqlselect $sqlwhere $sqlorder", $params); + $categories = array(); + foreach ($categoriesrs as $category) { + // Preload the context.. we'll need it when adding the category in order + // to format the category name. + context_helper::preload_from_record($category); + if (array_key_exists($category->id, $this->addedcategories)) { + // Do nothing, its already been added. + } else if ($category->parent == '0') { + // This is a root category lets add it immediately + $this->add_category($category, $this->rootnodes['courses']); + } else if (array_key_exists($category->parent, $this->addedcategories)) { + // This categories parent has already been added we can add this immediately + $this->add_category($category, $this->addedcategories[$category->parent]); + } else { + $categories[] = $category; + } + } + $categoriesrs->close(); + // Now we have an array of categories we need to add them to the navigation. while (!empty($categories)) { $category = reset($categories); From d4bb6462fcc24b5f3ee67a3d5a3203a5e78c6f87 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Fri, 30 Mar 2012 13:27:12 +1300 Subject: [PATCH 4/7] MDL-28967 navigation: Fixed up AJAX loading of categories --- lib/navigationlib.php | 80 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 65 insertions(+), 15 deletions(-) diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 7f0c3a48ac9..bb7216ee37c 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -1202,7 +1202,10 @@ class global_navigation extends navigation_node { case CONTEXT_COURSE : if ($issite) { // If it is the front page course, or a block on it then - // everything has already been loaded. + // all we need to do is load the root categories if required + if ($this->show_categories()) { + $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, $showcategories); + } break; } // Load the course associated with the page into the navigation @@ -1731,7 +1734,7 @@ class global_navigation extends navigation_node { } else if (is_object($category) && property_exists($category,'id')) { $coursecount = count($this->addedcategories[$category->id]->children->type(self::TYPE_COURSE)); } - return ($coursecount < $limit); + return ($coursecount <= $limit); } /** @@ -1771,10 +1774,10 @@ class global_navigation extends navigation_node { list($sql, $params) = $DB->get_in_or_equal(array_keys($this->addedcategories), SQL_PARAMS_NAMED, 'parent', false); if ($showbasecategories) { // We need to include categories with parent = 0 as well - $sqlwhere .= " AND (parent = :categoryid OR parent = 0) AND parent {$sql}"; + $sqlwhere .= " AND (cc.parent = :categoryid OR cc.parent = 0) AND cc.parent {$sql}"; } else { // All we need is categories that match the parent - $sqlwhere .= " AND parent = :categoryid AND parent {$sql}"; + $sqlwhere .= " AND cc.parent = :categoryid AND cc.parent {$sql}"; } $params['categoryid'] = $categoryid; } else { @@ -1785,8 +1788,8 @@ class global_navigation extends navigation_node { list($select, $params) = $DB->get_in_or_equal($categoriestoload); // We are going to use select twice so double the params $params = array_merge($params, $params); - $basecategorysql = ($showbasecategories)?' OR depth = 1':''; - $sqlwhere .= " AND (id {$select} OR parent {$select}{$basecategorysql})"; + $basecategorysql = ($showbasecategories)?' OR cc.depth = 1':''; + $sqlwhere .= " AND (cc.id {$select} OR cc.parent {$select}{$basecategorysql})"; } $categoriesrs = $DB->get_recordset_sql("$sqlselect $sqlwhere $sqlorder", $params); @@ -2877,15 +2880,7 @@ class global_navigation_for_ajax extends global_navigation { // Branchtype will be one of navigation_node::TYPE_* switch ($this->branchtype) { case self::TYPE_CATEGORY : - $this->load_all_categories($this->instanceid); - $limit = 20; - if (!empty($CFG->navcourselimit)) { - $limit = (int)$CFG->navcourselimit; - } - $courses = $DB->get_records('course', array('category' => $this->instanceid), 'sortorder','*', 0, $limit); - foreach ($courses as $course) { - $this->add_course($course); - } + $this->load_category($this->instanceid); break; case self::TYPE_COURSE : $course = $DB->get_record('course', array('id' => $this->instanceid), '*', MUST_EXIST); @@ -2945,6 +2940,61 @@ class global_navigation_for_ajax extends global_navigation { return $this->expandable; } + /** + * Loads a single category into the AJAX navigation. + * + * This function is special in that it doesn't concern itself with the parent of + * the requested category or its siblings. + * This is because with the AJAX navigation we know exactly what is wanted and only need to + * request that. + * + * @global moodle_database $DB + * @param int $categoryid + */ + protected function load_category($categoryid) { + global $CFG, $DB; + + $limit = 20; + if (!empty($CFG->navcourselimit)) { + $limit = (int)$CFG->navcourselimit; + } + + $catcontextsql = context_helper::get_preload_record_columns_sql('ctx'); + $sql = "SELECT cc.*, $catcontextsql + FROM {course_categories} cc + JOIN {context} ctx ON cc.id = ctx.instanceid + WHERE ctx.contextlevel = ".CONTEXT_COURSECAT." AND + (cc.id = :categoryid1 OR cc.parent = :categoryid2) + ORDER BY depth ASC, sortorder ASC, id ASC"; + $params = array('categoryid1' => $categoryid, 'categoryid2' => $categoryid); + $categories = $DB->get_recordset_sql($sql, $params, 0, $limit); + $subcategories = array(); + $basecategory = null; + foreach ($categories as $category) { + context_helper::preload_from_record($category); + if ($category->id == $categoryid) { + $this->add_category($category, $this); + $basecategory = $this->addedcategories[$category->id]; + } else { + $subcategories[] = $category; + } + } + $categories->close(); + + if (!is_null($basecategory)) { + //echo "
".print_r($subcategories, true).'
'; + foreach ($subcategories as $category) { + $this->add_category($category, $basecategory); + } + } + + $courses = $DB->get_recordset('course', array('category' => $categoryid), 'sortorder','*', 0, $limit); + foreach ($courses as $course) { + $this->add_course($course); + } + $courses->close(); + } + /** * Returns an array of expandable nodes * @return array From f7ee4baaee74ce1edecde3bc489e9c1f02fcd197 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Fri, 30 Mar 2012 13:32:05 +1300 Subject: [PATCH 5/7] MDL-28967 navigation: Removed buggy caching of course loading information --- lib/navigationlib.php | 224 ++++++++++++++++-------------------------- 1 file changed, 84 insertions(+), 140 deletions(-) diff --git a/lib/navigationlib.php b/lib/navigationlib.php index bb7216ee37c..a730557d309 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -1521,15 +1521,6 @@ class global_navigation extends navigation_node { $limit = $CFG->navcourselimit; } - // Work out the key to use for caching. - if (is_array($categoryids)) { - $cachekey = sprintf('load_all_courses_%d_%s', $limit, md5(join('_', $categoryids))); - } else if (is_string($categoryids) || is_int($categoryids)) { - $cachekey = sprintf('load_all_courses_%d_%s', $limit, md5($categoryids)); - } else { - $cachekey = sprintf('load_all_courses_%d', $limit); - } - $toload = (empty($CFG->navshowallcourses))?self::LOAD_ROOT_CATEGORIES:self::LOAD_ALL_CATEGORIES; // If we are going to show all courses AND we are showing categories then @@ -1541,70 +1532,90 @@ class global_navigation extends navigation_node { // Will be the return of our efforts $coursenodes = array(); - // Here we have a very important cache check. - // Loading all courses is a VERY costly buisness for two reasons. - // 1. We still have a limit per category of courses, however there is no - // great way to select a maximum per category in a single query. - // 2. Each course loaded is costly as we have permission checks, ajax tie - // in's and a great deal of code that will be executed. - if (!$this->cache->cached($cachekey)) { - // Check if we need to show categories. - if ($this->show_categories()) { - // Hmmm we need to show categories... this is going to be painful. - // We now need to fetch up to $limit courses for each category to - // be displayed. - if ($categoryids !== null) { - if (!is_array($categoryids)) { - $categoryids = array($categoryids); - } - list($categorywhere, $categoryparams) = $DB->get_in_or_equal($categoryids, SQL_PARAMS_NAMED, 'cc'); - $categorywhere = 'WHERE cc.id '.$categorywhere; - } else if ($toload == self::LOAD_ROOT_CATEGORIES) { - $categorywhere = 'WHERE cc.depth = 1 OR cc.depth = 2'; - $categoryparams = array(); - } else { - $categorywhere = ''; - $categoryparams = array(); + // Check if we need to show categories. + if ($this->show_categories()) { + // Hmmm we need to show categories... this is going to be painful. + // We now need to fetch up to $limit courses for each category to + // be displayed. + if ($categoryids !== null) { + if (!is_array($categoryids)) { + $categoryids = array($categoryids); } + list($categorywhere, $categoryparams) = $DB->get_in_or_equal($categoryids, SQL_PARAMS_NAMED, 'cc'); + $categorywhere = 'WHERE cc.id '.$categorywhere; + } else if ($toload == self::LOAD_ROOT_CATEGORIES) { + $categorywhere = 'WHERE cc.depth = 1 OR cc.depth = 2'; + $categoryparams = array(); + } else { + $categorywhere = ''; + $categoryparams = array(); + } - // First up we are going to get the categories that we are going to - // need so that we can determine how best to load the courses from them. - $sql = "SELECT cc.id, COUNT(c.id) AS coursecount - FROM {course_categories} cc - LEFT JOIN {course} c ON c.category = cc.id - {$categorywhere} - GROUP BY cc.id"; - $categories = $DB->get_recordset_sql($sql, $categoryparams); - $fullfetch = array(); - $partfetch = array(); - foreach ($categories as $category) { - if (!$this->can_add_more_courses_to_category($category->id)) { + // First up we are going to get the categories that we are going to + // need so that we can determine how best to load the courses from them. + $sql = "SELECT cc.id, COUNT(c.id) AS coursecount + FROM {course_categories} cc + LEFT JOIN {course} c ON c.category = cc.id + {$categorywhere} + GROUP BY cc.id"; + $categories = $DB->get_recordset_sql($sql, $categoryparams); + $fullfetch = array(); + $partfetch = array(); + foreach ($categories as $category) { + if (!$this->can_add_more_courses_to_category($category->id)) { + continue; + } + if ($category->coursecount > $limit * 5) { + $partfetch[] = $category->id; + } else if ($category->coursecount > 0) { + $fullfetch[] = $category->id; + } + } + $categories->close(); + + if (count($fullfetch)) { + // First up fetch all of the courses in categories where we know that we are going to + // need the majority of courses. + list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); + list($categoryids, $categoryparams) = $DB->get_in_or_equal($fullfetch, SQL_PARAMS_NAMED, 'lcategory'); + $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect + FROM {course} c + $ccjoin + WHERE c.category {$categoryids} AND + c.id {$courseids} + ORDER BY c.sortorder ASC"; + $coursesrs = $DB->get_recordset_sql($sql, $courseparams + $categoryparams); + foreach ($coursesrs as $course) { + if (!$this->can_add_more_courses_to_category($course->category)) { continue; } - if ($category->coursecount > $limit * 5) { - $partfetch[] = $category->id; - } else if ($category->coursecount > 0) { - $fullfetch[] = $category->id; + context_instance_preload($course); + if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + continue; } + $coursenodes[$course->id] = $this->add_course($course); } - $categories->close(); + $coursesrs->close(); + } - if (count($fullfetch)) { - // First up fetch all of the courses in categories where we know that we are going to - // need the majority of courses. + if (count($partfetch)) { + // Next we will work our way through the categories where we will likely only need a small + // proportion of the courses. + foreach ($partfetch as $categoryid) { list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); - list($categoryids, $categoryparams) = $DB->get_in_or_equal($fullfetch, SQL_PARAMS_NAMED, 'lcategory'); $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect - FROM {course} c - $ccjoin - WHERE c.category {$categoryids} AND - c.id {$courseids} - ORDER BY c.sortorder ASC"; - $coursesrs = $DB->get_recordset_sql($sql, $courseparams + $categoryparams); + FROM {course} c + $ccjoin + WHERE c.category = :categoryid AND + c.id {$courseids} + ORDER BY c.sortorder ASC"; + $courseparams['categoryid'] = $categoryid; + $coursesrs = $DB->get_recordset_sql($sql, $courseparams, 0, $limit * 5); foreach ($coursesrs as $course) { if (!$this->can_add_more_courses_to_category($course->category)) { - continue; + break; } context_instance_preload($course); if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { @@ -1614,97 +1625,30 @@ class global_navigation extends navigation_node { } $coursesrs->close(); } - - if (count($partfetch)) { - // Next we will work our way through the categories where we will likely only need a small - // proportion of the courses. - foreach ($partfetch as $categoryid) { - list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); - list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); - $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect - FROM {course} c - $ccjoin - WHERE c.category = :categoryid AND - c.id {$courseids} - ORDER BY c.sortorder ASC"; - $courseparams['categoryid'] = $categoryid; - $coursesrs = $DB->get_recordset_sql($sql, $courseparams, 0, $limit * 5); - foreach ($coursesrs as $course) { - if (!$this->can_add_more_courses_to_category($course->category)) { - break; - } - context_instance_preload($course); - if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { - continue; - } - $coursenodes[$course->id] = $this->add_course($course); - } - $coursesrs->close(); - } - } - } else { - // Prepare the SQL to load the courses and their contexts - list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); - list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses), SQL_PARAMS_NAMED, 'lc', false); - $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect - FROM {course} c - $ccjoin - WHERE c.id {$courseids} - ORDER BY c.sortorder ASC"; - $coursesrs = $DB->get_recordset_sql($sql, $courseparams); - foreach ($coursesrs as $course) { - context_instance_preload($course); - if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { - continue; - } - $coursenodes[$course->id] = $this->add_course($course); - if (count($coursenodes) >= $limit) { - break; - } - } - $coursesrs->close(); } - - // Cache the course id's that we've had to load to save us running these queries again. - $this->cache->set($cachekey, array_keys($coursenodes)); } else { - // YAY we've already cached this information - // First get the courses that we need to load from the navigation cache - $courseids = $this->cache->$cachekey; - - // Check to make sure we have some course nodes. - if (!count($courseids)) { - return $coursenodes; - } - - // Next check for courses that have already been loaded and remove them - // from the array we are about to load. - foreach (array_intersect($courseids, array_keys($this->addedcourses)) as $id) { - $key = array_search($id, $courseids); - unset($courseids[$key]); - } - - // Check that we still have course nodes. Any that have already been loaded - // will now have been removed. - if (!count($courseids)) { - return $coursenodes; - } - // Prepare the SQL to load the courses and their contexts list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); - list($courseids, $courseparams) = $DB->get_in_or_equal($courseids, SQL_PARAMS_NAMED, 'lc'); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses), SQL_PARAMS_NAMED, 'lc', false); $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect - FROM {course} c - $ccjoin - WHERE c.id {$courseids} - ORDER BY c.sortorder ASC"; + FROM {course} c + $ccjoin + WHERE c.id {$courseids} + ORDER BY c.sortorder ASC"; $coursesrs = $DB->get_recordset_sql($sql, $courseparams); foreach ($coursesrs as $course) { context_instance_preload($course); + if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + continue; + } $coursenodes[$course->id] = $this->add_course($course); + if (count($coursenodes) >= $limit) { + break; + } } $coursesrs->close(); } + return $coursenodes; } From 80c695228efe7800234698088d4148085dc4374c Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Thu, 19 Apr 2012 10:27:53 +1200 Subject: [PATCH 6/7] MDL-28967 navigation: courses branch is only shown if all courses are loaded or you're not enrolled in any course --- lib/navigationlib.php | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/lib/navigationlib.php b/lib/navigationlib.php index a730557d309..acb99d234ff 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -1056,6 +1056,11 @@ class global_navigation extends navigation_node { $this->rootnodes['courses'] = $this->add(get_string('courses'), new moodle_url('/course/index.php'), self::TYPE_ROOTNODE, null, 'courses'); $this->rootnodes['users'] = $this->add(get_string('users'), null, self::TYPE_ROOTNODE, null, 'users'); + // We always load the frontpage course to ensure it is available without + // JavaScript enabled. + $this->add_front_page_course_essentials($this->rootnodes['site'], $SITE); + $this->load_course_sections($SITE, $this->rootnodes['site']); + // Fetch all of the users courses. $mycourses = enrol_get_my_courses(); // We need to show categories if we can show categories and the user isn't enrolled in any courses or we're not showing all courses @@ -1063,7 +1068,7 @@ class global_navigation extends navigation_node { // $issite gets set to true if the current pages course is the sites frontpage course $issite = ($this->page->course->id == SITEID); // $ismycourse gets set to true if the user is enrolled in the current pages course. - $ismycourse = (array_key_exists($this->page->course->id, $mycourses)); + $ismycourse = !$issite && (array_key_exists($this->page->course->id, $mycourses)); // Check if any courses were returned. if (count($mycourses) > 0) { @@ -1176,26 +1181,20 @@ class global_navigation extends navigation_node { $this->load_all_courses(); } - // We always load the frontpage course to ensure it is available without - // JavaScript enabled. - $frontpagecourse = $this->load_course($SITE); - $this->add_front_page_course_essentials($frontpagecourse, $SITE); - $this->load_course_sections($SITE, $frontpagecourse); - $canviewcourseprofile = true; // Next load context specific content into the navigation switch ($this->page->context->contextlevel) { case CONTEXT_SYSTEM : // This has already been loaded we just need to map the variable - if ($this->show_categories()) { - $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, $showcategories); + if ($showcategories) { + $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, true); } break; case CONTEXT_COURSECAT : // This has already been loaded we just need to map the variable - if ($this->show_categories()) { - $this->load_all_categories($this->page->context->instanceid, $showcategories); + if ($showcategories) { + $this->load_all_categories($this->page->context->instanceid, true); } break; case CONTEXT_BLOCK : @@ -1203,14 +1202,16 @@ class global_navigation extends navigation_node { if ($issite) { // If it is the front page course, or a block on it then // all we need to do is load the root categories if required - if ($this->show_categories()) { - $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, $showcategories); + if ($showcategories) { + $this->load_all_categories(self::LOAD_ROOT_CATEGORIES, true); } break; } // Load the course associated with the page into the navigation $course = $this->page->course; if ($this->show_categories() && !$ismycourse) { + // The user isn't enrolled in the course and we need to show categories in which case we need + // to load the category relating to the course and depending up $showcategories all of the root categories as well. $this->load_all_categories($course->category, $showcategories); } $coursenode = $this->load_course($course); @@ -1383,6 +1384,7 @@ class global_navigation extends navigation_node { break; } + // Look for all categories which have been loaded if ($showcategories) { $categories = $this->find_all_of_type(self::TYPE_CATEGORY); if (count($categories) !== 0) { @@ -1842,10 +1844,13 @@ class global_navigation extends navigation_node { */ protected function load_course(stdClass $course) { if ($course->id == SITEID) { + // This is always loaded during initialisation return $this->rootnodes['site']; } else if (array_key_exists($course->id, $this->addedcourses)) { + // The course has already been loaded so return a reference return $this->addedcourses[$course->id]; } else { + // Add the course return $this->add_course($course); } } @@ -2502,7 +2507,7 @@ class global_navigation extends navigation_node { if (!empty($course->category) && $this->show_categories()) { if ($this->show_categories() && !$this->is_category_fully_loaded($course->category)) { // We need to load the category structure for this course - $this->load_all_categories($course->category); + $this->load_all_categories($course->category, false); } if (array_key_exists($course->category, $this->addedcategories)) { $parent = $this->addedcategories[$course->category]; From 98556b23c70b3987248b8a9a00d02e9a1c2bc3a6 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Mon, 7 May 2012 12:57:42 +1200 Subject: [PATCH 7/7] MDL-28967 navigation: PHPdoc and coding style cleanup --- lib/navigationlib.php | 93 +++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 48 deletions(-) diff --git a/lib/navigationlib.php b/lib/navigationlib.php index acb99d234ff..446b0117aac 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -1,5 +1,4 @@ . /** - * This file contains classes used to manage the navigation structures in Moodle - * and was introduced as part of the changes occuring in Moodle 2.0 + * This file contains classes used to manage the navigation structures within Moodle. * * @since 2.0 * @package core @@ -1066,7 +1064,7 @@ class global_navigation extends navigation_node { // We need to show categories if we can show categories and the user isn't enrolled in any courses or we're not showing all courses $showcategories = ($this->show_categories() && (count($mycourses) == 0 || !empty($CFG->navshowallcourses))); // $issite gets set to true if the current pages course is the sites frontpage course - $issite = ($this->page->course->id == SITEID); + $issite = ($this->page->course->id == $SITE->id); // $ismycourse gets set to true if the user is enrolled in the current pages course. $ismycourse = !$issite && (array_key_exists($this->page->course->id, $mycourses)); @@ -1312,7 +1310,7 @@ class global_navigation extends navigation_node { // Load the course sections into the page $sections = $this->load_course_sections($course, $coursenode); - if ($course->id != SITEID) { + if ($course->id != $SITE->id) { // Find the section for the $CM associated with the page and collect // its section number. if ($sectionnum) { @@ -1414,7 +1412,7 @@ class global_navigation extends navigation_node { // Load for the current user $this->load_for_user(); - if ($this->page->context->contextlevel >= CONTEXT_COURSE && $this->page->context->instanceid != SITEID && $canviewcourseprofile) { + if ($this->page->context->contextlevel >= CONTEXT_COURSE && $this->page->context->instanceid != $SITE->id && $canviewcourseprofile) { $this->load_for_user(null, true); } // Load each extending user into the navigation. @@ -1515,7 +1513,7 @@ class global_navigation extends navigation_node { * @return array An array of navigation_nodes one for each course */ protected function load_all_courses($categoryids = null) { - global $CFG, $DB; + global $CFG, $DB, $SITE; // Work out the limit of courses. $limit = 20; @@ -1579,7 +1577,7 @@ class global_navigation extends navigation_node { // First up fetch all of the courses in categories where we know that we are going to // need the majority of courses. list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); - list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array($SITE->id), SQL_PARAMS_NAMED, 'lcourse', false); list($categoryids, $categoryparams) = $DB->get_in_or_equal($fullfetch, SQL_PARAMS_NAMED, 'lcategory'); $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect FROM {course} c @@ -1593,7 +1591,7 @@ class global_navigation extends navigation_node { continue; } context_instance_preload($course); - if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + if ($course->id != $SITE->id && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { continue; } $coursenodes[$course->id] = $this->add_course($course); @@ -1606,7 +1604,7 @@ class global_navigation extends navigation_node { // proportion of the courses. foreach ($partfetch as $categoryid) { list($ccselect, $ccjoin) = context_instance_preload_sql('c.id', CONTEXT_COURSE, 'ctx'); - list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array(SITEID), SQL_PARAMS_NAMED, 'lcourse', false); + list($courseids, $courseparams) = $DB->get_in_or_equal(array_keys($this->addedcourses) + array($SITE->id), SQL_PARAMS_NAMED, 'lcourse', false); $sql = "SELECT c.id, c.sortorder, c.visible, c.fullname, c.shortname, c.category $ccselect FROM {course} c $ccjoin @@ -1620,7 +1618,7 @@ class global_navigation extends navigation_node { break; } context_instance_preload($course); - if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + if ($course->id != $SITE->id && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { continue; } $coursenodes[$course->id] = $this->add_course($course); @@ -1640,7 +1638,7 @@ class global_navigation extends navigation_node { $coursesrs = $DB->get_recordset_sql($sql, $courseparams); foreach ($coursesrs as $course) { context_instance_preload($course); - if ($course->id != SITEID && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { + if ($course->id != $SITE->id && !$course->visible && !is_role_switched($course->id) && !has_capability('moodle/course:viewhiddencourses', get_context_instance(CONTEXT_COURSE, $course->id))) { continue; } $coursenodes[$course->id] = $this->add_course($course); @@ -1657,9 +1655,8 @@ class global_navigation extends navigation_node { /** * Returns true if more courses can be added to the provided category. * - * @global type $CFG - * @param type $categoryid - * @return type + * @param int|navigation_node|stdClass $category + * @return bool */ protected function can_add_more_courses_to_category($category) { global $CFG; @@ -1843,7 +1840,8 @@ class global_navigation extends navigation_node { * @return navigation_node */ protected function load_course(stdClass $course) { - if ($course->id == SITEID) { + global $SITE; + if ($course->id == $SITE->id) { // This is always loaded during initialisation return $this->rootnodes['site']; } else if (array_key_exists($course->id, $this->addedcourses)) { @@ -1955,7 +1953,7 @@ class global_navigation extends navigation_node { * @return array An array of course section nodes */ public function load_generic_course_sections(stdClass $course, navigation_node $coursenode, $courseformat='unknown') { - global $CFG, $DB, $USER; + global $CFG, $DB, $USER, $SITE; require_once($CFG->dirroot.'/course/lib.php'); list($sections, $activities) = $this->generate_sections_and_activities($course); @@ -1981,7 +1979,7 @@ class global_navigation extends navigation_node { $navigationsections = array(); foreach ($sections as $sectionid => $section) { $section = clone($section); - if ($course->id == SITEID) { + if ($course->id == $SITE->id) { $this->load_section_activities($coursenode, $section->section, $activities); } else { if ((!$viewhiddensections && !$section->visible) || (!$this->showemptysections && @@ -2026,7 +2024,7 @@ class global_navigation extends navigation_node { * @return array Array of activity nodes */ protected function load_section_activities(navigation_node $sectionnode, $sectionnumber, array $activities, $course = null) { - global $CFG; + global $CFG, $SITE; // A static counter for JS function naming static $legacyonclickcounter = 0; @@ -2041,7 +2039,7 @@ class global_navigation extends navigation_node { } else { $courseid = $course->id; } - $showactivities = ($courseid != SITEID || !empty($CFG->navshowfrontpagemods)); + $showactivities = ($courseid != $SITE->id || !empty($CFG->navshowfrontpagemods)); foreach ($activities as $activity) { if ($activity->section != $sectionnumber) { @@ -2178,7 +2176,7 @@ class global_navigation extends navigation_node { * @return bool */ protected function load_for_user($user=null, $forceforcontext=false) { - global $DB, $CFG, $USER; + global $DB, $CFG, $USER, $SITE; if ($user === null) { // We can't require login here but if the user isn't logged in we don't @@ -2206,7 +2204,7 @@ class global_navigation extends navigation_node { // Get the course set against the page, by default this will be the site $course = $this->page->course; $baseargs = array('id'=>$user->id); - if ($course->id != SITEID && (!$iscurrentuser || $forceforcontext)) { + if ($course->id != $SITE->id && (!$iscurrentuser || $forceforcontext)) { $coursenode = $this->load_course($course); $baseargs['course'] = $course->id; $coursecontext = get_context_instance(CONTEXT_COURSE, $course->id); @@ -2367,7 +2365,7 @@ class global_navigation extends navigation_node { if ($haseditabletypes) { $usernode->add(get_string('repositories', 'repository'), new moodle_url('/repository/manage_instances.php', array('contextid' => $usercontext->id))); } - } else if ($course->id == SITEID && has_capability('moodle/user:viewdetails', $usercontext) && (!in_array('mycourses', $hiddenfields) || has_capability('moodle/user:viewhiddendetails', $coursecontext))) { + } else if ($course->id == $SITE->id && has_capability('moodle/user:viewdetails', $usercontext) && (!in_array('mycourses', $hiddenfields) || has_capability('moodle/user:viewhiddendetails', $coursecontext))) { // Add view grade report is permitted $reports = get_plugin_list('gradereport'); @@ -2468,7 +2466,7 @@ class global_navigation extends navigation_node { * @return navigation_node */ public function add_course(stdClass $course, $forcegeneric = false, $ismycourse = false) { - global $CFG; + global $CFG, $SITE; // We found the course... we can return it now :) if (!$forcegeneric && array_key_exists($course->id, $this->addedcourses)) { @@ -2477,7 +2475,7 @@ class global_navigation extends navigation_node { $coursecontext = get_context_instance(CONTEXT_COURSE, $course->id); - if ($course->id != SITEID && !$course->visible) { + if ($course->id != $SITE->id && !$course->visible) { if (is_role_switched($course->id)) { // user has to be able to access course in order to switch, let's skip the visibility test here } else if (!has_capability('moodle/course:viewhiddencourses', $coursecontext)) { @@ -2485,7 +2483,7 @@ class global_navigation extends navigation_node { } } - $issite = ($course->id == SITEID); + $issite = ($course->id == $SITE->id); $shortname = format_string($course->shortname, true, array('context' => $coursecontext)); if ($issite) { @@ -2550,9 +2548,9 @@ class global_navigation extends navigation_node { * @return bool returns true on successful addition of a node. */ public function add_course_essentials($coursenode, stdClass $course) { - global $CFG; + global $CFG, $SITE; - if ($course->id == SITEID) { + if ($course->id == $SITE->id) { return $this->add_front_page_course_essentials($coursenode, $course); } @@ -2564,7 +2562,7 @@ class global_navigation extends navigation_node { if (has_capability('moodle/course:viewparticipants', $this->page->context)) { $participants = $coursenode->add(get_string('participants'), new moodle_url('/user/index.php?id='.$course->id), self::TYPE_CONTAINER, get_string('participants'), 'participants'); $currentgroup = groups_get_course_group($course, true); - if ($course->id == SITEID) { + if ($course->id == $SITE->id) { $filterselect = ''; } else if ($course->id && !$currentgroup) { $filterselect = $course->id; @@ -2703,10 +2701,11 @@ class global_navigation extends navigation_node { * @return bool true when complete. */ public function set_expansion_limit($type) { + global $SITE; $nodes = $this->find_all_of_type($type); foreach ($nodes as &$node) { // We need to generate the full site node - if ($type == self::TYPE_COURSE && $node->key == SITEID) { + if ($type == self::TYPE_COURSE && $node->key == $SITE->id) { continue; } foreach ($node->children as &$child) { @@ -2768,8 +2767,7 @@ class global_navigation extends navigation_node { } /** - * The limited global navigation class used for the AJAX extension of the global - * navigation class. + * The global navigation class used especially for AJAX requests. * * The primary methods that are used in the global navigation class have been overriden * to ensure that only the relevant branch is generated at the root of the tree. @@ -2867,7 +2865,7 @@ class global_navigation_for_ajax extends global_navigation { require_course_login($course, true, $cm, false, true); $this->page->set_context(get_context_instance(CONTEXT_MODULE, $cm->id)); $coursenode = $this->load_course($course); - if ($course->id == SITEID) { + if ($course->id == $SITE->id) { $modulenode = $this->load_activity($cm, $course, $coursenode->find($cm->id, self::TYPE_ACTIVITY)); } else { $sections = $this->load_course_sections($course, $coursenode); @@ -2881,7 +2879,7 @@ class global_navigation_for_ajax extends global_navigation { return $this->expandable; } - if ($this->page->context->contextlevel == CONTEXT_COURSE && $this->page->context->instanceid != SITEID) { + if ($this->page->context->contextlevel == CONTEXT_COURSE && $this->page->context->instanceid != $SITE->id) { $this->load_for_user(null, true); } @@ -2937,7 +2935,7 @@ class global_navigation_for_ajax extends global_navigation { } } - $courses = $DB->get_recordset('course', array('category' => $categoryid), 'sortorder','*', 0, $limit); + $courses = $DB->get_recordset('course', array('category' => $categoryid), 'sortorder', '*' , 0, $limit); foreach ($courses as $course) { $this->add_course($course); } @@ -3131,11 +3129,11 @@ class navbar extends navigation_node { * end of the navbar * * @param string $text - * @param string|moodle_url $action - * @param int $type - * @param string|int $key + * @param string|moodle_url|action_link $action An action to associate with this node. + * @param int $type One of navigation_node::TYPE_* * @param string $shorttext - * @param string $icon + * @param string|int $key A key to identify this node with. Key + type is unique to a parent. + * @param pix_icon $icon An optional icon to use for this node. * @return navigation_node */ public function add($text, $action=null, $type=self::TYPE_CUSTOM, $shorttext=null, $key=null, pix_icon $icon=null) { @@ -3224,7 +3222,7 @@ class settings_navigation extends navigation_node { * */ public function initialise() { - global $DB, $SESSION; + global $DB, $SESSION, $SITE; if (during_initial_install()) { return false; @@ -3250,7 +3248,7 @@ class settings_navigation extends navigation_node { $this->load_category_settings(); break; case CONTEXT_COURSE: - if ($this->page->course->id != SITEID) { + if ($this->page->course->id != $SITE->id) { $this->load_course_settings(($context->id == $this->context->id)); } else { $this->load_front_page_settings(($context->id == $this->context->id)); @@ -3261,7 +3259,7 @@ class settings_navigation extends navigation_node { $this->load_course_settings(); break; case CONTEXT_USER: - if ($this->page->course->id != SITEID) { + if ($this->page->course->id != $SITE->id) { $this->load_course_settings(); } break; @@ -3880,7 +3878,7 @@ class settings_navigation extends navigation_node { * @param int $courseid The course id of the current course * @return navigation_node|false */ - protected function load_user_settings($courseid=SITEID) { + protected function load_user_settings($courseid = SITEID) { global $USER, $CFG; if (isguestuser() || !isloggedin()) { @@ -3956,7 +3954,7 @@ class settings_navigation extends navigation_node { protected function generate_user_settings($courseid, $userid, $gstitle='usercurrentsettings') { global $DB, $CFG, $USER, $SITE; - if ($courseid != SITEID) { + if ($courseid != $SITE->id) { if (!empty($this->page->course->id) && $this->page->course->id == $courseid) { $course = $this->page->course; } else { @@ -3997,7 +3995,7 @@ class settings_navigation extends navigation_node { $usercontext = get_context_instance(CONTEXT_USER, $user->id); // User context $canviewuser = has_capability('moodle/user:viewdetails', $usercontext); - if ($course->id == SITEID) { + if ($course->id == $SITE->id) { if ($CFG->forceloginforprofiles && !has_coursecontact_role($user->id) && !$canviewuser) { // Reduce possibility of "browsing" userbase at site level // Teachers can browse and be browsed at site level. If not forceloginforprofiles, allow access (bug #4366) return false; @@ -4037,7 +4035,7 @@ class settings_navigation extends navigation_node { $usersetting->add(get_string('userdeleted'), null, self::TYPE_SETTING); } else { // We can edit the user so show the user deleted message and link it to the profile - if ($course->id == SITEID) { + if ($course->id == $SITE->id) { $profileurl = new moodle_url('/user/profile.php', array('id'=>$user->id)); } else { $profileurl = new moodle_url('/user/view.php', array('id'=>$user->id, 'course'=>$course->id)); @@ -4469,8 +4467,7 @@ class navigation_json { } /** - * The cache class used by global navigation and settings navigation to cache bits - * and bobs that are used during their generation. + * The cache class used by global navigation and settings navigation. * * It is basically an easy access point to session with a bit of smarts to make * sure that the information that is cached is valid still.