UX: Fix chat separator alignment (#20669)

Also: work around 1px svg shift in scroll-to-bottom button

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

@@ -1,37 +0,0 @@
-# frozen_string_literal: true
-
-class Chat::Api::TrackingController < Chat::ApiController
-  def read
-    params.permit(:channel_id, :message_id)
-
-    channel_id = params[:channel_id]
-    message_id = params[:message_id]
-
-    if channel_id.present? && message_id.present?
-      mark_single_message_read(channel_id, message_id)
-    else
-      mark_all_messages_read
-    end
-  end
-
-  private
-
-  def mark_single_message_read(channel_id, message_id)
-    with_service(Chat::UpdateUserLastRead) do
-      on_failed_policy(:ensure_message_id_recency) do
-        raise Discourse::InvalidParameters.new(:message_id)
-      end
-      on_failed_policy(:ensure_message_exists) { raise Discourse::NotFound }
-      on_model_not_found(:active_membership) { raise Discourse::NotFound }
-      on_model_not_found(:channel) { raise Discourse::NotFound }
-    end
-  end
-
-  def mark_all_messages_read
-    with_service(Chat::MarkAllUserChannelsRead) do
-      on_success do
-        render(json: success_json.merge(updated_memberships: result.updated_memberships))
-      end
-    end
-  end
-end

plugins/chat/app/controllers/chat/chat_controller.rb

@@ -160,6 +160,47 @@ module Chat
       render json: success_json
     end
 
+    def update_user_last_read
+      membership =
+        Chat::ChannelMembershipManager.new(@chat_channel).find_for_user(
+          current_user,
+          following: true,
+        )
+      raise Discourse::NotFound if membership.nil?
+
+      if membership.last_read_message_id &&
+           params[:message_id].to_i < membership.last_read_message_id
+        raise Discourse::InvalidParameters.new(:message_id)
+      end
+
+      unless Chat::Message.with_deleted.exists?(
+               chat_channel_id: @chat_channel.id,
+               id: params[:message_id],
+             )
+        raise Discourse::NotFound
+      end
+
+      membership.update!(last_read_message_id: params[:message_id])
+
+      Notification
+        .where(notification_type: Notification.types[:chat_mention])
+        .where(user: current_user)
+        .where(read: false)
+        .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id")
+        .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id")
+        .where("chat_messages.id <= ?", params[:message_id].to_i)
+        .where("chat_messages.chat_channel_id = ?", @chat_channel.id)
+        .update_all(read: true)
+
+      Chat::Publisher.publish_user_tracking_state(
+        current_user,
+        @chat_channel.id,
+        params[:message_id],
+      )
+
+      render json: success_json
+    end
+
     def messages
       page_size = params[:page_size]&.to_i || 1000
       direction = params[:direction].to_s

plugins/chat/app/helpers/chat/with_service_helper.rb

@@ -5,9 +5,6 @@ module Chat
       @_result
     end
 
-    # @param service [Class] A class including {Chat::Service::Base}
-    # @param dependencies [kwargs] Any additional params to load into the service context,
-    #                              in addition to controller @params.
     def with_service(service, default_actions: true, **dependencies, &block)
       object = self
       merged_block =

plugins/chat/app/services/chat/mark_all_user_channels_read.rb

@@ -1,59 +0,0 @@
-# frozen_string_literal: true
-
-module Chat
-  # Service responsible for marking all the channels that a user is a
-  # member of _and following_ as read, including mentions.
-  #
-  # @example
-  #  Chat::MarkAllUserChannelsRead.call(guardian: guardian)
-  #
-  class MarkAllUserChannelsRead
-    include ::Service::Base
-
-    # @!method call(guardian:)
-    #   @param [Guardian] guardian
-    #   @return [Chat::Service::Base::Context]
-
-    transaction do
-      step :update_last_read_message_ids
-      step :mark_associated_mentions_as_read
-    end
-
-    private
-
-    def update_last_read_message_ids(guardian:, **)
-      updated_memberships = DB.query(<<~SQL, user_id: guardian.user.id)
-          UPDATE user_chat_channel_memberships
-          SET last_read_message_id = subquery.newest_message_id
-          FROM
-          (
-            SELECT chat_messages.chat_channel_id, MAX(chat_messages.id) AS newest_message_id
-            FROM chat_messages
-            WHERE chat_messages.deleted_at IS NULL
-            GROUP BY chat_messages.chat_channel_id
-          ) AS subquery
-          WHERE user_chat_channel_memberships.chat_channel_id = subquery.chat_channel_id AND
-            subquery.newest_message_id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) AND
-            user_chat_channel_memberships.user_id = :user_id AND
-            user_chat_channel_memberships.following
-          RETURNING user_chat_channel_memberships.id AS membership_id,
-                    user_chat_channel_memberships.chat_channel_id AS channel_id,
-                    user_chat_channel_memberships.last_read_message_id;
-        SQL
-      context[:updated_memberships] = updated_memberships
-    end
-
-    def mark_associated_mentions_as_read(guardian:, updated_memberships:, **)
-      return if updated_memberships.empty?
-
-      Notification
-        .where(notification_type: ::Notification.types[:chat_mention])
-        .where(user: guardian.user)
-        .where(read: false)
-        .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id")
-        .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id")
-        .where("chat_messages.chat_channel_id IN (?)", updated_memberships.map(&:channel_id))
-        .update_all(read: true)
-    end
-  end
-end

plugins/chat/app/services/chat/update_user_last_read.rb

@@ -4,23 +4,23 @@ module Chat
   # Service responsible for updating the last read message id of a membership.
   #
   # @example
-  #  Chat::UpdateUserLastRead.call(channel_id: 2, message_id: 3, guardian: guardian)
+  #  Chat::UpdateUserLastRead.call(user_id: 1, channel_id: 2, message_id: 3, guardian: guardian)
   #
   class UpdateUserLastRead
-    include ::Service::Base
+    include Service::Base
 
-    # @!method call(channel_id:, message_id:, guardian:)
+    # @!method call(user_id:, channel_id:, message_id:, guardian:)
+    #   @param [Integer] user_id
     #   @param [Integer] channel_id
     #   @param [Integer] message_id
     #   @param [Guardian] guardian
-    #   @return [Chat::Service::Base::Context]
+    #   @return [Service::Base::Context]
 
     contract
-    model :channel
-    model :active_membership
+    model :membership, :fetch_active_membership
     policy :invalid_access
-    policy :ensure_message_exists
     policy :ensure_message_id_recency
+    policy :ensure_message_exists
     step :update_last_read_message_id
     step :mark_associated_mentions_as_read
     step :publish_new_last_read_to_clients
@@ -28,52 +28,52 @@ module Chat
     # @!visibility private
     class Contract
       attribute :message_id, :integer
+      attribute :user_id, :integer
       attribute :channel_id, :integer
 
-      validates :message_id, :channel_id, presence: true
+      validates :message_id, :user_id, :channel_id, presence: true
     end
 
     private
 
-    def fetch_channel(contract:, **)
-      ::Chat::Channel.find_by(id: contract.channel_id)
+    def fetch_active_membership(user_id:, channel_id:, **)
+      Chat::UserChatChannelMembership.includes(:user, :chat_channel).find_by(
+        user_id: user_id,
+        chat_channel_id: channel_id,
+        following: true,
+      )
     end
 
-    def fetch_active_membership(guardian:, channel:, **)
-      ::Chat::ChannelMembershipManager.new(channel).find_for_user(guardian.user, following: true)
+    def invalid_access(guardian:, membership:, **)
+      guardian.can_join_chat_channel?(membership.chat_channel)
     end
 
-    def invalid_access(guardian:, active_membership:, **)
-      guardian.can_join_chat_channel?(active_membership.chat_channel)
+    def ensure_message_id_recency(message_id:, membership:, **)
+      !membership.last_read_message_id || message_id >= membership.last_read_message_id
     end
 
-    def ensure_message_exists(channel:, contract:, **)
-      ::Chat::Message.with_deleted.exists?(chat_channel_id: channel.id, id: contract.message_id)
+    def ensure_message_exists(channel_id:, message_id:, **)
+      Chat::Message.with_deleted.exists?(chat_channel_id: channel_id, id: message_id)
     end
 
-    def ensure_message_id_recency(contract:, active_membership:, **)
-      !active_membership.last_read_message_id ||
-        contract.message_id >= active_membership.last_read_message_id
+    def update_last_read_message_id(message_id:, membership:, **)
+      membership.update!(last_read_message_id: message_id)
     end
 
