Moodle 45 by bmbrands · Pull Request #49 · ModernLMS/moodle-availability_othercompleted

bmbrands · 2025-12-05T15:59:21Z

This pull requests combines some of the pull requests done on the original repository

Furthermore it contains fixes for the unit tests and ensures the Moodle plugin ci tests pass too

This is happening because PHP 8.2+ has made dynamic property creation deprecated. We need to properly declare the properties in the class.

Copilot

Pull request overview

This pull request updates the availability_othercompleted Moodle plugin for compatibility with Moodle 4.5 (and potentially 5.0). It combines multiple fixes including unit test improvements, code style updates, and CI/CD integration via GitHub Actions.

Key changes:

Updated version requirements from Moodle 3.7+ to Moodle 4.5+
Modernized test suite with proper namespaces, type hints, and PHPUnit best practices
Added comprehensive Behat tests for course completion restrictions
Improved code documentation and formatting throughout

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
version.php	Updated version to 2025111000 and minimum Moodle requirement to 4.5 (2024100100)
classes/condition.php	Added extensive documentation, modernized syntax (arrays, formatting), but introduced critical bugs in save() and is_available() methods
classes/frontend.php	Added PHPDoc comments, nullable type hints, but retained SQL injection vulnerability
classes/privacy/provider.php	Modernized formatting (removed blank lines, updated spacing)
tests/condition_test.php	Completely rewritten to test COURSE completion (not activity completion), added namespace, modern PHPUnit syntax, but has spelling errors and incomplete test coverage
tests/behat/availability_othercompleted.feature	Added comprehensive Behat tests (previously empty), but contains grammatical errors ("incompleted")
tests/behat/conditional_bug.feature	Removed (now empty file)
lang/en/availability_othercompleted.php	Reordered strings alphabetically, but retained grammatical error ("incompleted")
yui/src/form/js/form.js	Improved code formatting, added eslint directive, updated variable names from datcm to cms
yui/build/*.js	Rebuilt versions reflecting source changes
.github/workflows/gha.yml	Added comprehensive CI/CD workflow for automated testing with Moodle 4.5 and 5.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T16:08:44Z

classes/condition.php

        $allow = false;
-        foreach($datacompletes as $datacomplete){
+        if ($completion && $completion->timecompleted > 0) {
+            $allow = true;
+        }

-            if($datacomplete->timecompleted>0){
-                $allow = true; 
-            }
+        // Handle NOT condition.
+        if ($not) {
+            $allow = !$allow;
        }


The is_available() method doesn't handle the $not parameter correctly for the expected completion state. The logic should first check if the actual completion matches the expected completion ($this->expectedcompletion), then apply the $not inversion.

Currently, the code always checks if the course is complete (regardless of $this->expectedcompletion), then applies the $not inversion. This means when expectedcompletion is COMPLETION_INCOMPLETE, the condition logic is incorrect.

Suggested fix:

// Check if course is completed (has a completion time set). $iscomplete = ($completion && $completion->timecompleted > 0); // Check if the completion state matches what we expect. $allow = ($this->expectedcompletion === COMPLETION_COMPLETE) ? $iscomplete : !$iscomplete; // Handle NOT condition. if ($not) { $allow = !$allow; }

Copilot · 2025-12-05T16:08:45Z

lang/en/availability_othercompleted.php

-$string['requires_incomplete'] = 'You have incompleted course <strong>{$a}</strong>';
+$string['privacy:metadata'] = 'The Restriction by other course completion plugin does not store any personal data.';
 $string['requires_complete'] = 'You have completed course <strong>{$a}</strong>';
+$string['requires_incomplete'] = 'You have incompleted course <strong>{$a}</strong>';


"incompleted" is not a grammatically correct word in English. The correct term should be "incomplete" or "not completed".

Suggested fix:

$string['requires_incomplete'] = 'You have not completed course {$a}';

Alternatively, if you want to keep parallel structure with 'requires_complete':

$string['requires_incomplete'] = 'Course {$a} must be incomplete';

Suggested change

$string['requires_incomplete'] = 'You have incompleted course {$a}';

$string['requires_incomplete'] = 'You have not completed course {$a}';

Copilot · 2025-12-05T16:08:45Z

classes/frontend.php

+            $sql2 = "SELECT * FROM {course}
                    ORDER BY fullname ASC";
            $other = $DB->get_records_sql($sql2);


This SQL query is vulnerable to SQL injection and doesn't follow Moodle coding standards. The query should:

Use parameterized queries or Moodle's database API methods

Specify only the needed fields instead of SELECT *

Use proper filtering to exclude system/hidden courses

Suggested fix:

$courses = $DB->get_records_select( 'course', 'category > 0 AND id != :currentcourseid', ['currentcourseid' => $course->id], 'fullname ASC', 'id, fullname' );

Copilot · 2025-12-05T16:08:46Z

.github/workflows/gha.yml

+          php-version: ${{ matrix.php }}
+          extensions: ${{ matrix.extensions }}
+          ini-values: max_input_vars=5000
+          # If you are not using code coverage, keep "none". Otherwise, use "pcov" (Moodle 3.10 and up) or "xdebug".


[nitpick] This comment references Moodle 3.10 which is quite old. Since the plugin now requires Moodle 4.5+ (based on version.php line 28 requiring 2024100100), this comment should be updated to reflect current supported versions.

Suggested update:

# If you are not using code coverage, keep "none". Otherwise, use "pcov" (Moodle 4.0 and up).

Suggested change

# If you are not using code coverage, keep "none". Otherwise, use "pcov" (Moodle 3.10 and up) or "xdebug".

# If you are not using code coverage, keep "none". Otherwise, use "pcov" (Moodle 4.0 and up).

Copilot · 2025-12-05T16:08:46Z

classes/condition.php

    public function save() {
-        return (object)array('type' => 'othercompleted',
-                'course' => $this->courseid, 'e' => $this->expectedcompletion);
+        return (object)['type' => 'othercompleted',
+                'course' => $this->courseid, 'e' => $this->expectedcompletion];


There's an inconsistency between what the constructor accepts and what the save() method returns:

Constructor (line 71): accepts $structure->cm

save() method (line 99-100): returns property named course

JavaScript fillValue() (yui files line 75): sets value.cm

This creates a round-trip incompatibility. When JavaScript saves the form data using fillValue(), it creates a JSON with property cm. When PHP saves via save(), it returns a JSON with property course. This means the saved data structure is different from the loaded structure.

Recommended fix: The save() method should return 'cm' to match what the constructor expects and what JavaScript provides:

public function save() { return (object)['type' => 'othercompleted', 'cm' => $this->courseid, 'e' => $this->expectedcompletion]; }

Note: The get_json() static method (line 113) should also be updated for consistency.

Copilot · 2025-12-05T16:08:47Z

tests/condition_test.php

+    public function test_usage(): void {
+        global $CFG, $DB, $USER;
        $this->resetAfterTest();

-        // Create course with completion turned on.
        $CFG->enablecompletion = true;
        $CFG->enableavailability = true;
        $generator = $this->getDataGenerator();
-        $course = $generator->create_course(array('enablecompletion' => 1));
+
+        // Create current course.
+        $currentcourse = $generator->create_course(['enablecompletion' => 1]);
+
+        // Create other course with a recognizable name.
+        $othercourse = $generator->create_course([
+            'enablecompletion' => 1,
+            'fullname' => 'Required Course',
+        ]);
+
        $user = $generator->create_user();
-        $generator->enrol_user($user->id, $course->id);
+        $generator->enrol_user($user->id, $currentcourse->id);
+        $generator->enrol_user($user->id, $othercourse->id);
        $this->setUser($user);

-        // Create a Page with manual completion for basic checks.
-        $page = $generator->get_plugin_generator('mod_page')->create_instance(
-                array('course' => $course->id, 'name' => 'Page!',
-                'othercompleted' => COMPLETION_TRACKING_MANUAL));
-
-        // Create an assignment - we need to have something that can be graded
-        // so as to test the PASS/FAIL states.
-
-        $assignrow = $this->getDataGenerator()->create_module('assign', array(
-                'course' => $course->id, 'name' => 'Assign!',
-                'othercompleted' => COMPLETION_TRACKING_AUTOMATIC));
-        $DB->set_field('course_modules', 'completiongradeitemnumber', 0,
-                array('id' => $assignrow->cmid));
-        $assign = new assign(context_module::instance($assignrow->cmid), false, false);
-
-        // Get basic details.
-        $modinfo = get_fast_modinfo($course);
-        $pagecm = $modinfo->get_cm($page->cmid);
-        $assigncm = $assign->get_course_module();
-        $info = new \core_availability\mock_info($course, $user->id);
-
-        // LENGKAP state (false), positif dan TIDAK.
-        $cond = new condition((object)array(
-                'cm' => (int)$pagecm->id, 'e' => COMPLETION_COMPLETE));
+        $info = new \core_availability\mock_info($currentcourse, $user->id);
+
+        // Test COMPLETE requirement when course is NOT complete.
+        $cond = new condition((object)['cm' => (int)$othercourse->id, 'e' => COMPLETION_COMPLETE]);
        $this->assertFalse($cond->is_available(false, $info, true, $user->id));
+
        $information = $cond->get_description(false, false, $info);
-        $information = \core_availability\info::format_info($information, $course);
-        $this->assertRegExp('~Page!.*is marked complete~', $information);
+        $information = \core_availability\info::format_info($information, $currentcourse);
+        $this->assertStringContainsString('Required Course', $information);
+        $this->assertStringContainsString('completed course', $information);
+
+        // Test with NOT condition.
        $this->assertTrue($cond->is_available(true, $info, true, $user->id));

-        // COMPLETE state (false), positive and NOT.
-        $completion = new completion_info($course);
-        $completion->update_state($pagecm, COMPLETION_COMPLETE);
+        // Mark course complete.
+        $ccompletion = new \stdClass();
+        $ccompletion->course = $othercourse->id;
+        $ccompletion->userid = $user->id;
+        $ccompletion->timecompleted = time();
+        $DB->insert_record('course_completions', $ccompletion);

-        // COMPLETE state (true).
-        $cond = new condition((object)array(
-                'cm' => (int)$pagecm->id, 'e' => COMPLETION_COMPLETE));
+        // Now should be available.
        $this->assertTrue($cond->is_available(false, $info, true, $user->id));
        $this->assertFalse($cond->is_available(true, $info, true, $user->id));
-        $information = $cond->get_description(false, true, $info);
-        $information = \core_availability\info::format_info($information, $course);
-        $this->assertRegExp('~Page!.*is incomplete~', $information);
-
    }


The test case doesn't verify the scenario where expectedcompletion is COMPLETION_INCOMPLETE. The test only checks when the condition requires the course to be complete (COMPLETION_COMPLETE).

Given that the is_available() method has a bug with handling expectedcompletion (see related comment), tests should cover:

Course complete when requiring complete (currently tested)

Course incomplete when requiring incomplete (missing)

Course complete when requiring incomplete (missing)

Course incomplete when requiring complete (currently tested)

Consider adding test cases for COMPLETION_INCOMPLETE to ensure the condition works correctly in both directions.

Copilot · 2025-12-05T16:08:48Z

tests/condition_test.php

+        // Note: Saves as 'course' property, not 'cm'.
+        $this->assertEquals(42, $saved->course);


[nitpick] The comment states that the plugin now uses "course IDs" but the test is verifying that the saved property is called "course" (line 153) while the JSON input uses "cm" (line 148). This creates confusion.

More importantly, line 152-153 verify that the save() method returns a property called "course", but this contradicts the actual JSON structure expected by the JavaScript (which uses "cm" in yui/src/form/js/form.js line 78).

The comment on lines 258-259 acknowledges this is incorrect behavior but says it "matches existing behavior", which suggests this is a known bug that should be fixed rather than tested.

Suggested change

// Note: Saves as 'course' property, not 'cm'.

$this->assertEquals(42, $saved->course);

// Should save as 'cm' property to match expected frontend structure.

$this->assertEquals(42, $saved->cm);

Copilot · 2025-12-05T16:08:48Z

classes/condition.php

    public static function get_json($courseid, $expectedcompletion) {
-        return (object)array('type' => 'othercompleted', 'course' => (int)$courseid,
-                'e' => (int)$expectedcompletion);
+        return (object)['type' => 'othercompleted', 'course' => (int)$courseid,


The get_json() static method returns a structure with property 'course', but this is inconsistent with:

The constructor which expects property 'cm' (line 71)

The JavaScript which creates/expects property 'cm' (yui files)

For consistency with the rest of the codebase, this should return 'cm':

public static function get_json($courseid, $expectedcompletion) { return (object)['type' => 'othercompleted', 'cm' => (int)$courseid, 'e' => (int)$expectedcompletion]; }

Suggested change

return (object)['type' => 'othercompleted', 'course' => (int)$courseid,

return (object)['type' => 'othercompleted', 'cm' => (int)$courseid,

Copilot · 2025-12-05T16:08:48Z

tests/behat/availability_othercompleted.feature

+    # Verify TC3 restrictions now show correctly (TC1 complete)
+    And I am on the "Test course 3" "course" page
+    Then I should see "TC3 Activity 1" in the "region-main" "region"
+    And I should see "TC3 Activity 1" in the "region-main" "region"


Line 113 is a duplicate of line 112. Both lines assert the same thing: I should see "TC3 Activity 1" in the "region-main" "region". One of these assertions should be removed or the second one should verify something different.

Suggested change

And I should see "TC3 Activity 1" in the "region-main" "region"

Copilot · 2025-12-05T16:08:49Z

...ild/moodle-availability_othercompleted-form/moodle-availability_othercompleted-form-debug.js

+ * Initialises this plugin.
+ *
+ * @method initInner
+ * @param {Array} cms Array of objects containing cmid => name


The JSDoc comment says "cmid => name" but this is misleading. Based on the code context, this array contains course objects with properties id and name (where id is a course ID, not a cmid).

Suggested fix:

@param {Array} cms Array of objects containing course id and name

Suggested change

* @param {Array} cms Array of objects containing cmid => name

* @param {Array} cms Array of objects containing course id and name

ak4t0sh and others added 7 commits November 10, 2025 13:51

Fix ModernLMS#37 : handle 'must not' condition

b896597

Issue ModernLMS#35: Added default value for $modname

b746976

Update frontend.php - fix for moodle 4.5

f0705bc

This is happening because PHP 8.2+ has made dynamic property creation deprecated. We need to properly declare the properties in the class.

Fix unit tests and CI

b0d8ff3

Code cleanups, fixing unit tests

50aa60d

Fix behat tests

c657ffd

More CI fixes

50d2892

Copilot AI review requested due to automatic review settings December 5, 2025 15:59

Copilot started reviewing on behalf of bmbrands December 5, 2025 15:59 View session

This was referenced Dec 5, 2025

Compatibility with Moodle 4.5 #41

Open

Issues with backup/restore #48

Open

Copilot finished reviewing on behalf of bmbrands December 5, 2025 16:03

Copilot AI reviewed Dec 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moodle 45#49

Moodle 45#49
bmbrands wants to merge 7 commits intoModernLMS:masterfrom
bmbrands:moodle_45

bmbrands commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	$string['requires_incomplete'] = 'You have incompleted course <strong>{$a}</strong>';
	$string['requires_incomplete'] = 'You have not completed course <strong>{$a}</strong>';

	# If you are not using code coverage, keep "none". Otherwise, use "pcov" (Moodle 3.10 and up) or "xdebug".
	# If you are not using code coverage, keep "none". Otherwise, use "pcov" (Moodle 4.0 and up).

		// Note: Saves as 'course' property, not 'cm'.
		$this->assertEquals(42, $saved->course);

	return (object)['type' => 'othercompleted', 'course' => (int)$courseid,
	return (object)['type' => 'othercompleted', 'cm' => (int)$courseid,

	* @param {Array} cms Array of objects containing cmid => name
	* @param {Array} cms Array of objects containing course id and name

Conversation

bmbrands commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants