From 4da2994d1fab1a167406680876101d19603cc27a Mon Sep 17 00:00:00 2001
From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
Date: Mon, 8 Jun 2020 17:35:24 -0400
Subject: [PATCH] Group Gambit Improvements (#2192)

* - Add ID to fields searched in group gambit
- Use joins instead of looping in group gambit
* Add visibility scoping to group gambit
* call IDs userIds
* If group identifier is numerical, treat it as an ID
---
 src/Group/GroupRepository.php          | 13 -------------
 src/User/Search/Gambit/GroupGambit.php | 24 +++++++++++++++---------
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/src/Group/GroupRepository.php b/src/Group/GroupRepository.php
index adb1355a3..93745b1e2 100644
--- a/src/Group/GroupRepository.php
+++ b/src/Group/GroupRepository.php
@@ -41,19 +41,6 @@ class GroupRepository
         return $this->scopeVisibleTo($query, $actor)->firstOrFail();
     }
 
-    /**
-     * Find a group by name.
-     *
-     * @param string $name
-     * @return User|null
-     */
-    public function findByName($name, User $actor = null)
-    {
-        $query = Group::where('name_singular', $name)->orWhere('name_plural', $name);
-
-        return $this->scopeVisibleTo($query, $actor)->first();
-    }
-
     /**
      * Scope a query to only include records that are visible to a user.
      *
diff --git a/src/User/Search/Gambit/GroupGambit.php b/src/User/Search/Gambit/GroupGambit.php
index 926c9ca66..5e113e6df 100644
--- a/src/User/Search/Gambit/GroupGambit.php
+++ b/src/User/Search/Gambit/GroupGambit.php
@@ -9,6 +9,7 @@
 
 namespace Flarum\User\Search\Gambit;
 
+use Flarum\Group\Group;
 use Flarum\Group\GroupRepository;
 use Flarum\Search\AbstractRegexGambit;
 use Flarum\Search\AbstractSearch;
@@ -44,18 +45,23 @@ class GroupGambit extends AbstractRegexGambit
             throw new LogicException('This gambit can only be applied on a UserSearch');
         }
 
-        $groupNames = $this->extractGroupNames($matches);
+        $groupIdentifiers = $this->extractGroupIdentifiers($matches);
 
-        // TODO: Use a JOIN instead (and don't forget to remove the findByName() method again)
-        $ids = [];
-        foreach ($groupNames as $name) {
-            $group = $this->groups->findByName($name);
-            if ($group && count($group->users)) {
-                $ids = array_merge($ids, $group->users->pluck('id')->all());
+        $groupQuery = Group::whereVisibleTo($search->getActor());
+
+        foreach ($groupIdentifiers as $identifier) {
+            if (is_numeric($identifier)) {
+                $groupQuery->orWhere('id', $identifier);
+            } else {
+                $groupQuery->orWhere('name_singular', $identifier)->orWhere('name_plural', $identifier);
             }
         }
 
-        $search->getQuery()->whereIn('id', $ids, 'and', $negate);
+        $userIds = $groupQuery->join('group_user', 'groups.id', 'group_user.group_id')
+            ->pluck('group_user.user_id')
+            ->all();
+
+        $search->getQuery()->whereIn('id', $userIds, 'and', $negate);
     }
 
     /**
@@ -64,7 +70,7 @@ class GroupGambit extends AbstractRegexGambit
      * @param array $matches
      * @return array
      */
-    protected function extractGroupNames(array $matches)
+    protected function extractGroupIdentifiers(array $matches)
     {
         return explode(',', trim($matches[1], '"'));
     }