-    def update_last_read_message_id(contract:, active_membership:, **)
-      active_membership.update!(last_read_message_id: contract.message_id)
-    end
-
-    def mark_associated_mentions_as_read(active_membership:, contract:, **)
-      ::Notification
+    def mark_associated_mentions_as_read(membership:, message_id:, **)
+      Notification
         .where(notification_type: Notification.types[:chat_mention])
-        .where(user: active_membership.user)
+        .where(user: membership.user)
         .where(read: false)
         .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id")
         .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id")
-        .where("chat_messages.id <= ?", contract.message_id)
-        .where("chat_messages.chat_channel_id = ?", active_membership.chat_channel.id)
+        .where("chat_messages.id <= ?", message_id)
+        .where("chat_messages.chat_channel_id = ?", membership.chat_channel.id)
         .update_all(read: true)
     end
 
-    def publish_new_last_read_to_clients(guardian:, channel:, contract:, **)
-      ::Chat::Publisher.publish_user_tracking_state(guardian.user, channel.id, contract.message_id)
+    def publish_new_last_read_to_clients(guardian:, channel_id:, message_id:, **)
+      Chat::Publisher.publish_user_tracking_state(guardian.user, channel_id, message_id)
     end
   end
 end

plugins/chat/assets/javascripts/discourse/initializers/chat-keyboard-shortcuts.js

@@ -17,9 +17,6 @@ export default {
     const router = container.lookup("service:router");
     const appEvents = container.lookup("service:app-events");
     const chatStateManager = container.lookup("service:chat-state-manager");
-    const chatChannelsManager = container.lookup(
-      "service:chat-channels-manager"
-    );
     const openChannelSelector = (e) => {
       e.preventDefault();
       e.stopPropagation();
@@ -95,15 +92,6 @@ export default {
       appEvents.trigger("chat:toggle-close", event);
     };
 
-    const markAllChannelsRead = (event) => {
-      event.preventDefault();
-      event.stopPropagation();
-
-      if (chatStateManager.isActive) {
-        chatChannelsManager.markAllChannelsRead();
-      }
-    };
-
     withPluginApi("0.12.1", (api) => {
       api.addKeyboardShortcut(`${KEY_MODIFIER}+k`, openChannelSelector, {
         global: true,
@@ -213,21 +201,6 @@ export default {
           },
         },
       });
-      api.addKeyboardShortcut(
-        `shift+esc`,
-        (event) => markAllChannelsRead(event),
-        {
-          global: true,
-          help: {
-            category: "chat",
-            name: "chat.keyboard_shortcuts.mark_all_channels_read",
-            definition: {
-              keys1: ["shift", "esc"],
-              keysDelimiter: "plus",
-            },
-          },
-        }
-      );
     });
   },
 };

plugins/chat/assets/javascripts/discourse/models/chat-channel.js

@@ -192,11 +192,8 @@ export default class ChatChannel extends RestModel {
       return;
     }
 
-    // TODO (martin) Change this to use chatApi service once we change this
-    // class not to use RestModel
-    return ajax(`/chat/api/tracking/read/me`, {
+    return ajax(`/chat/${this.id}/read/${messageId}.json`, {
       method: "PUT",
-      data: { message_id: messageId, channel_id: this.id },
     }).then(() => {
       this.currentUserMembership.last_read_message_id = messageId;
     });

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

@@ -282,22 +282,6 @@ export default class ChatApi extends Service {
     );
   }
 
-  /**
-   * Marks messages for a user's chat channel memberships as read. If no
-   * channel ID and no message ID are provided, then all of the user's
-   * followed chat channel memberships will be marked as read.
-   *
-   * @param {number} channelId - The ID of the channel for the message being marked as read.
-   * @param {number} messageId - The ID of the message being marked as read.
-   * @returns {Promise}
-   */
-  updateCurrentUserTracking({ channelId, messageId } = {}) {
-    return this.#putRequest(`/tracking/read/me`, {
-      channel_id: channelId,
-      message_id: messageId,
-    });
-  }
-
   get #basePath() {
     return "/chat/api";
   }

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

@@ -1,5 +1,4 @@
 import Service, { inject as service } from "@ember/service";
-import { debounce } from "discourse-common/utils/decorators";
 import Promise from "rsvp";
 import ChatChannel from "discourse/plugins/chat/discourse/models/chat-channel";
 import { tracked } from "@glimmer/tracking";
@@ -83,19 +82,6 @@ export default class ChatChannelsManager extends Service {
     });
   }
 
