From ec9c8d446c3c8cdebd7778a00305900ea0569f04 Mon Sep 17 00:00:00 2001
From: John Hsu <ionparticle@gmail.com>
Date: Mon, 27 Nov 2023 21:55:51 -0800
Subject: [PATCH] FIX sysadmin editing user drops them from courses

When editing a user, a sysadmin can unknowingly drop them from courses
on save. This is because the user controller expects the form submit to
contain every single enrolment and this stops happening once you pass
around 1000 courses. Missing enrolments not sent in the form submit are
presumed to be the user dropping the course. While this can happen to
any role that can edit another user, it's most likely to happen to
sysadmins since they have access to every course.

The main fix is to make sure existing enrolments are always in the list
of enrolments to be processed. So missing enrolments not sent in the
form submit gets defaulted to existing enrolments. We also limit
enrolment changes to only the ones that are sent in the form submit.

I tried not to change the behaviour too much, as iPeer is very
sideffects prone and hard to work with. Example: we call
removeStudent/Tutor/Instructor even if the user's role just changed,
they didn't unenrol from the course entirely. I tried to keep that
behavior even though I suspect there are problematic edge cases there.
---
 app/controllers/users_controller.php | 58 ++++++++++++++++------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/app/controllers/users_controller.php b/app/controllers/users_controller.php
index 6783dbd8..2c62d596 100644
--- a/app/controllers/users_controller.php
+++ b/app/controllers/users_controller.php
@@ -730,32 +730,42 @@ public function edit($userId = null, $courseId = null) {
             $this->data['User'] = array_map('trim', $this->data['User']);
 
             // add list of courses the user is enrolled in but the logged
-            // in user does not have access to so that the user would not
-            // be unenrolled from the course when their profile is edited.
+            // in user does not have access to
             $hiddenCourses = $this->_notUnenrolCourses($this->Auth->user('id'), $userId);
-
-            // REMOVE OLD STUDENT STATUSES
-            // unenrol student from course, group, surveygroup
-            // only students will go in because only they have records in Enrolment
-            foreach ($enrolCourses as $course) {
-                if (!in_array($course, $hiddenCourses) && $this->data['CourseEnroll'][$course] != 5) {
-                    $this->User->removeStudent($userId, $course);
+            // create a map of existing course enrolment, course id => role id
+            $courseEnrol = [];
+            foreach ($enrolCourses as $course) $courseEnrol[$course] = 5;
+            foreach ($tutorCourses as $course) $courseEnrol[$course] = 4;
+            foreach ($instructors as $course) $courseEnrol[$course] = 3;
+            // NOTE: In previous implementation, sysadmins can mistakenly
+            // unenrol users when editing a user. This is because it required
+            // every submit to contain every enrolment and this stopped
+            // happening at around 1k courses.  To avoid this, this new
+            // implementation only modify enrolments that are actually sent in
+            // the submit.
+            foreach ($this->data['CourseEnroll'] as $course => $newRole) {
+                if (in_array($course, $hiddenCourses)) {
+                    // logged in user isn't allowed to edit enrolment for course
+                    continue;
                 }
-            }
-
-            // REMOVE OLD TUTOR STATUSES
-            // unenrol tutor from course, group
-            foreach ($tutorCourses as $course) {
-                if (!in_array($course, $hiddenCourses) && $this->data['CourseEnroll'][$course] != 4) {
-                    $this->User->removeTutor($userId, $course);
+                elseif (isset($courseEnrol[$course])) {
+                    // modify existing enrolment
+                    $oldRole = $courseEnrol[$course];
+                    if ($oldRole != $newRole) {
+                        // honestly not sure if the remove functions are good
+                        // but trying not to change too much, so calling them
+                        if ($oldRole == 5)
+                            $this->User->removeStudent($userId, $course);
+                        elseif ($oldRole == 4)
+                            $this->User->removeTutor($userId, $course);
+                        elseif ($oldRole == 3)
+                            $this->User->removeInstructor($userId, $course);
+                        $courseEnrol[$course] = $newRole;
+                    }
                 }
-            }
-
-            // REMOVE OLD INSTRUCTOR STATUSES
-            // unenrol instructor from course
-            foreach ($instructors as $course) {
-                if (!in_array($course, $hiddenCourses) && $this->data['CourseEnroll'][$course] != 3) {
-                    $this->User->removeInstructor($userId, $course);
+                elseif ($newRole > 0) {
+                    // new enrolment in a course
+                    $courseEnrol[$course] = $newRole;
                 }
             }
 
@@ -764,7 +774,7 @@ public function edit($userId = null, $courseId = null) {
             $newStudentCourses = array();
 
             // ADD NEW (possibly existing) STATUSES
-            foreach($this->data['CourseEnroll'] as $currCourseId => $currRoleId) {
+            foreach($courseEnrol as $currCourseId => $currRoleId) {
                 if(!is_numeric($currCourseId)) {
                     continue;
                 }