Skip to content

Moodle 45 #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bmbrands wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Moodle 45 #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b896597
Fix #37 : handle 'must not' condition
ak4t0sh Jun 10, 2024
b746976
Issue #35: Added default value for $modname
niko-hoogeveen Mar 11, 2025
f0705bc
Update frontend.php - fix for moodle 4.5
rieckt Mar 10, 2025
b0d8ff3
Fix unit tests and CI
bmbrands Nov 10, 2025
50aa60d
Code cleanups, fixing unit tests
bmbrands Nov 10, 2025
c657ffd
Fix behat tests
bmbrands Nov 11, 2025
50d2892
More CI fixes
bmbrands Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions .github/workflows/gha.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
name: Moodle Plugin CI

on: [push, pull_request]

jobs:
test:
runs-on: ubuntu-22.04

services:
postgres:
image: postgres:15
env:
POSTGRES_USER: 'postgres'
POSTGRES_HOST_AUTH_METHOD: 'trust'
ports:
- 5432:5432
options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 3

mariadb:
image: mariadb:10
env:
MYSQL_USER: 'root'
MYSQL_ALLOW_EMPTY_PASSWORD: "true"
MYSQL_CHARACTER_SET_SERVER: "utf8mb4"
MYSQL_COLLATION_SERVER: "utf8mb4_unicode_ci"
ports:
- 3306:3306
options: --health-cmd="mysqladmin ping" --health-interval 10s --health-timeout 5s --health-retries 3

strategy:
fail-fast: false
matrix:
include:
- php: '8.3'
# Main job. Run all checks that do not require setup and only need to be run once.
runchecks: 'all'
moodle-branch: 'MOODLE_405_STABLE'
database: 'mariadb'
- php: '8.4'
moodle-branch: 'MOODLE_500_STABLE'
database: 'mariadb'

steps:
- name: Check out repository code
uses: actions/checkout@v4
with:
path: plugin

- name: Setup PHP ${{ matrix.php }}
uses: shivammathur/setup-php@v2
with:
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".

Copilot AI Dec 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.
# If you try to use code coverage with "none", it will fallback to phpdbg (which has known problems).
coverage: none

- name: Initialise moodle-plugin-ci
run: |
composer create-project -n --no-dev --prefer-dist moodlehq/moodle-plugin-ci ci ^4
echo $(cd ci/bin; pwd) >> $GITHUB_PATH
echo $(cd ci/vendor/bin; pwd) >> $GITHUB_PATH
sudo locale-gen en_AU.UTF-8
echo "NVM_DIR=$HOME/.nvm" >> $GITHUB_ENV

- name: Install moodle-plugin-ci
run: moodle-plugin-ci install --plugin ./plugin --db-host=127.0.0.1
env:
DB: ${{ matrix.database }}
MOODLE_BRANCH: ${{ matrix.moodle-branch }}
# Uncomment this to run Behat tests using the Moodle App.
# MOODLE_APP: 'true'

- name: PHP Lint
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci phplint

- name: PHP Mess Detector
continue-on-error: true # This step will show errors but will not fail
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci phpmd

- name: Moodle Code Checker
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci phpcs --max-warnings 0

- name: Moodle PHPDoc Checker
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci phpdoc --max-warnings 0

- name: Validating
if: ${{ !cancelled() }}
run: moodle-plugin-ci validate

- name: Check upgrade savepoints
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci savepoints

- name: Mustache Lint
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci mustache

- name: Grunt
if: ${{ !cancelled() && matrix.runchecks == 'all' }}
run: moodle-plugin-ci grunt --max-lint-warnings 0

- name: PHPUnit tests
if: ${{ !cancelled() }}
run: moodle-plugin-ci phpunit --fail-on-warning

- name: Behat features
id: behat
if: ${{ !cancelled() }}
run: moodle-plugin-ci behat --profile chrome --scss-deprecations

- name: Upload Behat Faildump
if: ${{ failure() && steps.behat.outcome == 'failure' }}
uses: actions/upload-artifact@v4
with:
name: Behat Faildump (${{ join(matrix.*, ', ') }})
path: ${{ github.workspace }}/moodledata/behat_dump
retention-days: 7
if-no-files-found: ignore

