FIX: Check for group name availability should skip reserved usernames.

app/assets/javascripts/discourse/components/groups-form-profile-fields.js.es6

@@ -2,7 +2,7 @@ import {
   default as computed,
   observes
 } from "ember-addons/ember-computed-decorators";
-import User from "discourse/models/user";
+import Group from "discourse/models/group";
 import InputValidation from "discourse/models/input-validation";
 import debounce from "discourse/lib/debounce";
 
@@ -63,7 +63,7 @@ export default Ember.Component.extend({
     name = this.get("nameInput");
     if (Ember.isEmpty(name)) return;
 
-    User.checkUsername(name).then(response => {
+    Group.checkName(name).then(response => {
       const validationName = "uniqueNameValidation";
 
       if (response.available) {

app/assets/javascripts/discourse/models/group.js.es6

@@ -8,6 +8,7 @@ import RestModel from "discourse/models/rest";
 import Category from "discourse/models/category";
 import User from "discourse/models/user";
 import Topic from "discourse/models/topic";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 
 const Group = RestModel.extend({
   limit: 50,
@@ -303,6 +304,12 @@ Group.reopenClass({
 
   messageable(name) {
     return ajax(`/groups/${name}/messageable`);
+  },
+
+  checkName(name) {
+    return ajax("/groups/check-name", {
+      data: { group_name: name }
+    }).catch(popupAjaxError);
   }
 });
 

app/controllers/groups_controller.rb

@@ -3,6 +3,7 @@ class GroupsController < ApplicationController
     :set_notifications,
     :mentionable,
     :messageable,
+    :check_name,
     :update,
     :histories,
     :request_membership,
@@ -314,6 +315,12 @@ class GroupsController < ApplicationController
     end
   end
 
+  def check_name
+    group_name = params.require(:group_name)
+    checker = UsernameCheckerService.new(allow_reserved_username: true)
+    render json: checker.check_username(group_name, nil)
+  end
+
   def remove_member
     group = Group.find_by(id: params[:id])
     raise Discourse::NotFound unless group

app/models/user.rb

@@ -204,9 +204,9 @@ class User < ActiveRecord::Base
     SiteSetting.min_username_length.to_i..SiteSetting.max_username_length.to_i
   end
 
-  def self.username_available?(username, email = nil)
+  def self.username_available?(username, email = nil, allow_reserved_username: false)
     lower = username.downcase
-    return false if reserved_username?(lower)
+    return false if !allow_reserved_username && reserved_username?(lower)
     return true  if DB.exec(User::USERNAME_EXISTS_SQL, username: lower) == 0
 
     # staged users can use the same username since they will take over the account

app/services/username_checker_service.rb

@@ -1,4 +1,7 @@
 class UsernameCheckerService
+  def initialize(allow_reserved_username: false)
+    @allow_reserved_username = allow_reserved_username
+  end
 
   def check_username(username, email)
     if username && username.length > 0
@@ -12,7 +15,13 @@ class UsernameCheckerService
   end
 
   def check_username_availability(username, email)
-    if User.username_available?(username, email)
+    available = User.username_available?(
+      username,
+      email,
+      allow_reserved_username: @allow_reserved_username
+    )
+
+    if available
       { available: true, is_developer: is_developer?(email) }
     else
       { available: false, suggestion: UserNameSuggester.suggest(username) }

config/routes.rb

@@ -482,6 +482,7 @@ Discourse::Application.routes.draw do
     get 'logs' => 'groups#histories'
 
     collection do
+      get "check-name" => 'groups#check_name'
       get 'custom/new' => 'groups#new', constraints: AdminConstraint.new
       get "search" => "groups#search"
     end

spec/models/user_spec.rb

@@ -546,6 +546,16 @@ describe User do
       expect(User.username_available?('tESt')).to eq(false)
     end
 
+    it 'returns true when reserved username is explicity allowed' do
+      SiteSetting.reserved_usernames = 'test|donkey'
+
+      expect(User.username_available?(
+        'tESt',
+        nil,
+        allow_reserved_username: true)
+      ).to eq(true)
+    end
+
     it "returns true when username is associated to a staged user of the same email" do
       staged = Fabricate(:user, staged: true, email: "foo@bar.com")
       expect(User.username_available?(staged.username, staged.primary_email.email)).to eq(true)

spec/requests/groups_controller_spec.rb

@@ -1320,4 +1320,22 @@ describe GroupsController do
       end
     end
   end
+
+  describe '#check_name' do
+    describe 'for an anon user' do
+      it 'should return the right response' do
+        get "/groups/check-name.json", params: { group_name: 'test' }
+        expect(response.status).to eq(403)
+      end
+    end
+
+    it 'should return the right response' do
+      sign_in(Fabricate(:user))
+      SiteSetting.reserved_usernames = 'test|donkey'
+      get "/groups/check-name.json", params: { group_name: 'test' }
+
+      expect(response.status).to eq(200)
+      expect(JSON.parse(response.body)["available"]).to eq(true)
+    end
+  end
 end

spec/services/username_checker_service_spec.rb

@@ -15,6 +15,7 @@ describe UsernameCheckerService do
         result = @service.check_username('a', @nil_email)
         expect(result).to have_key(:errors)
       end
+
       it 'rejects too long usernames' do
         result = @service.check_username('a123456789b123456789c123456789', @nil_email)
         expect(result).to have_key(:errors)
@@ -29,6 +30,23 @@ describe UsernameCheckerService do
         result = @service.check_username('.vincent', @nil_email)
         expect(result).to have_key(:errors)
       end
+
+      describe 'reserved usernames' do
+        before do
+          SiteSetting.reserved_usernames = 'test|donkey'
+        end
+
+        it 'rejects usernames that are reserved' do
+          result = @service.check_username("test", @nil_email)
+          expect(result[:available]).to eq(false)
+        end
+
+        it 'allows reserved username checker to be skipped' do
+          @service = UsernameCheckerService.new(allow_reserved_username: true)
+          result = @service.check_username("test", @nil_email)
+          expect(result[:available]).to eq(true)
+        end
+      end
     end
 
     it 'username not available locally' do