catalyst / moodle-local_enrolgroup

Backport of MDL-58785 to allow external db group sync to be used on older versions of Moodle
Other
1 stars 2 forks source link

Name Trim issues #4

Open nbartley opened 1 year ago

nbartley commented 1 year ago

Heyo,

So ran into an issue with our system. We had a group name that had an escaped character in the name. on line 201 of helper.php you check if the system group name and the sync table are the same, and when they are not you trigger the update, but the update trims the name and triggers an error on the other side with name in use instead of just overwriting the name.

Starting group membershipsynchronisation... Updating group 横浜 : 未確定 (未確定) - in 929 with idnumber 40-8-99-X

Error from the script: Warning: Undefined property: stdClass::$id in /var/www/html/moodle41-20221213/group/lib.php on line 422 ... used 10882 dbqueries ... used 4.7551400661469 seconds Scheduled task failed: Sync enrolment groups (local_enrolgroup\task\sync_groups),IDナンバーがすでに使用されています。 Backtrace:

Code from helper.php that triggers the issue: // Check whether the group should be updated $localgroup = $localgroups[$idnumber]; if ($localgroup->name !== $name) { <----- the whitespace triggers this // Update it. $trace->output("Updating group {$name} in {$courseid} with idnumber {$idnumber}"); groups_update_group($data); }

Moodle code in group/lib.php where things happen:

function groups_update_group($data, $editform = false, $editoroptions = false) { global $CFG, $DB, $USER;

$context = context_course::instance($data->courseid);

$data->timemodified = time();
if (isset($data->name)) {
    $data->name = trim($data->name); <----------- This little devil screws it all up!
}
if (isset($data->idnumber)) {
    $data->idnumber = trim($data->idnumber);  <----------- I imagine this is also a similar bug point as well. 
    if (($existing = groups_get_group_by_idnumber($data->courseid, $data->idnumber)) && $existing->id != $data->id) {
        throw new moodle_exception('idnumbertaken');
    }
}
mastnym commented 1 week ago

Hi @nbartley, I've encountered similar problem. The thing is, that there is a missing groupid (that 'id' prop on StdClass) which you certainly need when you try to update your group Take a look here: https://github.com/catalyst/moodle-local_enrolgroup/pull/5/commits/d6cb4d8a6fb6d6960877701377526383fceb9860

Martin