FIX: Allow deletion of categories when chat channel is not present

Currently it’s not possible to delete a category if an associated chat
channel is present even if there are no messages in this channel.
This can lead to annoying situations for our users.

This patch addresses the issue by checking if the channel is empty
instead of just checking if there is a channel.

plugins/chat/app/models/chat_channel.rb

@@ -31,6 +31,8 @@ class ChatChannel < ActiveRecord::Base
           )
         }
 
+  delegate :empty?, to: :chat_messages, prefix: true
+
   class << self
     def public_channel_chatable_types
       ["Category"]

plugins/chat/lib/extensions/category_extension.rb

@@ -11,4 +11,9 @@ module Chat::CategoryExtension
     return I18n.t("category.cannot_delete.has_chat_channels") if category_channel
     super
   end
+
+  def deletable_for_chat?
+    return true if !category_channel
+    category_channel.chat_messages_empty?
+  end
 end

plugins/chat/lib/guardian_extensions.rb

@@ -177,6 +177,6 @@ module Chat::GuardianExtensions
   end
 
   def can_delete_category?(category)
-    super && !category.category_channel
+    super && category.deletable_for_chat?
   end
 end

plugins/chat/spec/lib/guardian_extensions_spec.rb

@@ -287,20 +287,61 @@ RSpec.describe Chat::GuardianExtensions do
 
       let(:category) { channel.chatable }
 
-      context "when category has no channel" do
-        before do
-          category.category_channel.destroy
-          category.reload
+      context "when user is staff" do
+        context "when category has no channel" do
+          before do
+            category.category_channel.destroy
+            category.reload
+          end
+
+          it "allows to delete the category" do
+            expect(staff_guardian).to be_able_to_delete_category(category)
+          end
         end
 
-        it "allows to delete the category" do
-          expect(staff_guardian).to be_able_to_delete_category(category)
+        context "when category has a channel" do
+          context "when channel has no messages" do
+            it "allows to delete the category" do
+              expect(staff_guardian).to be_able_to_delete_category(category)
+            end
+          end
+
+          context "when channel has messages" do
+            let!(:message) { Fabricate(:chat_message, chat_channel: channel) }
+
+            it "does not allow to delete the category" do
+              expect(staff_guardian).not_to be_able_to_delete_category(category)
+            end
+          end
         end
       end
 
-      context "when category has a channel" do
-        it "does not allow to delete the category" do
-          expect(staff_guardian).not_to be_able_to_delete_category(category)
+      context "when user is not staff" do
+        context "when category has no channel" do
+          before do
+            category.category_channel.destroy
+            category.reload
+          end
+
+          it "does not allow to delete the category" do
+            expect(guardian).not_to be_able_to_delete_category(category)
+          end
+        end
+
+        context "when category has a channel" do
+          context "when channel has no messages" do
+            it "does not allow to delete the category" do
+              expect(guardian).not_to be_able_to_delete_category(category)
+            end
+          end
+
+          context "when channel has messages" do
+            let!(:message) { Fabricate(:chat_message, chat_channel: channel) }
+
+            it "does not allow to delete the category" do
+              expect(guardian).not_to be_able_to_delete_category(category)
+            end
+          end
         end
       end
     end

plugins/chat/spec/models/category_spec.rb

@@ -22,4 +22,33 @@ RSpec.describe Category do
       end
     end
   end
+
+  describe "#deletable_for_chat?" do
+    subject(:category) { Fabricate.build(:category) }
+
+    context "when no category channel is present" do
+      it "returns true" do
+        expect(category).to be_deletable_for_chat
+      end
+    end
+
+    context "when a category channel is present" do
+      let(:channel) { Fabricate(:category_channel) }
+      let(:category) { channel.chatable }
+
+      context "when it has chat messages" do
+        before { Fabricate(:chat_message, chat_channel: channel) }
+
+        it "returns false" do
+          expect(category).not_to be_deletable_for_chat
+        end
+      end
+
+      context "when it has no chat messages" do
+        it "returns true" do
+          expect(category).to be_deletable_for_chat
+        end
+      end
+    end
+  end
 end

plugins/chat/spec/requests/categories_controller_spec.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+RSpec.describe CategoriesController do
+  describe '#destroy' do
+    subject(:destroy_category) { delete "/categories/#{category.slug}.json" }
+
+    fab!(:admin) { Fabricate(:admin) }
+    fab!(:category) { Fabricate(:category, user: admin) }
+    fab!(:user) { Fabricate(:user) }
+
+    context "when user is staff" do
+      before { sign_in(admin) }
+
+      context "when category has no channel" do
+        it "deletes the category" do
+          expect { destroy_category }.to change { Category.count }.by(-1)
+        end
+      end
+
+      context "when category has a channel" do
+        let!(:channel) { Fabricate(:category_channel, chatable: category) }
+
+        context "when channel has no messages" do
+          it "deletes the category" do
+            expect { destroy_category }.to change { Category.count }.by(-1)
+          end
+        end
+
+        context "when channel has messages" do
+          let!(:message) { Fabricate(:chat_message, chat_channel: channel) }
+
+          it "does not delete the category" do
+            expect { destroy_category }.not_to change { Category.count }
+            expect(response).to be_forbidden
+          end
+        end
+      end
+    end
+
+    context "when user is not staff" do
+      before { sign_in(user) }
+
+      context "when category has no channel" do
+        it "does not delete the category" do
+          expect { destroy_category }.not_to change { Category.count }
+          expect(response).to be_forbidden
+        end
+      end
+
+      context "when category has a channel" do
+        let!(:channel) { Fabricate(:category_channel, chatable: category) }
+
+        context "when channel has no messages" do
+          it "does not delete the category" do
+            expect { destroy_category }.not_to change { Category.count }
+            expect(response).to be_forbidden
+          end
+        end
+
+        context "when channel has messages" do
+          let!(:message) { Fabricate(:chat_message, chat_channel: channel) }
+
+          it "does not delete the category" do
+            expect { destroy_category }.not_to change { Category.count }
+            expect(response).to be_forbidden
+          end
+        end
+      end
+    end
+  end
+end

plugins/chat/spec/support/examples/chat_channel_model.rb

@@ -14,6 +14,7 @@ RSpec.shared_examples "a chat channel model" do
   it { is_expected.to have_many(:chat_messages) }
   it { is_expected.to have_many(:user_chat_channel_memberships) }
   it { is_expected.to have_one(:chat_channel_archive) }
+  it { is_expected.to delegate_method(:empty?).to(:chat_messages).with_prefix }
   it do
     is_expected.to define_enum_for(:status).with_values(
       open: 0,