FIX: leaving a group channel should destroy membership (#24631)

In other kind of channels we will only unfollow but for group channels we don't want people to keep appearing in members list.

This commit also creates appropriate services:
- `Chat::LeaveChannel`
- `Chat::UnfollowChannel`

And dedicated endpoint for unfollow: `DELETE /chat/api/channels/:id/memberships/me/follows`

plugins/chat/app/controllers/chat/api/channels_current_user_membership_controller.rb

@@ -12,10 +12,6 @@ class Chat::Api::ChannelsCurrentUserMembershipController < Chat::Api::ChannelsCo
   end
 
   def destroy
-    render_serialized(
+    with_service(Chat::LeaveChannel) { on_model_not_found(:channel) { raise Discourse::NotFound } }
-      channel_from_params.remove(current_user),
-      Chat::UserChannelMembershipSerializer,
-      root: "membership",
-    )
   end
 end

plugins/chat/app/controllers/chat/api/channels_current_user_membership_follows_controller.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class Chat::Api::ChannelsCurrentUserMembershipFollowsController < Chat::Api::ChannelsController
+  def destroy
+    with_service(Chat::UnfollowChannel) do
+      on_success do
+        render_serialized(
+          result.membership,
+          Chat::UserChannelMembershipSerializer,
+          root: "membership",
+        )
+      end
+      on_model_not_found(:channel) { raise Discourse::NotFound }
+    end
+  end
+end

plugins/chat/app/services/chat/leave_channel.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+module Chat
+  # Service responsible to flag a message.
+  #
+  # @example
+  #  ::Chat::LeaveChannel.call(
+  #    guardian: guardian,
+  #    channel_id: 1,
+  #  )
+  #
+  class LeaveChannel
+    include Service::Base
+
+    # @!method call(guardian:, channel_id:,)
+    #   @param [Guardian] guardian
+    #   @param [Integer] channel_id of the channel
+
+    #   @return [Service::Base::Context]
+    contract
+    model :channel
+    step :leave
+    step :recompute_users_count
+
+    # @!visibility private
+    class Contract
+      attribute :channel_id, :integer
+      validates :channel_id, presence: true
+    end
+
+    private
+
+    def fetch_channel(contract:, **)
+      Chat::Channel.find_by(id: contract.channel_id)
+    end
+
+    def leave(channel:, guardian:, **)
+      ActiveRecord::Base.transaction do
+        if channel.direct_message_channel? && channel.chatable&.group
+          channel.membership_for(guardian.user)&.destroy!
+          channel.chatable.direct_message_users.where(user_id: guardian.user.id).destroy_all
+        else
+          channel.remove(guardian.user)
+        end
+      end
+    end
+
+    def recompute_users_count(channel:, **)
+      channel.update!(
+        user_count: ::Chat::ChannelMembershipsQuery.count(channel),
+        user_count_stale: false,
+      )
+    end
+  end
+end

plugins/chat/app/services/chat/unfollow_channel.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+module Chat
+  # Service responsible to flag a message.
+  #
+  # @example
+  #  ::Chat::UnfollowChannel.call(
+  #    guardian: guardian,
+  #    channel_id: 1,
+  #  )
+  #
+  class UnfollowChannel
+    include Service::Base
+
+    # @!method call(guardian:, channel_id:,)
+    #   @param [Guardian] guardian
+    #   @param [Integer] channel_id of the channel
+
+    #   @return [Service::Base::Context]
+    contract
+    model :channel
+    step :unfollow
+
+    # @!visibility private
+    class Contract
+      attribute :channel_id, :integer
+      validates :channel_id, presence: true
+    end
+
+    private
+
+    def fetch_channel(contract:, **)
+      Chat::Channel.find_by(id: contract.channel_id)
+    end
+
+    def unfollow(channel:, guardian:, **)
+      context.membership = channel.remove(guardian.user)
+    end
+  end
+end

plugins/chat/assets/javascripts/discourse/components/chat-channel-row.gjs

@@ -131,7 +131,7 @@ export default class ChatChannelRow extends Component {
   }
 
   get leaveDirectMessageLabel() {
-    return I18n.t("chat.direct_messages.leave");
+    return I18n.t("chat.direct_messages.close");
   }
 
   get leaveChannelLabel() {

plugins/chat/assets/javascripts/discourse/components/chat-channel-settings.gjs

@@ -29,12 +29,14 @@ const NOTIFICATION_LEVELS = [
 export default class ChatAboutScreen extends Component {
   @service chatApi;
   @service chatGuardian;
+  @service chatChannelsManager;
   @service currentUser;
   @service siteSettings;
   @service dialog;
   @service modal;
   @service site;
   @service toasts;
+  @service router;
 
   notificationLevels = NOTIFICATION_LEVELS;
 
@@ -309,6 +311,12 @@ export default class ChatAboutScreen extends Component {
     });
   }
 
+  @action
+  onLeaveChannel(channel) {
+    this.chatChannelsManager.remove(channel);
+    return this.router.transitionTo("chat");
+  }
+
   @action
   onEditChannelDescription() {
     return this.modal.show(ChatModalEditChannelDescription, {
@@ -573,6 +581,7 @@ export default class ChatAboutScreen extends Component {
             <:action>
               <ToggleChannelMembershipButton
                 @channel={{@channel}}
+                @onLeave={{this.onLeaveChannel}}
                 @options={{hash
                   joinClass="btn-primary"
                   leaveClass="btn-danger"

plugins/chat/assets/javascripts/discourse/components/toggle-channel-membership-button.gjs

@@ -6,10 +6,13 @@ import DButton from "discourse/components/d-button";
 import concatClass from "discourse/helpers/concat-class";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import I18n from "discourse-i18n";
+
 export default class ToggleChannelMembershipButton extends Component {
   @service chat;
+  @service chatApi;
+
   @tracked isLoading = false;
-  onToggle = null;
+
   options = {};
 
   constructor() {
@@ -54,7 +57,7 @@ export default class ToggleChannelMembershipButton extends Component {
     return this.chat
       .followChannel(this.args.channel)
       .then(() => {
-        this.onToggle?.();
+        this.args.onJoin?.(this.args.channel);
       })
       .catch(popupAjaxError)
       .finally(() => {
@@ -67,22 +70,22 @@ export default class ToggleChannelMembershipButton extends Component {
   }
 
   @action
-  onLeaveChannel() {
+  async onLeaveChannel() {
     this.isLoading = true;
 
-    return this.chat
+    try {
-      .unfollowChannel(this.args.channel)
+      if (this.args.channel.chatable.group) {
-      .then(() => {
+        await this.chatApi.leaveChannel(this.args.channel.id);
-        this.onToggle?.();
+      } else {
-      })
+        await this.chat.unfollowChannel(this.args.channel);
-      .catch(popupAjaxError)
-      .finally(() => {
-        if (this.isDestroying || this.isDestroyed) {
-          return;
       }
 
+      this.args.onLeave?.(this.args.channel);
+    } catch (error) {
+      popupAjaxError(error);
+    } finally {
       this.isLoading = false;
-      });
+    }
   }
 
   <template>

plugins/chat/assets/javascripts/discourse/initializers/chat-sidebar.js

@@ -211,7 +211,7 @@ export default {
             suffixCSSClass = "urgent";
             hoverType = "icon";
             hoverValue = "times";
-            hoverTitle = I18n.t("chat.direct_messages.leave");
+            hoverTitle = I18n.t("chat.direct_messages.close");
 
             constructor({ channel, chatService, currentUser }) {
               super(...arguments);

plugins/chat/assets/javascripts/discourse/routes/chat-channel-decorator.js

@@ -34,6 +34,13 @@ export default function withChatChannel(extendedClass) {
       let { channelTitle } = this.paramsFor(this.routeName);
 
       if (channelTitle && channelTitle !== model.slugifiedTitle) {
+        if (this.routeName === "chat.channel.info") {
+          return this.router.replaceWith(
+            "chat.channel.info",
+            ...model.routeModels
+          );
+        }
+
         const messageId = this.paramsFor("chat.channel.near-message").messageId;
         const threadId = this.paramsFor("chat.channel.thread").threadId;
 

plugins/chat/assets/javascripts/discourse/services/chat-api.js

@@ -293,9 +293,19 @@ export default class ChatApi extends Service {
    * @returns {Promise}
    */
   unfollowChannel(channelId) {
-    return this.#deleteRequest(`/channels/${channelId}/memberships/me`).then(
+    return this.#deleteRequest(
-      (result) => UserChatChannelMembership.create(result.membership)
+      `/channels/${channelId}/memberships/me/follows`
-    );
+    ).then((result) => UserChatChannelMembership.create(result.membership));
+  }
+
+  /**
+   * Destroys the membership of current user on a channel.
+   *
+   * @param {number} channelId - The ID of the channel.
+   * @returns {Promise}
+   */
+  leaveChannel(channelId) {
+    return this.#deleteRequest(`/channels/${channelId}/memberships/me`);
   }
 
   /**

plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js

@@ -77,13 +77,15 @@ export default class ChatChannelsManager extends Service {
   }
 
   async unfollow(model) {
+    try {
       this.chatSubscriptionsManager.stopChannelSubscription(model);
-
+      model.currentUserMembership = await this.chatApi.unfollowChannel(
-    return this.chatApi.unfollowChannel(model.id).then((membership) => {
+        model.id
-      model.currentUserMembership = membership;
+      );
-
       return model;
-    });
+    } catch (error) {
+      popupAjaxError(error);
+    }
   }
 
   @debounce(300)

plugins/chat/config/locales/client.en.yml

@@ -495,6 +495,7 @@ en:
         new: "Create a personal chat"
         create: "Go"
         leave: "Leave this personal chat"
+        close: "Close this personal chat"
         cannot_create: "Sorry, you cannot send direct messages."
 
       incoming_webhooks:

plugins/chat/config/routes.rb

@@ -22,6 +22,9 @@ Chat::Engine.routes.draw do
     get "/channels/:channel_id/memberships" => "channels_memberships#index"
     post "/channels/:channel_id/memberships" => "channels_memberships#create"
     delete "/channels/:channel_id/memberships/me" => "channels_current_user_membership#destroy"
+    delete "/channels/:channel_id/memberships/me/follows" =>
+             "channels_current_user_membership_follows#destroy"
+    put "/channels/:channel_id/memberships/me" => "channels_current_user_membership#update"
     post "/channels/:channel_id/memberships/me" => "channels_current_user_membership#create"
     put "/channels/:channel_id/notifications-settings/me" =>
           "channels_current_user_notifications_settings#update"

plugins/chat/spec/requests/chat/api/channels_current_user_membership_controller_spec.rb

@@ -105,46 +105,19 @@ describe Chat::Api::ChannelsCurrentUserMembershipController do
   end
 
   describe "#destroy" do
-    context "when an existing membership exists" do
+    describe "success" do
-      before { channel_1.add(current_user) }
+      it "works" do
+        delete "/chat/api/channels/#{channel_1.id}/memberships/me"
 
-      it "enforces 'following' to false" do
-        membership_record = channel_1.membership_for(current_user)
-
-        expect { delete "/chat/api/channels/#{channel_1.id}/memberships/me" }.to change {
-          membership_record.reload.following
-        }.to(false).from(true)
         expect(response.status).to eq(200)
-        expect(response.parsed_body["membership"]["following"]).to eq(false)
-        expect(response.parsed_body["membership"]["chat_channel_id"]).to eq(channel_1.id)
-        expect(response.parsed_body["membership"]["user"]["id"]).to eq(current_user.id)
-      end
-
-      context "when current user can’t see the channel" do
-        fab!(:channel_2) { Fabricate(:private_category_channel, group: Fabricate(:group)) }
-
-        it "fails" do
-          expect { delete "/chat/api/channels/#{channel_2.id}/memberships/me" }.not_to change {
-            Chat::UserChatChannelMembership.where(user_id: current_user.id).count
-          }
-          expect(response.status).to eq(403)
       end
     end
 
-      context "when the channel is a direct message channel" do
+    context "when channel is not found" do
-        fab!(:dm_channel_1) { Fabricate(:direct_message_channel, users: [current_user]) }
+      it "returns a 404" do
+        delete "/chat/api/channels/-999/memberships/me"
 
-        it "enforces 'following' to false" do
+        expect(response.status).to eq(404)
-          membership_record = dm_channel_1.membership_for(current_user)
-
-          expect { delete "/chat/api/channels/#{dm_channel_1.id}/memberships/me" }.to change {
-            membership_record.reload.following
-          }.to(false).from(true)
-          expect(response.status).to eq(200)
-          expect(response.parsed_body["membership"]["following"]).to eq(false)
-          expect(response.parsed_body["membership"]["chat_channel_id"]).to eq(dm_channel_1.id)
-          expect(response.parsed_body["membership"]["user"]["id"]).to eq(current_user.id)
-        end
       end
     end
   end

plugins/chat/spec/requests/chat/api/channels_current_user_membership_follows_controller_spec.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe Chat::Api::ChannelsCurrentUserMembershipController do
+  fab!(:current_user) { Fabricate(:user) }
+  fab!(:channel_1) { Fabricate(:category_channel) }
+
+  before do
+    channel_1.add(current_user)
+    SiteSetting.chat_enabled = true
+    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
+    sign_in(current_user)
+  end
+
+  describe "#destroy" do
+    describe "success" do
+      it "works" do
+        delete "/chat/api/channels/#{channel_1.id}/memberships/me/follows"
+
+        expect(response.status).to eq(200)
+        expect(channel_1.membership_for(current_user).following).to eq(false)
+      end
+    end
+
+    context "when channel is not found" do
+      it "returns a 404" do
+        delete "/chat/api/channels/-999/memberships/me/follows"
+
+        expect(response.status).to eq(404)
+      end
+    end
+  end
+end

plugins/chat/spec/services/chat/leave_channel_spec.rb

@@ -0,0 +1,116 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::LeaveChannel do
+  describe described_class::Contract, type: :model do
+    subject(:contract) { described_class.new }
+
+    it { is_expected.to validate_presence_of(:channel_id) }
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(params) }
+
+    fab!(:channel_1) { Fabricate(:chat_channel) }
+    fab!(:current_user) { Fabricate(:user) }
+
+    let(:guardian) { Guardian.new(current_user) }
+    let(:channel_id) { channel_1.id }
+
+    before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:everyone] }
+
+    let(:params) { { guardian: guardian, channel_id: channel_id } }
+
+    context "when all steps pass" do
+      context "when category channel" do
+        context "with existing membership" do
+          before do
+            channel_1.add(current_user)
+            Chat::Channel.ensure_consistency!
+          end
+
+          it "unfollows the channel" do
+            membership = channel_1.membership_for(current_user)
+
+            expect { result }.to change { membership.reload.following }.from(true).to(false)
+          end
+
+          it "recomputes user count" do
+            expect { result }.to change { channel_1.reload.user_count }.from(1).to(0)
+          end
+        end
+
+        context "with no existing membership" do
+          it "does nothing" do
+            expect { result }.to_not change { Chat::UserChatChannelMembership }
+          end
+        end
+      end
+
+      context "when group channel" do
+        context "with existing membership" do
+          fab!(:channel_1) do
+            Fabricate(:direct_message_channel, group: true, users: [current_user, Fabricate(:user)])
+          end
+
+          before { Chat::Channel.ensure_consistency! }
+
+          it "leaves the channel" do
+            membership = channel_1.membership_for(current_user)
+
+            result
+
+            expect(Chat::UserChatChannelMembership.exists?(membership.id)).to eq(false)
+            expect(
+              channel_1.chatable.direct_message_users.where(user_id: current_user.id).exists?,
+            ).to eq(false)
+          end
+
+          it "recomputes user count" do
+            expect { result }.to change { channel_1.reload.user_count }.from(2).to(1)
+          end
+        end
+
+        context "with no existing membership" do
+          it "does nothing" do
+            expect { result }.to_not change { Chat::UserChatChannelMembership }
+          end
+        end
+      end
+
+      context "when direct channel" do
+        context "with existing membership" do
+          fab!(:channel_1) do
+            Fabricate(:direct_message_channel, users: [current_user, Fabricate(:user)])
+          end
+
+          before { Chat::Channel.ensure_consistency! }
+
+          it "unfollows the channel" do
+            membership = channel_1.membership_for(current_user)
+
+            expect { result }.to change { membership.reload.following }.from(true).to(false)
+            expect(
+              channel_1.chatable.direct_message_users.where(user_id: current_user.id).exists?,
+            ).to eq(true)
+          end
+
+          it "recomputes user count" do
+            expect { result }.to_not change { channel_1.reload.user_count }
+          end
+        end
+
+        context "with no existing membership" do
+          it "does nothing" do
+            expect { result }.to_not change { Chat::UserChatChannelMembership }
+          end
+        end
+      end
+    end
+
+    context "when channel is not found" do
+      before { params[:channel_id] = -999 }
+
+      it { is_expected.to fail_to_find_a_model(:channel) }
+    end
+  end
+end

plugins/chat/spec/services/chat/unfollow_channel_spec.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::UnfollowChannel do
+  describe described_class::Contract, type: :model do
+    subject(:contract) { described_class.new }
+
+    it { is_expected.to validate_presence_of(:channel_id) }
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(params) }
+
+    fab!(:channel_1) { Fabricate(:chat_channel) }
+    fab!(:current_user) { Fabricate(:user) }
+
+    let(:guardian) { Guardian.new(current_user) }
+    let(:channel_id) { channel_1.id }
+
+    before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:everyone] }
+
+    let(:params) { { guardian: guardian, channel_id: channel_id } }
+
+    context "when all steps pass" do
+      context "with existing membership" do
+        before { channel_1.add(current_user) }
+
+        it "unfollows the channel" do
+          membership = channel_1.membership_for(current_user)
+
+          expect { result }.to change { membership.reload.following }.from(true).to(false)
+        end
+      end
+
+      context "with no existing membership" do
+        it "does nothing" do
+          expect { result }.to_not change { Chat::UserChatChannelMembership }
+        end
+      end
+    end
+
+    context "when channel is not found" do
+      before { params[:channel_id] = -999 }
+
+      it { is_expected.to fail_to_find_a_model(:channel) }
+    end
+  end
+end

plugins/chat/spec/system/channel_settings_page_spec.rb

@@ -154,6 +154,37 @@ RSpec.describe "Channel - Info - Settings page", type: :system do
         expect(toasts).to have_success(I18n.t("js.saved"))
       }.to change { membership.reload.mobile_notification_level }.from("mention").to("never")
     end
+
+    it "can unfollow channel" do
+      membership = channel_1.membership_for(current_user)
+
+      chat_page.visit_channel_settings(channel_1)
+      click_button(I18n.t("js.chat.channel_settings.leave_channel"))
+
+      expect(page).to have_current_path("/chat/browse/open")
+      expect(membership.reload.following).to eq(false)
+    end
+
+    context "when group channel" do
+      fab!(:channel_1) do
+        Fabricate(:direct_message_channel, group: true, users: [current_user, Fabricate(:user)])
+      end
+
+      before { channel_1.add(current_user) }
+
+      it "can leave channel" do
+        membership = channel_1.membership_for(current_user)
+
+        chat_page.visit_channel_settings(channel_1)
+        click_button(I18n.t("js.chat.channel_settings.leave_channel"))
+
+        expect(page).to have_current_path("/chat/browse/open")
+        expect(Chat::UserChatChannelMembership.exists?(membership.id)).to eq(false)
+        expect(
+          channel_1.chatable.direct_message_users.where(user_id: current_user.id).exists?,
+        ).to eq(false)
+      end
+    end
   end
 
   context "as staff" do