fix: correctly assign existing users as institutional mangers

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;
@@ -149,8 +150,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),
             );
 
 
@@ -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();
 

src/Models/Institution.php

@@ -41,11 +41,38 @@ public function updateDomains(array $domains): self
         return $this;
     }
 
-    public function updateManagers(array $managers): self
+    public function syncManagers(array $ids): self
     {
-        $this->managers()->delete();
+        // 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();
 
-        $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;
     }