MDL-57769 format_topics: remove numsections option

backup/util/ui/tests/behat/restore_moodle2_courses.feature

@@ -41,11 +41,11 @@ Feature: Restore Moodle 2 course backups
     Then I should see "Course 1 restored in a new course"
     And I should see "Community finder" in the "Community finder" "block"
     And I should see "Test forum name"
+    And I should see "Topic 15"
+    And I should not see "Topic 16"
     And I navigate to "Edit settings" node in "Course administration"
     And I expand all fieldsets
     And the field "id_format" matches value "Topics format"
-    And the field "Number of sections" matches value "15"
-    And the field "Course layout" matches value "Show one section per page"
     And I press "Cancel"
 
   @javascript
@@ -122,11 +122,12 @@ Feature: Restore Moodle 2 course backups
     And I navigate to "Edit settings" node in "Course administration"
     And I expand all fieldsets
     Then the field "id_format" matches value "Topics format"
-    And the field "Number of sections" matches value "15"
     And the field "Course layout" matches value "Show one section per page"
     And I press "Cancel"
     And section "3" should be hidden
     And section "7" should be hidden
     And section "15" should be visible
+    And I should see "Topic 15"
+    And I should not see "Topic 16"
     And I should see "Test URL name" in the "Topic 3" "section"
     And I should see "Test forum name" in the "Topic 1" "section"

course/format/topics/db/upgrade.php

@@ -0,0 +1,47 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Upgrade scripts for course format "Topics"
+ *
+ * @package    format_topics
+ * @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_topics
+ *
+ * @param int $oldversion the version we are upgrading from
+ * @return bool result
+ */
+function xmldb_format_topics_upgrade($oldversion) {
+    global $CFG, $DB;
+
+    require_once($CFG->dirroot . '/course/format/topics/db/upgradelib.php');
+
+    if ($oldversion < 2017020200) {
+
+        // Remove 'numsections' option and hide or delete orphaned sections.
+        format_topics_upgrade_remove_numsections();
+
+        upgrade_plugin_savepoint(true, 2017020200, 'format', 'topics');
+    }
+
+    return true;
+}

course/format/topics/db/upgradelib.php

@@ -0,0 +1,117 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Upgrade scripts for course format "Topics"
+ *
+ * @package    format_topics
+ * @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 'topics' format that have actual number of sections
+ * bigger than their 'numsections' course format option.
+ * For each such course we call {@link format_topics_upgrade_hide_extra_sections()} and
+ * either delete or hide "orphaned" sections.
+ */
+function format_topics_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' => 'topics', 'format2' => 'topics', '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_topics_upgrade_hide_extra_sections($courseid, $numsections);
+    }
+
+    $DB->delete_records('course_format_options', ['format' => 'topics', '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_topics_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);
+    }
+}

course/format/topics/format.php

