MDL-37028 Integrity check for course modules and sections

This commit reinstates:
commit 0bac49dc1983824e4861086ed1ab9e13af99d76d
Author: Marina Glancy <marina@moodle.com>
Date:   Tue Sep 3 17:14:13 2013 +1000

    MDL-37028 Integrity check for course modules and sections

    If section mentioned in 'orphaned' module does not exist module is added to the first available section. Also corrected whitespaces

commit 1f0a9ce48b3e6f8c5dc6628aa692323909925032
Author: Marina Glancy <marina@moodle.com>
Date:   Mon Aug 12 14:06:48 2013 +1000

    MDL-37028 Integrity check for course modules and sections

    created function, unittest, build-in quick integrity check on each call to rebuild_course_cache()
    also added CLI script

diff --git a/admin/cli/fix_course_sequence.php b/admin/cli/fix_course_sequence.php
new file mode 100644 (file)
index 0000000..8715b13
--- /dev/null
+++ b/admin/cli/fix_course_sequence.php
@@ -0,0 +1,125 @@
+<?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/>.
+
+/**
+ * This script fixed incorrectly deleted users.
+ *
+ * @package    core
+ * @subpackage cli
+ * @copyright  2013 Marina Glancy
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+define('CLI_SCRIPT', true);
+
+require(__DIR__.'/../../config.php');
+require_once($CFG->libdir.'/clilib.php');
+
+// Get cli options.
+list($options, $unrecognized) = cli_get_params(
+    array(
+        'courses'           => false,
+        'fix'               => false,
+        'help'              => false
+    ),
+    array(
+        'h' => 'help',
+        'c' => 'courses',
+        'f' => 'fix'
+    )
+);
+
+if ($options['help'] || empty($options['courses'])) {
+    $help =
+"Checks and fixes that course modules and sections reference each other correctly.
+
+Compares DB fields course_sections.sequence and course_modules.section
+checking that:
+- course_sections.sequence contains each module id not more than once in the course
+- for each moduleid from course_sections.sequence the field course_modules.section
+  refers to the same section id (this means course_sections.sequence is more
+  important if they are different)
+- each module in the course is present in one of course_sections.sequence
+- section sequences do not contain non-existing course modules
+
+If there are any mismatches, the message is displayed. If --fix is specified,
+the records in DB are corrected.
+
+This script may run for a long time on big systems if called for all courses.
+
+Avoid executing the script when another user may simultaneously edit any of the
+courses being checked (recommended to run in mainenance mode).
+
+Options:
+-c, --courses         List courses that need to be checked (comma-separated
+                      values or * for all). Required
+-f, --fix             Fix the mismatches in DB. If not specified check only and
+                      report problems to STDERR
+-h, --help            Print out this help
+
+Example:
+\$sudo -u www-data /usr/bin/php admin/cli/fix_course_sequence.php --courses=*
+\$sudo -u www-data /usr/bin/php admin/cli/fix_course_sequence.php --courses=2,3,4 --fix
+";
+
+    echo $help;
+    die;
+}
+
+$courseslist = preg_split('/\s*,\s*/', $options['courses'], -1, PREG_SPLIT_NO_EMPTY);
+if (in_array('*', $courseslist)) {
+    $where = '';
+    $params = array();
+} else {
+    list($sql, $params) = $DB->get_in_or_equal($courseslist, SQL_PARAMS_NAMED, 'id');
+    $where = 'WHERE id '. $sql;
+}
+$coursescount = $DB->get_field_sql('SELECT count(id) FROM {course} '. $where, $params);
+
+if (!$coursescount) {
+    cli_error('No courses found');
+}
+echo "Checking $coursescount courses...\n\n";
+
+require_once($CFG->dirroot. '/course/lib.php');
+
+$problems = array();
+$courses = $DB->get_fieldset_sql('SELECT id FROM {course} '. $where, $params);
+foreach ($courses as $courseid) {
+    $errors = course_integrity_check($courseid, null, null, true, empty($options['fix']));
+    if ($errors) {
+        if (!empty($options['fix'])) {
+            // Reset the course cache to make sure cache is recalculated next time the course is viewed.
+            $DB->upgrade_record('course', array('modinfo' => null, 'id' => $courseid));
+        }
+        foreach ($errors as $error) {
+            cli_problem($error);
+        }
+        $problems[] = $courseid;
+    } else {
+        echo "Course [$courseid] is OK\n";
+    }
+}
+if (!count($problems)) {
+    echo "\n...All courses are OK\n";
+} else {
+    if (!empty($options['fix'])) {
+        echo "\n...Found and fixed ".count($problems)." courses with problems". "\n";
+    } else {
+        echo "\n...Found ".count($problems)." courses with problems. To fix run:\n";
+        echo "\$sudo -u www-data /usr/bin/php admin/cli/fix_course_sequence.php --courses=".join(',', $problems)." --fix". "\n";
+    }
+}
\ No newline at end of file