-  @debounce(300)
-  async markAllChannelsRead() {
-    return this.chatApi.updateCurrentUserTracking().then((response) => {
-      response.updated_memberships.forEach((membership) => {
-        let channel = this.channels.findBy("id", membership.channel_id);
-        if (channel) {
-          channel.currentUserMembership.unread_count = 0;
-          channel.currentUserMembership.unread_mentions = 0;
-        }
-      });
-    });
-  }
-
   get unreadCount() {
     let count = 0;
     this.publicMessageChannels.forEach((channel) => {

plugins/chat/assets/stylesheets/common/base-common.scss

@@ -384,6 +384,7 @@ $float-height: 530px;
 
       .d-icon {
         color: var(--secondary);
+        margin-left: 1px; // "fixes" the 1px svg shift
       }
     }
 

plugins/chat/assets/stylesheets/common/chat-message-separator.scss

@@ -56,7 +56,9 @@
     }
 
     .chat-message-separator__text-container {
-      padding-top: 7px;
+      align-items: center;
+      display: flex;
+      height: 40px;
       position: sticky;
       top: -1px;
 
@@ -98,11 +100,7 @@
 
       .chat-message-separator__line {
         border-top: 1px solid var(--secondary-high);
-        left: 0;
         margin: 0 0 -1px;
-        position: relative;
-        right: 0;
-        top: -1px;
       }
     }
   }

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

@@ -598,7 +598,6 @@ en:
           composer_code: "%{shortcut} Code (composer only)"
           drawer_open: "%{shortcut} Open chat drawer"
           drawer_close: "%{shortcut} Close chat drawer"
-          mark_all_channels_read: "%{shortcut} Mark all channels read"
     topic_statuses:
       chat:
         help: "Chat is enabled for this topic"

plugins/chat/config/routes.rb

@@ -17,7 +17,6 @@ Chat::Engine.routes.draw do
     post "/channels/:channel_id/memberships/me" => "channels_current_user_membership#create"
     put "/channels/:channel_id/notifications-settings/me" =>
           "channels_current_user_notifications_settings#update"
-    put "/tracking/read/me" => "tracking#read"
 
     # Category chatables controller hints. Only used by staff members, we don't want to leak category permissions.
     get "/category-chatables/:id/permissions" => "category_chatables#permissions",

plugins/chat/lib/service_runner.rb

@@ -19,8 +19,6 @@
 # * +on_model_not_found(name)+: will execute the provided block if the service
 #   fails and its model is not present
 #
-# Default actions for each of these are defined in [Chat::ApiController#default_actions_for_service]
-#
 # @example In a controller
 #   def create
 #     with_service MyService do

plugins/chat/plugin.rb

@@ -381,6 +381,25 @@ after_initialize do
     end
   end
 
+  Discourse::Application.routes.append do
+    mount ::Chat::Engine, at: "/chat"
+
+    get "/admin/plugins/chat" => "chat/admin/incoming_webhooks#index",
+        :constraints => StaffConstraint.new
+    post "/admin/plugins/chat/hooks" => "chat/admin/incoming_webhooks#create",
+         :constraints => StaffConstraint.new
+    put "/admin/plugins/chat/hooks/:incoming_chat_webhook_id" =>
+          "chat/admin/incoming_webhooks#update",
+        :constraints => StaffConstraint.new
+    delete "/admin/plugins/chat/hooks/:incoming_chat_webhook_id" =>
+             "chat/admin/incoming_webhooks#destroy",
+           :constraints => StaffConstraint.new
+    get "u/:username/preferences/chat" => "users#preferences",
+        :constraints => {
+          username: RouteFormat.username,
+        }
+  end
+
   if defined?(DiscourseAutomation)
     add_automation_scriptable("send_chat_message") do
       field :chat_channel_id, component: :text, required: true

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

