FIX: do not show private group flair on user avatars (#13872)

FIX: do not show private group flair on user avatars (#13872)

Meta ref: Visible flair for "invisible" groups: is that on purpose? - ux - Discourse Meta

diff --git a/app/models/group.rb b/app/models/group.rb
index 4832b9a..5cab53b 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -803,11 +803,16 @@ class Group < ActiveRecord::Base
   end
 
   def flair_url
-    case flair_type
-    when :icon
-      flair_icon
-    when :image
-      upload_cdn_path(flair_upload.url)
+    if members_visibility_level == Group.visibility_levels[:public] &&
+      visibility_level == Group.visibility_levels[:public]
+      case flair_type
+      when :icon
+        flair_icon
+      when :image
+        upload_cdn_path(flair_upload.url)
+      else
+        nil
+      end
     else
       nil
     end
diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb
index c7d8d5d..fba5505 100644
--- a/app/serializers/site_serializer.rb
+++ b/app/serializers/site_serializer.rb
@@ -63,7 +63,7 @@ class SiteSerializer < ApplicationSerializer
   def groups
     cache_anon_fragment("group_names") do
       object.groups.order(:name)
-        .select(:id, :name, :flair_icon, :flair_upload_id, :flair_bg_color, :flair_color)
+        .select(:id, :name, :flair_icon, :flair_upload_id, :flair_bg_color, :flair_color, :visibility_level, :members_visibility_level)
         .map do |g|
           {
             id: g.id,
@@ -71,6 +71,8 @@ class SiteSerializer < ApplicationSerializer
             flair_url: g.flair_url,
             flair_bg_color: g.flair_bg_color,
             flair_color: g.flair_color,
+            visibility_level: g.visibility_level,
+            members_visibility_level: g.members_visibility_level,
           }
         end.as_json
     end
diff --git a/lib/user_lookup.rb b/lib/user_lookup.rb
index 590c6e2..3ed0da7 100644
--- a/lib/user_lookup.rb
+++ b/lib/user_lookup.rb
@@ -6,7 +6,7 @@ class UserLookup
   end
 
   def self.group_lookup_columns
-    @group_lookup_columns ||= %i{id name flair_icon flair_upload_id flair_bg_color flair_color}
+    @group_lookup_columns ||= %i{id name flair_icon flair_upload_id flair_bg_color flair_color visibility_level members_visibility_level}
   end
 
   def initialize(user_ids = [])
diff --git a/spec/components/user_lookup_spec.rb b/spec/components/user_lookup_spec.rb
index b9fce92..65c5f2c 100644
--- a/spec/components/user_lookup_spec.rb
+++ b/spec/components/user_lookup_spec.rb
@@ -53,4 +53,32 @@ describe UserLookup do
       expect(user_lookup_group.name).to eq("testgroup")
     end
   end
+
+  describe '#flair_groups' do
+    fab!(:group) { Fabricate(:group, name: "flair_group", flair_icon: "icon", visibility_level: Group.visibility_levels[:public], members_visibility_level: Group.visibility_levels[:public]) }
+    fab!(:user2) { Fabricate(:user, flair_group: group) }
+
+    before do
+      @user_lookup = UserLookup.new([user.id, user2.id, nil])
+    end
+
+    it 'returns nil if user_id does not exists' do
+      expect(@user_lookup.flair_groups[0]).to eq(nil)
+    end
+
+    it 'returns nil if user_id is nil' do
+      expect(@user_lookup.flair_groups[nil]).to eq(nil)
+    end
+
+    it 'returns nil if user has no flair group' do
+      expect(@user_lookup.flair_groups[user.id]).to eq(nil)
+    end
+
+    it 'returns group if user has flair group' do
+      user_lookup_group = @user_lookup.flair_groups[user2.id]
+      expect(user_lookup_group).to eq(group)
+      expect(user_lookup_group.name).to eq("flair_group")
+      expect(user_lookup_group.flair_url).to eq("icon")
+    end
+  end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 5da7ff6..2cd66c8 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -1307,4 +1307,12 @@ describe Group do
       expect(Group.find_by_email("nope@test.com")).to eq(nil)
     end
   end
+
+  it "fetches flair_url based on group visibility" do
+    public_group = Fabricate(:group, flair_icon: "icon", visibility_level: Group.visibility_levels[:public], members_visibility_level: Group.visibility_levels[:public])
+    private_group = Fabricate(:group, flair_icon: "icon", visibility_level: Group.visibility_levels[:logged_on_users], members_visibility_level: Group.visibility_levels[:public])
+
+    expect(public_group.flair_url).to eq("icon")
+    expect(private_group.flair_url).to eq(nil)
+  end
 end
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index 9d62365..fd30fe8 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -725,7 +725,7 @@ describe GroupsController do
 
         expect(group.flair_bg_color).to eq('FFF')
         expect(group.flair_color).to eq('BBB')
-        expect(group.flair_url).to eq('fa-adjust')
+        expect(group.flair_url).to eq(nil)
         expect(group.bio_raw).to eq('testing')
         expect(group.full_name).to eq('awesome team')
         expect(group.public_admission).to eq(true)

GitHub sha: fe3e18f9814d94cf5ca19891262b9376861ce3d0

This commit appears in #13872 which was approved by eviltrout. It was merged by techAPJ.

This commit has been mentioned on Discourse Meta. There might be relevant details there:

Instead of having too many nested if / else I would try to “return early”

def flair_url
  return if members_visibility_level != Group.visibility_levels[:public]
  return if visibility_level != Group.visibility_levels[:public]
  return flair_icon if flair_type == :icon
  return upload_cdn_path(flair_upload.url) if flair_type == :image
end

thanks for the tip! implemented here: FIX: do not show flair bg color if flair is not visible by techAPJ · Pull Request #13969 · discourse/discourse · GitHub