FIX: Don't responde with error 500 if domain is invalid when adding automatic membership domain (#9655)

app/controllers/admin/groups_controller.rb

@@ -133,7 +133,7 @@ class Admin::GroupsController < Admin::AdminController
 
         return can_not_modify_automatic if group.automatic
 
-        existing_domains = group.automatic_membership_email_domains.split("|")
+        existing_domains = group.automatic_membership_email_domains&.split("|") || []
         domains -= existing_domains
       end
 

app/models/group.rb

@@ -91,6 +91,8 @@ class Group < ActiveRecord::Base
     everyone: 99
   }
 
+  VALID_DOMAIN_REGEX = /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i
+
   def self.visibility_levels
     @visibility_levels = Enum.new(
       public: 0,
@@ -777,7 +779,7 @@ class Group < ActiveRecord::Base
     return if self.automatic_membership_email_domains.blank?
 
     domains = Group.get_valid_email_domains(self.automatic_membership_email_domains) do |domain|
-      self.errors.add :base, (I18n.t('groups.errors.invalid_domain', domain: domain)) unless domain =~ /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i
+      self.errors.add :base, (I18n.t('groups.errors.invalid_domain', domain: domain))
     end
 
     self.automatic_membership_email_domains = domains.join("|")
@@ -862,10 +864,10 @@ class Group < ActiveRecord::Base
       domain.sub!(/^https?:\/\//, '')
       domain.sub!(/\/.*$/, '')
 
-      if domain =~ /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i
+      if domain =~ Group::VALID_DOMAIN_REGEX
         valid_domains << domain
       else
-        yield domain
+        yield domain if block_given?
       end
     end
 

spec/requests/admin/groups_controller_spec.rb

@@ -223,4 +223,31 @@ RSpec.describe Admin::GroupsController do
       end
     end
   end
+
+  describe '#automatic_membership_count' do
+    it 'returns count of users whose emails match the domain' do
+      Fabricate(:user, email: 'user1@somedomain.org')
+      Fabricate(:user, email: 'user1@somedomain.com')
+      Fabricate(:user, email: 'user1@notsomedomain.com')
+      group = Fabricate(:group)
+
+      put "/admin/groups/automatic_membership_count.json", params: {
+        automatic_membership_email_domains: 'somedomain.org|somedomain.com',
+        id: group.id
+      }
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["user_count"]).to eq(2)
+    end
+
+    it "doesn't responde with 500 if domain is invalid" do
+      group = Fabricate(:group)
+
+      put "/admin/groups/automatic_membership_count.json", params: {
+        automatic_membership_email_domains: '@somedomain.org|@somedomain.com',
+        id: group.id
+      }
+      expect(response.status).to eq(200)
+      expect(response.parsed_body["user_count"]).to eq(0)
+    end
+  end
 end