From e849d612ccaa084d1cf82e4bb1a2cd986b81699a Mon Sep 17 00:00:00 2001 From: PMKuipers Date: Mon, 26 Jun 2023 21:44:31 +0200 Subject: [PATCH] Cascade cohort sync fixes --- amd/src/studyplan-editor-components.js | 46 ++++--- classes/associationservice.php | 9 +- ...olcohortsync.php => cascadecohortsync.php} | 116 ++++++++++++++---- .../local/aggregators/bistate_aggregator.php | 2 +- classes/local/aggregators/core_aggregator.php | 2 +- classes/local/helpers/debugger.php | 8 +- classes/studyplan.php | 2 +- classes/task/autocohortsync.php | 4 +- lang/en/local_treestudyplan.php | 8 +- lang/nl/local_treestudyplan.php | 7 +- settings.php | 7 ++ version.php | 2 +- 12 files changed, 157 insertions(+), 56 deletions(-) rename classes/{enrolcohortsync.php => cascadecohortsync.php} (51%) diff --git a/amd/src/studyplan-editor-components.js b/amd/src/studyplan-editor-components.js index a02e864..b7beedd 100644 --- a/amd/src/studyplan-editor-components.js +++ b/amd/src/studyplan-editor-components.js @@ -58,8 +58,8 @@ export default { studyline_editmode: 'studyline_editmode', editmode_modules_hidden:'editmode_modules_hidden', studyline_add: 'studyline_add', - add$core: 'add', - edit$core: 'edit', + add$core: 'add$core', + edit$core: 'edit$core', studyline_name: 'studyline_name', studyline_name_ph: 'studyline_name_ph', studyline_shortname: 'studyline_shortname', @@ -83,8 +83,8 @@ export default { advanced_tools: 'advanced_tools', confirm_cancel: 'confirm_cancel', confirm_ok: 'confirm_ok', - success$core: 'success', - error$core: 'failed', + success$core: 'success$core', + error$core: 'failed$core', advanced_converted: 'advanced_converted', advanced_skipped: 'advanced_skipped', advanced_failed: 'advanced_failed', @@ -110,6 +110,9 @@ export default { advanced_import_from_file: 'advanced_import_from_file', advanced_purge: "advanced_purge", advanced_purge_expl: "advanced_purge_expl", + advanced_cascade_cohortsync_title: "advanced_cascade_cohortsync_title", + advanced_cascade_cohortsync_desc: "advanced_cascade_cohortsync_desc", + advanced_cascade_cohortsync: "advanced_cascade_cohortsync", }, studyplan_edit: { studyplan_edit: 'studyplan_edit', @@ -711,6 +714,19 @@ export default { }).fail(notification.exception); }, + cascade_cohortsync(){ + const self = this; + call([{ + methodname: 'local_treestudyplan_cascade_cohortsync', + args: { + studyplan_id: this.value.id, + }, + }])[0].done(function(response){ + self.$bvModal.msgBoxConfirm(self.text.cascade_cohortsync + "\n" + + (response.success?self.text.success$core:self.text.error$core) + + "\n" + response.msg); + }).fail(notification.exception); + }, modal_close(){ this.force_scales.result = []; } @@ -739,6 +755,16 @@ export default { +

{{ text.advanced_cascade_cohortsync_title}}

+ {{ text.advanced_cascade_cohortsync_desc}} +
+ + {{ text.advanced_cascade_cohortsync}} + +

{{ text.advanced_force_scale_title}}

{{ text.advanced_force_scale_desc}}
@@ -757,7 +783,7 @@ export default { - +
  • {{c.course.fullname}} @@ -2187,7 +2213,7 @@ export default { }, methods: { hasGrades() { - if(this.value.course.grades && this.value.course.grades > 0){ + if(this.value.course.grades && this.value.course.grades.length > 0){ for(const g of this.value.course.grades){ if(g.selected){ return true; @@ -2364,14 +2390,6 @@ export default { }, template: `
    - -
      diff --git a/classes/associationservice.php b/classes/associationservice.php index cf7ee41..ee36ad7 100644 --- a/classes/associationservice.php +++ b/classes/associationservice.php @@ -461,16 +461,15 @@ class associationservice extends \external_api } // Actual functions - public static function casdade_cohortsync($studyplan_id) + public static function cascade_cohortsync($studyplan_id) { $studyplan = studyplan::findById($studyplan_id); webservicehelper::require_capabilities(self::CAP_EDIT,$studyplan->context()); + $enroller = new cascadecohortsync($studyplan); + $enroller->sync(); - $enroller = new enrolcohortsync($studyplan); - $result = $enroller->sync(); - - return $result?success::success():success::fail(); + return success::success()->model(); } diff --git a/classes/enrolcohortsync.php b/classes/cascadecohortsync.php similarity index 51% rename from classes/enrolcohortsync.php rename to classes/cascadecohortsync.php index 327d0ce..d10eed7 100644 --- a/classes/enrolcohortsync.php +++ b/classes/cascadecohortsync.php @@ -3,42 +3,80 @@ namespace local_treestudyplan; require_once($CFG->libdir.'/externallib.php'); use \local_treestudyplan\studyplan; +use \local_treestudyplan\local\helpers\debugger; -class enrolcohortsync { +class cascadecohortsync { private const METHOD = "cohort"; private $studyplan; private $studyplanid; + private $debug; - function __construct($studyplan){ + function __construct(studyplan $studyplan){ $this->studyplan = $studyplan; $this->studyplanid = $studyplan->id(); + $this->debug = new debugger("/tmp/treestudyplan.log","ccsync/{$studyplan->shortname()}/{$this->studyplanid}"); } - static private function array_remove_value(&$array,$value){ - while (($key = array_search($value, $array)) !== false) { - unset($array[$key]); + static private function array_remove_value($array,$value){ + $a = []; + foreach($array as $v){ + if($v != $value){ + $a[] = $v; + } } + return $a; + } + + + static function uploadenrolmentmethods_get_group($courseid, $groupname) { + // Function shamelessly copied from tool/uploadenrolmentmethods/locallib.php + global $DB, $CFG; + + require_once($CFG->dirroot.'/group/lib.php'); + + // Check to see if the group name already exists in this course. + if ($DB->record_exists('groups', array('name' => $groupname, 'courseid' => $courseid))) { + $group = $DB->get_record('groups', array('name' => $groupname, 'courseid' => $courseid)); + return $group->id; + } + // The named group doesn't exist, so create a new one in the course. + $groupdata = new \stdClass(); + $groupdata->courseid = $courseid; + $groupdata->name = $groupname; + $groupid = groups_create_group($groupdata); + + return $groupid; } function sync(){ global $DB; - // TODO: Determine if it would be better to add a database table of our own to store references between studyplan and cohort sync enrolment - // instead of using customtext4 for this + + /* Explainer: + This script uses {enrol}.customtext4 to store a json array of all studyplans that need this cohort sync to exist. + Since the cohort-sync enrolment method uses only customint1 and customint2, this is a safe place to store the data. + (Should the cohortsync script at any future time be modified to use customtext fields, it is still extremely unlikely that + customtext4 will be used.) + Because of the overhead involved in keeping an extra table up to date and clean it up if cohort syncs are removed outside of this script, + it was determined to be the simplest and cleanest solution. + */ $enrol = enrol_get_plugin(self::METHOD); + $this->debug->dump($enrol,"Enrol"); // Find the courses that need to be synced to the associated cohorts $courseids = $this->studyplan->get_linked_course_ids(); - + $this->debug->dump($courseids,"courseids"); // And find the cohorts that are linked to this studyplan. $cohortids = $this->studyplan->get_linked_cohort_ids(); + $this->debug->dump($cohortids,"cohortids"); // Next, for each course that is linked: foreach($courseids as $courseid){ $course = get_course($courseid); + $this->debug->write("Processing Course {$courseid} {$course->shortname}"); // first create any nonexistent links foreach($cohortids as $cohortid){ $cohort = $DB->get_record('cohort',['id'=>$cohortid]); - + $this->debug->write("Processing cohort {$cohortid} {$cohort->shortname}"); $instanceparams = [ 'courseid' => $courseid, @@ -53,56 +91,75 @@ class enrolcohortsync { 'roleid' => get_config("local_treestudyplan","csync_roleid"), ]; - + $this->debug->dump($instanceparams,"instanceparams"); + $this->debug->dump($instancenewparams,"instancenewparams"); // Create group: // 1: check if a link exists // If not, make it (maybe use some of the custom text to list the studyplans involved) if ($instance = $DB->get_record('enrol', $instanceparams)) { + $this->debug->write("Instance exists"); // it already exists // check if this studyplan is already referenced in customtext4 in json format - if(empty($instance->customtext4)){ - // Cohort sync was already done, but without studyplan info - // make sure we add a "manual" link to the plan list - $plans = ["manual"]; - } - else{ - $plans = json_decode($instance->customtext4); + + //TODO: Check this code - Maybe add option to not remember manually added stuff + $plans = json_decode($instance->customtext4); + if($plans == false || !is_array(($plans))){ + // If the data was not an array (null or garbled), count it as manually added + // This will prevent it's deletion upon + if(get_config("local_treestudyplan","csync_remember_manual_csync")){ + $plans = ["manual"]; + } else { + $plans = []; + } } + + $this->debug->dump($plans,"plans"); if(!in_array($this->studyplanid ,$plans)){ // if not, add it to the reference + $this->debug->write("Adding this plan to the list"); $plans[] = $this->studyplanid; - $enrol->update_instance($instance,["customtext4"=>json_encode($plans)]); + $enrol->update_instance($instance,(object)["customtext4"=>json_encode($plans)]); } } else { + $this->debug->write("New instance should be made"); // If method members should be added to a group, create it or get its ID. if (get_config("local_treestudyplan","csync_creategroup")) { // Make or get new new cohort group - but only on creating of instances - $groupname = $cohort->name." ".strtolower(get_string('defaultgroupname', 'core')); + $groupname = $cohort->name." ".strtolower(\get_string('defaultgroupname', 'core_group')); + $this->debug->write("Adding group {$groupname} for this method"); // and make sure the - $instancenewparams['customint2'] = uploadenrolmentmethods_get_group($courseid, $groupname); + $instancenewparams['customint2'] = self::uploadenrolmentmethods_get_group($courseid, $groupname); } if ($instanceid = $enrol->add_instance($course, $instancenewparams)) { // also record the (as of yet only) studyplans id requiring this association // in the customtext4 field in json format - $enrol->update_instance($instance,["customtext4"=>json_encode([$this->studyplanid])]); + $this->debug->write("Instance ({$instanceid} created. Updateing instance with studyplan id"); + $instance = $DB->get_record('enrol', array('id' => $instanceid)); + $enrol->update_instance($instance,(object)["customtext4"=>json_encode([$this->studyplanid])]); + $this->debug->write("Synchronize the enrolment"); // Successfully added a valid new instance, so now instantiate it. // First synchronise the enrolment. $cohorttrace = new \null_progress_trace(); - enrol_cohort_sync($cohorttrace, $cohortid); + $result = enrol_cohort_sync($cohorttrace, $cohortid); + if($result > 0){ + $this->debug->write("Error during 'enrol_cohort_sync': code {$result}"); + } $cohorttrace->finished(); } else { // Instance not added for some reason, so report an error somewhere // (or not) + $this->debug->write("Error - instance not added for some reason"); } } } + // 2: Check if there are cohort links for this studyplan in this course that should be removed // A: Check if there are cohort links that are no longer related to this studyplan // B: Check if these links are valid through another studyplan... @@ -112,7 +169,7 @@ class enrolcohortsync { if(get_config("local_treestudyplan","csync_autoremove")){ // Only try the autoremove if the option is enabled - + $this->debug->write("Autoremove scan for course {$courseid} {$course->shortname}"); // find all cohort syncs for this course $searchparams = [ 'courseid' => $courseid, @@ -125,21 +182,27 @@ class enrolcohortsync { if(!empty($instance->customtext4)){ // only check the records that have studyplan information in the customtext4 field // first check if the cohort is not one of the cohort id's we have associated if(!in_array($instance->customint1,$cohortids)){ + $this->debug->write("Found cohort sync instance that is not currently liked to the studyplan: {$instance->id}"); // So it may or may not need to be removed $plans = json_decode($instance->customtext4); + $this->debug->dump($plans,"Plans attachted to instance {$instance->id}"); if($plans !== null && is_array($plans)){ //if a valid array is not returned, better leave it be, we don't want to mess with it // otherwise, check if we should remove it if(in_array($this->studyplanid,$plans)){ + $this->debug->write("Found this studyplan in the id list - removing from list"); //if this plan was referenced before // first remove the link - self::array_remove_value($plans,$this->studyplanid); - if(count($plans) == 0){ + $fplans = self::array_remove_value($plans,$this->studyplanid); + $this->debug->dump($fplans,"Plans proposed to attach to instance {$instance->id}"); + if(count($fplans) == 0){ // delete the sync if there are no studyplan references left + $this->debug->write("No references are left, removing instance"); $enrol->delete_instance($instance); } else { // otherwise just update the references so this studyplan is no longer linked - $enrol->update_instance($instance,["customtext4"=>json_encode($plans)]); + $this->debug->write("Still references left in the list, updating list..."); + $enrol->update_instance($instance,(object)["customtext4"=>json_encode($fplans)]); } } } @@ -148,5 +211,6 @@ class enrolcohortsync { } } } + $this->debug->write("Cascading complete"); } } \ No newline at end of file diff --git a/classes/local/aggregators/bistate_aggregator.php b/classes/local/aggregators/bistate_aggregator.php index 7932c5f..e7741c3 100644 --- a/classes/local/aggregators/bistate_aggregator.php +++ b/classes/local/aggregators/bistate_aggregator.php @@ -58,7 +58,7 @@ class bistate_aggregator extends \local_treestudyplan\aggregator { } } } else { - debug::msg("ARRAY is NOT CONFIG",""); + } } diff --git a/classes/local/aggregators/core_aggregator.php b/classes/local/aggregators/core_aggregator.php index ace562a..22ca11f 100644 --- a/classes/local/aggregators/core_aggregator.php +++ b/classes/local/aggregators/core_aggregator.php @@ -35,7 +35,7 @@ class core_aggregator extends \local_treestudyplan\aggregator { } } } else { - debug::msg("ARRAY is NOT CONFIG",""); + } } diff --git a/classes/local/helpers/debugger.php b/classes/local/helpers/debugger.php index b0e1404..c4099c6 100644 --- a/classes/local/helpers/debugger.php +++ b/classes/local/helpers/debugger.php @@ -22,10 +22,12 @@ class debugger { } - public function dump($object) + public function dump($object,string $tag="") { - global $CFG; - $this->writeblock(print_r($object,true)); + if(strlen($tag) > 0){ + $tag .= ":\n"; + } + $this->writeblock($tag.print_r($object,true)); } diff --git a/classes/studyplan.php b/classes/studyplan.php index 0bf271f..c0f9d52 100644 --- a/classes/studyplan.php +++ b/classes/studyplan.php @@ -771,7 +771,7 @@ class studyplan { $sql = "SELECT DISTINCT j.cohort_id FROM {local_treestudyplan_cohort} j WHERE j.studyplan_id = :studyplan_id"; - $fields = $DB->get_field_sql($sql, ['studyplan_id' => $this->id]); + $fields = $DB->get_fieldset_sql($sql, ['studyplan_id' => $this->id]); return $fields; } diff --git a/classes/task/autocohortsync.php b/classes/task/autocohortsync.php index 4a1b13e..8ecafa7 100644 --- a/classes/task/autocohortsync.php +++ b/classes/task/autocohortsync.php @@ -2,7 +2,7 @@ namespace local_treestudyplan\task; require_once($CFG->dirroot.'/course/externallib.php'); use local_treestudyplan\studyplan; -use local_treestudyplan\enrolcohortsync; +use local_treestudyplan\cascadecohortsync; class autocohortsync extends \core\task\scheduled_task { @@ -23,7 +23,7 @@ class autocohortsync extends \core\task\scheduled_task { $studyplans = studyplan::find_all(); foreach($studyplans as $studyplan) { - $enroller = new enrolcohortsync($studyplan); + $enroller = new cascadecohortsync($studyplan); $enroller->sync(); } } diff --git a/lang/en/local_treestudyplan.php b/lang/en/local_treestudyplan.php index 87fd978..5d256c3 100644 --- a/lang/en/local_treestudyplan.php +++ b/lang/en/local_treestudyplan.php @@ -67,6 +67,8 @@ $string['setting_csync_creategroup_field'] = 'Create groups'; $string['settingdesc_csync_creategroup_field'] = 'Create a group in the course for each cohort sync'; $string['setting_csync_role_field'] = 'Role'; $string['settingdesc_csync_role_field'] = 'The role to use for automatic cohort sync enrolment created in courses'; +$string['setting_csync_remember_manual_csync_field'] = 'Remember existing cohort-syncs'; +$string['settingdesc_csync_remember_manual_csync_field'] = 'Mark cohort syncs that were manually created earlier, so they won\'t be removed during autosync if cohorts are removed from the studyplan'; $string['studyplan_add'] = 'Add study plan'; $string['studyplan_edit'] = 'Edit study plan'; @@ -244,11 +246,15 @@ $string["advanced_import_export"] = 'Backup and restore'; $string["advanced_import"] = 'Restore studylines from backup'; $string["advanced_export"] = 'Backup studyplan'; $string["advanced_export_csv"] = 'Export as CSV'; - $string["advanced_import_from_file"] = "Open from file"; + $string["advanced_purge"] = "Delete"; $string["advanced_purge_expl"] = "Delete the entire studyplan. This is not recoverable"; +$string["advanced_cascade_cohortsync_title"] = "Cascade cohort sync"; +$string["advanced_cascade_cohortsync_desc"] = "Add cohort sync enrolment to each course in this studyplan for all cohorts linked to this studyplan"; +$string["advanced_cascade_cohortsync"] = "Cascade cohort sync"; + $string["advanced_course_manipulation_title"] = 'Course manipulation'; $string["advanced_disable_autoenddate_title"] = 'Disable automatic end date'; $string["advanced_disable_autoenddate_desc"] = 'Disable the default on automatic end date function weekly topics have set in all courses in the studyplan'; diff --git a/lang/nl/local_treestudyplan.php b/lang/nl/local_treestudyplan.php index 4fe1789..077115f 100644 --- a/lang/nl/local_treestudyplan.php +++ b/lang/nl/local_treestudyplan.php @@ -69,7 +69,8 @@ $string['setting_csync_creategroup_field'] = 'Groepen aanmaken'; $string['settingdesc_csync_creategroup_field'] = 'Maak voor elke site-groep synchronisatie een groep aan'; $string['setting_csync_role_field'] = 'Synchronisatierol'; $string['settingdesc_csync_role_field'] = 'Welke rol te gebruiken voor automatische site-groep sync koppelingen'; - +$string['setting_csync_remember_manual_csync_field'] = 'Bewaar bestaande site-group syncs'; +$string['settingdesc_csync_remember_manual_csync_field'] = 'Markeer site-group synchronisaties die eerder handmatig zijn aangemaakt, zodat deze niet worden verwijderd als ze uit het studieplan worden gehaald.'; $string['studyplan_add'] = 'Nieuw studieplan'; $string['studyplan_edit'] = 'Studieplan bewerken'; @@ -249,9 +250,13 @@ $string["advanced_import"] = 'Studieplan invullan vanuit backup'; $string["advanced_export"] = 'Backup downloaden'; $string["advanced_export_csv"] = 'Export als CSV'; $string["advanced_import_from_file"] = "Vanuit bestand"; + $string["advanced_purge"] = "Verwijderen"; $string["advanced_purge_expl"] = "Dit hele studieplan permanent verwijderen."; +$string["advanced_cascade_cohortsync_title"] = "Site-groep synchronisatie doorzetten"; +$string["advanced_cascade_cohortsync_desc"] = "Voeg een site-group synchronisatie aanmelding toe aan alle cursussen in dit studieplan, voor alle gekoppelde site-groepen"; +$string["advanced_cascade_cohortsync"] = "Site-groep sync doorzetten"; $string["advanced_course_manipulation_title"] = 'Cursussen aanpassen'; $string["advanced_disable_autoenddate_title"] = 'Automatische einddatum uitschakelen'; diff --git a/settings.php b/settings.php index e50e78a..fd5183d 100644 --- a/settings.php +++ b/settings.php @@ -178,6 +178,13 @@ if ($hassiteconfig) { true )); + // Enable/disable remembering previously added cohort syncs so they're not automatically deleted + $page_csync->add(new admin_setting_configcheckbox('local_treestudyplan/csync_remember_manual_csync', + get_string('setting_csync_remember_manual_csync_field', 'local_treestudyplan'), + get_string('settingdesc_csync_remember_manual_csync_field', 'local_treestudyplan'), + true + )); + // Enable/disable group creation $page_csync->add(new admin_setting_configcheckbox('local_treestudyplan/csync_creategroup', get_string('setting_csync_creategroup_field', 'local_treestudyplan'), diff --git a/version.php b/version.php index 6572853..fffac53 100644 --- a/version.php +++ b/version.php @@ -1,6 +1,6 @@ component = 'local_treestudyplan'; // Recommended since 2.0.2 (MDL-26035). Required since 3.0 (MDL-48494) -$plugin->version = 2023061601; // YYYYMMDDHH (year, month, day, iteration) +$plugin->version = 2023062602; // YYYYMMDDHH (year, month, day, iteration) $plugin->requires = 2021051700; // YYYYMMDDHH (This is the release version for Moodle 3.11) $plugin->dependencies = [