@@ -1,236 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe Chat::Api::TrackingController do
-  fab!(:current_user) { Fabricate(:user) }
-
-  before do
-    SiteSetting.chat_enabled = true
-    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
-    sign_in(current_user)
-  end
-
-  describe "#read" do
-    describe "marking a single message read" do
-      fab!(:chat_channel) { Fabricate(:chat_channel) }
-      fab!(:other_user) { Fabricate(:user) }
-      fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
-      fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
-
-      it "returns a 404 when the user is not a channel member" do
-        put "/chat/api/tracking/read/me.json",
-            params: {
-              channel_id: chat_channel.id,
-              message_id: message_1.id,
-            }
-
-        expect(response.status).to eq(404)
-      end
-
-      it "returns a 404 when the user is not following the channel" do
-        Fabricate(
-          :user_chat_channel_membership,
-          chat_channel: chat_channel,
-          user: current_user,
-          following: false,
-        )
-
-        put "/chat/api/tracking/read/me.json",
-            params: {
-              channel_id: chat_channel.id,
-              message_id: message_1.id,
-            }
-
-        expect(response.status).to eq(404)
-      end
-
-      describe "when the user is a channel member" do
-        fab!(:membership) do
-          Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: current_user)
-        end
-
-        context "when message_id param doesn't link to a message of the channel" do
-          it "raises a not found" do
-            put "/chat/api/tracking/read/me.json",
-                params: {
-                  channel_id: chat_channel.id,
-                  message_id: -999,
-                }
-
-            expect(response.status).to eq(404)
-          end
-        end
-
-        context "when message_id param is inferior to existing last read" do
-          before { membership.update!(last_read_message_id: message_2.id) }
-
-          it "raises an invalid request" do
-            put "/chat/api/tracking/read/me.json",
-                params: {
-                  channel_id: chat_channel.id,
-                  message_id: message_1.id,
-                }
-
-            expect(response.status).to eq(400)
-            expect(response.parsed_body["errors"][0]).to match(/message_id/)
-          end
-        end
-
-        context "when message_id refers to deleted message" do
-          before { message_1.trash!(Discourse.system_user) }
-
-          it "works" do
-            put "/chat/api/tracking/read/me",
-                params: {
-                  channel_id: chat_channel.id,
-                  message_id: message_1.id,
-                }
-
-            expect(response.status).to eq(200)
-          end
-        end
-
-        it "updates timing records" do
-          expect {
-            put "/chat/api/tracking/read/me.json",
-                params: {
-                  channel_id: chat_channel.id,
-                  message_id: message_1.id,
-                }
-          }.not_to change { Chat::UserChatChannelMembership.count }
-
-          membership.reload
-          expect(membership.chat_channel_id).to eq(chat_channel.id)
-          expect(membership.last_read_message_id).to eq(message_1.id)
-          expect(membership.user_id).to eq(current_user.id)
-        end
-
-        it "marks all mention notifications as read for the channel" do
-          notification = create_notification_and_mention_for(current_user, other_user, message_1)
-
-          put "/chat/api/tracking/read/me.json",
-              params: {
-                channel_id: chat_channel.id,
-                message_id: message_2.id,
-              }
-          expect(response.status).to eq(200)
-          expect(notification.reload.read).to eq(true)
-        end
-
-        it "doesn't mark notifications of messages that weren't read yet" do
-          message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user)
-          notification = create_notification_and_mention_for(current_user, other_user, message_3)
-
-          put "/chat/api/tracking/read/me.json",
-              params: {
-                channel_id: chat_channel.id,
-                message_id: message_2.id,
-              }
-          expect(response.status).to eq(200)
-          expect(notification.reload.read).to eq(false)
-        end
-      end
-    end
-
-    describe "marking all messages read" do
-      fab!(:chat_channel_1) { Fabricate(:chat_channel) }
-      fab!(:chat_channel_2) { Fabricate(:chat_channel) }
-      fab!(:chat_channel_3) { Fabricate(:chat_channel) }
-
-      fab!(:other_user) { Fabricate(:user) }
-
-      fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, user: other_user) }
-      fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel_1, user: other_user) }
-      fab!(:message_3) { Fabricate(:chat_message, chat_channel: chat_channel_2, user: other_user) }
-      fab!(:message_4) { Fabricate(:chat_message, chat_channel: chat_channel_2, user: other_user) }
-      fab!(:message_5) { Fabricate(:chat_message, chat_channel: chat_channel_3, user: other_user) }
-      fab!(:message_6) { Fabricate(:chat_message, chat_channel: chat_channel_3, user: other_user) }
-
-      fab!(:membership_1) do
-        Fabricate(
-          :user_chat_channel_membership,
-          chat_channel: chat_channel_1,
-          user: current_user,
-          following: true,
-        )
-      end
-      fab!(:membership_2) do
-        Fabricate(
-          :user_chat_channel_membership,
-          chat_channel: chat_channel_2,
-          user: current_user,
-          following: true,
-        )
-      end
-      fab!(:membership_3) do
-        Fabricate(
-          :user_chat_channel_membership,
-          chat_channel: chat_channel_3,
-          user: current_user,
-          following: true,
-        )
-      end
-
-      it "marks all messages as read across the user's channel memberships with the correct last_read_message_id" do
-        put "/chat/api/tracking/read/me.json"
-
-        expect(membership_1.reload.last_read_message_id).to eq(message_2.id)
-        expect(membership_2.reload.last_read_message_id).to eq(message_4.id)
-        expect(membership_3.reload.last_read_message_id).to eq(message_6.id)
-      end
-
-      it "doesn't mark messages for channels the user is not following as read" do
-        membership_1.update!(following: false)
-
-        put "/chat/api/tracking/read/me.json"
-
-        expect(membership_1.reload.last_read_message_id).to eq(nil)
-        expect(membership_2.reload.last_read_message_id).to eq(message_4.id)
-        expect(membership_3.reload.last_read_message_id).to eq(message_6.id)
-      end
-
-      it "returns the updated memberships, channels, and last message id" do
-        put "/chat/api/tracking/read/me.json"
-        expect(response.parsed_body["updated_memberships"]).to match_array(
-          [
-            {
-              "channel_id" => chat_channel_1.id,
-              "last_read_message_id" => message_2.id,
-              "membership_id" => membership_1.id,
-            },
-            {
-              "channel_id" => chat_channel_2.id,
-              "last_read_message_id" => message_4.id,
-              "membership_id" => membership_2.id,
-            },
-            {
-              "channel_id" => chat_channel_3.id,
-              "last_read_message_id" => message_6.id,
-              "membership_id" => membership_3.id,
-            },
-          ],
-        )
-      end
-    end
-  end
-
-  def create_notification_and_mention_for(user, sender, msg)
-    Notification
-      .create!(
-        notification_type: Notification.types[:chat_mention],
-        user: user,
-        high_priority: true,
-        read: false,
-        data: {
-          message: "chat.mention_notification",
-          chat_message_id: msg.id,
-          chat_channel_id: msg.chat_channel_id,
-          chat_channel_title: msg.chat_channel.title(user),
-          chat_channel_slug: msg.chat_channel.slug,
-          mentioned_by_username: sender.username,
-        }.to_json,
-      )
-      .tap do |notification|
-        Chat::Mention.create!(user: user, chat_message: msg, notification: notification)
-      end
-  end
-end

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -725,6 +725,117 @@ RSpec.describe Chat::ChatController do
     end
   end
 
