mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-18 05:58:34 +01:00
Code Issues Projects Releases Wiki Activity

MDL-31830 course: several small code improvements

Browse Source 
* I can rebase this for you/the integrator before it goes in for sure no probs :)
* Fixed double phpdoc block of course_change_visibility
* Moved permission checks out of course_move_after_course and into helper function.
* Reviewed setType calls for editcategory_form.php.
* Reviewed all uses of can_resort and added more specific methods.
* Fixed method mentioned in exception for resort methods.
* Converted calls to fetch courses to call get_course.
* Exceptions now thrown when trying to move courses and problems arise.
* Fixed unnecessary namespace hinting in core_course_management_renderer.
* Abstracted common logic of can_resort_any and can_change_parent_any.
* Removed check for system level capability from has_manage_capability_on_any.
* Reviewed debugging calls I've introduced.
This commit is contained in:
Sam Hemelryk 2013-10-01 09:02:08 +13:00
parent a3b7439ff2
commit 67e1f26874
9 changed files with 128 additions and 122 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
course/ajax/management.php
View File

@ -58,7 +58,7 @@ switch ($action) {
case 'movecourseafter' :
$courseid = required_param('courseid', PARAM_INT);
$moveaftercourseid = required_param('moveafter', PARAM_INT);
$outcome->outcome = course_move_after_course($courseid, $moveaftercourseid);
$outcome->outcome = \core_course\management\helper::action_course_move_after_course($courseid, $moveaftercourseid);
break;
case 'hidecourse' :
$courseid = required_param('courseid', PARAM_INT);

1
course/classes/editcategory_form.php
View File

@ -76,7 +76,6 @@ class core_course_editcategory_form extends moodleform {
$mform->addElement('editor', 'description_editor', get_string('description'), null,
$this->get_description_editor_options());
$mform->setType('description_editor', PARAM_RAW);
if (!empty($CFG->allowcategorythemes)) {
$themes = array(''=>get_string('forceno'));

53
course/classes/management/helper.php
View File

@ -176,7 +176,7 @@ class helper {
}
// Move up/down.
if ($category->can_resort()) {
if ($category->can_change_sortorder()) {
$actions['moveup'] = array(
'url' => new \moodle_url($baseurl, array('action' => 'movecategoryup')),
'icon' => new \pix_icon('t/up', new \lang_string('up')),
@ -318,7 +318,7 @@ class helper {
* @throws \moodle_exception
*/
public static function action_course_moveup(\course_in_list $course, \coursecat $category) {
if (!$category->can_resort()) {
if (!$category->can_resort_courses()) {
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
}
return course_move_by_one($course, true);
@ -333,7 +333,7 @@ class helper {
* @throws \moodle_exception
*/
public static function action_course_movedown(\course_in_list $course, \coursecat $category) {
if (!$category->can_resort()) {
if (!$category->can_resort_courses()) {
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
}
return course_move_by_one($course, false);
@ -347,9 +347,8 @@ class helper {
* @return bool
*/
public static function action_course_moveup_by_record($courserecordorid) {
global $DB;
if (is_int($courserecordorid)) {
$courserecordorid = $DB->get_record('course', array('id' => $courserecordorid), '*', MUST_EXIST);
$courserecordorid = get_course($courserecordorid);
}
$course = new \course_in_list($courserecordorid);
$category = \coursecat::get($course->category);
@ -364,15 +363,32 @@ class helper {
* @return bool
*/
public static function action_course_movedown_by_record($courserecordorid) {
global $DB;
if (is_int($courserecordorid)) {
$courserecordorid = $DB->get_record('course', array('id' => $courserecordorid), '*', MUST_EXIST);
$courserecordorid = get_course($courserecordorid);
}
$course = new \course_in_list($courserecordorid);
$category = \coursecat::get($course->category);
return self::action_course_movedown($course, $category);
}
/**
* Changes the sort order so that the first course appears after the second course.
*
* @param int|stdClass $courserecordorid
* @param int $moveaftercourseid
* @return bool
* @throws \moodle_exception
*/
public static function action_course_move_after_course($courserecordorid, $moveaftercourseid) {
$course = \get_course($courserecordorid);
$category = \coursecat::get($course->category);
if (!$category->can_resort_courses()) {
$url = '/course/management.php?categoryid='.$course->category;
throw new \moodle_exception('nopermissions', 'error', $url, \get_string('resortcourses', 'moodle'));
}
return \course_move_after_course($course, $moveaftercourseid);
}
/**
* Makes a course visible given a \course_in_list object.
*
@ -409,9 +425,8 @@ class helper {
* @return bool
*/
public static function action_course_show_by_record($courserecordorid) {
global $DB;
if (is_int($courserecordorid)) {
$courserecordorid = $DB->get_record('course', array('id' => $courserecordorid), '*', MUST_EXIST);
$courserecordorid = get_course($courserecordorid);
}
$course = new \course_in_list($courserecordorid);
return self::action_course_show($course);
@ -425,9 +440,8 @@ class helper {
* @return bool
*/
public static function action_course_hide_by_record($courserecordorid) {
global $DB;
if (is_int($courserecordorid)) {
$courserecordorid = $DB->get_record('course', array('id' => $courserecordorid), '*', MUST_EXIST);
$courserecordorid = get_course($courserecordorid);
}
$course = new \course_in_list($courserecordorid);
return self::action_course_hide($course);
@ -442,7 +456,7 @@ class helper {
*/
public static function action_category_moveup(\coursecat $category) {
if (!$category->can_change_sortorder()) {
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_change_sortorder');
}
return $category->move_by_one(true);
}
@ -456,7 +470,7 @@ class helper {
*/
public static function action_category_movedown(\coursecat $category) {
if (!$category->can_change_sortorder()) {
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_change_sortorder');
}
return $category->move_by_one(false);
}
@ -547,7 +561,7 @@ class helper {
* @throws \moodle_exception
*/
public static function action_category_resort_subcategories(\coursecat $category, $sort, $cleanup = true) {
if (!$category->can_resort()) {
if (!$category->can_resort_subcategories()) {
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
}
return $category->resort_subcategories($sort, $cleanup);
@ -562,7 +576,7 @@ class helper {
* @throws \moodle_exception
*/
public static function action_category_resort_courses(\coursecat $category, $sort) {
if (!$category->can_resort()) {
if (!$category->can_resort_courses()) {
throw new \moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
}
return $category->resort_courses($sort);
@ -671,8 +685,7 @@ class helper {
$moveto = \coursecat::get($categoryorid);
}
if (!$moveto->can_move_courses_out_of() || !$moveto->can_move_courses_into()) {
\debugging('Cannot move courses into the requested category.');
return false;
throw new \moodle_exception('cannotmovecourses');
}
list($where, $params) = $DB->get_in_or_equal($courseids, SQL_PARAMS_NAMED);
@ -682,8 +695,7 @@ class helper {
$checks = array();
foreach ($courseids as $id) {
if (!isset($courses[$id])) {
\debugging('Invalid course id given.', DEBUG_DEVELOPER);
return false;
throw new \moodle_exception('invalidcourseid');
}
$catid = $courses[$id]->category;
if (!isset($checks[$catid])) {
@ -691,8 +703,7 @@ class helper {
$checks[$catid] = $coursecat->can_move_courses_out_of() && $coursecat->can_move_courses_into();
}
if (!$checks[$catid]) {
\debugging("Cannot move course {$id} out of its category.", DEBUG_DEVELOPER);
return false;
throw new \moodle_exception('cannotmovecourses');
}
}
return \move_courses($courseids, $moveto->id);

12
course/classes/management_renderer.php
View File

@ -73,7 +73,7 @@ class core_course_management_renderer extends plugin_renderer_base {
$select = new single_select($this->page->url, 'categoryid', $categories, $categoryid, $nothing);
$html .= $this->render($select);
}
$html .= $this->view_mode_selector(core_course\management\helper::get_management_viewmodes(), $viewmode);
$html .= $this->view_mode_selector(\core_course\management\helper::get_management_viewmodes(), $viewmode);
}
$html .= html_writer::end_div();
return $html;
@ -119,7 +119,7 @@ class core_course_management_renderer extends plugin_renderer_base {
}
$catatlevel = array_shift($selectedparents);
$listing = \coursecat::get(0)->get_children();
$listing = coursecat::get(0)->get_children();
$html = html_writer::start_div('category-listing');
$html .= html_writer::tag('h3', get_string('categories'));
@ -258,7 +258,7 @@ class core_course_management_renderer extends plugin_renderer_base {
$menu->actionicon = new pix_icon('t/add', ' ', 'moodle', array('class' => 'iconsmall'));
$actions[] = $this->render($menu);
}
if ($category->can_resort()) {
if ($category->can_resort_subcategories()) {
$hasitems = true;
$params = $this->page->url->params();
$params['action'] = 'resortcategories';
@ -504,7 +504,7 @@ class core_course_management_renderer extends plugin_renderer_base {
$html = html_writer::start_tag('li', $attributes);
$html .= html_writer::start_div('clearfix');
if ($category->can_resort()) {
if ($category->can_resort_courses()) {
// In order for dnd to be available the user must be able to resort the category children..
$html .= html_writer::div($this->output->pix_icon('i/dragdrop', get_string('dndcourse')), 'float-left drag-handle');
}
@ -538,7 +538,7 @@ class core_course_management_renderer extends plugin_renderer_base {
$url = new moodle_url('/course/edit.php', array('category' => $category->id, 'returnto' => 'catmanage'));
$actions[] = html_writer::link($url, get_string('newcourse'));
}
if ($category->can_resort()) {
if ($category->can_resort_courses()) {
$params = $this->page->url->params();
$params['action'] = 'resortcourses';
$params['sesskey'] = sesskey();
@ -614,7 +614,7 @@ class core_course_management_renderer extends plugin_renderer_base {
);
}
// Move up/down.
if ($category->can_resort()) {
if ($category->can_resort_courses()) {
if (!$firstincategory) {
$actions[] = $this->action_icon(
new moodle_url($baseurl, array('action' => 'movecourseup')),

12
course/lib.php
View File

@ -3366,10 +3366,8 @@ function compare_activities_by_time_asc($a, $b) {
}
/**
* Changes the visibility of a course.
*
*/
/**
* Changes the visibility of a course
* @param int $courseid The course to change.
* @param bool $show True to make it visible, false otherwise.
* @return bool
@ -3417,7 +3415,7 @@ function course_move_by_one($course, $up) {
/**
* Move one course after another.
*
* @param int $courseid The course to move.
* @param int|stdClass $courseorid The course to move.
* @param int $moveaftercourseid The course to move after or 0 if you want it to be the first course in the category.
* @return bool
*/
@ -3429,12 +3427,6 @@ function course_move_after_course($courseorid, $moveaftercourseid) {
} else {
$course = $courseorid;
}
$category = coursecat::get($course->category);
if (!$category->can_resort()) {
// The user must be able to resort within the selected category.
\debugging('Cannot move courses within this category', DEBUG_DEVELOPER);
return false;
}
if ((int)$moveaftercourseid === 0) {
// We've moving the course to the start of the queue.

11
course/management.php
View File

@ -181,7 +181,7 @@ if ($action !== false && confirm_sesskey()) {
throw new moodle_exception('permissiondenied', 'error', '', null, 'coursecat::can_resort');
}
require_once($CFG->dirroot.'/course/delete_category_form.php');
$mform = new delete_category_form(null, $category);
$mform = new core_course_deletecategory_form(null, $category);
if ($mform->is_cancelled()) {
redirect(new moodle_url('/course/management.php'));
}
@ -233,7 +233,14 @@ if ($action !== false && confirm_sesskey()) {
break;
}
$moveto = coursecat::get($movetoid);
$redirectback = \core_course\management\helper::action_category_move_courses_into($category, $moveto, $courseids);
try {
// If this fails we want to catch the exception and report it.
$redirectback = \core_course\management\helper::action_category_move_courses_into($category, $moveto,
$courseids);
} catch (moodle_exception $ex) {
$redirectback = false;
$notificationsfail[] = $ex->getMessage();
}
} else if ($bulkmovecategories) {
$categoryids = optional_param_array('bcat', false, PARAM_INT);
$movetocatid = required_param('movecategoriesto', PARAM_INT);

36
course/tests/management_helper_test.php
View File

@ -491,9 +491,12 @@ class core_course_management_helper_test extends advanced_testcase {
// Try to move a course into sub2. This shouldn't be possible because you should always be able to undo what you've done.
// Try moving just one course.
$this->assertFalse(
\core_course\management\helper::action_category_move_courses_into($sub1, $sub2, array($course2->id))
);
try {
\core_course\management\helper::action_category_move_courses_into($sub1, $sub2, array($course2->id));
$this->fail('Invalid move of course between categories, action can\'t be undone.');
} catch (moodle_exception $ex) {
$this->assertEquals(get_string('cannotmovecourses', 'error'), $ex->getMessage());
}
// Nothing should have changed.
$this->assertEquals(1, $cat1->get_courses_count());
$this->assertEquals(0, $cat2->get_courses_count());
@ -503,9 +506,12 @@ class core_course_management_helper_test extends advanced_testcase {
// Now try moving a course out of sub2. Again should not be possible.
// Try to move a course into sub2. This shouldn't be possible because you should always be able to undo what you've done.
// Try moving just one course.
$this->assertFalse(
\core_course\management\helper::action_category_move_courses_into($sub2, $cat2, array($course4->id))
);
try {
\core_course\management\helper::action_category_move_courses_into($sub2, $cat2, array($course4->id));
$this->fail('Invalid move of course between categories, action can\'t be undone.');
} catch (moodle_exception $ex) {
$this->assertEquals(get_string('cannotmovecourses', 'error'), $ex->getMessage());
}
// Nothing should have changed.
$this->assertEquals(1, $cat1->get_courses_count());
$this->assertEquals(0, $cat2->get_courses_count());
@ -1259,9 +1265,12 @@ class core_course_management_helper_test extends advanced_testcase {
// Try to move a course into sub2. This shouldn't be possible because you should always be able to undo what you've done.
// Try moving just one course.
$this->assertFalse(
\core_course\management\helper::move_courses_into_category($sub2->id, array($course2->id))
);
try {
\core_course\management\helper::move_courses_into_category($sub2->id, array($course2->id));
$this->fail('Invalid move of course between categories, action can\'t be undone.');
} catch (moodle_exception $ex) {
$this->assertEquals(get_string('cannotmovecourses', 'error'), $ex->getMessage());
}
// Nothing should have changed.
$this->assertEquals(1, $cat1->get_courses_count());
$this->assertEquals(0, $cat2->get_courses_count());
@ -1271,9 +1280,12 @@ class core_course_management_helper_test extends advanced_testcase {
// Now try moving a course out of sub2. Again should not be possible.
// Try to move a course into sub2. This shouldn't be possible because you should always be able to undo what you've done.
// Try moving just one course.
$this->assertFalse(
\core_course\management\helper::move_courses_into_category($cat2->id, array($course4->id))
);
try {
\core_course\management\helper::move_courses_into_category($cat2->id, array($course4->id));
$this->fail('Invalid move of course between categories, action can\'t be undone.');
} catch (moodle_exception $ex) {
$this->assertEquals(get_string('cannotmovecourses', 'error'), $ex->getMessage());
}
// Nothing should have changed.
$this->assertEquals(1, $cat1->get_courses_count());
$this->assertEquals(0, $cat2->get_courses_count());

1
lang/en/error.php
View File

@ -110,6 +110,7 @@ $string['cannotmigratedatacomments'] = 'Cannot migrate data module comments';
$string['cannotmodulename'] = 'Cannot get the module name in build navigation';
$string['cannotmoduletype'] = 'Cannot get the module type in build navigation';
$string['cannotmovecategory'] = 'Cannot move category';
$string['cannotmovecourses'] = 'Cannot move courses from the category they are in to another.';
$string['cannotmoverolewithid'] = 'Cannot move role with ID {$a}';
$string['cannotopencsv'] = 'Cannot open CSV file';
$string['cannotopenfile'] = 'Cannot open file ({$a})';

122
lib/coursecatlib.php
View File

@ -1093,84 +1093,60 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate {
return $rv;
}
/**
* Returns true if the user has the manage capability on any category.
*
* This method uses the coursecat cache and an entry `has_manage_capability` to speed up
* calls to this method.
*
* @return bool
*/
protected static function has_manage_capability_on_any() {
global $DB;
if (!isloggedin() || isguestuser()) {
return false;
}
$cache = cache::make('core', 'coursecat');
$has = $cache->get('has_manage_capability');
if ($has !== false) {
return ((int)$has === 1);
}
$has = false;
$fields = context_helper::get_preload_record_columns_sql('ctx');
$sql = "SELECT ctx.instanceid AS categoryid, $fields
FROM {context} ctx
WHERE contextlevel = :contextlevel
ORDER BY depth ASC";
$params = array('contextlevel' => CONTEXT_COURSECAT);
$recordset = $DB->get_recordset_sql($sql, $params);
foreach ($recordset as $context) {
context_helper::preload_from_record($context);
$context = context_coursecat::instance($context->categoryid);
if (has_capability('moodle/category:manage', $context)) {
$has = true;
break;
}
}
$recordset->close();
$cache->set('has_manage_capability', ($has) ? '1' : '0');
return $has;
}
/**
* Returns true if the user can resort any category.
* @return bool
*/
public static function can_resort_any() {
global $DB;
if (!isloggedin() || isguestuser()) {
return false;
}
$cache = cache::make('core', 'coursecat');
$canresortany = $cache->get('can_resort_any');
if ($canresortany !== false) {
return ((int)$canresortany === 1);
}
$canresortany = false;
if (self::get(0)->can_resort()) {
$canresortany = true;
} else {
$fields = context_helper::get_preload_record_columns_sql('ctx');
$sql = "SELECT ctx.instanceid AS categoryid, $fields
FROM {context} ctx
WHERE contextlevel = :contextlevel
ORDER BY depth ASC";
$params = array('contextlevel' => CONTEXT_COURSECAT);
$recordset = $DB->get_recordset_sql($sql, $params);
foreach ($recordset as $context) {
context_helper::preload_from_record($context);
$context = context_coursecat::instance($context->categoryid);
if (has_capability('moodle/category:manage', $context)) {
$canresortany = true;
break;
}
}
$recordset->close();
}
$cache->set('can_resort_any', $canresortany ? 1 : 0);
$cache->set('can_change_parent_any', $canresortany ? 1 : 0);
return $canresortany;
return self::has_manage_capability_on_any();
}
/**
* Returns trye if the user can change the parent of any category.
* Returns true if the user can change the parent of any category.
* @return bool
*/
public static function can_change_parent_any() {
global $DB;
if (!isloggedin() || isguestuser()) {
return false;
}
$cache = cache::make('core', 'coursecat');
$canchangeany = $cache->get('can_change_parent_any');
if ($canchangeany !== false) {
return ((int)$canchangeany === 1);
}
$canchangeany = false;
if (self::get(0)->has_manage_capability()) {
$canchangeany = true;
} else {
$fields = context_helper::get_preload_record_columns_sql('ctx');
$sql = "SELECT ctx.instanceid AS categoryid, $fields
FROM {context} ctx
WHERE contextlevel = :contextlevel
ORDER BY depth ASC";
$params = array('contextlevel' => CONTEXT_COURSECAT);
$recordset = $DB->get_recordset_sql($sql, $params);
foreach ($recordset as $context) {
context_helper::preload_from_record($context);
$context = context_coursecat::instance($context->categoryid);
if (has_capability('moodle/category:manage', $context)) {
$canchangeany = true;
break;
}
}
$recordset->close();
}
$cache->set('can_resort_any', $canchangeany ? 1 : 0);
$cache->set('can_change_parent_any', $canchangeany ? 1 : 0);
return $canchangeany;
return self::has_manage_capability_on_any();
}
/**
@ -2246,12 +2222,20 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate {
* Returns true if the user can resort this categories sub categories and courses.
* @return bool
*/
public function can_resort() {
public function can_resort_subcategories() {
return $this->has_manage_capability();
}
/**
* Returns true of the user can change the sortorder of this category.
* Returns true if the user can resort the courses within this category.
* @return bool
*/
public function can_resort_courses() {
return $this->has_manage_capability();
}
/**
* Returns true of the user can change the sortorder of this category (resort the parent category)
* @return bool
*/
public function can_change_sortorder() {