From f7b694d15f3417eab0129c6b289ba21ba4a16dce Mon Sep 17 00:00:00 2001
From: Jonathon Fowler <fowlerj@usq.edu.au>
Date: Tue, 28 Jan 2025 14:43:59 +1000
Subject: [PATCH] MDL-60799 enrol: update_instance() needs separate objects
 passed

Passing the same object means enrol_cohort does not recognise a change
in role when an edit or batch course upload operation is done.

 * enrol/editinstance.php was modifying $instance before passing it to
   enrol_plugin::update_instance() so the latter never sees a difference
   versus $data. This is redundant since update_instance() is doing the
   same thing a second time.
 * admin/tool/uploadcourse/classes/course.php is passing the same object
   as both arguments to enrol_plugin::update_instance().

There is an expectation that update_enrol_plugin_data() is to mutate its
$instance argument and then return it based on how derived classes in
enrol_guest and enrol_self behave, so cloning $instance within
update_enrol_plugin_data() before mutating it isn't an option.
---
 admin/tool/uploadcourse/classes/course.php    |  2 +-
 .../uploadcourse/tests/behat/cohorts.feature  | 16 +++++++++++++
 enrol/cohort/tests/behat/enrolcohorts.feature | 23 +++++++++++++++++++
 enrol/editinstance.php                        |  6 -----
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/admin/tool/uploadcourse/classes/course.php b/admin/tool/uploadcourse/classes/course.php
index f808817ccfc..81b3f871018 100644
--- a/admin/tool/uploadcourse/classes/course.php
+++ b/admin/tool/uploadcourse/classes/course.php
@@ -1146,7 +1146,7 @@ class tool_uploadcourse_course {
 
                     // Now update values.
                     // Sort out plugin specific fields.
-                    $modifiedinstance = $plugin->update_enrol_plugin_data($course->id, $method, $instance);
+                    $modifiedinstance = $plugin->update_enrol_plugin_data($course->id, $method, clone $instance);
                     $plugin->update_instance($instance, $modifiedinstance);
                 } else {
                     foreach ($errors as $key => $message) {
diff --git a/admin/tool/uploadcourse/tests/behat/cohorts.feature b/admin/tool/uploadcourse/tests/behat/cohorts.feature
index 9a3676c6ea6..a1096362328 100644
--- a/admin/tool/uploadcourse/tests/behat/cohorts.feature
+++ b/admin/tool/uploadcourse/tests/behat/cohorts.feature
@@ -126,3 +126,19 @@ Feature: An admin can create courses with cohort enrolments using a CSV file
     And I press "Cancel"
     And I click on "Edit" "link" in the "Cohort 4" "table_row"
     And the field "Assign role" matches value "Non-editing teacher"
+
+  @javascript
+  Scenario: Uploading a cohort enrolment method with a non-default role applies only the specified role
+    Given the following "users" exist:
+      | username | firstname | lastname | email          |
+      | t1       | Teacher   | 1        | s1@example.com |
+    And the following "cohort members" exist:
+      | user | cohort |
+      | t1   | CV4    |
+    And I upload "admin/tool/uploadcourse/tests/fixtures/enrolment_cohort_multiple.csv" file to "File" filemanager
+    And I click on "Preview" "button"
+    And I click on "Upload courses" "button"
+    When I am on the "Course 1" "course" page
+    And I navigate to course participants
+    Then I should see "Non-editing teacher" in the "Teacher 1" "table_row"
+    And I should not see "Student" in the "Teacher 1" "table_row"
diff --git a/enrol/cohort/tests/behat/enrolcohorts.feature b/enrol/cohort/tests/behat/enrolcohorts.feature
index a59b8a48029..a68c63ca86a 100644
--- a/enrol/cohort/tests/behat/enrolcohorts.feature
+++ b/enrol/cohort/tests/behat/enrolcohorts.feature
@@ -110,3 +110,26 @@ Feature: Cohort enrolment management
     And I set the field "groups[]" to "Beta2 cohort (2)"
     Then the "members" select box should contain "Bianca McAfee (s3@example.com)"
     And the "members" select box should contain "Abigail Wyatt (s5@example.com)"
+
+  @javascript
+  Scenario: Editing a cohort enrolment method's role should change participants' role
+    Given the following "users" exist:
+      | username | firstname | lastname | email          |
+      | s1       | Sandra    | Cole     | s1@example.com |
+    And the following "cohort members" exist:
+      | user | cohort |
+      | s1   | A1     |
+    When I log in as "teacher001"
+    And I add "Cohort sync" enrolment method in "Course 001" with:
+      | Cohort | Alpha1 |
+    And I should see "Cohort sync (Alpha1 - Student)"
+    And I navigate to course participants
+    And I should see "Student" in the "Sandra Cole" "table_row"
+    And I am on the "Course 001" "enrolment methods" page
+    And I click on "Edit" "link" in the "Alpha1" "table_row"
+    And I set the field "Assign role" to "Non-editing teacher"
+    And I click on "Save" "button"
+    Then I should see "Cohort sync (Alpha1 - Non-editing teacher)"
+    And I navigate to course participants
+    And I should see "Non-editing teacher" in the "Sandra Cole" "table_row"
+    And I should not see "Student" in the "Sandra Cole" "table_row"
diff --git a/enrol/editinstance.php b/enrol/editinstance.php
index 047ab93e933..7623df24c8e 100644
--- a/enrol/editinstance.php
+++ b/enrol/editinstance.php
@@ -89,12 +89,6 @@ if ($mform->is_cancelled()) {
             $reset = ($instance->status != $data->status);
         }
 
-        foreach ($data as $key => $value) {
-            $instance->$key = $value;
-        }
-
-        $instance->timemodified   = time();
-
         $plugin->update_instance($instance, $data);
 
         if ($reset) {