FEATURE: Add the group show endpoint to search groups by id instead of only the slug name (#32442)

jdmartinez1062 · web-flow · commit 507261529006 · 2025-05-13T14:22:06.000-05:00
**Description**
As part of a customer request, we have added the option to search groups
by ID when doing API calls. However, in doing so we have decided to
correct the confusion around the group's routes. Previously the route
would look like `g/:id` while taking the `name` of the group as the
param. For example, when getting a group the route would be like this:

```
GET /g/admins
```

This would make the code in the controller seem as if it was handling
the group IDs instead of names. With these changes, this should be
addressed.
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
@@ -111,7 +111,7 @@ def index
 
   def show
     respond_to do |format|
-      group = find_group(:id)
+      group = find_group_for_show
 
       format.html do
         @title = group.full_name.present? ? group.full_name.capitalize : group.name
@@ -190,7 +190,7 @@ def update
   end
 
   def posts
-    group = find_group(:group_id)
+    group = find_group(:name)
     guardian.ensure_can_see_group_members!(group)
 
     posts =
@@ -208,7 +208,7 @@ def posts
   end
 
   def posts_feed
-    group = find_group(:group_id)
+    group = find_group(:name)
     guardian.ensure_can_see_group_members!(group)
 
     @posts =
@@ -224,7 +224,7 @@ def posts_feed
   def mentions
     raise Discourse::NotFound unless SiteSetting.enable_mentions?
 
-    group = find_group(:group_id)
+    group = find_group(:name)
     guardian.ensure_can_see_group_members!(group)
 
     posts =
@@ -247,7 +247,7 @@ def mentions
   def mentions_feed
     raise Discourse::NotFound unless SiteSetting.enable_mentions?
 
-    group = find_group(:group_id)
+    group = find_group(:name)
     guardian.ensure_can_see_group_members!(group)
 
     @posts =
@@ -267,7 +267,7 @@ def mentions_feed
   MEMBERS_DEFAULT_PAGE_SIZE = 50
 
   def members
-    group = find_group(:group_id)
+    group = find_group(:name)
 
     guardian.ensure_can_see_group_members!(group)
 
@@ -370,7 +370,7 @@ def members
   end
 
   def add_members
-    group = Group.find(params[:id])
+    group = Group.find(params[:name])
     guardian.ensure_can_edit!(group)
 
     users = users_from_params.to_a
@@ -527,7 +527,7 @@ def handle_membership_request
   end
 
   def mentionable
-    group = find_group(:group_id, ensure_can_see: false)
+    group = find_group(:name, ensure_can_see: false)
 
     if group
       render json: { mentionable: Group.mentionable(current_user).where(id: group.id).present? }
@@ -537,7 +537,7 @@ def mentionable
   end
 
   def messageable
-    group = find_group(:group_id, ensure_can_see: false)
+    group = find_group(:name, ensure_can_see: false)
 
     if group
       render json: { messageable: guardian.can_send_private_message?(group) }
@@ -605,7 +605,7 @@ def leave
   def request_membership
     params.require(:reason)
 
-    group = find_group(:id)
+    group = find_group(:name)
 
     begin
       GroupRequest.create!(group: group, user: current_user, reason: params[:reason])
@@ -644,7 +644,7 @@ def request_membership
   end
 
   def set_notifications
-    group = find_group(:id)
+    group = find_group(:name)
     notification_level = params.require(:notification_level)
 
     user_id = current_user.id
@@ -659,7 +659,7 @@ def set_notifications
   end
 
   def histories
-    group = find_group(:group_id)
+    group = find_group(:name)
     guardian.ensure_can_edit!(group) unless guardian.can_admin_group?(group)
 
     page_size = 25
@@ -701,7 +701,7 @@ def search
   end
 
   def permissions
-    group = find_group(:id)
+    group = find_group(:name)
     category_groups =
       group.category_groups.select do |category_group|
         guardian.can_see_category?(category_group.category)
@@ -713,19 +713,19 @@ def permissions
   end
 
   def test_email_settings
-    params.require(:group_id)
+    params.require(:name)
     params.require(:protocol)
     params.require(:port)
     params.require(:host)
     params.require(:username)
     params.require(:password)
 
-    group = Group.find(params[:group_id])
+    group = Group.find(params[:name])
     guardian.ensure_can_edit!(group)
 
     RateLimiter.new(current_user, "group_test_email_settings", 5, 1.minute).performed!
 
-    settings = params.except(:group_id, :protocol)
+    settings = params.except(:name, :protocol)
     email_host = params[:host]
 
     if !%w[smtp imap].include?(params[:protocol])
@@ -868,9 +868,23 @@ def group_params(automatic: false)
     params.require(:group).permit(*attributes)
   end
 
+  def find_group_for_show(ensure_can_see: true)
+    group =
+      if params[:id]
+        Group.find_by(id: params[:id])
+      elsif params[:name]
+        Group.find_by("LOWER(name) = ?", params[:name].downcase)
+      end
+
+    raise Discourse::NotFound if ensure_can_see && !guardian.can_see_group?(group)
+    group
+  end
+
   def find_group(param_name, ensure_can_see: true)
     name = params.require(param_name)
+
     group = Group.find_by("LOWER(name) = ?", name.downcase)
+
     raise Discourse::NotFound if ensure_can_see && !guardian.can_see_group?(group)
     group
   end
diff --git a/app/models/group.rb b/app/models/group.rb
@@ -6,6 +6,7 @@ class Group < ActiveRecord::Base
   # Maximum 255 characters including terminator.
   # https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.4
   MAX_EMAIL_DOMAIN_LENGTH = 253
+  RESERVED_NAMES = %w[by-id]
 
   # TODO: Remove flair_url when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy
   # TODO: Remove smtp_ssl when db/post_migrate/20240717053710_drop_groups_smtp_ssl has been promoted to pre-deploy
@@ -91,6 +92,7 @@ def expire_imap_mailbox_cache
   validates :bio_raw, length: { maximum: 3000 }
   validates :membership_request_template, length: { maximum: 5000 }
   validates :full_name, length: { maximum: 100 }
+  validate :name_cannot_be_reserved
 
   AUTO_GROUPS = {
     everyone: 0,
@@ -1144,6 +1146,12 @@ def name_format_validator
       end
   end
 
+  def name_cannot_be_reserved
+    if RESERVED_NAMES.include?(name.to_s.downcase)
+      errors.add(:name, I18n.t("activerecord.errors.messages.reserved", name: name))
+    end
+  end
+
   def automatic_membership_email_domains_validator
     return if self.automatic_membership_email_domains.blank?
 
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
@@ -288,6 +288,7 @@ en:
         other: "is limited to %{count} characters; you entered %{length}."
       invalid_boolean: "Invalid boolean."
       taken: "has already been taken"
+      reserved: "%{name} group name is reserved, choose a different name"
       accepted: must be accepted
       blank: can't be blank
       present: must be blank
diff --git a/config/routes.rb b/config/routes.rb
@@ -1146,56 +1146,62 @@ def patch(*)
 
     %w[groups g].each do |root_path|
       resources :groups,
-                only: %i[index show new edit update],
+                only: %i[index new edit update],
                 id: RouteFormat.username,
                 path: root_path do
-        get "posts.rss" => "groups#posts_feed", :format => :rss
-        get "mentions.rss" => "groups#mentions_feed", :format => :rss
-
-        get "members"
-        get "posts"
-        get "mentions"
-        get "mentionable"
-        get "messageable"
-        get "logs" => "groups#histories"
-        post "test_email_settings"
-
         collection do
           get "check-name" => "groups#check_name"
           get "custom/new" => "groups#new", :constraints => StaffConstraint.new
           get "search" => "groups#search"
         end
 
         member do
-          %w[
-            activity
-            activity/:filter
-            requests
-            messages
-            messages/inbox
-            messages/archive
-            manage
-            manage/profile
-            manage/members
-            manage/membership
-            manage/interaction
-            manage/email
-            manage/categories
-            manage/tags
-            manage/logs
-          ].each { |path| get path => "groups#show" }
-
-          get "permissions" => "groups#permissions"
-          put "members" => "groups#add_members"
           put "owners" => "groups#add_owners"
           put "join" => "groups#join"
           delete "members" => "groups#remove_member"
           delete "leave" => "groups#leave"
-          post "request_membership" => "groups#request_membership"
           put "handle_membership_request" => "groups#handle_membership_request"
-          post "notifications" => "groups#set_notifications"
         end
       end
+
+      get "#{root_path}/by-id/:id" => "groups#show"
+
+      scope path: "#{root_path}/:name", constraints: { name: RouteFormat.username } do
+        get "/" => "groups#show"
+        get "activity" => "groups#show"
+        get "activity/:filter" => "groups#show"
+        get "requests" => "groups#show"
+        get "messages" => "groups#show"
+        get "messages/inbox" => "groups#show"
+        get "messages/archive" => "groups#show"
+        get "manage" => "groups#show"
+        get "manage/profile" => "groups#show"
+        get "manage/members" => "groups#show"
+        get "manage/membership" => "groups#show"
+        get "manage/interaction" => "groups#show"
+        get "manage/email" => "groups#show"
+        get "manage/categories" => "groups#show"
+        get "manage/tags" => "groups#show"
+        get "manage/logs" => "groups#show"
+        get "permissions" => "groups#permissions"
+        put "members" => "groups#add_members"
+
+        post "request_membership" => "groups#request_membership"
+
+        post "notifications" => "groups#set_notifications",
+             :as => :"#{root_path}_group_set_notifications"
+
+        get "posts.rss" => "groups#posts_feed", :format => :rss
+        get "mentions.rss" => "groups#mentions_feed", :format => :rss
+
+        get "members" => "groups#members"
+        get "posts" => "groups#posts"
+        get "mentions" => "groups#mentions"
+        get "mentionable" => "groups#mentionable"
+        get "messageable" => "groups#messageable"
+        get "logs" => "groups#histories"
+        post "test_email_settings" => "groups#test_email_settings"
+      end
     end
 
     resources :associated_groups, only: %i[index], constraints: AdminConstraint.new
diff --git a/db/post_migrate/20250513161753_rename_by_id_group.rb b/db/post_migrate/20250513161753_rename_by_id_group.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+class RenameByIdGroup < ActiveRecord::Migration[7.2]
+  def change
+    execute <<~SQL
+      UPDATE groups
+      SET name = 'by-id1'
+      WHERE name = 'by-id'
+        AND NOT EXISTS (
+          SELECT 1 FROM groups WHERE name = 'by-id1'
+        );
+    SQL
+  end
+end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
@@ -50,6 +50,17 @@
           )
         end
       end
+
+      context "when a group with a reserved name is created" do
+        it "should not be valid" do
+          new_group = Fabricate.build(:group, name: "by-id")
+          expect(new_group).to_not be_valid
+
+          expect(new_group.errors.full_messages.first).to include(
+            I18n.t("activerecord.errors.messages.reserved", name: "by-id"),
+          )
+        end
+      end
     end
   end
 
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