+  describe "#update_user_last_read" do
+    before { sign_in(user) }
+
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
+
+    it "returns a 404 when the user is not a channel member" do
+      put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    it "returns a 404 when the user is not following the channel" do
+      Fabricate(
+        :user_chat_channel_membership,
+        chat_channel: chat_channel,
+        user: user,
+        following: false,
+      )
+
+      put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    describe "when the user is a channel member" do
+      fab!(:membership) do
+        Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: user)
+      end
+
+      context "when message_id param doesn't link to a message of the channel" do
+        it "raises a not found" do
+          put "/chat/#{chat_channel.id}/read/-999.json"
+
+          expect(response.status).to eq(404)
+        end
+      end
+
+      context "when message_id param is inferior to existing last read" do
+        before { membership.update!(last_read_message_id: message_2.id) }
+
+        it "raises an invalid request" do
+          put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
+
+          expect(response.status).to eq(400)
+          expect(response.parsed_body["errors"][0]).to match(/message_id/)
+        end
+      end
+
+      context "when message_id refers to deleted message" do
+        before { message_1.trash!(Discourse.system_user) }
+
+        it "works" do
+          put "/chat/#{chat_channel.id}/read/#{message_1.id}.json"
+
+          expect(response.status).to eq(200)
+        end
+      end
+
+      it "updates timing records" do
+        expect { put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" }.not_to change {
+          Chat::UserChatChannelMembership.count
+        }
+
+        membership.reload
+        expect(membership.chat_channel_id).to eq(chat_channel.id)
+        expect(membership.last_read_message_id).to eq(message_1.id)
+        expect(membership.user_id).to eq(user.id)
+      end
+
+      def create_notification_and_mention_for(user, sender, msg)
+        Notification
+          .create!(
+            notification_type: Notification.types[:chat_mention],
+            user: user,
+            high_priority: true,
+            read: false,
+            data: {
+              message: "chat.mention_notification",
+              chat_message_id: msg.id,
+              chat_channel_id: msg.chat_channel_id,
+              chat_channel_title: msg.chat_channel.title(user),
+              chat_channel_slug: msg.chat_channel.slug,
+              mentioned_by_username: sender.username,
+            }.to_json,
+          )
+          .tap do |notification|
+            Chat::Mention.create!(user: user, chat_message: msg, notification: notification)
+          end
+      end
+
+      it "marks all mention notifications as read for the channel" do
+        notification = create_notification_and_mention_for(user, other_user, message_1)
+
+        put "/chat/#{chat_channel.id}/read/#{message_2.id}.json"
+        expect(response.status).to eq(200)
+        expect(notification.reload.read).to eq(true)
+      end
+
+      it "doesn't mark notifications of messages that weren't read yet" do
+        message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user)
+        notification = create_notification_and_mention_for(user, other_user, message_3)
+
+        put "/chat/#{chat_channel.id}/read/#{message_2.id}.json"
+
+        expect(response.status).to eq(200)
+        expect(notification.reload.read).to eq(false)
+      end
+    end
+  end
+
   describe "react" do
     fab!(:chat_channel) { Fabricate(:category_channel) }
     fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user) }

plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb

@@ -1,133 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe Chat::MarkAllUserChannelsRead do
-  describe ".call" do
-    subject(:result) { described_class.call(params) }
-
-    let(:params) { { guardian: guardian } }
-    let(:guardian) { Guardian.new(current_user) }
-
-    fab!(:current_user) { Fabricate(:user) }
-
-    fab!(:channel_1) { Fabricate(:chat_channel) }
-    fab!(:channel_2) { Fabricate(:chat_channel) }
-    fab!(:channel_3) { Fabricate(:chat_channel) }
-
-    fab!(:other_user) { Fabricate(:user) }
-
-    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: other_user) }
-    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: other_user) }
-    fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel_2, user: other_user) }
-    fab!(:message_4) { Fabricate(:chat_message, chat_channel: channel_2, user: other_user) }
-    fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_3, user: other_user) }
-    fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_3, user: other_user) }
-
-    fab!(:membership_1) do
-      Fabricate(
-        :user_chat_channel_membership,
-        chat_channel: channel_1,
-        user: current_user,
-        following: true,
-      )
-    end
-    fab!(:membership_2) do
-      Fabricate(
-        :user_chat_channel_membership,
-        chat_channel: channel_2,
-        user: current_user,
-        following: true,
-      )
-    end
-    fab!(:membership_3) do
-      Fabricate(
-        :user_chat_channel_membership,
-        chat_channel: channel_3,
-        user: current_user,
-        following: true,
-      )
-    end
-
-    context "when the user has no memberships" do
-      let(:guardian) { Guardian.new(Fabricate(:user)) }
-
-      it "sets the service result as successful" do
-        expect(result).to be_a_success
-      end
-
-      it "returns the updated_memberships in context" do
-        expect(result.updated_memberships).to eq([])
-      end
-    end
-
-    context "when everything is fine" do
-      fab!(:notification_1) do
-        Fabricate(
-          :notification,
-          notification_type: Notification.types[:chat_mention],
-          user: current_user,
-        )
-      end
-      fab!(:notification_2) do
-        Fabricate(
-          :notification,
-          notification_type: Notification.types[:chat_mention],
-          user: current_user,
-        )
-      end
-
-      let(:messages) { MessageBus.track_publish { result } }
-
-      before do
-        Chat::Mention.create!(
-          notification: notification_1,
-          user: current_user,
-          chat_message: message_1,
-        )
-        Chat::Mention.create!(
-          notification: notification_2,
-          user: current_user,
-          chat_message: message_3,
-        )
-      end
-
-      it "sets the service result as successful" do
-        expect(result).to be_a_success
-      end
-
-      it "updates the last_read_message_ids" do
-        result
-        expect(membership_1.reload.last_read_message_id).to eq(message_2.id)
-        expect(membership_2.reload.last_read_message_id).to eq(message_4.id)
-        expect(membership_3.reload.last_read_message_id).to eq(message_6.id)
-      end
-
-      it "does not update memberships where the user is not following" do
-        membership_1.update!(following: false)
-        result
-        expect(membership_1.reload.last_read_message_id).to eq(nil)
-      end
-
-      it "does not use deleted messages for the last_read_message_id" do
-        message_2.trash!
-        result
-        expect(membership_1.reload.last_read_message_id).to eq(message_1.id)
-      end
-
-      it "returns the updated_memberships in context" do
-        expect(result.updated_memberships.map(&:channel_id)).to match_array(
-          [channel_1.id, channel_2.id, channel_3.id],
-        )
-      end
-
-      it "marks existing notifications for all affected channels as read" do
-        expect { result }.to change {
-          Notification.where(
-            notification_type: Notification.types[:chat_mention],
-            user: current_user,
-            read: false,
-          ).count
-        }.by(-2)
-      end
-    end
-  end
-end