- name: Mark cancelled jobs as failed.
if: ${{ cancelled() }}
run: exit 1
140 changes: 116 additions & 24 deletions classes/condition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,29 @@
defined('MOODLE_INTERNAL') || die();
require_once($CFG->libdir . '/completionlib.php');

/**
* Other course completion condition.
*
* This plugin restricts access based on completion of OTHER COURSES (not activities).
*
* ARCHITECTURAL NOTE:
* The Moodle availability framework (core_availability) is designed to handle dependencies
* on course modules (activities), not courses. This creates a naming inconsistency:
*
* - JSON property is named "cm" (suggesting course module) but stores a COURSE ID
* - Internal property $courseid correctly identifies it as a course ID
* - The framework's update_dependency_id() expects 'course_modules' table references
* - This plugin checks the 'course_completions' table instead
*
* This means:
* 1. Backup/restore will NOT automatically remap course IDs (courses are global, not course-specific)
* 2. The "cm" property name is misleading but required for framework compatibility
* 3. Tests must create actual courses and course completions, not just activities
*
* @package availability_othercompleted
* @copyright MU DOT MY PLT <support@mu.my>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class condition extends \core_availability\condition {
/** @var int ID of course that this depends on */
protected $courseid;
Expand All @@ -35,7 +58,7 @@ class condition extends \core_availability\condition {
protected $expectedcompletion;

/** @var array Array of modules used in these conditions for course */
protected static $modsusedincondition = array();
protected static $modsusedincondition = [];

/**
* Constructor.
Expand All @@ -52,17 +75,29 @@ public function __construct($structure) {
}

// Get expected completion.
if (isset($structure->e) && in_array($structure->e,
array(COMPLETION_COMPLETE, COMPLETION_INCOMPLETE))) {
if (
isset($structure->e) && in_array(
$structure->e,
[COMPLETION_COMPLETE, COMPLETION_INCOMPLETE]
)
) {
$this->expectedcompletion = $structure->e;
} else {
throw new \coding_exception('Missing or invalid ->e for completion condition');
}
}

/**
* Saves tree data back to a structure object.
*
* NOTE: Returns 'course' property containing a COURSE ID (not module ID).
* Constructor accepts 'cm' property for framework compatibility.
*
* @return \stdClass Structure object (ready to be made into JSON format)
*/
public function save() {
return (object)array('type' => 'othercompleted',
'course' => $this->courseid, 'e' => $this->expectedcompletion);
return (object)['type' => 'othercompleted',
'course' => $this->courseid, 'e' => $this->expectedcompletion];
Comment on lines 98 to +100

Copilot AI Dec 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}

/**
Expand All @@ -75,24 +110,43 @@ public function save() {
* @param int $expectedcompletion Expected completion value (COMPLETION_xx)
*/
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,

Copilot AI Dec 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The constructor which expects property 'cm' (line 71)
  2. 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 uses AI. Check for mistakes.
'e' => (int)$expectedcompletion];
}

/**
* Determines whether this restriction is available for a given user.
*
* Checks if the specified course has been completed by the user.
*
* @param bool $not True if we are inverting the condition
* @param \core_availability\info $info Item we're checking
* @param bool $grabthelot Performance hint: if true, caches information
* @param int $userid User ID to check availability for
* @return bool True if available
*/
public function is_available($not, \core_availability\info $info, $grabthelot, $userid) {

global $DB;

$course = $this->courseid;
$sqlcoursecomplete = "SELECT * FROM {course_completions} as a WHERE a.course = $course AND a.userid = $userid";
$datacompletes = $DB->get_records_sql($sqlcoursecomplete);
// Use parameterized query to prevent SQL injection.
$sql = "SELECT timecompleted
FROM {course_completions}
WHERE course = :courseid AND userid = :userid";
$params = ['courseid' => $this->courseid, 'userid' => $userid];

$completion = $DB->get_record_sql($sql, $params);

// Check if course is completed (has a completion time set).
$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;
}
Comment on lines 140 to 148

Copilot AI Dec 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

return $allow;
}