diff --git a/course/lib.php b/course/lib.php
index 9ceefb5..6025325 100644 (file)
--- a/course/lib.php
+++ b/course/lib.php
@@ -853,6 +853,138 @@ function print_log_ods($course, $user, $date, $order='l.time DESC', $modname,
     return true;
 }
 
     return true;
 }
 

+/**
+ * Checks the integrity of the course data.
+ *
+ * In summary - compares course_sections.sequence and course_modules.section.
+ *
+ * More detailed, checks that:
+ * - course_sections.sequence contains each module id not more than once in the course
+ * - for each moduleid from course_sections.sequence the field course_modules.section
+ *   refers to the same section id (this means course_sections.sequence is more
+ *   important if they are different)
+ * - ($fullcheck only) each module in the course is present in one of
+ *   course_sections.sequence
+ * - ($fullcheck only) removes non-existing course modules from section sequences
+ *
+ * If there are any mismatches, the changes are made and records are updated in DB.
+ *
+ * Course cache is NOT rebuilt if there are any errors!
+ *
+ * This function is used each time when course cache is being rebuilt with $fullcheck = false
+ * and in CLI script admin/cli/fix_course_sequence.php with $fullcheck = true
+ *
+ * @param int $courseid id of the course
+ * @param array $rawmods result of funciton {@link get_course_mods()} - containst
+ *     the list of enabled course modules in the course. Retrieved from DB if not specified.
+ *     Argument ignored in cashe of $fullcheck, the list is retrieved form DB anyway.
+ * @param array $sections records from course_sections table for this course.
+ *     Retrieved from DB if not specified
+ * @param bool $fullcheck Will add orphaned modules to their sections and remove non-existing
+ *     course modules from sequences. Only to be used in site maintenance mode when we are
+ *     sure that another user is not in the middle of the process of moving/removing a module.
+ * @param bool $checkonly Only performs the check without updating DB, outputs all errors as debug messages.
+ * @return array array of messages with found problems. Empty output means everything is ok
+ */
+function course_integrity_check($courseid, $rawmods = null, $sections = null, $fullcheck = false, $checkonly = false) {
+    global $DB;
+    $messages = array();
+    if ($sections === null) {
+        $sections = $DB->get_records('course_sections', array('course' => $courseid), 'section', 'id,section,sequence');
+    }
+    if ($fullcheck) {
+        // Retrieve all records from course_modules regardless of module type visibility.
+        $rawmods = $DB->get_records('course_modules', array('course' => $courseid), 'id', 'id,section');
+    }
+    if ($rawmods === null) {
+        $rawmods = get_course_mods($courseid);
+    }
+    if (!$fullcheck && (empty($sections) || empty($rawmods))) {
+        // If either of the arrays is empty, no modules are displayed anyway.
+        return true;
+    }
+    $debuggingprefix = 'Failed integrity check for course ['.$courseid.']. ';
+
+    // First make sure that each module id appears in section sequences only once.
+    // If it appears in several section sequences the last section wins.
+    // If it appears twice in one section sequence, the first occurence wins.
+    $modsection = array();
+    foreach ($sections as $sectionid => $section) {
+        $sections[$sectionid]->newsequence = $section->sequence;
+        if (!empty($section->sequence)) {
+            $sequence = explode(",", $section->sequence);
+            $sequenceunique = array_unique($sequence);
+            if (count($sequenceunique) != count($sequence)) {
+                // Some course module id appears in this section sequence more than once.
+                ksort($sequenceunique); // Preserve initial order of modules.
+                $sequence = array_values($sequenceunique);
+                $sections[$sectionid]->newsequence = join(',', $sequence);
+                $messages[] = $debuggingprefix.'Sequence for course section ['.
+                        $sectionid.'] is "'.$sections[$sectionid]->sequence.'", must be "'.$sections[$sectionid]->newsequence.'"';
+            }
+            foreach ($sequence as $cmid) {
+                if (array_key_exists($cmid, $modsection) && isset($rawmods[$cmid])) {
+                    // Some course module id appears to be in more than one section's sequences.
+                    $wrongsectionid = $modsection[$cmid];
+                    $sections[$wrongsectionid]->newsequence = trim(preg_replace("/,$cmid,/", ',', ','.$sections[$wrongsectionid]->newsequence. ','), ',');
+                    $messages[] = $debuggingprefix.'Course module ['.$cmid.'] must be removed from sequence of section ['.
+                            $wrongsectionid.'] because it is also present in sequence of section ['.$sectionid.']';
+                }
+                $modsection[$cmid] = $sectionid;
+            }
+        }
+    }
+
+    // Add orphaned modules to their sections if they exist or to section 0 otherwise.
+    if ($fullcheck) {
+        foreach ($rawmods as $cmid => $mod) {
+            if (!isset($modsection[$cmid])) {
+                // This is a module that is not mentioned in course_section.sequence at all.
+                // Add it to the section $mod->section or to the last available section.
+                if ($mod->section && isset($sections[$mod->section])) {
+                    $modsection[$cmid] = $mod->section;
+                } else {
+                    $firstsection = reset($sections);
+                    $modsection[$cmid] = $firstsection->id;
+                }
+                $sections[$modsection[$cmid]]->newsequence = trim($sections[$modsection[$cmid]]->newsequence.','.$cmid, ',');
+                $messages[] = $debuggingprefix.'Course module ['.$cmid.'] is missing from sequence of section ['.
+                        $sectionid.']';
+            }
+        }
+        foreach ($modsection as $cmid => $sectionid) {
+            if (!isset($rawmods[$cmid])) {
+                // Section $sectionid refers to module id that does not exist.
+                $sections[$sectionid]->newsequence = trim(preg_replace("/,$cmid,/", ',', ','.$sections[$sectionid]->newsequence.','), ',');
+                $messages[] = $debuggingprefix.'Course module ['.$cmid.
+                        '] does not exist but is present in the sequence of section ['.$sectionid.']';
+            }
+        }
+    }
+
+    // Update changed sections.
+    if (!$checkonly && !empty($messages)) {
+        foreach ($sections as $sectionid => $section) {
+            if ($section->newsequence !== $section->sequence) {
+                $DB->update_record('course_sections', array('id' => $sectionid, 'sequence' => $section->newsequence));
+            }
+        }
+    }
+
+    // Now make sure that all modules point to the correct sections.
+    foreach ($rawmods as $cmid => $mod) {
+        if (isset($modsection[$cmid]) && $modsection[$cmid] != $mod->section) {
+            if (!$checkonly) {
+                $DB->update_record('course_modules', array('id' => $cmid, 'section' => $modsection[$cmid]));
+            }
+            $messages[] = $debuggingprefix.'Course module ['.$cmid.
+                    '] points to section ['.$mod->section.'] instead of ['.$modsection[$cmid].']';
+        }
+    }
+
+    return $messages;
+}
+

 /**
  * For a given course, returns an array of course activity objects
  * Each item in the array contains he following properties:
 /**
  * For a given course, returns an array of course activity objects
  * Each item in the array contains he following properties:


@@ -884,7 +1016,14 @@ function get_array_of_activities($courseid) {
         return $mod; // always return array
     }
 
         return $mod; // always return array
     }
 

-    if ($sections = $DB->get_records("course_sections", array("course"=>$courseid), "section ASC")) {
+    if ($sections = $DB->get_records('course_sections', array('course' => $courseid), 'section ASC', 'id,section,sequence')) {
+        // First check and correct obvious mismatches between course_sections.sequence and course_modules.section.
+        if ($errormessages = course_integrity_check($courseid, $rawmods, $sections)) {
+            debugging(join('<br>', $errormessages));
+            $rawmods = get_course_mods($courseid);
+            $sections = $DB->get_records('course_sections', array('course' => $courseid), 'section ASC', 'id,section,sequence');
+        }
+        // Build array of activities.

        foreach ($sections as $section) {
            if (!empty($section->sequence)) {
                $sequence = explode(",", $section->sequence);
        foreach ($sections as $section) {
            if (!empty($section->sequence)) {
                $sequence = explode(",", $section->sequence);

diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php
index dc68f34..1c19a15 100644 (file)
--- a/course/tests/courselib_test.php
+++ b/course/tests/courselib_test.php
@@ -1772,4 +1772,124 @@ class core_course_courselib_testcase extends advanced_testcase {
         $expectedlegacydata = array($course->id, "course", "editsection", 'editsection.php?id=' . $id, $sectionnum);
         $this->assertEventLegacyLogData($expectedlegacydata, $event);
     }
         $expectedlegacydata = array($course->id, "course", "editsection", 'editsection.php?id=' . $id, $sectionnum);
         $this->assertEventLegacyLogData($expectedlegacydata, $event);
     }

+
+    public function test_course_integrity_check() {
+        global $DB;
+
+        $this->resetAfterTest(true);
+        $course = $this->getDataGenerator()->create_course(array('numsections' => 1),
+           array('createsections'=>true));
+
+        $forum = $this->getDataGenerator()->create_module('forum', array('course' => $course->id),
+                array('section' => 0));
+        $page = $this->getDataGenerator()->create_module('page', array('course' => $course->id),
+                array('section' => 0));
+        $quiz = $this->getDataGenerator()->create_module('quiz', array('course' => $course->id),
+                array('section' => 0));
+        $correctseq = join(',', array($forum->cmid, $page->cmid, $quiz->cmid));
+
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($correctseq, $section0->sequence);
+        $this->assertEmpty($section1->sequence);
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$page->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+        $this->assertEmpty(course_integrity_check($course->id));
+
+        // Now let's make manual change in DB and let course_integrity_check() fix it:
+
+        // 1. Module appears twice in one section.
+        $DB->update_record('course_sections', array('id' => $section0->id, 'sequence' => $section0->sequence. ','. $page->cmid));
+        $this->assertEquals(
+                array('Failed integrity check for course ['. $course->id.
+                ']. Sequence for course section ['. $section0->id. '] is "'.
+                $section0->sequence. ','. $page->cmid. '", must be "'.
+                $section0->sequence. '"'),
+                course_integrity_check($course->id));
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($correctseq, $section0->sequence);
+        $this->assertEmpty($section1->sequence);
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$page->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+
+        // 2. Module appears in two sections (last section wins).
+        $DB->update_record('course_sections', array('id' => $section1->id, 'sequence' => ''. $page->cmid));
+        // First message about double mentioning in sequence, second message about wrong section field for $page.
+        $this->assertEquals(array(
+            'Failed integrity check for course ['. $course->id. ']. Course module ['. $page->cmid.
+            '] must be removed from sequence of section ['. $section0->id.
+            '] because it is also present in sequence of section ['. $section1->id. ']',
+            'Failed integrity check for course ['. $course->id. ']. Course module ['. $page->cmid.
+            '] points to section ['. $section0->id. '] instead of ['. $section1->id. ']'),
+                course_integrity_check($course->id));
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($forum->cmid. ','. $quiz->cmid, $section0->sequence);
+        $this->assertEquals(''. $page->cmid, $section1->sequence);
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section1->id, $cms[$page->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+
+        // 3. Module id is not present in course_section.sequence (integrity check with $fullcheck = false).
+        $DB->update_record('course_sections', array('id' => $section1->id, 'sequence' => ''));
+        $this->assertEmpty(course_integrity_check($course->id)); // Not an error!
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($forum->cmid. ','. $quiz->cmid, $section0->sequence);
+        $this->assertEmpty($section1->sequence);
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section1->id, $cms[$page->cmid]->section); // Not changed.
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+
+        // 4. Module id is not present in course_section.sequence (integrity check with $fullcheck = true).
+        $this->assertEquals(array('Failed integrity check for course ['. $course->id. ']. Course module ['.
+                $page->cmid. '] is missing from sequence of section ['. $section1->id. ']'),
+                course_integrity_check($course->id, null, null, true)); // Error!
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($forum->cmid. ','. $quiz->cmid, $section0->sequence);
+        $this->assertEquals(''. $page->cmid, $section1->sequence);  // Yay, module added to section.
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section1->id, $cms[$page->cmid]->section); // Not changed.
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+
+        // 5. Module id is not present in course_section.sequence and it's section is invalid (integrity check with $fullcheck = true).
+        $DB->update_record('course_modules', array('id' => $page->cmid, 'section' => 8765));
+        $DB->update_record('course_sections', array('id' => $section1->id, 'sequence' => ''));
+        $this->assertEquals(array(
+            'Failed integrity check for course ['. $course->id. ']. Course module ['. $page->cmid.
+            '] is missing from sequence of section ['. $section1->id. ']',
+            'Failed integrity check for course ['. $course->id. ']. Course module ['. $page->cmid.
+            '] points to section [8765] instead of ['. $section0->id. ']'),
+                course_integrity_check($course->id, null, null, true));
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($forum->cmid. ','. $quiz->cmid. ','. $page->cmid, $section0->sequence); // Module added to section.
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$page->cmid]->section); // Section changed to section0.
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+
+        // 6. Module is deleted from course_modules but not deleted in sequence (integrity check with $fullcheck = true).
+        $DB->delete_records('course_modules', array('id' => $page->cmid));
+        $this->assertEquals(array('Failed integrity check for course ['. $course->id. ']. Course module ['.
+                $page->cmid. '] does not exist but is present in the sequence of section ['. $section0->id. ']'),
+                course_integrity_check($course->id, null, null, true));
+        $section0 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 0));
+        $section1 = $DB->get_record('course_sections', array('course' => $course->id, 'section' => 1));
+        $cms = $DB->get_records('course_modules', array('course' => $course->id), 'id', 'id,section');
+        $this->assertEquals($forum->cmid. ','. $quiz->cmid, $section0->sequence);
+        $this->assertEmpty($section1->sequence);
+        $this->assertEquals($section0->id, $cms[$forum->cmid]->section);
+        $this->assertEquals($section0->id, $cms[$quiz->cmid]->section);
+        $this->assertEquals(2, count($cms));
+    }

 }
 }