@@ -38,15 +38,16 @@ if ($topic = optional_param('topic', 0, PARAM_INT)) {
 // End backwards-compatible aliasing..
 
 $context = context_course::instance($course->id);
+// Retrieve course format option fields and add them to the $course object.
+$course = course_get_format($course)->get_course();
 
 if (($marker >=0) && has_capability('moodle/course:setcurrentsection', $context) && confirm_sesskey()) {
     $course->marker = $marker;
     course_set_marker($course->id, $marker);
 }
 
-// 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, 0);
-course_create_sections_if_missing($course, range(0, $course->numsections));
 
 $renderer = $PAGE->get_renderer('format_topics');
 

course/format/topics/lang/en/format_topics.php

@@ -23,6 +23,7 @@
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 
+$string['addsection'] = 'Add topic';
 $string['currentsection'] = 'This topic';
 $string['editsection'] = 'Edit topic';
 $string['editsectionname'] = 'Edit topic name';

course/format/topics/lib.php

@@ -214,7 +214,6 @@ class format_topics extends format_base {
      *
      * Topics format uses the following options:
      * - coursedisplay
-     * - numsections
      * - hiddensections
      *
      * @param bool $foreditform
@@ -225,10 +224,6 @@ class format_topics 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,
@@ -240,21 +235,7 @@ class format_topics extends format_base {
             );
         }
         if ($foreditform && !isset($courseformatoptions['coursedisplay']['label'])) {
-            $courseconfig = get_config('moodlecourse');
-            $max = $courseconfig->maxsections;
-            if (!isset($max) || !is_numeric($max)) {
-                $max = 52;
-            }
-            $sectionmenu = array();
-            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',
@@ -295,24 +276,24 @@ class format_topics 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
+        if (!$forsection && (empty($COURSE->id) || $COURSE->id == SITEID)) {
-        // using the icon on the course page beyond course 'maxsections' or course 'maxsections' has been
+            // Add "numsections" element to the create course form - it will force new course to be prepopulated
-        // reduced below the number of sections already set for the course on the site administration course
+            // with empty sections.
-        // defaults page.  This is so that the number of sections is not reduced leaving unintended orphaned
+            // The "Number of sections" option is no longer available when editing course, instead teachers should
-        // activities / resources.
+            // delete and add sections when needed.
-        if (!$forsection) {
+            $courseconfig = get_config('moodlecourse');
-            $maxsections = get_config('moodlecourse', 'maxsections');
+            $max = (int)$courseconfig->maxsections;
-            $numsections = $mform->getElementValue('numsections');
+            $element = $mform->addElement('select', 'numsections', get_string('numberweeks'), range(0, $max ?: 52));
-            $numsections = $numsections[0];
+            $mform->setType('numsections', PARAM_INT);
-            if ($numsections > $maxsections) {
+            if (is_null($mform->getElementValue('numsections'))) {
-                $element = $mform->getElement('numsections');
+                $mform->setDefault('numsections', $courseconfig->numsections);
-                for ($i = $maxsections+1; $i <= $numsections; $i++) {
-                    $element->addOption("$i", $i);
-                }
             }
+            array_unshift($elements, $element);
         }
+
         return $elements;
     }
 
@@ -320,9 +301,7 @@ class format_topics extends format_base {
      * Updates format options for a course
      *
      * In case if course format was changed to 'topics', we try to copy options
-     * 'coursedisplay', 'numsections' and 'hiddensections' from the previous format.
+     * 'coursedisplay' and 'hiddensections' from the previous format.
-     * If previous course format did not have 'numsections' option, we populate it with the
-     * current number of sections
      *
      * @param stdClass|array $data return value from {@link moodleform::get_data()} or array with data
      * @param stdClass $oldcourse if this function is called from {@link update_course()}
@@ -330,7 +309,6 @@ class format_topics extends format_base {
      * @return bool whether there were any changes to the options values
      */
     public function update_course_format_options($data, $oldcourse = null) {
-        global $DB;
         $data = (array)$data;
         if ($oldcourse !== null) {
             $oldcourse = (array)$oldcourse;
@@ -339,33 +317,11 @@ class format_topics 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);
+        return $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;
     }
 
     /**
@@ -420,8 +376,8 @@ class format_topics 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.
+        // Allow the third visibility state inside visible sections or in section 0.
-        return !$section->section || ($section->visible && $section->section <= $this->get_course()->numsections);
+        return !$section->section || $section->visible;
     }
 
     public function section_action($section, $action, $sr) {

course/format/topics/renderer.php

@@ -119,9 +119,8 @@ class format_topics_renderer extends format_section_renderer_base {
         }
         $url->param('sesskey', sesskey());
 
-        $isstealth = $section->section > $course->numsections;
         $controls = array();
-        if (!$isstealth && $section->section && has_capability('moodle/course:setcurrentsection', $coursecontext)) {
+        if ($section->section && has_capability('moodle/course:setcurrentsection', $coursecontext)) {
             if ($course->marker == $section->section) {  // Show the "light globe" on/off.
                 $url->param('marker', 0);
                 $markedthistopic = get_string('markedthistopic');

course/format/topics/tests/behat/edit_delete_sections.feature

@@ -71,9 +71,7 @@ Feature: Sections can be edited and deleted in topics format
     Then I should see "Are you absolutely sure you want to completely delete \"Topic 5\" and all the activities it contains?"
     And I press "Delete"
     And I should not see "Topic 5"
-    And I navigate to "Edit settings" node in "Course administration"
+    And I should see "Topic 4"
-    And I expand all fieldsets
-    And the field "Number of sections" matches value "4"
 
   Scenario: Deleting the middle section in topics format
     When I delete section "4"
@@ -81,31 +79,4 @@ Feature: Sections can be edited and deleted in topics format
     Then I should not see "Topic 5"
     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 should see "Topic 4"
-    And I expand all fieldsets
-    And the field "Number of sections" matches value "4"
-
-  Scenario: Deleting the orphaned section in topics 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 "Topic 5"
-    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 topics 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

course/format/topics/tests/format_topics_test.php

@@ -36,32 +36,6 @@ require_once($CFG->dirroot . '/course/lib.php');
  */
 class format_topics_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' => 'topics'),
-            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_topics::get_section_name method with default section names.
      */

course/format/topics/tests/format_topics_upgrade_test.php

@@ -0,0 +1,128 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * format_topics unit tests for upgradelib
+ *
+ * @package    format_topics
+ * @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/topics/db/upgradelib.php');
+
+/**
+ * format_topics unit tests for upgradelib
+ *
+ * @package    format_topics
+ * @copyright  2017 Marina Glancy
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class format_topics_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' => 'topics', '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' => 'topics',
+            '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_topics_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' => 'topics', 'numsections' => 5, 'startdate' => 1445644800);
+        $course1 = $this->getDataGenerator()->create_course($params1);
+        $params2 = array('format' => 'topics', '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' => 'topics',
+            '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_topics_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' => 'topics', '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' => 'topics',
+            'sectionid' => 0, 'name' => 'numsections', 'value' => '3']);
+
+        // There are 6 sections.
+        $this->assertEquals(6, $DB->count_records('course_sections', ['course' => $course->id]));
+
+        format_topics_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]));
+    }
+}

course/format/topics/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_topics';    // Full name of the plugin (used for diagnostics).