From 98d9af3cdbe1fe2e0986c406aab634761003da01 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Thu, 2 Feb 2017 16:21:51 +0800 Subject: [PATCH] MDL-57769 format_weeks: remove numsections option --- course/format/weeks/db/upgrade.php | 47 +++++++ course/format/weeks/db/upgradelib.php | 117 ++++++++++++++++ course/format/weeks/format.php | 4 +- course/format/weeks/lib.php | 78 +++-------- .../tests/behat/edit_delete_sections.feature | 33 +---- .../format/weeks/tests/format_weeks_test.php | 29 +--- .../weeks/tests/format_weeks_upgrade_test.php | 128 ++++++++++++++++++ course/format/weeks/version.php | 2 +- 8 files changed, 319 insertions(+), 119 deletions(-) create mode 100644 course/format/weeks/db/upgrade.php create mode 100644 course/format/weeks/db/upgradelib.php create mode 100644 course/format/weeks/tests/format_weeks_upgrade_test.php diff --git a/course/format/weeks/db/upgrade.php b/course/format/weeks/db/upgrade.php new file mode 100644 index 00000000000..97516e77cf2 --- /dev/null +++ b/course/format/weeks/db/upgrade.php @@ -0,0 +1,47 @@ +. + +/** + * Upgrade scripts for course format "Weeks" + * + * @package format_weeks + * @copyright 2017 Marina Glancy + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Upgrade script for format_weeks + * + * @param int $oldversion the version we are upgrading from + * @return bool result + */ +function xmldb_format_weeks_upgrade($oldversion) { + global $CFG, $DB; + + require_once($CFG->dirroot . '/course/format/weeks/db/upgradelib.php'); + + if ($oldversion < 2017020200) { + + // Remove 'numsections' option and hide or delete orphaned sections. + format_weeks_upgrade_remove_numsections(); + + upgrade_plugin_savepoint(true, 2017020200, 'format', 'weeks'); + } + + return true; +} diff --git a/course/format/weeks/db/upgradelib.php b/course/format/weeks/db/upgradelib.php new file mode 100644 index 00000000000..26571f1a56f --- /dev/null +++ b/course/format/weeks/db/upgradelib.php @@ -0,0 +1,117 @@ +. + +/** + * Upgrade scripts for course format "Weeks" + * + * @package format_weeks + * @copyright 2017 Marina Glancy + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * This method finds all courses in 'weeks' format that have actual number of sections + * bigger than their 'numsections' course format option. + * For each such course we call {@link format_weeks_upgrade_hide_extra_sections()} and + * either delete or hide "orphaned" sections. + */ +function format_weeks_upgrade_remove_numsections() { + global $DB; + + $sql1 = "SELECT c.id, max(cs.section) AS sectionsactual + FROM {course} c + JOIN {course_sections} cs ON cs.course = c.id + WHERE c.format = :format1 + GROUP BY c.id"; + + $sql2 = "SELECT c.id, n.value AS numsections + FROM {course} c + JOIN {course_format_options} n ON n.courseid = c.id AND n.format = :format1 AND n.name = :numsections AND n.sectionid = 0 + WHERE c.format = :format2"; + + $params = ['format1' => 'weeks', 'format2' => 'weeks', 'numsections' => 'numsections']; + + $actual = $DB->get_records_sql_menu($sql1, $params); + $numsections = $DB->get_records_sql_menu($sql2, $params); + $needfixing = []; + + $defaultnumsections = get_config('moodlecourse', 'numsections'); + + foreach ($actual as $courseid => $sectionsactual) { + if (array_key_exists($courseid, $numsections)) { + $n = (int)$numsections[$courseid]; + } else { + $n = $defaultnumsections; + } + if ($sectionsactual > $n) { + $needfixing[$courseid] = $n; + } + } + unset($actual); + unset($numsections); + + foreach ($needfixing as $courseid => $numsections) { + format_weeks_upgrade_hide_extra_sections($courseid, $numsections); + } + + $DB->delete_records('course_format_options', ['format' => 'weeks', 'sectionid' => 0, 'name' => 'numsections']); +} + +/** + * Find all sections in the course with sectionnum bigger than numsections. + * Either delete these sections or hide them + * + * We will only delete a section if it is completely empty and all sections below + * it are also empty + * + * @param int $courseid + * @param int $numsections + */ +function format_weeks_upgrade_hide_extra_sections($courseid, $numsections) { + global $DB; + $sections = $DB->get_records_sql('SELECT id, name, summary, sequence, visible + FROM {course_sections} + WHERE course = ? AND section > ? + ORDER BY section DESC', [$courseid, $numsections]); + $candelete = true; + $tohide = []; + $todelete = []; + foreach ($sections as $section) { + if ($candelete && (!empty($section->summary) || !empty($section->sequence) || !empty($section->name))) { + $candelete = false; + } + if ($candelete) { + $todelete[] = $section->id; + } else if ($section->visible) { + $tohide[] = $section->id; + } + } + if ($todelete) { + // Delete empty sections in the end. + // This is an upgrade script - no events or cache resets are needed. + // We also know that these sections do not have any modules so it is safe to just delete records in the table. + $DB->delete_records_list('course_sections', 'id', $todelete); + } + if ($tohide) { + // Hide other orphaned sections. + // This is different from what set_section_visible() does but we want to preserve actual + // module visibility in this case. + list($sql, $params) = $DB->get_in_or_equal($tohide); + $DB->execute("UPDATE {course_sections} SET visible = 0 WHERE id " . $sql, $params); + } +} diff --git a/course/format/weeks/format.php b/course/format/weeks/format.php index 2f988551ab8..da82d080a72 100644 --- a/course/format/weeks/format.php +++ b/course/format/weeks/format.php @@ -37,9 +37,9 @@ if ($week = optional_param('week', 0, PARAM_INT)) { } // End backwards-compatible aliasing.. -// make sure all sections are created +// Make sure section 0 is created. $course = course_get_format($course)->get_course(); -course_create_sections_if_missing($course, range(0, $course->numsections)); +course_create_sections_if_missing($course, 0); $renderer = $PAGE->get_renderer('format_weeks'); diff --git a/course/format/weeks/lib.php b/course/format/weeks/lib.php index 408b5900ed7..b41a8a06e63 100644 --- a/course/format/weeks/lib.php +++ b/course/format/weeks/lib.php @@ -219,7 +219,6 @@ class format_weeks extends format_base { * * Weeks format uses the following options: * - coursedisplay - * - numsections * - hiddensections * * @param bool $foreditform @@ -230,10 +229,6 @@ class format_weeks extends format_base { if ($courseformatoptions === false) { $courseconfig = get_config('moodlecourse'); $courseformatoptions = array( - 'numsections' => array( - 'default' => $courseconfig->numsections, - 'type' => PARAM_INT, - ), 'hiddensections' => array( 'default' => $courseconfig->hiddensections, 'type' => PARAM_INT, @@ -245,21 +240,7 @@ class format_weeks extends format_base { ); } if ($foreditform && !isset($courseformatoptions['coursedisplay']['label'])) { - $courseconfig = get_config('moodlecourse'); - $sectionmenu = array(); - $max = $courseconfig->maxsections; - if (!isset($max) || !is_numeric($max)) { - $max = 52; - } - for ($i = 0; $i <= $max; $i++) { - $sectionmenu[$i] = "$i"; - } $courseformatoptionsedit = array( - 'numsections' => array( - 'label' => new lang_string('numberweeks'), - 'element_type' => 'select', - 'element_attributes' => array($sectionmenu), - ), 'hiddensections' => array( 'label' => new lang_string('hiddensections'), 'help' => 'hiddensections', @@ -300,24 +281,24 @@ class format_weeks extends format_base { * @return array array of references to the added form elements. */ public function create_edit_form_elements(&$mform, $forsection = false) { + global $COURSE; $elements = parent::create_edit_form_elements($mform, $forsection); - // Increase the number of sections combo box values if the user has increased the number of sections - // using the icon on the course page beyond course 'maxsections' or course 'maxsections' has been - // reduced below the number of sections already set for the course on the site administration course - // defaults page. This is so that the number of sections is not reduced leaving unintended orphaned - // activities / resources. - if (!$forsection) { - $maxsections = get_config('moodlecourse', 'maxsections'); - $numsections = $mform->getElementValue('numsections'); - $numsections = $numsections[0]; - if ($numsections > $maxsections) { - $element = $mform->getElement('numsections'); - for ($i = $maxsections+1; $i <= $numsections; $i++) { - $element->addOption("$i", $i); - } + if (!$forsection && (empty($COURSE->id) || $COURSE->id == SITEID)) { + // Add "numsections" element to the create course form - it will force new course to be prepopulated + // with empty sections. + // The "Number of sections" option is no longer available when editing course, instead teachers should + // delete and add sections when needed. + $courseconfig = get_config('moodlecourse'); + $max = (int)$courseconfig->maxsections; + $element = $mform->addElement('select', 'numsections', get_string('numberweeks'), range(0, $max ?: 52)); + $mform->setType('numsections', PARAM_INT); + if (is_null($mform->getElementValue('numsections'))) { + $mform->setDefault('numsections', $courseconfig->numsections); } + array_unshift($elements, $element); } + return $elements; } @@ -344,33 +325,11 @@ class format_weeks extends format_base { if (!array_key_exists($key, $data)) { if (array_key_exists($key, $oldcourse)) { $data[$key] = $oldcourse[$key]; - } else if ($key === 'numsections') { - // If previous format does not have the field 'numsections' - // and $data['numsections'] is not set, - // we fill it with the maximum section number from the DB - $maxsection = $DB->get_field_sql('SELECT max(section) from {course_sections} - WHERE course = ?', array($this->courseid)); - if ($maxsection) { - // If there are no sections, or just default 0-section, 'numsections' will be set to default - $data['numsections'] = $maxsection; - } } } } } - $changed = $this->update_format_options($data); - if ($changed && array_key_exists('numsections', $data)) { - // If the numsections was decreased, try to completely delete the orphaned sections (unless they are not empty). - $numsections = (int)$data['numsections']; - $maxsection = $DB->get_field_sql('SELECT max(section) from {course_sections} - WHERE course = ?', array($this->courseid)); - for ($sectionnum = $maxsection; $sectionnum > $numsections; $sectionnum--) { - if (!$this->delete_section($sectionnum, false)) { - break; - } - } - } - return $changed; + return $this->update_format_options($data); } /** @@ -479,6 +438,9 @@ class format_weeks extends format_base { if ($mform->elementExists($fieldnames['numsections'])) { $numsections = $mform->getElementValue($fieldnames['numsections']); $numsections = $mform->getElement($fieldnames['numsections'])->exportValue($numsections); + } else if ($this->get_courseid()) { + // For existing courses get the number of sections. + $numsections = $this->get_last_section_number(); } else { // Fallback to the default value for new courses. $numsections = get_config('moodlecourse', $fieldnames['numsections']); @@ -507,8 +469,8 @@ class format_weeks extends format_base { * @return bool */ public function allow_stealth_module_visibility($cm, $section) { - // Allow the third visibility state inside visible sections or in section 0, not allow in orphaned sections. - return !$section->section || ($section->visible && $section->section <= $this->get_course()->numsections); + // Allow the third visibility state inside visible sections or in section 0. + return !$section->section || $section->visible; } public function section_action($section, $action, $sr) { diff --git a/course/format/weeks/tests/behat/edit_delete_sections.feature b/course/format/weeks/tests/behat/edit_delete_sections.feature index 6269da09f5a..db90bbb1fc4 100644 --- a/course/format/weeks/tests/behat/edit_delete_sections.feature +++ b/course/format/weeks/tests/behat/edit_delete_sections.feature @@ -77,9 +77,7 @@ Feature: Sections can be edited and deleted in weeks format Then I should see "Are you absolutely sure you want to completely delete \"29 May - 4 June\" and all the activities it contains?" And I press "Delete" And I should not see "29 May - 4 June" - And I navigate to "Edit settings" node in "Course administration" - And I expand all fieldsets - And the field "Number of sections" matches value "4" + And I should see "22 May - 28 May" Scenario: Deleting the middle section in weeks format Given I should see "29 May - 4 June" in the "li#section-5" "css_element" @@ -88,31 +86,4 @@ Feature: Sections can be edited and deleted in weeks format Then I should not see "29 May - 4 June" And I should not see "Test chat name" And I should see "Test choice name" in the "li#section-4" "css_element" - And I navigate to "Edit settings" node in "Course administration" - And I expand all fieldsets - And the field "Number of sections" matches value "4" - - Scenario: Deleting the orphaned section in weeks format - When I follow "Reduce the number of sections" - Then I should see "Orphaned activities (section 5)" in the "li#section-5" "css_element" - And I delete section "5" - And I press "Delete" - And I should not see "29 May - 4 June" - And I should not see "Orphaned activities" - And "li#section-5" "css_element" should not exist - And I navigate to "Edit settings" node in "Course administration" - And I expand all fieldsets - And the field "Number of sections" matches value "4" - - Scenario: Deleting a section when orphaned section is present in weeks format - When I follow "Reduce the number of sections" - Then I should see "Orphaned activities (section 5)" in the "li#section-5" "css_element" - And "li#section-5.orphaned" "css_element" should exist - And "li#section-4.orphaned" "css_element" should not exist - And I delete section "1" - And I press "Delete" - And I should not see "Test book name" - And I should see "Orphaned activities (section 4)" in the "li#section-4" "css_element" - And "li#section-5" "css_element" should not exist - And "li#section-4.orphaned" "css_element" should exist - And "li#section-3.orphaned" "css_element" should not exist + And I should see "22 May - 28 May" diff --git a/course/format/weeks/tests/format_weeks_test.php b/course/format/weeks/tests/format_weeks_test.php index 481caa771db..32a25f6106b 100644 --- a/course/format/weeks/tests/format_weeks_test.php +++ b/course/format/weeks/tests/format_weeks_test.php @@ -36,32 +36,6 @@ require_once($CFG->dirroot . '/course/lib.php'); */ class format_weeks_testcase extends advanced_testcase { - public function test_update_course_numsections() { - global $DB; - $this->resetAfterTest(true); - - $generator = $this->getDataGenerator(); - - $course = $generator->create_course(array('numsections' => 10, 'format' => 'weeks'), - array('createsections' => true)); - $generator->create_module('assign', array('course' => $course, 'section' => 7)); - - $this->setAdminUser(); - - $this->assertEquals(11, $DB->count_records('course_sections', array('course' => $course->id))); - - // Change the numsections to 8, last two sections did not have any activities, they should be deleted. - update_course((object)array('id' => $course->id, 'numsections' => 8)); - $this->assertEquals(9, $DB->count_records('course_sections', array('course' => $course->id))); - $this->assertEquals(9, count(get_fast_modinfo($course)->get_section_info_all())); - - // Change the numsections to 5, section 8 should be deleted but section 7 should remain as it has activities. - update_course((object)array('id' => $course->id, 'numsections' => 6)); - $this->assertEquals(8, $DB->count_records('course_sections', array('course' => $course->id))); - $this->assertEquals(8, count(get_fast_modinfo($course)->get_section_info_all())); - $this->assertEquals(6, course_get_format($course)->get_course()->numsections); - } - /** * Tests for format_weeks::get_section_name method with default section names. */ @@ -224,7 +198,7 @@ class format_weeks_testcase extends advanced_testcase { * @return void */ public function test_default_course_enddate() { - global $CFG, $DB; + global $CFG, $DB, $PAGE; $this->resetAfterTest(true); @@ -247,6 +221,7 @@ class format_weeks_testcase extends advanced_testcase { 'returnurl' => new moodle_url('/'), ]; + $PAGE->set_course($course); $courseform = new testable_course_edit_form(null, $args); $courseform->definition_after_data(); diff --git a/course/format/weeks/tests/format_weeks_upgrade_test.php b/course/format/weeks/tests/format_weeks_upgrade_test.php new file mode 100644 index 00000000000..e44cc5a204f --- /dev/null +++ b/course/format/weeks/tests/format_weeks_upgrade_test.php @@ -0,0 +1,128 @@ +. + +/** + * format_weeks unit tests for upgradelib + * + * @package format_weeks + * @copyright 2015 Marina Glancy + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/course/lib.php'); +require_once($CFG->dirroot . '/course/format/weeks/db/upgradelib.php'); + +/** + * format_weeks unit tests for upgradelib + * + * @package format_weeks + * @copyright 2017 Marina Glancy + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class format_weeks_upgrade_testcase extends advanced_testcase { + + /** + * Test upgrade step to remove orphaned sections. + */ + public function test_numsections_no_actions() { + global $DB; + + $this->resetAfterTest(true); + + $params = array('format' => 'weeks', 'numsections' => 5, 'startdate' => 1445644800); + $course = $this->getDataGenerator()->create_course($params); + // This test is executed after 'numsections' option was already removed, add it manually. + $DB->insert_record('course_format_options', ['courseid' => $course->id, 'format' => 'weeks', + 'sectionid' => 0, 'name' => 'numsections', 'value' => '5']); + + // There are 6 sections in the course (0-section and sections 1, ... 5). + $this->assertEquals(6, $DB->count_records('course_sections', ['course' => $course->id])); + + format_weeks_upgrade_remove_numsections(); + + // There are still 6 sections in the course. + $this->assertEquals(6, $DB->count_records('course_sections', ['course' => $course->id])); + + } + + /** + * Test upgrade step to remove orphaned sections. + */ + public function test_numsections_delete_empty() { + global $DB; + + $this->resetAfterTest(true); + + // Set default number of sections to 10. + set_config('numsections', 10, 'moodlecourse'); + + $params1 = array('format' => 'weeks', 'numsections' => 5, 'startdate' => 1445644800); + $course1 = $this->getDataGenerator()->create_course($params1); + $params2 = array('format' => 'weeks', 'numsections' => 20, 'startdate' => 1445644800); + $course2 = $this->getDataGenerator()->create_course($params2); + // This test is executed after 'numsections' option was already removed, add it manually and + // set it to be 2 less than actual number of sections. + $DB->insert_record('course_format_options', ['courseid' => $course1->id, 'format' => 'weeks', + 'sectionid' => 0, 'name' => 'numsections', 'value' => '3']); + + // There are 6 sections in the first course (0-section and sections 1, ... 5). + $this->assertEquals(6, $DB->count_records('course_sections', ['course' => $course1->id])); + // There are 21 sections in the second course. + $this->assertEquals(21, $DB->count_records('course_sections', ['course' => $course2->id])); + + format_weeks_upgrade_remove_numsections(); + + // Two sections were deleted in the first course. + $this->assertEquals(4, $DB->count_records('course_sections', ['course' => $course1->id])); + // The second course was reset to 11 sections (default plus 0-section). + $this->assertEquals(11, $DB->count_records('course_sections', ['course' => $course2->id])); + + } + + /** + * Test upgrade step to remove orphaned sections. + */ + public function test_numsections_hide_non_empty() { + global $DB; + + $this->resetAfterTest(true); + + $params = array('format' => 'weeks', 'numsections' => 5, 'startdate' => 1445644800); + $course = $this->getDataGenerator()->create_course($params); + + // Add a module to the second last section. + $cm = $this->getDataGenerator()->create_module('forum', ['course' => $course->id, 'section' => 4]); + + // This test is executed after 'numsections' option was already removed, add it manually and + // set it to be 2 less than actual number of sections. + $DB->insert_record('course_format_options', ['courseid' => $course->id, 'format' => 'weeks', + 'sectionid' => 0, 'name' => 'numsections', 'value' => '3']); + + // There are 6 sections. + $this->assertEquals(6, $DB->count_records('course_sections', ['course' => $course->id])); + + format_weeks_upgrade_remove_numsections(); + + // One section was deleted and one hidden. + $this->assertEquals(5, $DB->count_records('course_sections', ['course' => $course->id])); + $this->assertEquals(0, $DB->get_field('course_sections', 'visible', ['course' => $course->id, 'section' => 4])); + // The module is still visible. + $this->assertEquals(1, $DB->get_field('course_modules', 'visible', ['id' => $cm->cmid])); + } +} diff --git a/course/format/weeks/version.php b/course/format/weeks/version.php index 7c6b84336ab..9defc109a2d 100644 --- a/course/format/weeks/version.php +++ b/course/format/weeks/version.php @@ -25,6 +25,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2016120500; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2017020200; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2016112900; // Requires this Moodle version. $plugin->component = 'format_weeks'; // Full name of the plugin (used for diagnostics).