reworked specs of existing group behavior

app/controllers/groups_controller.rb

@@ -355,8 +355,6 @@ class GroupsController < ApplicationController
       end
     end
 
-    user.primary_group_id = nil if user.primary_group_id == group.id
-
     group.remove(user)
     GroupActionLogger.new(current_user, group).log_remove_user_from_group(user)
 

spec/models/group_spec.rb

@@ -194,70 +194,30 @@ describe Group do
     Group[:staff].user_ids.reject { |id| id < 0 }
   end
 
-  it "Correctly handles primary groups" do
+  describe '#primary_group=' do
-    group = Fabricate(:group, primary_group: true)
+    it "updates all members' #primary_group" do
-    user = Fabricate(:user)
+      group.add(user)
-
-    group.add(user)
-
-    user.reload
-    expect(user.primary_group_id).to eq group.id
-
-    group.remove(user)
-
-    user.reload
-    expect(user.primary_group_id).to eq nil
-
-    group.add(user)
-    group.primary_group = false
-    group.save
-
-    user.reload
-    expect(user.primary_group_id).to eq nil
 
+      expect { group.update(primary_group: true) }.to change { user.reload.primary_group }.from(nil).to(group)
+      expect { group.update(primary_group: false) }.to change { user.reload.primary_group }.from(group).to(nil)
+    end
   end
 
-  it "Correctly handles title" do
+  describe '#title=' do
+    it "updates the member's title only if it was blank or exact match" do
+      group.add(user)
 
-    group = Fabricate(:group, title: 'Super Awesome')
+      expect { group.update(title: 'Awesome') }.to change { user.reload.title }.from(nil).to('Awesome')
-    user = Fabricate(:user)
+      expect { group.update(title: 'Super') }.to change { user.reload.title }.from('Awesome').to('Super')
 
-    expect(user.title).to eq nil
+      user.update(title: 'Differently Awesome')
-
+      expect { group.update(title: 'Awesome') }.to_not change { user.reload.title }
-    group.add(user)
+    end
-    user.reload
-
-    expect(user.title).to eq 'Super Awesome'
-
-    group.title = 'BOOM'
-    group.save
-
-    user.reload
-    expect(user.title).to eq 'BOOM'
-
-    group.title = nil
-    group.save
-
-    user.reload
-    expect(user.title).to eq nil
-
-    group.title = "BOB"
-    group.save
-
-    user.reload
-    expect(user.title).to eq "BOB"
-
-    group.remove(user)
-
-    user.reload
-    expect(user.title).to eq nil
-
-    group.add(user)
-    group.destroy
-
-    user.reload
-    expect(user.title).to eq nil
 
+    it "doesn't update non-member's title" do
+      user.update(title: group.title)
+      expect { group.update(title: 'Super') }.to_not change { user.reload.title }
+    end
   end
 
   describe '.refresh_automatic_group!' do
@@ -494,6 +454,29 @@ describe Group do
       expect(event[:event_name]).to eq(:group_destroyed)
       expect(event[:params].first).to eq(group)
     end
+
+    it "strips the user's title and unsets the user's primary group when exact match" do
+      group.update(title: 'Awesome')
+      user.update(primary_group: group)
+
+      group.destroy!
+
+      user.reload
+      expect(user.title).to eq(nil)
+      expect(user.primary_group).to eq(nil)
+    end
+
+    it "does not strip title or unset primary group when not exact match" do
+      primary_group = Fabricate(:group, primary_group: true, title: 'Different')
+      primary_group.add(user)
+      group.update(title: 'Awesome')
+
+      group.destroy!
+
+      user.reload
+      expect(user.title).to eq('Different')
+      expect(user.primary_group).to eq(primary_group)
+    end
   end
 
   it "has custom fields" do
@@ -689,7 +672,42 @@ describe Group do
 
   end
 
+  describe '#remove' do
+    before { group.add(user) }
+
+    it "only strips user's title if exact match" do
+      group.update(title: 'Awesome')
+      expect { group.remove(user) }.to change { user.reload.title }.from('Awesome').to(nil)
+
+      group.add(user)
+      user.update_columns(title: 'Different')
+      expect { group.remove(user) }.to_not change { user.reload.title }
+    end
+
+    it "unsets the user's primary group" do
+      user.update(primary_group: group)
+      expect { group.remove(user) }.to change { user.reload.primary_group }.from(group).to(nil)
+    end
+  end
+
   describe '#add' do
+    it 'grants the title only if the new member does not have title' do
+      group.update(title: 'Awesome')
+      expect { group.add(user) }.to change { user.reload.title }.from(nil).to('Awesome')
+
+      group.remove(user)
+      user.update(title: 'Already Awesome')
+      expect { group.add(user) }.not_to change { user.reload.title }
+    end
+
+    it "always sets user's primary group" do
+      group.update(primary_group: true)
+      expect { group.add(user) }.to change { user.reload.primary_group }.from(nil).to(group)
+
+      new_group = Fabricate(:group, primary_group: true)
+      expect { new_group.add(user) }.to change { user.reload.primary_group }.from(group).to(new_group)
+    end
+
     context 'when adding a user into a public group' do
       let(:category) { Fabricate(:category) }