plugins/chat/spec/services/chat/update_user_last_read_spec.rb

@@ -18,10 +18,17 @@ RSpec.describe Chat::UpdateUserLastRead do
     fab!(:message_1) { Fabricate(:chat_message, chat_channel: membership.chat_channel) }
 
     let(:guardian) { Guardian.new(current_user) }
-    let(:params) { { guardian: guardian, channel_id: channel.id, message_id: message_1.id } }
+    let(:params) do
+      {
+        guardian: guardian,
+        user_id: current_user.id,
+        channel_id: channel.id,
+        message_id: message_1.id,
+      }
+    end
 
     context "when params are not valid" do
-      before { params.delete(:message_id) }
+      before { params.delete(:user_id) }
 
       it { is_expected.to fail_a_contract }
     end
@@ -30,7 +37,7 @@ RSpec.describe Chat::UpdateUserLastRead do
       context "when user has no membership" do
         before { membership.destroy! }
 
-        it { is_expected.to fail_to_find_a_model(:active_membership) }
+        it { is_expected.to fail_to_find_a_model(:membership) }
       end
 
       context "when user can’t access the channel" do
@@ -49,10 +56,8 @@ RSpec.describe Chat::UpdateUserLastRead do
 
       context "when message_id is older than membership's last_read_message_id" do
         before do
-          message_old = Fabricate(:chat_message, chat_channel: channel)
-          message_new = Fabricate(:chat_message, chat_channel: channel)
-          params[:message_id] = message_old.id
-          membership.update!(last_read_message_id: message_new.id)
+          params[:message_id] = -2
+          membership.update!(last_read_message_id: -1)
         end
 
         it { is_expected.to fail_a_policy(:ensure_message_id_recency) }

plugins/chat/spec/system/shortcuts/mark_all_read_spec.rb

@@ -1,43 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe "Shortcuts | mark all read", type: :system, js: true do
-  fab!(:user_1) { Fabricate(:admin) }
-  fab!(:channel_1) { Fabricate(:chat_channel) }
-  fab!(:channel_2) { Fabricate(:chat_channel) }
-  fab!(:channel_3) { Fabricate(:chat_channel) }
-
-  let(:chat_page) { PageObjects::Pages::Chat.new }
-  let(:drawer) { PageObjects::Pages::ChatDrawer.new }
-
-  before do
-    SiteSetting.navigation_menu = "sidebar"
-    chat_system_bootstrap(user_1, [channel_1, channel_2, channel_3])
-    sign_in(user_1)
-    Fabricate(:chat_message, chat_channel: channel_1)
-    Fabricate(:chat_message, chat_channel: channel_1)
-    Fabricate(:chat_message, chat_channel: channel_2)
-    Fabricate(:chat_message, chat_channel: channel_2)
-  end
-
-  context "when chat is open" do
-    before { visit(channel_3.url) }
-
-    context "when pressing shift+esc" do
-      it "marks all channels read" do
-        expect(page).to have_css(
-          ".sidebar-section-link.channel-#{channel_1.id} .sidebar-section-link-suffix.unread",
-        )
-        expect(page).to have_css(
-          ".sidebar-section-link.channel-#{channel_2.id} .sidebar-section-link-suffix.unread",
-        )
-        find("body").send_keys(%i[shift escape])
-        expect(page).not_to have_css(
-          ".sidebar-section-link.channel-#{channel_1.id} .sidebar-section-link-suffix.unread",
-        )
-        expect(page).not_to have_css(
-          ".sidebar-section-link.channel-#{channel_2.id} .sidebar-section-link-suffix.unread",
-        )
-      end
-    end
-  end
-end