From f8b70f4ca3f17efde7f8d5e626f692ab0f16e92e Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 15 Feb 2019 10:24:29 +1100
Subject: [PATCH] FIX: unable to create new categories

Previous attempt at 70adb940 missed the critical "everyone" group from
staff, leading to a case where staff was no longer able to create categories
---
 app/models/group.rb                       |  8 ++++++--
 app/models/site.rb                        |  4 +---
 app/serializers/application_serializer.rb |  8 ++++++++
 app/serializers/site_serializer.rb        |  4 +++-
 spec/models/site_spec.rb                  | 10 +++++-----
 5 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/app/models/group.rb b/app/models/group.rb
index b3e2ce2f506..9ed02989bb2 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -88,8 +88,12 @@ class Group < ActiveRecord::Base
   validates :mentionable_level, inclusion: { in: ALIAS_LEVELS.values }
   validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values }
 
-  scope :visible_groups, Proc.new { |user, order|
-    groups = Group.order(order || "name ASC").where("groups.id > 0")
+  scope :visible_groups, Proc.new { |user, order, opts|
+    groups = Group.order(order || "name ASC")
+
+    if !opts || !opts[:include_everyone]
+      groups = groups.where("groups.id > 0")
+    end
 
     unless user&.admin
       sql = <<~SQL
diff --git a/app/models/site.rb b/app/models/site.rb
index 956d6331b41..4dbff4bc41b 100644
--- a/app/models/site.rb
+++ b/app/models/site.rb
@@ -72,9 +72,7 @@ class Site
   end
 
   def groups
-    groups = Group.visible_groups(@guardian.user)
-    groups = groups.where("automatic IS FALSE OR groups.id = #{Group::AUTO_GROUPS[:moderators]}") if !@guardian.is_staff?
-    groups
+    Group.visible_groups(@guardian.user, "name ASC", include_everyone: true)
   end
 
   def suppressed_from_latest_category_ids
diff --git a/app/serializers/application_serializer.rb b/app/serializers/application_serializer.rb
index 245336ef753..29c42994d9f 100644
--- a/app/serializers/application_serializer.rb
+++ b/app/serializers/application_serializer.rb
@@ -25,4 +25,12 @@ class ApplicationSerializer < ActiveModel::Serializer
   def cache_fragment(name)
     ApplicationSerializer.fragment_cache[name] ||= yield
   end
+
+  def cache_anon_fragment(name, &blk)
+    if scope.anonymous?
+      cache_fragment(name, &blk)
+    else
+      blk.call
+    end
+  end
 end
diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb
index aecdccd9d8c..58d3046be30 100644
--- a/app/serializers/site_serializer.rb
+++ b/app/serializers/site_serializer.rb
@@ -50,7 +50,9 @@ class SiteSerializer < ApplicationSerializer
   end
 
   def groups
-    object.groups.pluck(:id, :name).map { |id, name| { id: id, name: name } }.as_json
+    cache_anon_fragment("group_names") do
+      object.groups.order(:name).pluck(:id, :name).map { |id, name| { id: id, name: name } }.as_json
+    end
   end
 
   def post_action_types
diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb
index bd9910cba31..399600b84f6 100644
--- a/spec/models/site_spec.rb
+++ b/spec/models/site_spec.rb
@@ -70,15 +70,15 @@ describe Site do
     user = Fabricate(:user)
     site = Site.new(Guardian.new(user))
 
-    group = Fabricate(:group, visibility_level: Group.visibility_levels[:staff])
-    expect(site.groups.pluck(:name)).to contain_exactly("moderators")
+    staff_group = Fabricate(:group, visibility_level: Group.visibility_levels[:staff])
+    expect(site.groups.pluck(:name)).not_to include(staff_group.name)
 
-    group = Fabricate(:group)
-    expect(site.groups.pluck(:name)).to contain_exactly("moderators", group.name)
+    public_group = Fabricate(:group)
+    expect(site.groups.pluck(:name)).to include(public_group.name)
 
     admin = Fabricate(:admin)
     site = Site.new(Guardian.new(admin))
-    expect(site.groups.pluck(:name)).to match_array(Group.visible_groups(admin).pluck(:name))
+    expect(site.groups.pluck(:name)).to include(staff_group.name, public_group.name, "everyone")
   end
 
   it "includes all enabled authentication providers" do