From dcf52a7beb3cd4a32ab78ff29a6bcd8e9691ad94 Mon Sep 17 00:00:00 2001
From: Felipe Dalcin <felipe@pressbooks.com>
Date: Tue, 6 Feb 2024 17:04:07 -0500
Subject: [PATCH 1/3] fix: correct sync associated users

---
 src/Controllers/InstitutionsController.php |  4 +--
 src/Models/Institution.php                 | 31 +++++++++++++++++++---
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/Controllers/InstitutionsController.php b/src/Controllers/InstitutionsController.php
index 265cf1f9..892fb437 100644
--- a/src/Controllers/InstitutionsController.php
+++ b/src/Controllers/InstitutionsController.php
@@ -149,8 +149,8 @@ protected function save(bool $canUpdateLimits): array
             ->updateDomains(
                 array_map(fn (string $domain) => ['domain' => $domain], $domains)
             )
-            ->updateManagers(
-                array_map(fn (string $id) => ['user_id' => (int) $id, 'manager' => true], $managers)
+            ->syncManagers(
+                array_map(fn (string $id) => (int) $id, $managers),
             );
 
 
diff --git a/src/Models/Institution.php b/src/Models/Institution.php
index fa7c6fb8..39162108 100644
--- a/src/Models/Institution.php
+++ b/src/Models/Institution.php
@@ -41,11 +41,36 @@ public function updateDomains(array $domains): self
         return $this;
     }
 
-    public function updateManagers(array $managers): self
+    public function syncManagers(array $ids): self
     {
-        $this->managers()->delete();
+        $current = $this->users()->pluck('manager', 'user_id')->all();
 
-        $this->managers()->createMany($managers);
+        $managers = array_keys(
+            array_filter($current, fn (bool $isManager) => $isManager)
+        );
+
+        $detach = array_diff($managers, $ids);
+
+        $this->managers()->whereIn('user_id', $detach)->update([
+            'manager' => false,
+        ]);
+
+        $users = array_keys($current);
+
+        foreach ($ids as $id) {
+            if (in_array($id, $users)) {
+                $this->users()->where('user_id', $id)->update([
+                    'manager' => true,
+                ]);
+
+                continue;
+            }
+
+            $this->users()->create([
+                'user_id' => $id,
+                'manager' => true,
+            ]);
+        }
 
         return $this;
     }

From b9e093e4873db3c63b7de8c7ca1e177adc9549aa Mon Sep 17 00:00:00 2001
From: Felipe Dalcin <felipe@pressbooks.com>
Date: Wed, 7 Feb 2024 10:15:55 -0500
Subject: [PATCH 2/3] fix: improve duplicate checks

---
 src/Controllers/InstitutionsController.php | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/Controllers/InstitutionsController.php b/src/Controllers/InstitutionsController.php
index 892fb437..f64fbbcc 100644
--- a/src/Controllers/InstitutionsController.php
+++ b/src/Controllers/InstitutionsController.php
@@ -5,6 +5,7 @@
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Support\Arr;
 use Illuminate\Support\Collection;
+use PressbooksMultiInstitution\Models\EmailDomain;
 use PressbooksMultiInstitution\Models\Institution;
 use PressbooksMultiInstitution\Views\InstitutionsTable;
 use PressbooksMultiInstitution\Support\ConvertEmptyStringsToNull;
@@ -231,21 +232,20 @@ protected function checkForDuplicateDomains(array $domains, ?int $id): array
             return [];
         }
 
-        /** @var Collection $duplicates */
-        $duplicates = Institution::query()
-            ->select('institutions.name as institution', 'institutions_email_domains.domain')
-            ->join('institutions_email_domains', 'institutions.id', '=', 'institutions_email_domains.institution_id')
-            ->whereIn('institutions_email_domains.domain', $domains)
-            ->when($id, fn (Builder $query) => $query->where('institutions.id', '<>', $id))
+        /** @var Collection<EmailDomain> $duplicates */
+        $duplicates = EmailDomain::query()
+            ->with('institution:id,name')
+            ->whereIn('domain', $domains)
+            ->when($id, fn (Builder $query) => $query->where('institution_id', '<>', $id))
             ->get();
 
-        return $duplicates->map(function (object $duplicate) {
+        return $duplicates->map(function (EmailDomain $duplicate) {
             $message = __(
                 'Email domain %s is already in use with %s. Please use a different address.',
                 'pressbooks-multi-institution',
             );
 
-            return sprintf($message, "<strong>{$duplicate->domain}</strong>", "<strong>{$duplicate->institution}</strong>");
+            return sprintf($message, "<strong>{$duplicate->domain}</strong>", "<strong>{$duplicate->institution->name}</strong>");
         })->toArray();
     }
 
@@ -268,6 +268,7 @@ protected function checkForDuplicateManagers(array $managers, ?int $id): array
             ])
             ->join('institutions_users', 'institutions.id', '=', 'institutions_users.institution_id')
             ->whereIn('institutions_users.user_id', $managers)
+            ->where('institutions_users.manager', true)
             ->when($id, fn (Builder $query) => $query->where('institutions.id', '<>', $id))
             ->get();
 

From b9ca690906f0819a894d1f07f77edd3f6d10ff8b Mon Sep 17 00:00:00 2001
From: Felipe Dalcin <felipe@pressbooks.com>
Date: Wed, 7 Feb 2024 10:27:39 -0500
Subject: [PATCH 3/3] fix: add todo note

---
 src/Models/Institution.php | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/Models/Institution.php b/src/Models/Institution.php
index 39162108..1cd65ac5 100644
--- a/src/Models/Institution.php
+++ b/src/Models/Institution.php
@@ -43,6 +43,8 @@ public function updateDomains(array $domains): self
 
     public function syncManagers(array $ids): self
     {
+        // TODO: if a user is assigned to a different institution, we should remove
+        // them from the old institution and assign them to the new one instead.
         $current = $this->users()->pluck('manager', 'user_id')->all();
 
         $managers = array_keys(