Expand All @@ -105,7 +159,7 @@ public function is_available($not, \core_availability\info $info, $grabthelot, $
* @return string Readable keyword
*/
protected static function get_lang_string_keyword($completionstate) {
switch($completionstate) {
switch ($completionstate) {
case COMPLETION_INCOMPLETE:
return 'incomplete';
case COMPLETION_COMPLETE:
Expand All @@ -115,12 +169,24 @@ protected static function get_lang_string_keyword($completionstate) {
}
}

/**
* Obtains a string describing this restriction (whether or not it actually applies).
*
* NOTE: Despite accepting an info parameter for course_module info, this condition
* checks COURSE completion, not module completion.
*
* @param bool $full Set true if this is the 'full information' view
* @param bool $not Set true if we are inverting the condition
* @param \core_availability\info $info Info about context/item being checked
* @return string Information string (for admin) about all restrictions on this item
*/
public function get_description($full, $not, \core_availability\info $info) {
// Get name for module.
$modc = get_courses();

$modname = '';
foreach ($modc as $modcs) {
if($modcs->id == $this->courseid){
if ($modcs->id == $this->courseid) {
$modname = $modcs->fullname;
}
}
Comment on lines 184 to 192

Copilot AI Dec 5, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_courses() function retrieves ALL courses in the system, which can be very inefficient and cause performance issues on sites with many courses. This is especially problematic since get_description() is called frequently when displaying availability conditions.

Recommended fix: Fetch only the specific course needed:

public function get_description($full, $not, \core_availability\info $info) {
    global $DB;
    
    // Get course name.
    $course = $DB->get_record('course', ['id' => $this->courseid], 'fullname', IGNORE_MISSING);
    $modname = $course ? $course->fullname : get_string('missingcourse', 'availability_othercompleted');
    
    // ... rest of the method
}

Note: You'll need to add a language string for 'missingcourse' to handle deleted courses gracefully.

Copilot uses AI. Check for mistakes.
Expand All @@ -143,16 +209,21 @@ public function get_description($full, $not, \core_availability\info $info) {
} else {
$str = 'requires_' . self::get_lang_string_keyword($this->expectedcompletion);
}

return get_string($str, 'availability_othercompleted', $modname);
}

/**
* Obtains a representation of the options of this condition as a string, for debugging.
*
* @return string Text representation of parameters
*/
protected function get_debug_string() {
switch ($this->expectedcompletion) {
case COMPLETION_COMPLETE :
case COMPLETION_COMPLETE:
$type = 'COMPLETE';
break;
case COMPLETION_INCOMPLETE :
case COMPLETION_INCOMPLETE:
$type = 'INCOMPLETE';
break;
default:
Expand All @@ -161,6 +232,16 @@ protected function get_debug_string() {
return 'course' . $this->courseid . ' ' . $type;
}

/**
* Tests against a course ID to see if this restriction should be included after restore.
*
* @param string $restoreid The restore identifier
* @param int $courseid The id of the course
* @param \base_logger $logger Logger for any warnings
* @param string $name Name of this item (for use in warning messages)
* @param \base_task $task Current restore task
* @return bool True if this should be included in restore
*/
public function include_after_restore($restoreid, $courseid, \base_logger $logger, $name, \base_task $task) {
global $DB;

Expand All @@ -184,10 +265,9 @@ public static function completion_value_used($course, $cmid) {
if (!array_key_exists($course->id, self::$modsusedincondition)) {
// We don't have data for this course, build it.
$modinfo = get_fast_modinfo($course);
self::$modsusedincondition[$course->id] = array();
self::$modsusedincondition[$course->id] = [];

// Activities.
// foreach ($modinfo->datcm as $othercm) {
// Check all activities.
foreach ($modinfo->cms as $othercm) {
if (is_null($othercm->availability)) {
continue;
Expand Down Expand Up @@ -218,9 +298,21 @@ public static function completion_value_used($course, $cmid) {
* Wipes the static cache of modules used in a condition (for unit testing).
*/
public static function wipe_static_cache() {
self::$modsusedincondition = array();
self::$modsusedincondition = [];
}

/**
* Updates the dependency id stored in this condition if it's relevant.
*
* NOTE: This implementation accepts 'course_modules' table for framework
* compatibility, but actually stores COURSE IDs (not module IDs).
* This is part of the architectural workaround explained in the class docblock.
*
* @param string $table Name of table containing items being restored
* @param int $oldid Previous ID of the item
* @param int $newid New ID of the item
* @return bool True if this condition updated its data
*/
public function update_dependency_id($table, $oldid, $newid) {
if ($table === 'course_modules' && (int)$this->courseid === (int)$oldid) {
$this->courseid = $newid;
Expand Down
Loading