From 3fae800936c7f39371f229e09cbfae8e5c209a05 Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Wed, 20 May 2026 14:26:21 +0530 Subject: [PATCH 1/8] feat: base layer for unread counts (store, counter and builder) (1/3)[CW-6851] (#14368) ## Description This is the first PR in a series of PRs for Introducing unread counts in the sidebar for inboxes and labels. In this PR: * Added the unread store, counter and builder modules * Added redis keys for unread count management * Added specs for all 3 modules, some specs are for testing enterprise only feature like specific roles and permissions which are added in the respective enterprise folder itself. **Note** None of this changes affect anything else and nothing is wired to existing modules. Issue: https://linear.app/chatwoot/issue/CW-6851/support-unread-conversation-counts ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality not to work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Sojan Jose --- app/services/conversations/unread_counts.rb | 4 + .../conversations/unread_counts/builder.rb | 75 ++++++ .../conversations/unread_counts/counter.rb | 200 ++++++++++++++++ .../conversations/unread_counts/store.rb | 220 ++++++++++++++++++ lib/redis/alfred.rb | 9 + lib/redis/lock_manager.rb | 12 + lib/redis/redis_keys.rb | 22 ++ .../unread_counts/counter_spec.rb | 72 ++++++ spec/lib/redis/lock_manager_spec.rb | 22 ++ spec/rails_helper.rb | 1 + .../unread_counts/builder_spec.rb | 93 ++++++++ .../unread_counts/counter_spec.rb | 95 ++++++++ .../conversations/unread_counts/store_spec.rb | 183 +++++++++++++++ .../conversations_unread_counts_helpers.rb | 11 + 14 files changed, 1019 insertions(+) create mode 100644 app/services/conversations/unread_counts.rb create mode 100644 app/services/conversations/unread_counts/builder.rb create mode 100644 app/services/conversations/unread_counts/counter.rb create mode 100644 app/services/conversations/unread_counts/store.rb create mode 100644 spec/enterprise/services/conversations/unread_counts/counter_spec.rb create mode 100644 spec/services/conversations/unread_counts/builder_spec.rb create mode 100644 spec/services/conversations/unread_counts/counter_spec.rb create mode 100644 spec/services/conversations/unread_counts/store_spec.rb create mode 100644 spec/support/conversations_unread_counts_helpers.rb diff --git a/app/services/conversations/unread_counts.rb b/app/services/conversations/unread_counts.rb new file mode 100644 index 000000000..668395063 --- /dev/null +++ b/app/services/conversations/unread_counts.rb @@ -0,0 +1,4 @@ +module Conversations::UnreadCounts + READY_TTL = 24.hours.to_i + SET_TTL = 25.hours.to_i +end diff --git a/app/services/conversations/unread_counts/builder.rb b/app/services/conversations/unread_counts/builder.rb new file mode 100644 index 000000000..54e466dbd --- /dev/null +++ b/app/services/conversations/unread_counts/builder.rb @@ -0,0 +1,75 @@ +class Conversations::UnreadCounts::Builder + BATCH_SIZE = 1000 + + attr_reader :account + + def initialize(account) + @account = account + end + + def build_base! + store.clear_account!(account.id) + write_memberships(assignment: false) + store.mark_base_ready!(account.id) + end + + def build_assignment! + store.clear_assignment!(account.id) + write_memberships(assignment: true) + store.mark_assignment_ready!(account.id) + end + + def build_all! + build_base! + build_assignment! + end + + private + + def write_memberships(assignment:) + unread_conversations.in_batches(of: BATCH_SIZE) do |relation| + columns = %i[id inbox_id assignee_id cached_label_list team_id] + memberships = relation.pluck(*columns).map do |id, inbox_id, assignee_id, cached_label_list, team_id| + { + conversation_id: id, + inbox_id: inbox_id, + assignee_id: assignee_id, + team_id: team_id, + label_ids: label_ids_for(cached_label_list) + } + end + + store.add_memberships(account_id: account.id, memberships: memberships, assignment: assignment) + end + end + + def unread_conversations + account.conversations + .open + .joins(:messages) + .merge(Message.incoming.reorder(nil)) + .where(messages: { account_id: account.id }) + .where(unread_since_last_seen_condition) + .distinct + end + + def unread_since_last_seen_condition + conversations = Conversation.arel_table + messages = Message.arel_table + + conversations[:agent_last_seen_at].eq(nil).or(messages[:created_at].gt(conversations[:agent_last_seen_at])) + end + + def label_ids_for(cached_label_list) + label_titles = cached_label_list.to_s.split(',').map(&:strip).compact_blank + labels_by_title.values_at(*label_titles).compact + end + + def labels_by_title + @labels_by_title ||= account.labels.pluck(:title, :id).to_h + end + + def store + ::Conversations::UnreadCounts::Store + end +end diff --git a/app/services/conversations/unread_counts/counter.rb b/app/services/conversations/unread_counts/counter.rb new file mode 100644 index 000000000..f4126b61f --- /dev/null +++ b/app/services/conversations/unread_counts/counter.rb @@ -0,0 +1,200 @@ +class Conversations::UnreadCounts::Counter + MANAGE_ALL_PERMISSION = 'conversation_manage'.freeze + UNASSIGNED_PERMISSION = 'conversation_unassigned_manage'.freeze + PARTICIPATING_PERMISSION = 'conversation_participating_manage'.freeze + BUILD_LOCK_TTL = 15.minutes.to_i + BUILD_WAIT_TIMEOUT = 30.seconds.to_i + BUILD_WAIT_INTERVAL = 0.1.seconds + + attr_reader :account, :user + + def initialize(account:, user:) + @account = account + @user = user + end + + def perform + return empty_counts if permission_mode == :none + + ensure_base_cache! + ensure_assignment_cache! if assignment_mode? + + { + inboxes: unread_inbox_counts, + labels: unread_label_counts, + teams: unread_team_counts + } + end + + private + + def ensure_base_cache! + ensure_cache_ready!( + ready: -> { store.base_ready?(account.id) }, + lock_key: base_build_lock_key + ) { ::Conversations::UnreadCounts::Builder.new(account).build_base! } + end + + def ensure_assignment_cache! + ensure_cache_ready!( + ready: -> { store.assignment_ready?(account.id) }, + lock_key: assignment_build_lock_key + ) { ::Conversations::UnreadCounts::Builder.new(account).build_assignment! } + end + + def ensure_cache_ready!(ready:, lock_key:) + lock_manager = Redis::LockManager.new + + loop do + return if ready.call + + return if lock_manager.with_lock(lock_key, BUILD_LOCK_TTL) { yield unless ready.call } + + wait_for_cache_ready(ready) + end + end + + def wait_for_cache_ready(ready) + deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + BUILD_WAIT_TIMEOUT + sleep BUILD_WAIT_INTERVAL until ready.call || Process.clock_gettime(Process::CLOCK_MONOTONIC) >= deadline + end + + def base_build_lock_key + format(Redis::Alfred::UNREAD_CONVERSATIONS_BASE_BUILD_LOCK, account_id: account.id) + end + + def assignment_build_lock_key + format(Redis::Alfred::UNREAD_CONVERSATIONS_ASSIGNMENT_BUILD_LOCK, account_id: account.id) + end + + def unread_inbox_counts + counts_for_grouped_keys(visible_inbox_ids.index_with { |inbox_id| inbox_keys_for_mode(inbox_id) }) + end + + def unread_label_counts + keys_by_id = Hash.new { |hash, key| hash[key] = [] } + sidebar_label_ids.each do |label_id| + visible_inbox_ids.each do |inbox_id| + keys_by_id[label_id].concat(label_inbox_keys_for_mode(label_id, inbox_id)) + end + end + + counts_for_grouped_keys(keys_by_id) + end + + def unread_team_counts + keys_by_id = Hash.new { |hash, key| hash[key] = [] } + visible_team_ids.each do |team_id| + visible_inbox_ids.each do |inbox_id| + keys_by_id[team_id].concat(team_inbox_keys_for_mode(team_id, inbox_id)) + end + end + + counts_for_grouped_keys(keys_by_id) + end + + def inbox_keys_for_mode(inbox_id) + case permission_mode + when :base + [store.inbox_key(account.id, inbox_id)] + when :unassigned_and_mine + [store.inbox_unassigned_key(account.id, inbox_id), store.inbox_assignee_key(account.id, inbox_id, user.id)] + when :mine + [store.inbox_assignee_key(account.id, inbox_id, user.id)] + end + end + + def label_inbox_keys_for_mode(label_id, inbox_id) + case permission_mode + when :base + [store.label_inbox_key(account.id, label_id, inbox_id)] + when :unassigned_and_mine + [ + store.label_inbox_unassigned_key(account.id, label_id, inbox_id), + store.label_inbox_assignee_key(account.id, label_id, inbox_id, user.id) + ] + when :mine + [store.label_inbox_assignee_key(account.id, label_id, inbox_id, user.id)] + end + end + + def team_inbox_keys_for_mode(team_id, inbox_id) + case permission_mode + when :base + [store.team_inbox_key(account.id, team_id, inbox_id)] + when :unassigned_and_mine + [ + store.team_inbox_unassigned_key(account.id, team_id, inbox_id), + store.team_inbox_assignee_key(account.id, team_id, inbox_id, user.id) + ] + when :mine + [store.team_inbox_assignee_key(account.id, team_id, inbox_id, user.id)] + end + end + + def counts_for_grouped_keys(keys_by_id) + counts_by_key = store.counts_for_keys(keys_by_id.values.flatten) + + keys_by_id.each_with_object({}) do |(id, keys), result| + count = keys.sum { |key| counts_by_key[key].to_i } + result[id.to_s] = count if count.positive? + end + end + + def assignment_mode? + %i[unassigned_and_mine mine].include?(permission_mode) + end + + def permission_mode + @permission_mode ||= + if !custom_role_agent? || permissions.include?(MANAGE_ALL_PERMISSION) + :base + elsif permissions.include?(UNASSIGNED_PERMISSION) + :unassigned_and_mine + elsif permissions.include?(PARTICIPATING_PERMISSION) + :mine + else + :none + end + end + + def custom_role_agent? + account_user&.agent? && account_user.custom_role_id.present? + end + + def permissions + account_user&.permissions || [] + end + + def account_user + @account_user ||= account.account_users.find_by(user_id: user.id) + end + + def visible_inbox_ids + @visible_inbox_ids ||= if account_user&.administrator? + account.inboxes.pluck(:id) + else + user.inboxes.where(account_id: account.id).pluck(:id) + end + end + + def sidebar_label_ids + @sidebar_label_ids ||= account.labels.where(show_on_sidebar: true).pluck(:id) + end + + def visible_team_ids + @visible_team_ids ||= if account_user&.administrator? + account.teams.pluck(:id) + else + user.teams.where(account_id: account.id).pluck(:id) + end + end + + def empty_counts + { inboxes: {}, labels: {}, teams: {} } + end + + def store + ::Conversations::UnreadCounts::Store + end +end diff --git a/app/services/conversations/unread_counts/store.rb b/app/services/conversations/unread_counts/store.rb new file mode 100644 index 000000000..dbd01f235 --- /dev/null +++ b/app/services/conversations/unread_counts/store.rb @@ -0,0 +1,220 @@ +class Conversations::UnreadCounts::Store + class << self + def base_ready?(account_id) + Redis::Alfred.exists?(base_ready_key(account_id)) + end + + def assignment_ready?(account_id) + Redis::Alfred.exists?(assignment_ready_key(account_id)) + end + + def mark_base_ready!(account_id) + Redis::Alfred.set(base_ready_key(account_id), Time.current.to_i, ex: Conversations::UnreadCounts::READY_TTL) + end + + def mark_assignment_ready!(account_id) + Redis::Alfred.set(assignment_ready_key(account_id), Time.current.to_i, ex: Conversations::UnreadCounts::READY_TTL) + end + + def clear_account!(account_id) + delete_matching("#{account_prefix(account_id)}::*") + end + + def clear_assignment!(account_id) + assignment_key_patterns(account_id).each { |pattern| delete_matching(pattern) } + end + + def add_base_membership(account_id:, inbox_id:, label_ids:, conversation_id:, team_id: nil) + add_to_sets(base_keys(account_id, inbox_id, label_ids, team_id), conversation_id) + end + + def remove_base_membership(account_id:, inbox_ids:, label_ids:, conversation_id:, team_ids: []) + keys = Array(inbox_ids).flat_map { |inbox_id| removable_base_keys(account_id, inbox_id, label_ids, team_ids) } + remove_from_sets(keys, conversation_id) + end + + def add_assignment_membership(account_id:, conversation_id:, **membership) + add_to_sets( + assignment_keys(account_id, membership[:inbox_id], membership[:label_ids], membership[:assignee_id], membership[:team_id]), + conversation_id + ) + end + + def remove_assignment_membership(account_id:, conversation_id:, **membership) + keys = Array(membership[:inbox_ids]).flat_map do |inbox_id| + Array(membership[:assignee_ids]).flat_map do |assignee_id| + removable_assignment_keys(account_id, inbox_id, membership[:label_ids], assignee_id, membership[:team_ids]) + end + end + remove_from_sets(keys, conversation_id) + end + + def add_memberships(account_id:, memberships:, assignment: false) + return if memberships.blank? + + Redis::Alfred.pipelined do |pipeline| + memberships.each do |membership| + keys = if assignment + assignment_keys(account_id, membership[:inbox_id], membership[:label_ids], membership[:assignee_id], membership[:team_id]) + else + base_keys(account_id, membership[:inbox_id], membership[:label_ids], membership[:team_id]) + end + + keys.each do |key| + pipeline.sadd(key, membership[:conversation_id]) + pipeline.expire(key, Conversations::UnreadCounts::SET_TTL) + end + end + end + end + + def counts_for_keys(keys) + keys = keys.compact_blank + return {} if keys.blank? + + counts = Redis::Alfred.pipelined do |pipeline| + keys.each { |key| pipeline.scard(key) } + end + keys.zip(counts).to_h + end + + def inbox_key(account_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX, account_id: account_id, inbox_id: inbox_id) + end + + def label_inbox_key(account_id, label_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX, account_id: account_id, label_id: label_id, inbox_id: inbox_id) + end + + def team_inbox_key(account_id, team_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX, account_id: account_id, team_id: team_id, inbox_id: inbox_id) + end + + def inbox_unassigned_key(account_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX_UNASSIGNED, account_id: account_id, inbox_id: inbox_id) + end + + def inbox_assignee_key(account_id, inbox_id, user_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX_ASSIGNEE, account_id: account_id, inbox_id: inbox_id, user_id: user_id) + end + + def label_inbox_unassigned_key(account_id, label_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX_UNASSIGNED, account_id: account_id, label_id: label_id, inbox_id: inbox_id) + end + + def label_inbox_assignee_key(account_id, label_id, inbox_id, user_id) + format( + Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX_ASSIGNEE, + account_id: account_id, + label_id: label_id, + inbox_id: inbox_id, + user_id: user_id + ) + end + + def team_inbox_unassigned_key(account_id, team_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX_UNASSIGNED, account_id: account_id, team_id: team_id, inbox_id: inbox_id) + end + + def team_inbox_assignee_key(account_id, team_id, inbox_id, user_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX_ASSIGNEE, account_id: account_id, team_id: team_id, inbox_id: inbox_id, user_id: user_id) + end + + private + + def base_ready_key(account_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_BASE_READY, account_id: account_id) + end + + def assignment_ready_key(account_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_ASSIGNMENT_READY, account_id: account_id) + end + + def account_prefix(account_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_ACCOUNT_PREFIX, account_id: account_id) + end + + def base_keys(account_id, inbox_id, label_ids, team_id = nil) + keys = [inbox_key(account_id, inbox_id)] + Array(label_ids).map { |label_id| label_inbox_key(account_id, label_id, inbox_id) } + keys << team_inbox_key(account_id, team_id, inbox_id) if team_id.present? + keys + end + + def removable_base_keys(account_id, inbox_id, label_ids, team_ids) + keys = base_keys(account_id, inbox_id, label_ids) + keys.concat(Array(team_ids).compact_blank.map { |team_id| team_inbox_key(account_id, team_id, inbox_id) }) + end + + def assignment_keys(account_id, inbox_id, label_ids, assignee_id, team_id = nil) + keys = assignment_keys_without_team(account_id, inbox_id, label_ids, assignee_id) + keys << team_assignment_key(account_id, team_id, inbox_id, assignee_id) if team_id.present? + keys + end + + def removable_assignment_keys(account_id, inbox_id, label_ids, assignee_id, team_ids) + keys = assignment_keys_without_team(account_id, inbox_id, label_ids, assignee_id) + keys.concat(Array(team_ids).compact_blank.map { |team_id| team_assignment_key(account_id, team_id, inbox_id, assignee_id) }) + end + + def assignment_keys_without_team(account_id, inbox_id, label_ids, assignee_id) + return assignee_keys(account_id, inbox_id, label_ids, assignee_id) if assignee_id.present? + + unassigned_keys(account_id, inbox_id, label_ids) + end + + def assignee_keys(account_id, inbox_id, label_ids, assignee_id) + [inbox_assignee_key(account_id, inbox_id, assignee_id)] + + Array(label_ids).map { |label_id| label_inbox_assignee_key(account_id, label_id, inbox_id, assignee_id) } + end + + def unassigned_keys(account_id, inbox_id, label_ids) + [inbox_unassigned_key(account_id, inbox_id)] + + Array(label_ids).map { |label_id| label_inbox_unassigned_key(account_id, label_id, inbox_id) } + end + + def team_assignment_key(account_id, team_id, inbox_id, assignee_id) + return team_inbox_assignee_key(account_id, team_id, inbox_id, assignee_id) if assignee_id.present? + + team_inbox_unassigned_key(account_id, team_id, inbox_id) + end + + def add_to_sets(keys, conversation_id) + write_to_sets(keys) { |pipeline, key| pipeline.sadd(key, conversation_id) } + end + + def remove_from_sets(keys, conversation_id) + write_to_sets(keys) { |pipeline, key| pipeline.srem(key, conversation_id) } + end + + def write_to_sets(keys) + keys = keys.compact_blank + return if keys.blank? + + Redis::Alfred.pipelined do |pipeline| + keys.each do |key| + yield(pipeline, key) + pipeline.expire(key, Conversations::UnreadCounts::SET_TTL) + end + end + end + + def delete_matching(pattern) + Redis::Alfred.scan_each(match: pattern, count: 1000) do |key| + Redis::Alfred.delete(key) + end + end + + def assignment_key_patterns(account_id) + prefix = account_prefix(account_id) + [ + assignment_ready_key(account_id), + "#{prefix}::INBOX::*::UNASSIGNED", + "#{prefix}::INBOX::*::ASSIGNEE::*", + "#{prefix}::LABEL::*::INBOX::*::UNASSIGNED", + "#{prefix}::LABEL::*::INBOX::*::ASSIGNEE::*", + "#{prefix}::TEAM::*::INBOX::*::UNASSIGNED", + "#{prefix}::TEAM::*::INBOX::*::ASSIGNEE::*" + ] + end + end +end diff --git a/lib/redis/alfred.rb b/lib/redis/alfred.rb index e7e04ed60..1554b8806 100644 --- a/lib/redis/alfred.rb +++ b/lib/redis/alfred.rb @@ -40,6 +40,11 @@ module Redis::Alfred $alfred.with { |conn| conn.expire(key, seconds) } end + # get expiry of a key in seconds + def ttl(key) + $alfred.with { |conn| conn.ttl(key) } + end + # scan keys matching a pattern def scan_each(match: nil, count: 100, &) $alfred.with do |conn| @@ -80,6 +85,10 @@ module Redis::Alfred $alfred.with { |conn| conn.lrem(key, count, value) } end + def pipelined(&) + $alfred.with { |conn| conn.pipelined(&) } + end + # hash operations # add a key value to redis hash diff --git a/lib/redis/lock_manager.rb b/lib/redis/lock_manager.rb index 63063542c..4064d692c 100644 --- a/lib/redis/lock_manager.rb +++ b/lib/redis/lock_manager.rb @@ -49,6 +49,18 @@ class Redis::LockManager true end + def with_lock(key, timeout = LOCK_TIMEOUT) + return false unless lock(key, timeout) + + begin + yield + ensure + unlock(key) + end + + true + end + # Checks if the given key is currently locked. # # === Parameters diff --git a/lib/redis/redis_keys.rb b/lib/redis/redis_keys.rb index 15ff553cc..812553844 100644 --- a/lib/redis/redis_keys.rb +++ b/lib/redis/redis_keys.rb @@ -9,6 +9,28 @@ module Redis::RedisKeys # Whether a conversation is muted ? CONVERSATION_MUTE_KEY = 'CONVERSATION::%d::MUTED'.freeze CONVERSATION_DRAFT_MESSAGE = 'CONVERSATION::%d::DRAFT_MESSAGE'.freeze + UNREAD_CONVERSATIONS_ACCOUNT_PREFIX = 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d'.freeze + UNREAD_CONVERSATIONS_BASE_READY = 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::READY::BASE'.freeze + UNREAD_CONVERSATIONS_ASSIGNMENT_READY = 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::READY::ASSIGNMENT'.freeze + UNREAD_CONVERSATIONS_BASE_BUILD_LOCK = 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::BUILD_LOCK::BASE'.freeze + UNREAD_CONVERSATIONS_ASSIGNMENT_BUILD_LOCK = 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::BUILD_LOCK::ASSIGNMENT'.freeze + UNREAD_CONVERSATIONS_INBOX = 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::INBOX::%d'.freeze + UNREAD_CONVERSATIONS_LABEL_INBOX = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::LABEL::%d::INBOX::%d'.freeze + UNREAD_CONVERSATIONS_TEAM_INBOX = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::TEAM::%d::INBOX::%d'.freeze + UNREAD_CONVERSATIONS_INBOX_UNASSIGNED = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::INBOX::%d::UNASSIGNED'.freeze + UNREAD_CONVERSATIONS_INBOX_ASSIGNEE = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::INBOX::%d::ASSIGNEE::%d'.freeze + UNREAD_CONVERSATIONS_LABEL_INBOX_UNASSIGNED = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::LABEL::%d::INBOX::%d::UNASSIGNED'.freeze + UNREAD_CONVERSATIONS_LABEL_INBOX_ASSIGNEE = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::LABEL::%d::INBOX::%d::ASSIGNEE::%d'.freeze + UNREAD_CONVERSATIONS_TEAM_INBOX_UNASSIGNED = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::TEAM::%d::INBOX::%d::UNASSIGNED'.freeze + UNREAD_CONVERSATIONS_TEAM_INBOX_ASSIGNEE = + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::%d::TEAM::%d::INBOX::%d::ASSIGNEE::%d'.freeze ## User Keys # SSO Auth Tokens diff --git a/spec/enterprise/services/conversations/unread_counts/counter_spec.rb b/spec/enterprise/services/conversations/unread_counts/counter_spec.rb new file mode 100644 index 000000000..cbb2d6c98 --- /dev/null +++ b/spec/enterprise/services/conversations/unread_counts/counter_spec.rb @@ -0,0 +1,72 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Counter do + let(:account) { create(:account) } + let(:agent) { create(:user, account: account, role: :agent) } + let(:other_agent) { create(:user, account: account, role: :agent) } + let(:inbox) { create(:inbox, account: account) } + let(:label) { create(:label, account: account, title: 'support', show_on_sidebar: true) } + let(:team) { create(:team, account: account, allow_auto_assign: false) } + let(:account_user) { account.account_users.find_by(user: agent) } + let(:store) { Conversations::UnreadCounts::Store } + + before do + create(:inbox_member, user: agent, inbox: inbox) + create(:team_member, user: agent, team: team) + end + + after do + store.clear_account!(account.id) + end + + it 'uses base counts for custom roles with conversation_manage permission' do + account_user.update!(custom_role: create(:custom_role, account: account, permissions: ['conversation_manage'])) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: other_agent, team: team) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], team: team) + + result = described_class.new(account: account, user: agent).perform + + expect(result[:inboxes]).to eq(inbox.id.to_s => 2) + expect(result[:labels]).to eq(label.id.to_s => 2) + expect(result[:teams]).to eq(team.id.to_s => 2) + expect(store.assignment_ready?(account.id)).to be(false) + end + + it 'counts assigned and unassigned conversations for conversation_unassigned_manage permission' do + account_user.update!(custom_role: create(:custom_role, account: account, permissions: ['conversation_unassigned_manage'])) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: agent, team: team) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], team: team) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: other_agent, team: team) + + result = described_class.new(account: account, user: agent).perform + + expect(result[:inboxes]).to eq(inbox.id.to_s => 2) + expect(result[:labels]).to eq(label.id.to_s => 2) + expect(result[:teams]).to eq(team.id.to_s => 2) + expect(store.assignment_ready?(account.id)).to be(true) + end + + it 'counts only assigned conversations for conversation_participating_manage permission' do + account_user.update!(custom_role: create(:custom_role, account: account, permissions: ['conversation_participating_manage'])) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: agent, team: team) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], team: team) + + result = described_class.new(account: account, user: agent).perform + + expect(result[:inboxes]).to eq(inbox.id.to_s => 1) + expect(result[:labels]).to eq(label.id.to_s => 1) + expect(result[:teams]).to eq(team.id.to_s => 1) + expect(store.assignment_ready?(account.id)).to be(true) + end + + it 'returns zero for custom roles without conversation permissions' do + account_user.update!(custom_role: create(:custom_role, account: account, permissions: [])) + create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: agent, team: team) + + result = described_class.new(account: account, user: agent).perform + + expect(result).to eq(inboxes: {}, labels: {}, teams: {}) + expect(store.base_ready?(account.id)).to be(false) + expect(store.assignment_ready?(account.id)).to be(false) + end +end diff --git a/spec/lib/redis/lock_manager_spec.rb b/spec/lib/redis/lock_manager_spec.rb index 208594322..8e5feafb0 100644 --- a/spec/lib/redis/lock_manager_spec.rb +++ b/spec/lib/redis/lock_manager_spec.rb @@ -35,6 +35,28 @@ RSpec.describe Redis::LockManager do end end + describe '#with_lock' do + it 'yields when the lock is acquired and releases the lock' do + yielded = false + + expect(lock_manager.with_lock(lock_key) { yielded = true }).to be true + expect(yielded).to be true + expect(lock_manager.locked?(lock_key)).to be false + end + + it 'returns false without yielding when the lock is already acquired' do + lock_manager.lock(lock_key) + + expect { |block| lock_manager.with_lock(lock_key, &block) }.not_to yield_control + expect(lock_manager.with_lock(lock_key) { raise 'should not run' }).to be false + end + + it 'releases the lock when the block raises' do + expect { lock_manager.with_lock(lock_key) { raise 'boom' } }.to raise_error('boom') + expect(lock_manager.locked?(lock_key)).to be false + end + end + describe '#locked?' do it 'returns true if a key is locked' do lock_manager.lock(lock_key) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index d863dd58c..e83db4046 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -71,6 +71,7 @@ RSpec.configure do |config| config.include FileUploadHelpers config.include CsvSpecHelpers config.include InstagramSpecHelpers + config.include ConversationsUnreadCountsHelpers config.include Devise::Test::IntegrationHelpers, type: :request config.include ActiveSupport::Testing::TimeHelpers config.include ActionCable::TestHelper diff --git a/spec/services/conversations/unread_counts/builder_spec.rb b/spec/services/conversations/unread_counts/builder_spec.rb new file mode 100644 index 000000000..4b3aec230 --- /dev/null +++ b/spec/services/conversations/unread_counts/builder_spec.rb @@ -0,0 +1,93 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Builder do + let(:account) { create(:account) } + let(:inbox) { create(:inbox, account: account) } + let(:label) { create(:label, account: account, title: 'urgent', show_on_sidebar: true) } + let(:assignee) { create(:user, account: account, role: :agent) } + let(:team) { create(:team, account: account, allow_auto_assign: false) } + let(:store) { Conversations::UnreadCounts::Store } + + after do + store.clear_account!(account.id) + end + + describe '#build_base!' do + it 'stores unread open conversations by inbox and label inbox' do + unread_conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title], team: team) + create_read_conversation + create_resolved_unread_conversation + + described_class.new(account).build_base! + + expect(store.base_ready?(account.id)).to be(true) + expect(redis_set_members(store.inbox_key(account.id, inbox.id))).to contain_exactly(unread_conversation.id.to_s) + expect(redis_set_members(store.label_inbox_key(account.id, label.id, inbox.id))).to contain_exactly(unread_conversation.id.to_s) + expect(redis_set_members(store.team_inbox_key(account.id, team.id, inbox.id))).to contain_exactly(unread_conversation.id.to_s) + end + + it 'clears assignment-aware cache data before rebuilding base data' do + assigned_conversation = create_unread_conversation( + account: account, + inbox: inbox, + labels: [label.title], + assignee: assignee, + team: team + ) + + described_class.new(account).build_assignment! + described_class.new(account).build_base! + + expect(store.assignment_ready?(account.id)).to be(false) + expect(redis_set_members(store.inbox_assignee_key(account.id, inbox.id, assignee.id))).to be_empty + expect(redis_set_members(store.inbox_key(account.id, inbox.id))).to contain_exactly(assigned_conversation.id.to_s) + end + end + + describe '#build_assignment!' do + it 'stores unread open conversations by unassigned and assignee dimensions' do + assigned_conversation = create_unread_conversation( + account: account, + inbox: inbox, + labels: [label.title], + assignee: assignee, + team: team + ) + unassigned_conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title], team: team) + + described_class.new(account).build_assignment! + + expect(store.assignment_ready?(account.id)).to be(true) + expect(redis_set_members(store.inbox_assignee_key(account.id, inbox.id, assignee.id))).to contain_exactly(assigned_conversation.id.to_s) + expect(redis_set_members(store.label_inbox_assignee_key(account.id, label.id, inbox.id, assignee.id))).to contain_exactly( + assigned_conversation.id.to_s + ) + expect(redis_set_members(store.team_inbox_assignee_key(account.id, team.id, inbox.id, assignee.id))).to contain_exactly( + assigned_conversation.id.to_s + ) + expect(redis_set_members(store.inbox_unassigned_key(account.id, inbox.id))).to contain_exactly(unassigned_conversation.id.to_s) + expect(redis_set_members(store.label_inbox_unassigned_key(account.id, label.id, inbox.id))).to contain_exactly( + unassigned_conversation.id.to_s + ) + expect(redis_set_members(store.team_inbox_unassigned_key(account.id, team.id, inbox.id))).to contain_exactly( + unassigned_conversation.id.to_s + ) + end + end + + def create_read_conversation + conversation = create(:conversation, account: account, inbox: inbox, agent_last_seen_at: 1.minute.from_now) + create(:message, account: account, inbox: inbox, conversation: conversation, message_type: :incoming) + conversation + end + + def create_resolved_unread_conversation + conversation = create_unread_conversation(account: account, inbox: inbox) + conversation.update!(status: :resolved) + conversation + end + + def redis_set_members(key) + Redis::Alfred.pipelined { |pipeline| pipeline.smembers(key) }.first + end +end diff --git a/spec/services/conversations/unread_counts/counter_spec.rb b/spec/services/conversations/unread_counts/counter_spec.rb new file mode 100644 index 000000000..bfd10436e --- /dev/null +++ b/spec/services/conversations/unread_counts/counter_spec.rb @@ -0,0 +1,95 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Counter do + let(:account) { create(:account) } + let(:agent) { create(:user, account: account, role: :agent) } + let(:admin) { create(:user, account: account, role: :administrator) } + let(:visible_inbox) { create(:inbox, account: account) } + let(:hidden_inbox) { create(:inbox, account: account) } + let(:label) { create(:label, account: account, title: 'billing', show_on_sidebar: true) } + let(:hidden_label) { create(:label, account: account, title: 'internal', show_on_sidebar: false) } + let(:visible_team) { create(:team, account: account, allow_auto_assign: false) } + let(:store) { Conversations::UnreadCounts::Store } + + before do + create(:inbox_member, user: agent, inbox: visible_inbox) + create(:team_member, user: agent, team: visible_team) + end + + after do + store.clear_account!(account.id) + end + + it 'builds the base cache on demand' do + create_unread_conversation(account: account, inbox: visible_inbox, labels: [label.title], team: visible_team) + + described_class.new(account: account, user: agent).perform + + expect(store.base_ready?(account.id)).to be(true) + end + + it 'uses a Redis lock while building the base cache on demand' do + lock_key = "UNREAD_CONVERSATIONS::V1::ACCOUNT::#{account.id}::BUILD_LOCK::BASE" + lock_manager = instance_double(Redis::LockManager) + allow(Redis::LockManager).to receive(:new).and_return(lock_manager) + allow(lock_manager).to receive(:with_lock).with(lock_key, described_class::BUILD_LOCK_TTL).and_yield.and_return(true) + + create_unread_conversation(account: account, inbox: visible_inbox, labels: [label.title], team: visible_team) + + described_class.new(account: account, user: agent).perform + + expect(lock_manager).to have_received(:with_lock).with(lock_key, described_class::BUILD_LOCK_TTL) + end + + it 'waits instead of rebuilding when another process owns the base build lock' do + lock_manager = instance_double(Redis::LockManager, with_lock: false) + counter = described_class.new(account: account, user: agent) + + allow(Redis::LockManager).to receive(:new).and_return(lock_manager) + allow(counter).to receive(:wait_for_cache_ready) { store.mark_base_ready!(account.id) } + expect(Conversations::UnreadCounts::Builder).not_to receive(:new) + + counter.perform + + expect(counter).to have_received(:wait_for_cache_ready) + expect(store.base_ready?(account.id)).to be(true) + end + + it 'counts unread conversations only across inboxes visible to a normal agent' do + create_unread_conversation(account: account, inbox: visible_inbox, labels: [label.title], team: visible_team) + create_unread_conversation(account: account, inbox: hidden_inbox, labels: [label.title], team: visible_team) + + result = described_class.new(account: account, user: agent).perform + + expect(result).to eq( + inboxes: { visible_inbox.id.to_s => 1 }, + labels: { label.id.to_s => 1 }, + teams: { visible_team.id.to_s => 1 } + ) + end + + it 'counts unread conversations across all account inboxes for admins' do + create_unread_conversation(account: account, inbox: visible_inbox, labels: [label.title], team: visible_team) + create_unread_conversation(account: account, inbox: hidden_inbox, labels: [label.title], team: visible_team) + + result = described_class.new(account: account, user: admin).perform + + expect(result).to eq( + inboxes: { visible_inbox.id.to_s => 1, hidden_inbox.id.to_s => 1 }, + labels: { label.id.to_s => 2 }, + teams: { visible_team.id.to_s => 2 } + ) + end + + it 'does not return zero counts or labels hidden from the sidebar' do + create_unread_conversation(account: account, inbox: visible_inbox, labels: [hidden_label.title], team: visible_team) + + result = described_class.new(account: account, user: agent).perform + + expect(result).to eq( + inboxes: { visible_inbox.id.to_s => 1 }, + labels: {}, + teams: { visible_team.id.to_s => 1 } + ) + end +end diff --git a/spec/services/conversations/unread_counts/store_spec.rb b/spec/services/conversations/unread_counts/store_spec.rb new file mode 100644 index 000000000..652c74e46 --- /dev/null +++ b/spec/services/conversations/unread_counts/store_spec.rb @@ -0,0 +1,183 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Store do + let(:account_id) { 1 } + let(:inbox_id) { 2 } + let(:label_id) { 3 } + let(:user_id) { 4 } + let(:conversation_id) { 5 } + let(:team_id) { 6 } + + after do + described_class.clear_account!(account_id) + end + + describe 'key builders' do + it 'builds base keys using the Redis key naming convention' do + expect(described_class.inbox_key(account_id, inbox_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::INBOX::2' + ) + expect(described_class.label_inbox_key(account_id, label_id, inbox_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::LABEL::3::INBOX::2' + ) + expect(described_class.team_inbox_key(account_id, team_id, inbox_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::TEAM::6::INBOX::2' + ) + end + + it 'builds assignment-aware keys using the Redis key naming convention' do + expect(described_class.inbox_unassigned_key(account_id, inbox_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::INBOX::2::UNASSIGNED' + ) + expect(described_class.inbox_assignee_key(account_id, inbox_id, user_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::INBOX::2::ASSIGNEE::4' + ) + expect(described_class.label_inbox_unassigned_key(account_id, label_id, inbox_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::LABEL::3::INBOX::2::UNASSIGNED' + ) + expect(described_class.label_inbox_assignee_key(account_id, label_id, inbox_id, user_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::LABEL::3::INBOX::2::ASSIGNEE::4' + ) + expect(described_class.team_inbox_unassigned_key(account_id, team_id, inbox_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::TEAM::6::INBOX::2::UNASSIGNED' + ) + expect(described_class.team_inbox_assignee_key(account_id, team_id, inbox_id, user_id)).to eq( + 'UNREAD_CONVERSATIONS::V1::ACCOUNT::1::TEAM::6::INBOX::2::ASSIGNEE::4' + ) + end + end + + describe 'ready markers' do + it 'tracks base and assignment readiness independently' do + expect(described_class.base_ready?(account_id)).to be(false) + expect(described_class.assignment_ready?(account_id)).to be(false) + + described_class.mark_base_ready!(account_id) + described_class.mark_assignment_ready!(account_id) + + expect(described_class.base_ready?(account_id)).to be(true) + expect(described_class.assignment_ready?(account_id)).to be(true) + expect(ttl_for('UNREAD_CONVERSATIONS::V1::ACCOUNT::1::READY::BASE')).to be_within(5).of(Conversations::UnreadCounts::READY_TTL) + expect(ttl_for('UNREAD_CONVERSATIONS::V1::ACCOUNT::1::READY::ASSIGNMENT')).to be_within(5).of(Conversations::UnreadCounts::READY_TTL) + end + end + + describe 'set operations' do + it 'adds, counts, and removes base memberships' do + described_class.add_base_membership( + account_id: account_id, + inbox_id: inbox_id, + label_ids: [label_id], + team_id: team_id, + conversation_id: conversation_id + ) + + expect(described_class.counts_for_keys(base_keys)).to eq( + described_class.inbox_key(account_id, inbox_id) => 1, + described_class.label_inbox_key(account_id, label_id, inbox_id) => 1, + described_class.team_inbox_key(account_id, team_id, inbox_id) => 1 + ) + expect(base_keys.map { |key| ttl_for(key) }).to all(be_within(5).of(Conversations::UnreadCounts::SET_TTL)) + + described_class.remove_base_membership( + account_id: account_id, + inbox_ids: [inbox_id], + label_ids: [label_id], + team_ids: [team_id], + conversation_id: conversation_id + ) + + expect(described_class.counts_for_keys(base_keys).values).to all(eq(0)) + end + + it 'adds, counts, and removes assignment-aware memberships' do + described_class.add_assignment_membership( + account_id: account_id, + inbox_id: inbox_id, + label_ids: [label_id], + assignee_id: user_id, + team_id: team_id, + conversation_id: conversation_id + ) + + expect(described_class.counts_for_keys(assignment_keys)).to eq( + described_class.inbox_assignee_key(account_id, inbox_id, user_id) => 1, + described_class.label_inbox_assignee_key(account_id, label_id, inbox_id, user_id) => 1, + described_class.team_inbox_assignee_key(account_id, team_id, inbox_id, user_id) => 1 + ) + expect(assignment_keys.map { |key| ttl_for(key) }).to all(be_within(5).of(Conversations::UnreadCounts::SET_TTL)) + + described_class.remove_assignment_membership( + account_id: account_id, + inbox_ids: [inbox_id], + label_ids: [label_id], + assignee_ids: [user_id], + team_ids: [team_id], + conversation_id: conversation_id + ) + + expect(described_class.counts_for_keys(assignment_keys).values).to all(eq(0)) + end + + it 'sets expiry on bulk membership writes' do + described_class.add_memberships( + account_id: account_id, + memberships: [{ + inbox_id: inbox_id, + label_ids: [label_id], + team_id: team_id, + conversation_id: conversation_id + }] + ) + + expect(base_keys.map { |key| ttl_for(key) }).to all(be_within(5).of(Conversations::UnreadCounts::SET_TTL)) + end + + it 'clears all account memberships' do + described_class.mark_base_ready!(account_id) + described_class.mark_assignment_ready!(account_id) + described_class.add_base_membership( + account_id: account_id, + inbox_id: inbox_id, + label_ids: [label_id], + team_id: team_id, + conversation_id: conversation_id + ) + described_class.add_assignment_membership( + account_id: account_id, + inbox_id: inbox_id, + label_ids: [label_id], + assignee_id: user_id, + team_id: team_id, + conversation_id: conversation_id + ) + + described_class.clear_account!(account_id) + + expect(described_class.base_ready?(account_id)).to be(false) + expect(described_class.assignment_ready?(account_id)).to be(false) + expect(described_class.counts_for_keys(base_keys).values).to all(eq(0)) + expect(described_class.counts_for_keys(assignment_keys).values).to all(eq(0)) + end + end + + def base_keys + [ + described_class.inbox_key(account_id, inbox_id), + described_class.label_inbox_key(account_id, label_id, inbox_id), + described_class.team_inbox_key(account_id, team_id, inbox_id) + ] + end + + def assignment_keys + [ + described_class.inbox_assignee_key(account_id, inbox_id, user_id), + described_class.label_inbox_assignee_key(account_id, label_id, inbox_id, user_id), + described_class.team_inbox_assignee_key(account_id, team_id, inbox_id, user_id) + ] + end + + def ttl_for(key) + Redis::Alfred.ttl(key) + end +end diff --git a/spec/support/conversations_unread_counts_helpers.rb b/spec/support/conversations_unread_counts_helpers.rb new file mode 100644 index 000000000..e1a98d034 --- /dev/null +++ b/spec/support/conversations_unread_counts_helpers.rb @@ -0,0 +1,11 @@ +module ConversationsUnreadCountsHelpers + def create_unread_conversation(account:, inbox:, labels: [], assignee: nil, team: nil) + create(:team_member, user: assignee, team: team) if assignee.present? && team.present? && !team.members.exists?(assignee.id) + + conversation = create(:conversation, account: account, inbox: inbox, assignee: assignee, team: team, agent_last_seen_at: 1.hour.ago) + conversation.update_labels(labels) if labels.present? + + create(:message, account: account, inbox: inbox, conversation: conversation, message_type: :incoming, created_at: 5.minutes.ago) + conversation + end +end From 40deaef458c4343bdc109caef0dbdc85a156a854 Mon Sep 17 00:00:00 2001 From: Muhsin Keloth Date: Wed, 20 May 2026 13:36:43 +0400 Subject: [PATCH 2/8] feat: Store WhatsApp BSUID identifiers from inbound webhooks (#14436) Adds storage support for WhatsApp business-scoped user identifiers received from Meta Cloud API and Twilio WhatsApp webhooks. The change keeps existing phone-based behavior intact, stores BSUID and parent BSUID values as additional `contact_inboxes.source_id` rows for the same contact, and allows BSUID-only inbound messages to create contacts, conversations, and messages without requiring a phone number. Related: https://github.com/chatwoot/chatwoot/issues/13837 **What changed** - Extended WhatsApp source ID validation to accept regular BSUID and parent BSUID formats. - For Meta Cloud API, stores phone, `user_id`, and `parent_user_id` identifiers as contact inbox source IDs when they are present. - For Twilio WhatsApp, stores phone, `ExternalUserId`, and `ParentExternalUserId` identifiers as contact inbox source IDs while preserving the existing `whatsapp:` Twilio source ID shape. - Supports BSUID-only inbound messages by creating a contact, contact inbox, conversation, and message even when the phone number is missing. - Links phone-first and later BSUID-only messages to the same contact when the first payload contains both phone and BSUID. - Stores WhatsApp usernames in contact `additional_attributes`, matching existing social channel patterns. - Keeps existing phone-based outbound and new-conversation behavior unchanged for this milestone. **How to test** 1. Send a Meta Cloud webhook payload with both `wa_id` and `user_id`. 2. Verify Chatwoot creates or finds the phone `contact_inbox` and also creates a BSUID `contact_inbox` for the same contact. 3. Send a later Meta Cloud payload for the same user with only `user_id` / `from_user_id`. 4. Verify Chatwoot finds the BSUID `contact_inbox` and creates the inbound message without requiring a phone number. 5. Send a Twilio WhatsApp webhook with `From: whatsapp:+E164`, `ExternalUserId`, and optionally `ParentExternalUserId`. 6. Verify Chatwoot stores the Twilio phone and BSUID identifiers as `whatsapp:`-prefixed source IDs for the same contact. 7. Send a Twilio WhatsApp webhook where `From` is `whatsapp:` and there is no phone number. 8. Verify Chatwoot creates the contact, contact inbox, conversation, and message without a phone number. --------- Co-authored-by: Muhsin <12408980+muhsin-k@users.noreply.github.com> --- app/controllers/twilio/callback_controller.rb | 6 +- app/jobs/webhooks/whatsapp_events_job.rb | 31 ++++- .../contact_inbox_source_id_resolver.rb | 30 +++++ .../twilio/incoming_message_service.rb | 33 +++-- .../twilio/whatsapp_identifier_helper.rb | 69 ++++++++++ .../whatsapp/identifier_sync_service.rb | 68 ++++++++++ .../whatsapp/incoming_message_base_service.rb | 46 ++----- .../incoming_message_identifier_helper.rb | 108 ++++++++++++++++ .../incoming_message_service_helpers.rb | 8 ++ lib/regex_helper.rb | 8 +- .../twilio/callbacks_controller_spec.rb | 5 +- .../jobs/webhooks/whatsapp_events_job_spec.rb | 120 +++++++++++++++++ spec/models/contact_inbox_spec.rb | 37 +++++- .../twilio/incoming_message_service_spec.rb | 108 ++++++++++++++++ .../whatsapp/incoming_message_service_spec.rb | 122 ++++++++++++++++++ ...ing_message_whatsapp_cloud_service_spec.rb | 92 +++++++++++++ 16 files changed, 828 insertions(+), 63 deletions(-) create mode 100644 app/services/contact_inbox_source_id_resolver.rb create mode 100644 app/services/twilio/whatsapp_identifier_helper.rb create mode 100644 app/services/whatsapp/identifier_sync_service.rb create mode 100644 app/services/whatsapp/incoming_message_identifier_helper.rb diff --git a/app/controllers/twilio/callback_controller.rb b/app/controllers/twilio/callback_controller.rb index d607ba151..53075a555 100644 --- a/app/controllers/twilio/callback_controller.rb +++ b/app/controllers/twilio/callback_controller.rb @@ -31,7 +31,11 @@ class Twilio::CallbackController < ApplicationController :Latitude, :Longitude, :MessageType, - :ProfileName + :ProfileName, + :ExternalUserId, + :ParentExternalUserId, + :ProfileUsername, + :Username ) end end diff --git a/app/jobs/webhooks/whatsapp_events_job.rb b/app/jobs/webhooks/whatsapp_events_job.rb index 49b7265b8..f904b3723 100644 --- a/app/jobs/webhooks/whatsapp_events_job.rb +++ b/app/jobs/webhooks/whatsapp_events_job.rb @@ -91,10 +91,37 @@ class Webhooks::WhatsappEventsJob < MutexApplicationJob # Returns nil for status-only webhooks so they bypass the lock. def contact_sender_id(params) value = params.dig(:entry, 0, :changes, 0, :value) || params - message = (value[:messages] || value[:message_echoes])&.first + return contact_sender_id_from_message_echoes(value[:message_echoes]) if value[:message_echoes].present? + + contact_sender_id_from_messages(value[:messages], value[:contacts]) + end + + # Echo payloads are outbound messages from the WhatsApp Business app, so `to` + # points to the contact. Prefer parent BSUID when present so payloads that have + # both regular+parent BSUIDs serialize with parent-BSUID-only payloads. + def contact_sender_id_from_message_echoes(message_echoes) + message = message_echoes&.first return if message.blank? - message[:to] || message[:from] + [message[:to_parent_user_id], message[:to_user_id], message[:to]].compact_blank.first + end + + # Regular inbound payloads are sent by the contact, so `from` points to the + # contact. Prefer parent BSUID when present so payloads that have both + # regular+parent BSUIDs serialize with parent-BSUID-only payloads. + def contact_sender_id_from_messages(messages, contacts) + message = messages&.first + return if message.blank? + + contact = contacts&.first || {} + + [ + message[:from_parent_user_id], + contact[:parent_user_id], + message[:from_user_id], + contact[:user_id], + message[:from] + ].compact_blank.first end def channel_is_inactive?(channel) diff --git a/app/services/contact_inbox_source_id_resolver.rb b/app/services/contact_inbox_source_id_resolver.rb new file mode 100644 index 000000000..2fcea90db --- /dev/null +++ b/app/services/contact_inbox_source_id_resolver.rb @@ -0,0 +1,30 @@ +class ContactInboxSourceIdResolver + pattr_initialize [:inbox!, :source_ids!, :contact_attributes!] + + def perform + existing_contact_inbox || create_contact_inbox + end + + private + + def existing_contact_inbox + normalized_source_ids.each do |source_id| + contact_inbox = inbox.contact_inboxes.find_by(source_id: source_id) + return contact_inbox if contact_inbox + end + + nil + end + + def create_contact_inbox + ::ContactInboxWithContactBuilder.new( + source_id: normalized_source_ids.first, + inbox: inbox, + contact_attributes: contact_attributes + ).perform + end + + def normalized_source_ids + @normalized_source_ids ||= source_ids.compact_blank.uniq + end +end diff --git a/app/services/twilio/incoming_message_service.rb b/app/services/twilio/incoming_message_service.rb index d67b6d515..dab55e521 100644 --- a/app/services/twilio/incoming_message_service.rb +++ b/app/services/twilio/incoming_message_service.rb @@ -1,5 +1,6 @@ class Twilio::IncomingMessageService include ::FileTypeHelper + include ::Twilio::WhatsappIdentifierHelper pattr_initialize [:params!] @@ -51,17 +52,27 @@ class Twilio::IncomingMessageService @account ||= inbox.account end + # Twilio WhatsApp phone payloads arrive as `whatsapp:+E164`. BSUID-only + # payloads use `whatsapp:` in `From`, so this intentionally returns + # nil when `From` is not phone-shaped. def phone_number - twilio_channel.sms? ? params[:From] : params[:From].gsub('whatsapp:', '') + return params[:From] if twilio_channel.sms? + return unless twilio_whatsapp_phone_source? + + params[:From].gsub('whatsapp:', '') end + # Keep Twilio WhatsApp source ids in Twilio's native shape. Phone messages use + # `whatsapp:+E164`; BSUID-only messages fall back to `whatsapp:`. def normalized_phone_number return phone_number unless twilio_channel.whatsapp? - Whatsapp::PhoneNumberNormalizationService.new(inbox).normalize_and_find_contact_by_provider("whatsapp:#{phone_number}", :twilio) + twilio_whatsapp_primary_source_id end def formatted_phone_number + return if phone_number.blank? + TelephoneNumber.parse(phone_number).international_number end @@ -71,15 +82,9 @@ class Twilio::IncomingMessageService def set_contact source_id = twilio_channel.whatsapp? ? normalized_phone_number : params[:From] - - contact_inbox = ::ContactInboxWithContactBuilder.new( - source_id: source_id, - inbox: inbox, - contact_attributes: contact_attributes - ).perform - - @contact_inbox = contact_inbox - @contact = contact_inbox.contact + @contact_inbox = twilio_contact_inbox(source_id) + @contact = @contact_inbox.contact + update_twilio_whatsapp_identifiers # Update existing contact name if ProfileName is available and current name is just phone number update_contact_name_if_needed @@ -111,13 +116,13 @@ class Twilio::IncomingMessageService def contact_attributes { name: contact_name, - phone_number: phone_number, + phone_number: phone_number.presence, additional_attributes: additional_attributes } end def contact_name - params[:ProfileName].presence || formatted_phone_number + params[:ProfileName].presence || formatted_phone_number || twilio_whatsapp_display_identifier || params[:From] end def additional_attributes @@ -207,6 +212,8 @@ class Twilio::IncomingMessageService end def contact_name_matches_phone_number? + return false if phone_number.blank? + @contact.name == phone_number || @contact.name == formatted_phone_number end end diff --git a/app/services/twilio/whatsapp_identifier_helper.rb b/app/services/twilio/whatsapp_identifier_helper.rb new file mode 100644 index 000000000..731823903 --- /dev/null +++ b/app/services/twilio/whatsapp_identifier_helper.rb @@ -0,0 +1,69 @@ +module Twilio::WhatsappIdentifierHelper + TWILIO_WHATSAPP_BSUID_SOURCE_ID_REGEX = Regexp.new("\\Awhatsapp:#{RegexHelper::WHATSAPP_BSUID_PATTERN}\\z") + + def update_twilio_whatsapp_identifiers + return unless twilio_channel.whatsapp? + + Whatsapp::IdentifierSyncService.new(contact_inbox: @contact_inbox, contact: @contact).perform( + source_ids: twilio_whatsapp_source_ids, + username: params[:ProfileUsername].presence || params[:Username], + phone_number: phone_number.presence + ) + end + + def twilio_whatsapp_phone_source? + params[:From].to_s.match?(/\Awhatsapp:\+\d{1,15}\z/) + end + + def twilio_whatsapp_bsuid + params[:ExternalUserId].presence || twilio_whatsapp_bsuid_source_id + end + + def twilio_whatsapp_display_identifier + twilio_whatsapp_bsuid.to_s.delete_prefix('whatsapp:').presence + end + + def twilio_whatsapp_source_ids + [ + twilio_whatsapp_phone_source_id, + twilio_whatsapp_source_id(params[:ExternalUserId].presence) || twilio_whatsapp_bsuid_source_id, + twilio_whatsapp_source_id(params[:ParentExternalUserId].presence) + ].compact_blank.uniq + end + + def twilio_whatsapp_primary_source_id + twilio_whatsapp_source_ids.first + end + + def twilio_whatsapp_phone_source_id + return if phone_number.blank? + + Whatsapp::PhoneNumberNormalizationService.new(inbox).normalize_and_find_contact_by_provider("whatsapp:#{phone_number}", :twilio) + end + + def twilio_whatsapp_source_id(identifier) + identifier = identifier.to_s + return if identifier.blank? + + "whatsapp:#{identifier.delete_prefix('whatsapp:')}" + end + + def twilio_whatsapp_bsuid_source_id + from = params[:From].to_s + return from if from.match?(TWILIO_WHATSAPP_BSUID_SOURCE_ID_REGEX) + end + + def twilio_contact_inbox(source_id) + ContactInboxSourceIdResolver.new( + inbox: inbox, + source_ids: twilio_contact_inbox_source_ids(source_id), + contact_attributes: contact_attributes + ).perform + end + + def twilio_contact_inbox_source_ids(source_id) + return [source_id] unless twilio_channel.whatsapp? + + twilio_whatsapp_source_ids.presence || [source_id] + end +end diff --git a/app/services/whatsapp/identifier_sync_service.rb b/app/services/whatsapp/identifier_sync_service.rb new file mode 100644 index 000000000..f0701a3fc --- /dev/null +++ b/app/services/whatsapp/identifier_sync_service.rb @@ -0,0 +1,68 @@ +class Whatsapp::IdentifierSyncService + pattr_initialize [:contact_inbox!, :contact] + + def perform(source_ids: [], username: nil, phone_number: nil) + create_contact_inboxes(source_ids) + update_contact(username, phone_number) + end + + private + + def create_contact_inboxes(source_ids) + source_ids.compact_blank.uniq.each do |source_id| + next if inbox.contact_inboxes.exists?(source_id: source_id) + + inbox.contact_inboxes.create!(contact: synced_contact, source_id: source_id) + rescue ActiveRecord::RecordNotUnique + # A concurrent webhook (e.g. a status update bypassing the per-contact + # mutex) just inserted the same (inbox_id, source_id). Treat it as a + # no-op instead of falling through to ContactInboxBuilder's retry path, + # which would scramble the freshly-written row. + end + end + + def update_contact(username, phone_number) + return if synced_contact.blank? + + update_contact_phone_number(phone_number) + update_contact_username(username) + end + + def update_contact_phone_number(phone_number) + phone_number = phone_number.presence + return if phone_number.blank? || synced_contact.phone_number.present? + return if synced_contact.account.contacts.where(phone_number: phone_number).where.not(id: synced_contact.id).exists? + + synced_contact.update!(phone_number: phone_number) + end + + def update_contact_username(username) + username = normalize_username(username) + return if username.blank? + + synced_contact.update!(additional_attributes: additional_attributes_with_username(username)) + end + + def synced_contact + @synced_contact ||= contact || contact_inbox.contact + end + + def inbox + @inbox ||= contact_inbox.inbox + end + + def normalize_username(value) + value.to_s.sub(/\A@+/, '').presence + end + + def additional_attributes_with_username(username) + attributes = synced_contact.additional_attributes.deep_dup + social_profiles = attributes['social_profiles'] || {} + social_profiles['whatsapp'] = username + + attributes.merge( + 'social_profiles' => social_profiles, + 'social_whatsapp_user_name' => username + ) + end +end diff --git a/app/services/whatsapp/incoming_message_base_service.rb b/app/services/whatsapp/incoming_message_base_service.rb index 5449c4740..82aa7ab18 100644 --- a/app/services/whatsapp/incoming_message_base_service.rb +++ b/app/services/whatsapp/incoming_message_base_service.rb @@ -3,6 +3,7 @@ # https://developers.facebook.com/docs/whatsapp/api/media/ class Whatsapp::IncomingMessageBaseService include ::Whatsapp::IncomingMessageServiceHelpers + include ::Whatsapp::IncomingMessageIdentifierHelper pattr_initialize [:inbox!, :params!, :outgoing_echo] @@ -46,9 +47,11 @@ class Whatsapp::IncomingMessageBaseService end def process_statuses - return unless find_message_by_source_id(@processed_params[:statuses].first[:id]) + status = @processed_params[:statuses].first + return unless find_message_by_source_id(status[:id]) - update_message_with_status(@message, @processed_params[:statuses].first) + update_whatsapp_identifiers_from_status(status) + update_message_with_status(@message, status) rescue ArgumentError => e Rails.logger.error "Error while processing whatsapp status update #{e.message}" end @@ -95,40 +98,6 @@ class Whatsapp::IncomingMessageBaseService end end - def set_contact_from_echo - # For echo messages, contact phone is in the 'to' field - phone_number = messages_data.first[:to] - waid = processed_waid(phone_number) - - contact_inbox = ::ContactInboxWithContactBuilder.new( - source_id: waid, - inbox: inbox, - contact_attributes: { name: "+#{phone_number}", phone_number: "+#{phone_number}" } - ).perform - - @contact_inbox = contact_inbox - @contact = contact_inbox.contact - end - - def set_contact_from_message - contact_params = @processed_params[:contacts]&.first - return if contact_params.blank? - - waid = processed_waid(contact_params[:wa_id]) - - contact_inbox = ::ContactInboxWithContactBuilder.new( - source_id: waid, - inbox: inbox, - contact_attributes: { name: contact_params.dig(:profile, :name), phone_number: "+#{messages_data.first[:from]}" } - ).perform - - @contact_inbox = contact_inbox - @contact = contact_inbox.contact - - # Update existing contact name if ProfileName is available and current name is just phone number - update_contact_with_profile_name(contact_params) - end - def set_conversation # if lock to single conversation is disabled, we will create a new conversation if previous conversation is resolved @conversation = if @inbox.lock_to_single_conversation @@ -224,7 +193,10 @@ class Whatsapp::IncomingMessageBaseService end def contact_name_matches_phone_number? - phone_number = "+#{messages_data.first[:from]}" + message_phone_number = whatsapp_phone_number(messages_data.first[:from]) + return false if message_phone_number.blank? + + phone_number = "+#{message_phone_number}" formatted_phone_number = TelephoneNumber.parse(phone_number).international_number @contact.name == phone_number || @contact.name == formatted_phone_number end diff --git a/app/services/whatsapp/incoming_message_identifier_helper.rb b/app/services/whatsapp/incoming_message_identifier_helper.rb new file mode 100644 index 000000000..449b894d7 --- /dev/null +++ b/app/services/whatsapp/incoming_message_identifier_helper.rb @@ -0,0 +1,108 @@ +module Whatsapp::IncomingMessageIdentifierHelper + def set_contact_from_echo + message = messages_data.first + source_ids = outgoing_message_source_ids(message) + return if source_ids.blank? + + contact_attributes = contact_attributes_for_identifier(source_ids.first, message[:to]) + @contact_inbox = find_or_create_contact_inbox( + source_ids: source_ids, + contact_attributes: contact_attributes + ) + @contact = @contact_inbox.contact + update_whatsapp_identifiers(source_ids: source_ids, phone_number: contact_attributes[:phone_number]) + end + + def set_contact_from_message + contact_params = @processed_params[:contacts]&.first + return if contact_params.blank? + + source_ids = incoming_message_source_ids(contact_params) + return if source_ids.blank? + + attrs = contact_attributes_from_contact_params(contact_params, source_ids.first) + @contact_inbox = find_or_create_contact_inbox( + source_ids: source_ids, + contact_attributes: attrs + ) + @contact = @contact_inbox.contact + update_whatsapp_identifiers(source_ids: source_ids, username: contact_params.dig(:profile, :username), phone_number: attrs[:phone_number]) + update_contact_with_profile_name(contact_params) + end + + def find_or_create_contact_inbox(source_ids:, contact_attributes:) + ContactInboxSourceIdResolver.new( + inbox: inbox, + source_ids: source_ids, + contact_attributes: contact_attributes + ).perform + end + + def incoming_message_source_ids(contact_params) + [ + whatsapp_phone_source_id(contact_params[:wa_id].presence || messages_data.first[:from].presence), + whatsapp_source_id(contact_params[:user_id].presence || messages_data.first[:from_user_id].presence), + whatsapp_source_id(contact_params[:parent_user_id].presence || messages_data.first[:from_parent_user_id].presence) + ].compact_blank.uniq + end + + def outgoing_message_source_ids(message) + [ + whatsapp_phone_source_id(message[:to].presence), + whatsapp_source_id(message[:to_user_id].presence), + whatsapp_source_id(message[:to_parent_user_id].presence) + ].compact_blank.uniq + end + + def whatsapp_phone_source_id(identifier) + phone_number = whatsapp_phone_number(identifier) + return if phone_number.blank? + + processed_waid(phone_number) + end + + def whatsapp_source_id(identifier) + identifier.to_s.presence + end + + def contact_attributes_from_contact_params(contact_params, source_identifier) + contact_attributes_for_identifier( + contact_params.dig(:profile, :name).presence || source_identifier, + contact_params[:wa_id].presence || messages_data.first[:from].presence + ) + end + + def contact_attributes_for_identifier(name, phone_identifier) + phone_number = whatsapp_phone_number(phone_identifier) + return { name: name } if phone_number.blank? + + formatted_phone_number = "+#{phone_number}" + display_name = name == phone_identifier ? formatted_phone_number : name + { name: display_name, phone_number: formatted_phone_number } + end + + def update_whatsapp_identifiers(source_ids: [], username: nil, phone_number: nil) + Whatsapp::IdentifierSyncService.new(contact_inbox: @contact_inbox, contact: @contact).perform(source_ids: source_ids, username: username, + phone_number: phone_number) + end + + def update_whatsapp_identifiers_from_status(status) + contact_inbox = @message&.conversation&.contact_inbox + return if contact_inbox.blank? + + Whatsapp::IdentifierSyncService.new(contact_inbox: contact_inbox, contact: contact_inbox.contact).perform( + source_ids: status_source_ids(status) + ) + end + + def status_source_ids(status) + contact_params = @processed_params[:contacts]&.first || {} + + [ + whatsapp_source_id(status[:recipient_user_id]), + whatsapp_source_id(status[:recipient_parent_user_id]), + whatsapp_source_id(contact_params[:user_id]), + whatsapp_source_id(contact_params[:parent_user_id]) + ].compact_blank.uniq + end +end diff --git a/app/services/whatsapp/incoming_message_service_helpers.rb b/app/services/whatsapp/incoming_message_service_helpers.rb index bac6f6222..8ec884268 100644 --- a/app/services/whatsapp/incoming_message_service_helpers.rb +++ b/app/services/whatsapp/incoming_message_service_helpers.rb @@ -51,6 +51,14 @@ module Whatsapp::IncomingMessageServiceHelpers Whatsapp::PhoneNumberNormalizationService.new(inbox).normalize_and_find_contact_by_provider(waid, :cloud) end + def whatsapp_phone_number(identifier) + identifier = identifier.to_s + return if identifier.blank? + return unless identifier.match?(/\A\d{1,15}\z/) + + identifier + end + def error_webhook_event?(message) message.key?('errors') end diff --git a/lib/regex_helper.rb b/lib/regex_helper.rb index 2eeb895ea..e9304f581 100644 --- a/lib/regex_helper.rb +++ b/lib/regex_helper.rb @@ -13,7 +13,9 @@ module RegexHelper # while notifications use CommonMarker for better markdown processing MENTION_REGEX = Regexp.new('\[(@[^\\]]+)\]\(mention://(?:user|team)/\d+/([^)]+)\)') - TWILIO_CHANNEL_SMS_REGEX = Regexp.new('^\+\d{1,15}\z') - TWILIO_CHANNEL_WHATSAPP_REGEX = Regexp.new('^whatsapp:\+\d{1,15}\z') - WHATSAPP_CHANNEL_REGEX = Regexp.new('^\d{1,15}\z') + TWILIO_CHANNEL_SMS_REGEX = Regexp.new('\A\+\d{1,15}\z') + WHATSAPP_BSUID_PATTERN = '[A-Z]{2}\.(?:ENT\.)?[A-Za-z0-9]{1,128}'.freeze + WHATSAPP_BSUID_REGEX = Regexp.new("\\A#{WHATSAPP_BSUID_PATTERN}\\z") + TWILIO_CHANNEL_WHATSAPP_REGEX = Regexp.new("\\A(?:whatsapp:\\+\\d{1,15}|whatsapp:#{WHATSAPP_BSUID_PATTERN})\\z") + WHATSAPP_CHANNEL_REGEX = Regexp.new("\\A(?:\\d{1,15}|#{WHATSAPP_BSUID_PATTERN})\\z") end diff --git a/spec/controllers/twilio/callbacks_controller_spec.rb b/spec/controllers/twilio/callbacks_controller_spec.rb index d16acf229..1dc2991ae 100644 --- a/spec/controllers/twilio/callbacks_controller_spec.rb +++ b/spec/controllers/twilio/callbacks_controller_spec.rb @@ -10,7 +10,10 @@ RSpec.describe 'Twilio::CallbacksController', type: :request do 'To' => '+0987654321', 'Body' => 'Test message', 'AccountSid' => 'AC123', - 'SmsSid' => 'SM123' + 'SmsSid' => 'SM123', + 'ExternalUserId' => 'IN.2081978709342942', + 'ParentExternalUserId' => 'IN.ENT.9081726354', + 'ProfileUsername' => 'muhsin' } end diff --git a/spec/jobs/webhooks/whatsapp_events_job_spec.rb b/spec/jobs/webhooks/whatsapp_events_job_spec.rb index ba8a19413..d82658102 100644 --- a/spec/jobs/webhooks/whatsapp_events_job_spec.rb +++ b/spec/jobs/webhooks/whatsapp_events_job_spec.rb @@ -97,6 +97,126 @@ RSpec.describe Webhooks::WhatsappEventsJob do expect(Rails.logger).to receive(:warn).with("Inactive WhatsApp channel: unknown - #{unknown_phone}") job.perform_now(phone_number: unknown_phone) end + + it 'uses from_user_id as the mutex sender for BSUID-only inbound messages' do + bsuid = 'IN.2081978709342942' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:value][:messages] = [ + { from: '', from_user_id: bsuid, id: 'wamid-test', text: { body: 'Hello' }, type: 'text' } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end + + it 'prefers from_user_id as the mutex sender for mixed phone and BSUID inbound messages' do + bsuid = 'IN.2081978709342942' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:value][:messages] = [ + { from: '919745786257', from_user_id: bsuid, id: 'wamid-test', text: { body: 'Hello' }, type: 'text' } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end + + it 'uses contact user_id as the mutex sender when message from_user_id is missing' do + bsuid = 'IN.2081978709342942' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:value][:contacts] = [ + { profile: { name: 'Muhsin' }, wa_id: '919745786257', user_id: bsuid } + ] + wb_params[:entry].first[:changes].first[:value][:messages] = [ + { from: '919745786257', id: 'wamid-test', text: { body: 'Hello' }, type: 'text' } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end + + it 'prefers parent BSUID as the mutex sender for inbound messages with both identifiers' do + bsuid = 'IN.2081978709342942' + parent_bsuid = 'IN.ENT.9081726354' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:value][:contacts] = [ + { profile: { name: 'Muhsin' }, user_id: bsuid, parent_user_id: parent_bsuid } + ] + wb_params[:entry].first[:changes].first[:value][:messages] = [ + { from_user_id: bsuid, from_parent_user_id: parent_bsuid, id: 'wamid-test', text: { body: 'Hello' }, type: 'text' } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: parent_bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end + + it 'uses to_user_id as the mutex sender for BSUID-only echo messages' do + bsuid = 'IN.2081978709342942' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:field] = 'smb_message_echoes' + wb_params[:entry].first[:changes].first[:value][:message_echoes] = [ + { from: channel.phone_number.delete('+'), to: '', to_user_id: bsuid, id: 'wamid-test', text: { body: 'Hello' }, type: 'text' } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end + + it 'prefers parent BSUID as the mutex sender for echo messages with both identifiers' do + bsuid = 'IN.2081978709342942' + parent_bsuid = 'IN.ENT.9081726354' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:field] = 'smb_message_echoes' + wb_params[:entry].first[:changes].first[:value][:message_echoes] = [ + { + from: channel.phone_number.delete('+'), to: '919745786257', to_user_id: bsuid, to_parent_user_id: parent_bsuid, + id: 'wamid-test', text: { body: 'Hello' }, type: 'text' + } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: parent_bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end + + it 'prefers to_user_id as the mutex sender for mixed phone and BSUID echo messages' do + bsuid = 'IN.2081978709342942' + wb_params = params.deep_dup + wb_params[:entry].first[:changes].first[:field] = 'smb_message_echoes' + wb_params[:entry].first[:changes].first[:value][:message_echoes] = [ + { from: channel.phone_number.delete('+'), to: '919745786257', to_user_id: bsuid, id: 'wamid-test', text: { body: 'Hello' }, + type: 'text' } + ] + job_instance = described_class.new + mutex_key = format(Redis::Alfred::WHATSAPP_MESSAGE_MUTEX, inbox_id: channel.inbox.id, sender_id: bsuid) + + allow(Whatsapp::IncomingMessageWhatsappCloudService).to receive(:new).and_return(process_service) + expect(job_instance).to receive(:with_lock).with(mutex_key, 30.seconds).and_yield + + job_instance.perform(wb_params) + end end context 'when default provider' do diff --git a/spec/models/contact_inbox_spec.rb b/spec/models/contact_inbox_spec.rb index c0a2faf9e..c58615e9e 100644 --- a/spec/models/contact_inbox_spec.rb +++ b/spec/models/contact_inbox_spec.rb @@ -56,16 +56,20 @@ RSpec.describe ContactInbox do whatsapp_inbox = create(:channel_whatsapp, sync_templates: false, validate_provider_config: false).inbox contact = create(:contact) valid_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: '1234567890') + valid_bsuid_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: 'IN.2081978709342942') + valid_parent_bsuid_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: 'IN.ENT.9081726354') ci_character_in_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: '1234567890aaa') ci_plus_in_source_id = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: '+1234567890') expect(valid_source_id.valid?).to be(true) + expect(valid_bsuid_source_id.valid?).to be(true) + expect(valid_parent_bsuid_source_id.valid?).to be(true) expect(ci_character_in_source_id.valid?).to be(false) expect(ci_character_in_source_id.errors.full_messages).to eq( - ['Source invalid source id for whatsapp inbox. valid Regex (?-mix:^\\d{1,15}\\z)'] + ["Source invalid source id for whatsapp inbox. valid Regex #{RegexHelper::WHATSAPP_CHANNEL_REGEX}"] ) expect(ci_plus_in_source_id.valid?).to be(false) expect(ci_plus_in_source_id.errors.full_messages).to eq( - ['Source invalid source id for whatsapp inbox. valid Regex (?-mix:^\\d{1,15}\\z)'] + ["Source invalid source id for whatsapp inbox. valid Regex #{RegexHelper::WHATSAPP_CHANNEL_REGEX}"] ) end @@ -78,11 +82,11 @@ RSpec.describe ContactInbox do expect(valid_source_id.valid?).to be(true) expect(ci_character_in_source_id.valid?).to be(false) expect(ci_character_in_source_id.errors.full_messages).to eq( - ['Source invalid source id for twilio sms inbox. valid Regex (?-mix:^\\+\\d{1,15}\\z)'] + ["Source invalid source id for twilio sms inbox. valid Regex #{RegexHelper::TWILIO_CHANNEL_SMS_REGEX}"] ) expect(ci_without_plus_in_source_id.valid?).to be(false) expect(ci_without_plus_in_source_id.errors.full_messages).to eq( - ['Source invalid source id for twilio sms inbox. valid Regex (?-mix:^\\+\\d{1,15}\\z)'] + ["Source invalid source id for twilio sms inbox. valid Regex #{RegexHelper::TWILIO_CHANNEL_SMS_REGEX}"] ) end @@ -90,18 +94,39 @@ RSpec.describe ContactInbox do twilio_whatsapp_inbox = create(:channel_twilio_sms, medium: :whatsapp).inbox contact = create(:contact) valid_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:+1234567890') + valid_bsuid_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:IN.2081978709342942') + valid_parent_bsuid_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:IN.ENT.9081726354') ci_character_in_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:+1234567890aaa') ci_without_plus_in_source_id = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, source_id: 'whatsapp:1234567890') expect(valid_source_id.valid?).to be(true) + expect(valid_bsuid_source_id.valid?).to be(true) + expect(valid_parent_bsuid_source_id.valid?).to be(true) expect(ci_character_in_source_id.valid?).to be(false) expect(ci_character_in_source_id.errors.full_messages).to eq( - ['Source invalid source id for twilio whatsapp inbox. valid Regex (?-mix:^whatsapp:\\+\\d{1,15}\\z)'] + ["Source invalid source id for twilio whatsapp inbox. valid Regex #{RegexHelper::TWILIO_CHANNEL_WHATSAPP_REGEX}"] ) expect(ci_without_plus_in_source_id.valid?).to be(false) expect(ci_without_plus_in_source_id.errors.full_messages).to eq( - ['Source invalid source id for twilio whatsapp inbox. valid Regex (?-mix:^whatsapp:\\+\\d{1,15}\\z)'] + ["Source invalid source id for twilio whatsapp inbox. valid Regex #{RegexHelper::TWILIO_CHANNEL_WHATSAPP_REGEX}"] ) end + + it 'rejects whatsapp BSUID source_id values longer than 128 alphanumeric characters' do + whatsapp_inbox = create(:channel_whatsapp, sync_templates: false, validate_provider_config: false).inbox + contact = create(:contact) + contact_inbox = build(:contact_inbox, contact: contact, inbox: whatsapp_inbox, source_id: "IN.#{'1' * 129}") + + expect(contact_inbox.valid?).to be(false) + end + + it 'rejects twilio whatsapp parent BSUID source_id values longer than 128 alphanumeric characters' do + twilio_whatsapp_inbox = create(:channel_twilio_sms, medium: :whatsapp).inbox + contact = create(:contact) + contact_inbox = build(:contact_inbox, contact: contact, inbox: twilio_whatsapp_inbox, + source_id: "whatsapp:IN.ENT.#{'1' * 129}") + + expect(contact_inbox.valid?).to be(false) + end end end end diff --git a/spec/services/twilio/incoming_message_service_spec.rb b/spec/services/twilio/incoming_message_service_spec.rb index 190a6c45a..d50a0113d 100644 --- a/spec/services/twilio/incoming_message_service_spec.rb +++ b/spec/services/twilio/incoming_message_service_spec.rb @@ -403,6 +403,114 @@ describe Twilio::IncomingMessageService do expect(existing_contact.name).to eq('Alice Johnson') end + describe 'When the incoming WhatsApp message only has BSUID identifiers' do + let!(:whatsapp_twilio_channel) do + create(:channel_twilio_sms, :whatsapp, account: account, account_sid: 'ACxxx', + inbox: create(:inbox, account: account, greeting_enabled: false)) + end + + it 'creates a contact and conversation without a phone number' do + params = { + SmsSid: 'SMxx', + From: 'whatsapp:IN.2081978709342942', + AccountSid: 'ACxxx', + MessagingServiceSid: whatsapp_twilio_channel.messaging_service_sid, + Body: 'testing bsuid', + ProfileName: 'Muhsin', + ProfileUsername: 'muhsin', + ExternalUserId: 'IN.2081978709342942', + ParentExternalUserId: 'IN.ENT.9081726354' + } + + described_class.new(params: params).perform + + contact_inbox = whatsapp_twilio_channel.inbox.contact_inboxes.find_by!(source_id: 'whatsapp:IN.2081978709342942') + contact = contact_inbox.contact + parent_contact_inbox = whatsapp_twilio_channel.inbox.contact_inboxes.find_by!(source_id: 'whatsapp:IN.ENT.9081726354') + expect(whatsapp_twilio_channel.inbox.conversations.count).to eq(1) + expect(whatsapp_twilio_channel.inbox.messages.first.content).to eq('testing bsuid') + expect(contact).to have_attributes(name: 'Muhsin', phone_number: nil) + expect(contact.additional_attributes).to include( + 'social_whatsapp_user_name' => 'muhsin', + 'social_profiles' => { 'whatsapp' => 'muhsin' } + ) + expect(parent_contact_inbox.contact).to eq(contact) + end + + it 'uses the BSUID without the provider prefix as the fallback contact name' do + params = { + SmsSid: 'SMxx', + From: 'whatsapp:IN.2081978709342942', + AccountSid: 'ACxxx', + MessagingServiceSid: whatsapp_twilio_channel.messaging_service_sid, + Body: 'testing bsuid', + ExternalUserId: 'IN.2081978709342942' + } + + described_class.new(params: params).perform + + expect(whatsapp_twilio_channel.inbox.contacts.first.name).to eq('IN.2081978709342942') + end + + it 'links phone and BSUID source ids to the same contact' do + phone_with_bsuid_params = { + SmsSid: 'SMxx1', + From: 'whatsapp:+919745786257', + AccountSid: 'ACxxx', + MessagingServiceSid: whatsapp_twilio_channel.messaging_service_sid, + Body: 'phone and bsuid', + ProfileName: 'Muhsin', + ExternalUserId: 'IN.2081978709342942' + } + bsuid_only_params = { + SmsSid: 'SMxx2', + From: 'whatsapp:IN.2081978709342942', + AccountSid: 'ACxxx', + MessagingServiceSid: whatsapp_twilio_channel.messaging_service_sid, + Body: 'bsuid only', + ExternalUserId: 'IN.2081978709342942' + } + + described_class.new(params: phone_with_bsuid_params).perform + contact_inbox = whatsapp_twilio_channel.inbox.contact_inboxes.find_by!(source_id: 'whatsapp:+919745786257') + bsuid_contact_inbox = whatsapp_twilio_channel.inbox.contact_inboxes.find_by!(source_id: 'whatsapp:IN.2081978709342942') + + expect { described_class.new(params: bsuid_only_params).perform }.not_to raise_error + expect(whatsapp_twilio_channel.inbox.contact_inboxes.count).to eq(2) + expect(whatsapp_twilio_channel.inbox.messages.pluck(:content)).to contain_exactly('phone and bsuid', 'bsuid only') + expect(bsuid_contact_inbox.contact).to eq(contact_inbox.contact) + end + + it 'backfills contact phone number when a phone arrives after BSUID-only creation' do + bsuid_only_params = { + SmsSid: 'SMxx1', + From: 'whatsapp:IN.2081978709342942', + AccountSid: 'ACxxx', + MessagingServiceSid: whatsapp_twilio_channel.messaging_service_sid, + Body: 'bsuid first', + ExternalUserId: 'IN.2081978709342942' + } + phone_with_bsuid_params = { + SmsSid: 'SMxx2', + From: 'whatsapp:+919745786257', + AccountSid: 'ACxxx', + MessagingServiceSid: whatsapp_twilio_channel.messaging_service_sid, + Body: 'phone follow up', + ProfileName: 'Muhsin', + ExternalUserId: 'IN.2081978709342942' + } + + described_class.new(params: bsuid_only_params).perform + bsuid_contact_inbox = whatsapp_twilio_channel.inbox.contact_inboxes.find_by!(source_id: 'whatsapp:IN.2081978709342942') + + described_class.new(params: phone_with_bsuid_params).perform + + phone_contact_inbox = whatsapp_twilio_channel.inbox.contact_inboxes.find_by!(source_id: 'whatsapp:+919745786257') + expect(phone_contact_inbox.contact).to eq(bsuid_contact_inbox.contact) + expect(bsuid_contact_inbox.contact.reload.phone_number).to eq('+919745786257') + end + end + describe 'When the incoming number is a Brazilian number in new format with 9 included' do let!(:whatsapp_twilio_channel) do create(:channel_twilio_sms, :whatsapp, account: account, account_sid: 'ACxxx', diff --git a/spec/services/whatsapp/incoming_message_service_spec.rb b/spec/services/whatsapp/incoming_message_service_spec.rb index 2ecf60acb..dbaea621c 100644 --- a/spec/services/whatsapp/incoming_message_service_spec.rb +++ b/spec/services/whatsapp/incoming_message_service_spec.rb @@ -86,6 +86,110 @@ describe Whatsapp::IncomingMessageService do described_class.new(inbox: whatsapp_channel.inbox, params: params).perform expect(whatsapp_channel.inbox.messages.count).to eq(1) end + + it 'creates a contact and conversation when only BSUID is present' do + params = { + 'contacts' => [{ + 'profile' => { 'name' => 'Muhsin', 'username' => 'muhsin' }, + 'user_id' => 'IN.2081978709342942', + 'parent_user_id' => 'IN.ENT.9081726354' + }], + 'messages' => [{ + 'from_user_id' => 'IN.2081978709342942', + 'from_parent_user_id' => 'IN.ENT.9081726354', + 'id' => 'wamid.bsuid-only-message', + 'text' => { 'body' => 'testing bsuid' }, + 'timestamp' => '1778579582', + 'type' => 'text' + }] + }.with_indifferent_access + + described_class.new(inbox: whatsapp_channel.inbox, params: params).perform + + contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.2081978709342942') + contact = contact_inbox.contact + parent_contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.ENT.9081726354') + expect(whatsapp_channel.inbox.conversations.count).to eq(1) + expect(whatsapp_channel.inbox.messages.first.content).to eq('testing bsuid') + expect(contact).to have_attributes(name: 'Muhsin', phone_number: nil) + expect(contact.additional_attributes).to include( + 'social_whatsapp_user_name' => 'muhsin', + 'social_profiles' => { 'whatsapp' => 'muhsin' } + ) + expect(parent_contact_inbox.contact).to eq(contact) + end + + it 'links phone and BSUID source ids to the same contact' do + phone_with_bsuid_params = { + 'contacts' => [{ 'profile' => { 'name' => 'Muhsin' }, 'wa_id' => '919745786257', 'user_id' => 'IN.2081978709342942' }], + 'messages' => [{ + 'from' => '919745786257', + 'from_user_id' => 'IN.2081978709342942', + 'id' => 'wamid.phone-bsuid-message', + 'text' => { 'body' => 'phone and bsuid' }, + 'timestamp' => '1778579582', + 'type' => 'text' + }] + }.with_indifferent_access + bsuid_only_params = { + 'contacts' => [{ 'profile' => { 'name' => 'Muhsin' }, 'user_id' => 'IN.2081978709342942' }], + 'messages' => [{ + 'from_user_id' => 'IN.2081978709342942', + 'id' => 'wamid.bsuid-follow-up-message', + 'text' => { 'body' => 'bsuid only' }, + 'timestamp' => '1778579583', + 'type' => 'text' + }] + }.with_indifferent_access + + described_class.new(inbox: whatsapp_channel.inbox, params: phone_with_bsuid_params).perform + contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: '919745786257') + bsuid_contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.2081978709342942') + + expect { described_class.new(inbox: whatsapp_channel.inbox, params: bsuid_only_params).perform }.not_to raise_error + expect(whatsapp_channel.inbox.contact_inboxes.count).to eq(2) + expect(whatsapp_channel.inbox.messages.pluck(:content)).to contain_exactly('phone and bsuid', 'bsuid only') + expect(bsuid_contact_inbox.contact).to eq(contact_inbox.contact) + end + + it 'backfills contact phone number when a phone arrives after BSUID-only creation' do + bsuid_only_params = { + 'contacts' => [{ 'profile' => { 'name' => 'Muhsin' }, 'user_id' => 'IN.2081978709342942' }], + 'messages' => [{ + 'from_user_id' => 'IN.2081978709342942', + 'id' => 'wamid.bsuid-first-message', + 'text' => { 'body' => 'bsuid first' }, + 'timestamp' => '1778579582', + 'type' => 'text' + }] + }.with_indifferent_access + phone_with_bsuid_params = { + 'contacts' => [{ 'profile' => { 'name' => 'Muhsin' }, 'wa_id' => '919745786257', 'user_id' => 'IN.2081978709342942' }], + 'messages' => [{ + 'from' => '919745786257', + 'from_user_id' => 'IN.2081978709342942', + 'id' => 'wamid.phone-follow-up-message', + 'text' => { 'body' => 'phone follow up' }, + 'timestamp' => '1778579583', + 'type' => 'text' + }] + }.with_indifferent_access + + described_class.new(inbox: whatsapp_channel.inbox, params: bsuid_only_params).perform + bsuid_contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.2081978709342942') + + described_class.new(inbox: whatsapp_channel.inbox, params: phone_with_bsuid_params).perform + + phone_contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: '919745786257') + expect(phone_contact_inbox.contact).to eq(bsuid_contact_inbox.contact) + expect(bsuid_contact_inbox.contact.reload.phone_number).to eq('+919745786257') + end + + it 'keeps cloud BSUID source ids in the Meta-provided shape' do + service = described_class.new(inbox: whatsapp_channel.inbox, params: params) + + expect(service.send(:whatsapp_source_id, 'whatsapp:IN.2081978709342942')).to eq('whatsapp:IN.2081978709342942') + end end context 'when unsupported message types' do @@ -145,6 +249,24 @@ describe Whatsapp::IncomingMessageService do expect(message.reload.status).to eq('read') end + it 'stores BSUID source ids from status contacts' do + bsuid = 'IN.2081978709342942' + parent_bsuid = 'IN.ENT.9081726354' + status_params = { + 'contacts' => [{ 'wa_id' => from, 'user_id' => bsuid, 'parent_user_id' => parent_bsuid }], + 'statuses' => [{ 'recipient_id' => from, 'id' => from, 'status' => 'delivered' }] + }.with_indifferent_access + + described_class.new(inbox: whatsapp_channel.inbox, params: status_params).perform + + source_rows = whatsapp_channel.inbox.contact_inboxes.where(source_id: [from, bsuid, parent_bsuid]).pluck(:source_id, :contact_id) + expect(source_rows).to contain_exactly( + [from, contact_inbox.contact_id], + [bsuid, contact_inbox.contact_id], + [parent_bsuid, contact_inbox.contact_id] + ) + end + it 'update status message to failed' do status_params = { 'statuses' => [{ 'recipient_id' => from, 'id' => from, 'status' => 'failed', diff --git a/spec/services/whatsapp/incoming_message_whatsapp_cloud_service_spec.rb b/spec/services/whatsapp/incoming_message_whatsapp_cloud_service_spec.rb index 4b6841811..70c29c092 100644 --- a/spec/services/whatsapp/incoming_message_whatsapp_cloud_service_spec.rb +++ b/spec/services/whatsapp/incoming_message_whatsapp_cloud_service_spec.rb @@ -97,6 +97,98 @@ describe Whatsapp::IncomingMessageWhatsappCloudService do end end + context 'when BSUID identifiers are present' do + it 'creates a contact and conversation when only BSUID is present' do + bsuid_params = { + phone_number: whatsapp_channel.phone_number, + object: 'whatsapp_business_account', + entry: [{ + changes: [{ + value: { + contacts: [{ + profile: { name: 'Muhsin', username: 'muhsin' }, + user_id: 'IN.2081978709342942', + parent_user_id: 'IN.ENT.9081726354' + }], + messages: [{ + from_user_id: 'IN.2081978709342942', + from_parent_user_id: 'IN.ENT.9081726354', + id: 'wamid.cloud-bsuid-only-message', + text: { body: 'testing bsuid' }, + timestamp: '1778579582', + type: 'text' + }] + } + }] + }] + }.with_indifferent_access + + described_class.new(inbox: whatsapp_channel.inbox, params: bsuid_params).perform + + contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.2081978709342942') + contact = contact_inbox.contact + parent_contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.ENT.9081726354') + + expect(whatsapp_channel.inbox.conversations.count).to eq(1) + expect(whatsapp_channel.inbox.messages.first.content).to eq('testing bsuid') + expect(contact).to have_attributes(name: 'Muhsin', phone_number: nil) + expect(contact.additional_attributes).to include( + 'social_whatsapp_user_name' => 'muhsin', + 'social_profiles' => { 'whatsapp' => 'muhsin' } + ) + expect(parent_contact_inbox.contact).to eq(contact) + end + + it 'links phone and BSUID source ids to the same contact' do + phone_with_bsuid_params = { + phone_number: whatsapp_channel.phone_number, + object: 'whatsapp_business_account', + entry: [{ + changes: [{ + value: { + contacts: [{ profile: { name: 'Muhsin' }, wa_id: '919745786257', user_id: 'IN.2081978709342942' }], + messages: [{ + from: '919745786257', + from_user_id: 'IN.2081978709342942', + id: 'wamid.cloud-phone-bsuid-message', + text: { body: 'phone and bsuid' }, + timestamp: '1778579582', + type: 'text' + }] + } + }] + }] + }.with_indifferent_access + bsuid_only_params = { + phone_number: whatsapp_channel.phone_number, + object: 'whatsapp_business_account', + entry: [{ + changes: [{ + value: { + contacts: [{ profile: { name: 'Muhsin' }, user_id: 'IN.2081978709342942' }], + messages: [{ + from_user_id: 'IN.2081978709342942', + id: 'wamid.cloud-bsuid-follow-up-message', + text: { body: 'bsuid only' }, + timestamp: '1778579583', + type: 'text' + }] + } + }] + }] + }.with_indifferent_access + + described_class.new(inbox: whatsapp_channel.inbox, params: phone_with_bsuid_params).perform + contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: '919745786257') + bsuid_contact_inbox = whatsapp_channel.inbox.contact_inboxes.find_by!(source_id: 'IN.2081978709342942') + + expect { described_class.new(inbox: whatsapp_channel.inbox, params: bsuid_only_params).perform }.not_to raise_error + expect(whatsapp_channel.inbox.contact_inboxes.count).to eq(2) + expect(whatsapp_channel.inbox.messages.pluck(:content)).to contain_exactly('phone and bsuid', 'bsuid only') + expect(bsuid_contact_inbox.contact).to eq(contact_inbox.contact) + end + end + context 'when invalid params' do it 'will not throw error' do described_class.new(inbox: whatsapp_channel.inbox, params: { phone_number: whatsapp_channel.phone_number, From 27f2c2b3920a5f4a4fd3c2c93adaf94bdeed51eb Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Wed, 20 May 2026 17:36:09 +0530 Subject: [PATCH 3/8] feat: Unread Count: added api, store refresher, invalidation and events (2/3)[CW-6851] (#14369) # Pull Request Template ## Description This is the second PR in a series of PRs for Introducing unread counts in the sidebar for inboxes and labels. In this PR: * added api for unread counts * Added the store refresher and invalidation with event listeners * Added action cable event * Added specs for the changes Issue: https://linear.app/chatwoot/issue/CW-6851/support-unread-conversation-counts ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality not to work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Sojan Jose --- .../conversations/unread_counts_controller.rb | 16 ++ .../v1/accounts/conversations_controller.rb | 2 + app/dispatchers/async_dispatcher.rb | 1 + app/listeners/action_cable_listener.rb | 9 + app/models/account.rb | 5 + app/models/concerns/cache_keys.rb | 1 + app/models/conversation.rb | 19 ++ .../unread_counts/broadcast_scope.rb | 36 ++++ .../conversations/unread_counts/listener.rb | 96 ++++++++++ .../conversations/unread_counts/notifier.rb | 19 ++ .../conversations/unread_counts/refresher.rb | 178 ++++++++++++++++++ .../conversations/unread_counts/store.rb | 59 ++---- .../conversations/unread_counts/store_keys.rb | 43 +++++ config/features.yml | 8 +- config/locales/en.yml | 3 + config/routes.rb | 1 + ...ter_flag_for_conversation_unread_counts.rb | 20 ++ lib/events/types.rb | 2 + .../accounts/conversations_controller_spec.rb | 103 ++++++++++ .../super_admin/accounts_controller_spec.rb | 19 ++ spec/listeners/action_cable_listener_spec.rb | 64 +++++++ spec/models/account_spec.rb | 38 ++++ spec/models/conversation_spec.rb | 19 ++ .../unread_counts/listener_spec.rb | 164 ++++++++++++++++ .../unread_counts/notifier_spec.rb | 48 +++++ .../unread_counts/refresher_spec.rb | 145 ++++++++++++++ .../conversations/unread_counts/store_spec.rb | 17 ++ 27 files changed, 1091 insertions(+), 44 deletions(-) create mode 100644 app/controllers/api/v1/accounts/conversations/unread_counts_controller.rb create mode 100644 app/services/conversations/unread_counts/broadcast_scope.rb create mode 100644 app/services/conversations/unread_counts/listener.rb create mode 100644 app/services/conversations/unread_counts/notifier.rb create mode 100644 app/services/conversations/unread_counts/refresher.rb create mode 100644 app/services/conversations/unread_counts/store_keys.rb create mode 100644 db/migrate/20260508000000_repurpose_channel_twitter_flag_for_conversation_unread_counts.rb create mode 100644 spec/services/conversations/unread_counts/listener_spec.rb create mode 100644 spec/services/conversations/unread_counts/notifier_spec.rb create mode 100644 spec/services/conversations/unread_counts/refresher_spec.rb diff --git a/app/controllers/api/v1/accounts/conversations/unread_counts_controller.rb b/app/controllers/api/v1/accounts/conversations/unread_counts_controller.rb new file mode 100644 index 000000000..d9f15613b --- /dev/null +++ b/app/controllers/api/v1/accounts/conversations/unread_counts_controller.rb @@ -0,0 +1,16 @@ +class Api::V1::Accounts::Conversations::UnreadCountsController < Api::V1::Accounts::BaseController + before_action :ensure_unread_counts_enabled + + def index + counts = ::Conversations::UnreadCounts::Counter.new(account: Current.account, user: Current.user).perform + render json: { payload: counts } + end + + private + + def ensure_unread_counts_enabled + return if Current.account.feature_enabled?('conversation_unread_counts') + + render json: { error: I18n.t('errors.conversations.unread_counts.feature_not_enabled') }, status: :forbidden + end +end diff --git a/app/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index 6cc77cd54..2856c7817 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/conversations_controller.rb @@ -162,6 +162,8 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro # rubocop:disable Rails/SkipsModelValidations @conversation.update_columns(updates) # rubocop:enable Rails/SkipsModelValidations + + ::Conversations::UnreadCounts::Notifier.new(@conversation).perform end def should_update_last_seen? diff --git a/app/dispatchers/async_dispatcher.rb b/app/dispatchers/async_dispatcher.rb index 7416b7861..abf3ca354 100644 --- a/app/dispatchers/async_dispatcher.rb +++ b/app/dispatchers/async_dispatcher.rb @@ -17,6 +17,7 @@ class AsyncDispatcher < BaseDispatcher InstallationWebhookListener.instance, NotificationListener.instance, ParticipationListener.instance, + Conversations::UnreadCounts::Listener.instance, ReportingEventListener.instance, WebhookListener.instance ] diff --git a/app/listeners/action_cable_listener.rb b/app/listeners/action_cable_listener.rb index ff099618c..3bc221504 100644 --- a/app/listeners/action_cable_listener.rb +++ b/app/listeners/action_cable_listener.rb @@ -90,6 +90,15 @@ class ActionCableListener < BaseListener broadcast(account, tokens, CONVERSATION_UPDATED, conversation.push_event_data) end + def conversation_unread_count_changed(event) + account, inbox_members = ::Conversations::UnreadCounts::BroadcastScope.new(event).perform + return if account.blank? || !account.feature_enabled?('conversation_unread_counts') + + tokens = user_tokens(account, inbox_members) + + broadcast(account, tokens, CONVERSATION_UNREAD_COUNT_CHANGED, {}) + end + def conversation_typing_on(event) conversation = event.data[:conversation] account = conversation.account diff --git a/app/models/account.rb b/app/models/account.rb index b4cc03337..efaca8850 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -109,6 +109,7 @@ class Account < ApplicationRecord before_validation :validate_limit_keys after_create_commit :notify_creation + after_update_commit :clear_unread_conversation_counts_cache, if: :saved_change_to_feature_conversation_unread_counts? after_destroy :remove_account_sequences def agents @@ -174,6 +175,10 @@ class Account < ApplicationRecord Rails.configuration.dispatcher.dispatch(ACCOUNT_CREATED, Time.zone.now, account: self) end + def clear_unread_conversation_counts_cache + ::Conversations::UnreadCounts::Store.clear_account!(id) + end + trigger.after(:insert).for_each(:row) do "execute format('create sequence IF NOT EXISTS conv_dpid_seq_%s', NEW.id);" end diff --git a/app/models/concerns/cache_keys.rb b/app/models/concerns/cache_keys.rb index 3ad9bbadc..b37d7faa6 100644 --- a/app/models/concerns/cache_keys.rb +++ b/app/models/concerns/cache_keys.rb @@ -30,6 +30,7 @@ module CacheKeys update_cache_key_for_account(id, model.name.underscore) end + ::Conversations::UnreadCounts::Store.clear_account!(id) dispatch_cache_update_event end diff --git a/app/models/conversation.rb b/app/models/conversation.rb index 911cfdac6..0005ae4a4 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -121,6 +121,8 @@ class Conversation < ApplicationRecord after_update_commit :execute_after_update_commit_callbacks after_create_commit :notify_conversation_creation after_create_commit :load_attributes_created_by_db_triggers + before_destroy :set_unread_count_deletion_data + after_destroy_commit :notify_conversation_deletion delegate :auto_resolve_after, to: :account @@ -270,6 +272,12 @@ class Conversation < ApplicationRecord dispatcher_dispatch(CONVERSATION_CREATED) end + def notify_conversation_deletion + return if @unread_count_deletion_data.blank? + + Rails.configuration.dispatcher.dispatch(CONVERSATION_DELETED, Time.zone.now, conversation_data: @unread_count_deletion_data) + end + def notify_conversation_updation return unless previous_changes.keys.present? && allowed_keys? @@ -315,6 +323,17 @@ class Conversation < ApplicationRecord performed_by: Current.executed_by) end + def set_unread_count_deletion_data + @unread_count_deletion_data = { + id: id, + account_id: account_id, + inbox_id: inbox_id, + assignee_id: assignee_id, + team_id: team_id, + cached_label_list: cached_label_list + } + end + def conversation_status_changed_to_open? return false unless open? # saved_change_to_status? method only works in case of update diff --git a/app/services/conversations/unread_counts/broadcast_scope.rb b/app/services/conversations/unread_counts/broadcast_scope.rb new file mode 100644 index 000000000..8e47050e3 --- /dev/null +++ b/app/services/conversations/unread_counts/broadcast_scope.rb @@ -0,0 +1,36 @@ +class Conversations::UnreadCounts::BroadcastScope + attr_reader :event + + def initialize(event) + @event = event + end + + def perform + return [conversation.account, conversation.inbox.members] if conversation.present? + + deleted_conversation_scope + end + + private + + def conversation + event.data[:conversation] + end + + def deleted_conversation_scope + conversation_data = event.data[:conversation_data]&.with_indifferent_access + return if conversation_data.blank? + + account = Account.find_by(id: conversation_data[:account_id]) + return if account.blank? + + [account, inbox_members_for(account, conversation_data[:inbox_id])] + end + + def inbox_members_for(account, inbox_id) + inbox = account.inboxes.find_by(id: inbox_id) + return User.none if inbox.blank? + + inbox.members + end +end diff --git a/app/services/conversations/unread_counts/listener.rb b/app/services/conversations/unread_counts/listener.rb new file mode 100644 index 000000000..28792d884 --- /dev/null +++ b/app/services/conversations/unread_counts/listener.rb @@ -0,0 +1,96 @@ +class Conversations::UnreadCounts::Listener < BaseListener + include Events::Types + + def message_created(event) + message, = extract_message_and_account(event) + return unless message.incoming? + return unless message.account.feature_enabled?('conversation_unread_counts') + + refresh(message.conversation) + end + + def conversation_status_changed(event) + conversation, = extract_conversation_and_account(event) + refresh(conversation, event.data[:changed_attributes]) + end + + def conversation_updated(event) + return unless label_changed?(event.data[:changed_attributes]) + + conversation, = extract_conversation_and_account(event) + refresh(conversation, event.data[:changed_attributes]) + end + + def assignee_changed(event) + conversation, = extract_conversation_and_account(event) + refresh(conversation, event.data[:changed_attributes]) + end + + def team_changed(event) + conversation, = extract_conversation_and_account(event) + refresh(conversation, event.data[:changed_attributes]) + end + + def conversation_deleted(event) + conversation_data = event.data[:conversation_data]&.with_indifferent_access + return if conversation_data.blank? + + account = Account.find_by(id: conversation_data[:account_id]) + return unless account&.feature_enabled?('conversation_unread_counts') + return unless remove_deleted_conversation(account, conversation_data) + + Rails.configuration.dispatcher.dispatch(CONVERSATION_UNREAD_COUNT_CHANGED, Time.zone.now, conversation_data: conversation_data.to_h) + end + + private + + def refresh(conversation, changed_attributes = nil) + ::Conversations::UnreadCounts::Notifier.new(conversation, changed_attributes: changed_attributes).perform + end + + def remove_deleted_conversation(account, conversation_data) + return false unless store.base_ready?(account.id) || store.assignment_ready?(account.id) + + removed = false + removed = remove_deleted_base_membership(account, conversation_data) || removed if store.base_ready?(account.id) + removed = remove_deleted_assignment_membership(account, conversation_data) || removed if store.assignment_ready?(account.id) + removed + end + + def remove_deleted_base_membership(account, conversation_data) + store.remove_base_membership( + account_id: account.id, + inbox_ids: [conversation_data[:inbox_id]], + label_ids: label_ids_for(account, conversation_data[:cached_label_list]), + team_ids: [conversation_data[:team_id]], + conversation_id: conversation_data[:id] + ) + end + + def remove_deleted_assignment_membership(account, conversation_data) + store.remove_assignment_membership( + account_id: account.id, + inbox_ids: [conversation_data[:inbox_id]], + label_ids: label_ids_for(account, conversation_data[:cached_label_list]), + assignee_ids: [conversation_data[:assignee_id]], + team_ids: [conversation_data[:team_id]], + conversation_id: conversation_data[:id] + ) + end + + def label_ids_for(account, label_list) + label_titles = label_list.to_s.split(',').map(&:strip).compact_blank + account.labels.pluck(:title, :id).to_h.values_at(*label_titles).compact + end + + def label_changed?(changed_attributes) + return false if changed_attributes.blank? + + changed_attributes.key?('label_list') || changed_attributes.key?(:label_list) || + changed_attributes.key?('cached_label_list') || changed_attributes.key?(:cached_label_list) + end + + def store + ::Conversations::UnreadCounts::Store + end +end diff --git a/app/services/conversations/unread_counts/notifier.rb b/app/services/conversations/unread_counts/notifier.rb new file mode 100644 index 000000000..652fbde3a --- /dev/null +++ b/app/services/conversations/unread_counts/notifier.rb @@ -0,0 +1,19 @@ +class Conversations::UnreadCounts::Notifier + include Events::Types + + attr_reader :conversation, :changed_attributes + + def initialize(conversation, changed_attributes: nil) + @conversation = conversation + @changed_attributes = changed_attributes + end + + def perform + return false unless conversation.account.feature_enabled?('conversation_unread_counts') + + return false unless ::Conversations::UnreadCounts::Refresher.new(conversation, changed_attributes: changed_attributes).perform + + Rails.configuration.dispatcher.dispatch(CONVERSATION_UNREAD_COUNT_CHANGED, Time.zone.now, conversation: conversation) + true + end +end diff --git a/app/services/conversations/unread_counts/refresher.rb b/app/services/conversations/unread_counts/refresher.rb new file mode 100644 index 000000000..9456cab55 --- /dev/null +++ b/app/services/conversations/unread_counts/refresher.rb @@ -0,0 +1,178 @@ +class Conversations::UnreadCounts::Refresher + attr_reader :conversation, :changed_attributes + + def initialize(conversation, changed_attributes: nil) + @conversation = conversation + @changed_attributes = changed_attributes.is_a?(Hash) ? changed_attributes : {} + end + + def perform + return false unless base_ready? || assignment_ready? + + before_memberships = store.memberships_for_keys(affected_cache_keys, conversation.id) + refresh_base_membership if base_ready? + refresh_assignment_membership if assignment_ready? + after_memberships = store.memberships_for_keys(affected_cache_keys, conversation.id) + + before_memberships != after_memberships + end + + private + + def affected_cache_keys + keys = [] + keys.concat(affected_base_keys) if base_ready? + keys.concat(affected_assignment_keys) if assignment_ready? + keys.uniq + end + + def affected_base_keys + affected_inbox_ids.flat_map do |inbox_id| + [store.inbox_key(account.id, inbox_id)] + + affected_label_ids.map { |label_id| store.label_inbox_key(account.id, label_id, inbox_id) } + + affected_team_ids.map { |team_id| store.team_inbox_key(account.id, team_id, inbox_id) } + end + end + + def affected_assignment_keys + affected_inbox_ids.flat_map do |inbox_id| + affected_assignee_ids.flat_map do |assignee_id| + assignment_keys_for(inbox_id, assignee_id) + end + end + end + + def assignment_keys_for(inbox_id, assignee_id) + keys = assignee_id.present? ? assignee_keys_for(inbox_id, assignee_id) : unassigned_keys_for(inbox_id) + + keys + affected_team_ids.map { |team_id| team_assignment_key_for(team_id, inbox_id, assignee_id) } + end + + def assignee_keys_for(inbox_id, assignee_id) + [store.inbox_assignee_key(account.id, inbox_id, assignee_id)] + + affected_label_ids.map { |label_id| store.label_inbox_assignee_key(account.id, label_id, inbox_id, assignee_id) } + end + + def unassigned_keys_for(inbox_id) + [store.inbox_unassigned_key(account.id, inbox_id)] + + affected_label_ids.map { |label_id| store.label_inbox_unassigned_key(account.id, label_id, inbox_id) } + end + + def refresh_base_membership + store.remove_base_membership( + account_id: account.id, + inbox_ids: affected_inbox_ids, + label_ids: affected_label_ids, + team_ids: affected_team_ids, + conversation_id: conversation.id + ) + return unless unread? + + store.add_base_membership( + account_id: account.id, + inbox_id: conversation.inbox_id, + label_ids: current_label_ids, + team_id: conversation.team_id, + conversation_id: conversation.id + ) + end + + def refresh_assignment_membership + store.remove_assignment_membership( + account_id: account.id, + inbox_ids: affected_inbox_ids, + label_ids: affected_label_ids, + assignee_ids: affected_assignee_ids, + team_ids: affected_team_ids, + conversation_id: conversation.id + ) + return unless unread? + + store.add_assignment_membership( + account_id: account.id, + inbox_id: conversation.inbox_id, + label_ids: current_label_ids, + assignee_id: conversation.assignee_id, + team_id: conversation.team_id, + conversation_id: conversation.id + ) + end + + def unread? + # Sidebar unread counts intentionally track only open conversations. + return false unless conversation.open? + + incoming_messages = conversation.messages.incoming.where(account_id: account.id) + if conversation.agent_last_seen_at + incoming_messages = incoming_messages.where(Message.arel_table[:created_at].gt(conversation.agent_last_seen_at)) + end + incoming_messages.exists? + end + + def affected_inbox_ids + [previous_value_for(:inbox_id), conversation.inbox_id].compact.uniq + end + + def affected_assignee_ids + return [conversation.assignee_id].compact unless changed_attribute?(:assignee_id) + + [previous_value_for(:assignee_id), conversation.assignee_id].uniq + end + + def affected_label_ids + (previous_label_ids + current_label_ids).uniq + end + + def affected_team_ids + [previous_value_for(:team_id), conversation.team_id].compact.uniq + end + + def previous_label_ids + label_ids_for(previous_value_for(:label_list) || previous_value_for(:cached_label_list) || conversation.cached_label_list) + end + + def current_label_ids + @current_label_ids ||= label_ids_for(conversation.cached_label_list) + end + + def label_ids_for(label_list) + labels = label_list.is_a?(Array) ? label_list : label_list.to_s.split(',') + label_titles = labels.map(&:to_s).map(&:strip).compact_blank + labels_by_title.values_at(*label_titles).compact + end + + def previous_value_for(attribute) + change = changed_attributes[attribute.to_s] || changed_attributes[attribute.to_sym] + change&.first + end + + def changed_attribute?(attribute) + changed_attributes.key?(attribute.to_s) || changed_attributes.key?(attribute.to_sym) + end + + def team_assignment_key_for(team_id, inbox_id, assignee_id) + return store.team_inbox_assignee_key(account.id, team_id, inbox_id, assignee_id) if assignee_id.present? + + store.team_inbox_unassigned_key(account.id, team_id, inbox_id) + end + + def labels_by_title + @labels_by_title ||= account.labels.pluck(:title, :id).to_h + end + + def base_ready? + @base_ready ||= store.base_ready?(account.id) + end + + def assignment_ready? + @assignment_ready ||= store.assignment_ready?(account.id) + end + + def account + conversation.account + end + + def store + ::Conversations::UnreadCounts::Store + end +end diff --git a/app/services/conversations/unread_counts/store.rb b/app/services/conversations/unread_counts/store.rb index dbd01f235..e51fc5c09 100644 --- a/app/services/conversations/unread_counts/store.rb +++ b/app/services/conversations/unread_counts/store.rb @@ -1,4 +1,6 @@ class Conversations::UnreadCounts::Store + extend ::Conversations::UnreadCounts::StoreKeys + class << self def base_ready?(account_id) Redis::Alfred.exists?(base_ready_key(account_id)) @@ -78,46 +80,14 @@ class Conversations::UnreadCounts::Store keys.zip(counts).to_h end - def inbox_key(account_id, inbox_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX, account_id: account_id, inbox_id: inbox_id) - end + def memberships_for_keys(keys, conversation_id) + keys = keys.compact_blank + return {} if keys.blank? - def label_inbox_key(account_id, label_id, inbox_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX, account_id: account_id, label_id: label_id, inbox_id: inbox_id) - end - - def team_inbox_key(account_id, team_id, inbox_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX, account_id: account_id, team_id: team_id, inbox_id: inbox_id) - end - - def inbox_unassigned_key(account_id, inbox_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX_UNASSIGNED, account_id: account_id, inbox_id: inbox_id) - end - - def inbox_assignee_key(account_id, inbox_id, user_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX_ASSIGNEE, account_id: account_id, inbox_id: inbox_id, user_id: user_id) - end - - def label_inbox_unassigned_key(account_id, label_id, inbox_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX_UNASSIGNED, account_id: account_id, label_id: label_id, inbox_id: inbox_id) - end - - def label_inbox_assignee_key(account_id, label_id, inbox_id, user_id) - format( - Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX_ASSIGNEE, - account_id: account_id, - label_id: label_id, - inbox_id: inbox_id, - user_id: user_id - ) - end - - def team_inbox_unassigned_key(account_id, team_id, inbox_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX_UNASSIGNED, account_id: account_id, team_id: team_id, inbox_id: inbox_id) - end - - def team_inbox_assignee_key(account_id, team_id, inbox_id, user_id) - format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX_ASSIGNEE, account_id: account_id, team_id: team_id, inbox_id: inbox_id, user_id: user_id) + memberships = Redis::Alfred.pipelined do |pipeline| + keys.each { |key| pipeline.sismember(key, conversation_id) } + end + keys.zip(memberships.map { |membership| membership == true || membership == 1 }).to_h end private @@ -183,7 +153,16 @@ class Conversations::UnreadCounts::Store end def remove_from_sets(keys, conversation_id) - write_to_sets(keys) { |pipeline, key| pipeline.srem(key, conversation_id) } + keys = keys.compact_blank + return false if keys.blank? + + results = Redis::Alfred.pipelined do |pipeline| + keys.each do |key| + pipeline.srem(key, conversation_id) + pipeline.expire(key, Conversations::UnreadCounts::SET_TTL) + end + end + results.each_slice(2).any? { |removed, _| removed == true || (removed.respond_to?(:to_i) && removed.to_i.positive?) } end def write_to_sets(keys) diff --git a/app/services/conversations/unread_counts/store_keys.rb b/app/services/conversations/unread_counts/store_keys.rb new file mode 100644 index 000000000..bbb6d101a --- /dev/null +++ b/app/services/conversations/unread_counts/store_keys.rb @@ -0,0 +1,43 @@ +module Conversations::UnreadCounts::StoreKeys + def inbox_key(account_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX, account_id: account_id, inbox_id: inbox_id) + end + + def label_inbox_key(account_id, label_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX, account_id: account_id, label_id: label_id, inbox_id: inbox_id) + end + + def team_inbox_key(account_id, team_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX, account_id: account_id, team_id: team_id, inbox_id: inbox_id) + end + + def inbox_unassigned_key(account_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX_UNASSIGNED, account_id: account_id, inbox_id: inbox_id) + end + + def inbox_assignee_key(account_id, inbox_id, user_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_INBOX_ASSIGNEE, account_id: account_id, inbox_id: inbox_id, user_id: user_id) + end + + def label_inbox_unassigned_key(account_id, label_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX_UNASSIGNED, account_id: account_id, label_id: label_id, inbox_id: inbox_id) + end + + def label_inbox_assignee_key(account_id, label_id, inbox_id, user_id) + format( + Redis::Alfred::UNREAD_CONVERSATIONS_LABEL_INBOX_ASSIGNEE, + account_id: account_id, + label_id: label_id, + inbox_id: inbox_id, + user_id: user_id + ) + end + + def team_inbox_unassigned_key(account_id, team_id, inbox_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX_UNASSIGNED, account_id: account_id, team_id: team_id, inbox_id: inbox_id) + end + + def team_inbox_assignee_key(account_id, team_id, inbox_id, user_id) + format(Redis::Alfred::UNREAD_CONVERSATIONS_TEAM_INBOX_ASSIGNEE, account_id: account_id, team_id: team_id, inbox_id: inbox_id, user_id: user_id) + end +end diff --git a/config/features.yml b/config/features.yml index c469fed90..03105588b 100644 --- a/config/features.yml +++ b/config/features.yml @@ -17,10 +17,10 @@ display_name: Facebook Channel enabled: true help_url: https://chwt.app/hc/fb -- name: channel_twitter - display_name: Twitter Channel - enabled: true - deprecated: true +- name: conversation_unread_counts + display_name: Conversation Unread Counts + enabled: false + chatwoot_internal: true - name: ip_lookup display_name: IP Lookup enabled: false diff --git a/config/locales/en.yml b/config/locales/en.yml index 8c66632d8..18b721caf 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -81,6 +81,9 @@ en: saml: feature_not_enabled: SAML feature not enabled for this account sso_not_enabled: SAML SSO is not enabled for this installation + conversations: + unread_counts: + feature_not_enabled: Conversation unread counts feature not enabled for this account data_import: data_type: invalid: Invalid data type diff --git a/config/routes.rb b/config/routes.rb index 355491d5b..e7f8a4745 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -133,6 +133,7 @@ Rails.application.routes.draw do collection do get :meta get :search + get :unread_counts, to: 'conversations/unread_counts#index' post :filter end scope module: :conversations do diff --git a/db/migrate/20260508000000_repurpose_channel_twitter_flag_for_conversation_unread_counts.rb b/db/migrate/20260508000000_repurpose_channel_twitter_flag_for_conversation_unread_counts.rb new file mode 100644 index 000000000..4c0bff5bd --- /dev/null +++ b/db/migrate/20260508000000_repurpose_channel_twitter_flag_for_conversation_unread_counts.rb @@ -0,0 +1,20 @@ +class RepurposeChannelTwitterFlagForConversationUnreadCounts < ActiveRecord::Migration[7.1] + def up + # The channel_twitter flag (deprecated) has been renamed to conversation_unread_counts. + # Disable it on any accounts that had channel_twitter enabled so the repurposed + # flag starts in its intended default-off state. + Account.feature_conversation_unread_counts.find_each(batch_size: 100) do |account| + account.disable_features(:conversation_unread_counts) + account.save!(validate: false) + end + + # Remove the stale channel_twitter entry from ACCOUNT_LEVEL_FEATURE_DEFAULTS. + # ConfigLoader only adds new flags; it never removes renamed ones. + config = InstallationConfig.find_by(name: 'ACCOUNT_LEVEL_FEATURE_DEFAULTS') + return if config&.value.blank? + + config.value = config.value.reject { |feature| feature['name'] == 'channel_twitter' } + config.save! + GlobalConfig.clear_cache + end +end diff --git a/lib/events/types.rb b/lib/events/types.rb index d742232c4..171649a7d 100644 --- a/lib/events/types.rb +++ b/lib/events/types.rb @@ -16,6 +16,7 @@ module Events::Types # conversation events CONVERSATION_CREATED = 'conversation.created' CONVERSATION_UPDATED = 'conversation.updated' + CONVERSATION_DELETED = 'conversation.deleted' CONVERSATION_READ = 'conversation.read' CONVERSATION_BOT_HANDOFF = 'conversation.bot_handoff' # FIXME: deprecate the opened and resolved events in future in favor of status changed event. @@ -26,6 +27,7 @@ module Events::Types CONVERSATION_STATUS_CHANGED = 'conversation.status_changed' CONVERSATION_CONTACT_CHANGED = 'conversation.contact_changed' + CONVERSATION_UNREAD_COUNT_CHANGED = 'conversation.unread_count_changed' ASSIGNEE_CHANGED = 'assignee.changed' TEAM_CHANGED = 'team.changed' CONVERSATION_TYPING_ON = 'conversation.typing_on' diff --git a/spec/controllers/api/v1/accounts/conversations_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations_controller_spec.rb index 19d080b47..f8fd446d2 100644 --- a/spec/controllers/api/v1/accounts/conversations_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations_controller_spec.rb @@ -101,6 +101,76 @@ RSpec.describe 'Conversations API', type: :request do end end + describe 'GET /api/v1/accounts/{account.id}/conversations/unread_counts' do + context 'when it is an unauthenticated user' do + it 'returns unauthorized' do + get "/api/v1/accounts/#{account.id}/conversations/unread_counts" + + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when it is an authenticated user' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:visible_inbox) { create(:inbox, account: account) } + let(:hidden_inbox) { create(:inbox, account: account) } + let(:label) { create(:label, account: account, title: 'billing', show_on_sidebar: true) } + let(:team) { create(:team, account: account, allow_auto_assign: false) } + + before do + create(:inbox_member, user: agent, inbox: visible_inbox) + create(:team_member, user: agent, team: team) + end + + after do + Conversations::UnreadCounts::Store.clear_account!(account.id) + end + + context 'when conversation unread counts feature is enabled' do + before do + account.enable_features!(:conversation_unread_counts) + end + + it 'returns unread conversation counts scoped to the signed-in user' do + create_unread_conversation(account: account, inbox: visible_inbox, labels: [label.title]) + create_unread_conversation(account: account, inbox: hidden_inbox, labels: [label.title]) + + get "/api/v1/accounts/#{account.id}/conversations/unread_counts", + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + expect(response.parsed_body['payload']).to eq( + 'inboxes' => { visible_inbox.id.to_s => 1 }, + 'labels' => { label.id.to_s => 1 }, + 'teams' => {} + ) + end + + it 'returns unread team conversation counts scoped to the signed-in user' do + create_unread_conversation(account: account, inbox: visible_inbox, team: team) + create_unread_conversation(account: account, inbox: hidden_inbox, team: team) + + get "/api/v1/accounts/#{account.id}/conversations/unread_counts", + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + expect(response.parsed_body['payload']['teams']).to eq(team.id.to_s => 1) + end + end + + it 'returns forbidden when conversation unread counts feature is disabled' do + get "/api/v1/accounts/#{account.id}/conversations/unread_counts", + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:forbidden) + expect(response.parsed_body['error']).to eq('Conversation unread counts feature not enabled for this account') + end + end + end + describe 'GET /api/v1/accounts/{account.id}/conversations/search' do context 'when it is an unauthenticated user' do it 'returns unauthorized' do @@ -777,6 +847,23 @@ RSpec.describe 'Conversations API', type: :request do expect(conversation.reload.agent_last_seen_at).to be > initial_last_seen end + it 'refreshes unread count cache when conversation is marked read' do + account.enable_features!(:conversation_unread_counts) + conversation.update!(agent_last_seen_at: 1.hour.ago) + create(:message, account: account, inbox: conversation.inbox, conversation: conversation, message_type: :incoming, created_at: 5.minutes.ago) + Conversations::UnreadCounts::Builder.new(account).build_base! + + post "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}/update_last_seen", + headers: agent.create_new_auth_token, + as: :json + + inbox_key = Conversations::UnreadCounts::Store.inbox_key(account.id, conversation.inbox_id) + expect(response).to have_http_status(:success) + expect(Conversations::UnreadCounts::Store.counts_for_keys([inbox_key])).to eq(inbox_key => 0) + ensure + Conversations::UnreadCounts::Store.clear_account!(account.id) + end + it 'updates both if one timestamp is old even when the other is recent' do conversation.update!(assignee_id: agent.id, agent_last_seen_at: 2.hours.ago, assignee_last_seen_at: 30.minutes.ago) # Ensure all messages are older than assignee_last_seen_at (no unread messages) @@ -847,6 +934,22 @@ RSpec.describe 'Conversations API', type: :request do expect(conversation.reload.agent_last_seen_at).to eq(last_seen_at) expect(conversation.reload.assignee_last_seen_at).to eq(last_seen_at) end + + it 'refreshes unread count cache when conversation is marked unread' do + account.enable_features!(:conversation_unread_counts) + conversation.update!(agent_last_seen_at: 1.minute.from_now, assignee_last_seen_at: 1.minute.from_now) + Conversations::UnreadCounts::Builder.new(account).build_base! + + post "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}/unread", + headers: agent.create_new_auth_token, + as: :json + + inbox_key = Conversations::UnreadCounts::Store.inbox_key(account.id, conversation.inbox_id) + expect(response).to have_http_status(:success) + expect(Conversations::UnreadCounts::Store.counts_for_keys([inbox_key])).to eq(inbox_key => 1) + ensure + Conversations::UnreadCounts::Store.clear_account!(account.id) + end end end diff --git a/spec/controllers/super_admin/accounts_controller_spec.rb b/spec/controllers/super_admin/accounts_controller_spec.rb index 6b7e6eeed..e4ff81a08 100644 --- a/spec/controllers/super_admin/accounts_controller_spec.rb +++ b/spec/controllers/super_admin/accounts_controller_spec.rb @@ -32,6 +32,10 @@ RSpec.describe 'Super Admin accounts API', type: :request do create(:team, account: account) end + after do + Conversations::UnreadCounts::Store.clear_account!(account.id) + end + context 'when it is an unauthenticated user' do it 'returns unauthorized' do post "/super_admin/accounts/#{account.id}/reset_cache" @@ -52,6 +56,21 @@ RSpec.describe 'Super Admin accounts API', type: :request do range = now_timestamp..(now_timestamp + 10) expect(account.reload.cache_keys.values.all? { |v| range.cover?(v.to_i) }).to be(true) end + + it 'clears conversation unread count cache' do + inbox = account.inboxes.first + store = Conversations::UnreadCounts::Store + inbox_key = store.inbox_key(account.id, inbox.id) + store.mark_base_ready!(account.id) + store.add_base_membership(account_id: account.id, inbox_id: inbox.id, label_ids: [], conversation_id: 1) + + sign_in(super_admin, scope: :super_admin) + post "/super_admin/accounts/#{account.id}/reset_cache" + + expect(response).to have_http_status(:redirect) + expect(store.base_ready?(account.id)).to be(false) + expect(store.counts_for_keys([inbox_key])).to eq(inbox_key => 0) + end end end diff --git a/spec/listeners/action_cable_listener_spec.rb b/spec/listeners/action_cable_listener_spec.rb index 8b18f1582..cdb9a93cb 100644 --- a/spec/listeners/action_cable_listener_spec.rb +++ b/spec/listeners/action_cable_listener_spec.rb @@ -231,4 +231,68 @@ describe ActionCableListener do listener.conversation_updated(event) end end + + describe '#conversation_unread_count_changed' do + let(:event_name) { :'conversation.unread_count_changed' } + let!(:agent_without_inbox_access) { create(:user, account: account, role: :agent) } + let!(:event) { Events::Base.new(event_name, Time.zone.now, conversation: conversation) } + + before do + account.enable_features!(:conversation_unread_counts) + end + + it 'sends a lightweight refresh event to inbox agents and admins' do + expect(conversation.inbox.reload.inbox_members.count).to eq(1) + + expect(ActionCableBroadcastJob).to receive(:perform_later).with( + a_collection_containing_exactly(agent.pubsub_token, admin.pubsub_token), + 'conversation.unread_count_changed', + { + account_id: account.id + } + ) + + listener.conversation_unread_count_changed(event) + end + + it 'does not broadcast unread count refresh to agents outside the inbox' do + expect(ActionCableBroadcastJob).not_to receive(:perform_later).with( + array_including(agent_without_inbox_access.pubsub_token), + anything, + anything + ) + + listener.conversation_unread_count_changed(event) + end + + it 'does not broadcast when conversation unread counts feature is disabled' do + account.disable_features!(:conversation_unread_counts) + + expect(ActionCableBroadcastJob).not_to receive(:perform_later) + + listener.conversation_unread_count_changed(event) + end + + it 'supports deleted conversation data' do + event = Events::Base.new( + event_name, + Time.zone.now, + conversation_data: { + id: conversation.id, + account_id: account.id, + inbox_id: conversation.inbox_id + } + ) + + expect(ActionCableBroadcastJob).to receive(:perform_later).with( + a_collection_containing_exactly(agent.pubsub_token, admin.pubsub_token), + 'conversation.unread_count_changed', + { + account_id: account.id + } + ) + + listener.conversation_unread_count_changed(event) + end + end end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 76dbbcba2..38ca9694a 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -50,6 +50,44 @@ RSpec.describe Account do end end + describe 'conversation unread counts feature flag' do + let(:account) { create(:account) } + let(:inbox) { create(:inbox, account: account) } + let(:store) { Conversations::UnreadCounts::Store } + let(:inbox_key) { store.inbox_key(account.id, inbox.id) } + + after do + store.clear_account!(account.id) + end + + it 'clears unread count cache when the feature is enabled' do + build_unread_count_cache + + account.enable_features!(:conversation_unread_counts) + + expect(store.base_ready?(account.id)).to be(false) + expect(store.assignment_ready?(account.id)).to be(false) + expect(store.counts_for_keys([inbox_key])).to eq(inbox_key => 0) + end + + it 'clears unread count cache when the feature is disabled' do + account.enable_features!(:conversation_unread_counts) + build_unread_count_cache + + account.disable_features!(:conversation_unread_counts) + + expect(store.base_ready?(account.id)).to be(false) + expect(store.assignment_ready?(account.id)).to be(false) + expect(store.counts_for_keys([inbox_key])).to eq(inbox_key => 0) + end + + def build_unread_count_cache + store.mark_base_ready!(account.id) + store.mark_assignment_ready!(account.id) + store.add_base_membership(account_id: account.id, inbox_id: inbox.id, label_ids: [], conversation_id: 1) + end + end + describe 'inbound_email_domain' do let(:account) { create(:account) } diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb index 0bf90859e..58d64ea94 100644 --- a/spec/models/conversation_spec.rb +++ b/spec/models/conversation_spec.rb @@ -717,6 +717,25 @@ RSpec.describe Conversation do expect { notification.reload }.to raise_error ActiveRecord::RecordNotFound end + + it 'dispatches conversation deleted event with unread count cache data' do + allow(Rails.configuration.dispatcher).to receive(:dispatch) + + conversation.destroy! + + expect(Rails.configuration.dispatcher).to have_received(:dispatch).with( + 'conversation.deleted', + kind_of(Time), + conversation_data: { + id: conversation.id, + account_id: conversation.account_id, + inbox_id: conversation.inbox_id, + assignee_id: conversation.assignee_id, + team_id: conversation.team_id, + cached_label_list: conversation.cached_label_list + } + ) + end end describe 'validate invalid referer url' do diff --git a/spec/services/conversations/unread_counts/listener_spec.rb b/spec/services/conversations/unread_counts/listener_spec.rb new file mode 100644 index 000000000..fbb0a0835 --- /dev/null +++ b/spec/services/conversations/unread_counts/listener_spec.rb @@ -0,0 +1,164 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Listener do + let(:listener) { described_class.instance } + let(:account) { create(:account) } + let(:conversation) { create(:conversation, account: account) } + let(:notifier) { instance_double(Conversations::UnreadCounts::Notifier, perform: true) } + + before do + allow(Conversations::UnreadCounts::Notifier).to receive(:new).and_return(notifier) + end + + it 'refreshes unread counts when an incoming message is created' do + account.enable_features!(:conversation_unread_counts) + message = create(:message, account: account, inbox: conversation.inbox, conversation: conversation, message_type: :incoming) + event = Events::Base.new('message.created', Time.zone.now, message: message) + + listener.message_created(event) + + expect(Conversations::UnreadCounts::Notifier).to have_received(:new).with(conversation, changed_attributes: nil) + expect(notifier).to have_received(:perform) + end + + it 'ignores outgoing message creation' do + message = create(:message, account: account, inbox: conversation.inbox, conversation: conversation, message_type: :outgoing) + event = Events::Base.new('message.created', Time.zone.now, message: message) + + listener.message_created(event) + + expect(Conversations::UnreadCounts::Notifier).not_to have_received(:new) + end + + it 'ignores incoming message creation when conversation unread counts are disabled' do + message = create(:message, account: account, inbox: conversation.inbox, conversation: conversation, message_type: :incoming) + event = Events::Base.new('message.created', Time.zone.now, message: message) + + expect(message).not_to receive(:conversation) + + listener.message_created(event) + + expect(Conversations::UnreadCounts::Notifier).not_to have_received(:new) + end + + it 'refreshes unread counts when conversation status changes' do + changed_attributes = { 'status' => %w[open resolved] } + event = Events::Base.new('conversation.status_changed', Time.zone.now, conversation: conversation, changed_attributes: changed_attributes) + + listener.conversation_status_changed(event) + + expect(Conversations::UnreadCounts::Notifier).to have_received(:new).with(conversation, changed_attributes: changed_attributes) + expect(notifier).to have_received(:perform) + end + + it 'refreshes unread counts when labels change' do + changed_attributes = { label_list: [%w[old], %w[new]] } + event = Events::Base.new('conversation.updated', Time.zone.now, conversation: conversation, changed_attributes: changed_attributes) + + listener.conversation_updated(event) + + expect(Conversations::UnreadCounts::Notifier).to have_received(:new).with(conversation, changed_attributes: changed_attributes) + expect(notifier).to have_received(:perform) + end + + it 'ignores conversation updates unrelated to unread count dimensions' do + event = Events::Base.new('conversation.updated', Time.zone.now, conversation: conversation, changed_attributes: { priority: [nil, 'high'] }) + + listener.conversation_updated(event) + + expect(Conversations::UnreadCounts::Notifier).not_to have_received(:new) + end + + it 'refreshes unread counts when assignee changes' do + changed_attributes = { assignee_id: [nil, 1] } + event = Events::Base.new('assignee.changed', Time.zone.now, conversation: conversation, changed_attributes: changed_attributes) + + listener.assignee_changed(event) + + expect(Conversations::UnreadCounts::Notifier).to have_received(:new).with(conversation, changed_attributes: changed_attributes) + expect(notifier).to have_received(:perform) + end + + it 'refreshes unread counts when team changes' do + changed_attributes = { team_id: [nil, 1] } + event = Events::Base.new('team.changed', Time.zone.now, conversation: conversation, changed_attributes: changed_attributes) + + listener.team_changed(event) + + expect(Conversations::UnreadCounts::Notifier).to have_received(:new).with(conversation, changed_attributes: changed_attributes) + expect(notifier).to have_received(:perform) + end + + it 'removes unread count memberships when a conversation is deleted' do + account.enable_features!(:conversation_unread_counts) + label = create(:label, account: account) + team = create(:team, account: account) + assignee = create(:user, account: account) + create(:team_member, team: team, user: assignee) + conversation.update!(assignee_id: assignee.id, team: team) + conversation.update_labels([label.title]) + conversation.reload + conversation_data = deleted_conversation_data(conversation) + store.mark_base_ready!(account.id) + store.mark_assignment_ready!(account.id) + store.add_base_membership( + account_id: account.id, + inbox_id: conversation.inbox_id, + label_ids: [label.id], + team_id: team.id, + conversation_id: conversation.id + ) + store.add_assignment_membership( + account_id: account.id, + inbox_id: conversation.inbox_id, + label_ids: [label.id], + assignee_id: assignee.id, + team_id: team.id, + conversation_id: conversation.id + ) + allow(Rails.configuration.dispatcher).to receive(:dispatch) + + listener.conversation_deleted(Events::Base.new('conversation.deleted', Time.zone.now, conversation_data: conversation_data)) + + expect(store.counts_for_keys(deleted_base_keys(conversation, label, team)).values).to all(eq(0)) + expect(store.counts_for_keys(deleted_assignment_keys(conversation, label, team, assignee)).values).to all(eq(0)) + expect(Rails.configuration.dispatcher).to have_received(:dispatch).with( + 'conversation.unread_count_changed', + kind_of(Time), + conversation_data: conversation_data.stringify_keys + ) + ensure + store.clear_account!(account.id) + end + + def deleted_conversation_data(conversation) + { + id: conversation.id, + account_id: conversation.account_id, + inbox_id: conversation.inbox_id, + assignee_id: conversation.assignee_id, + team_id: conversation.team_id, + cached_label_list: conversation.cached_label_list + } + end + + def deleted_base_keys(conversation, label, team) + [ + store.inbox_key(account.id, conversation.inbox_id), + store.label_inbox_key(account.id, label.id, conversation.inbox_id), + store.team_inbox_key(account.id, team.id, conversation.inbox_id) + ] + end + + def deleted_assignment_keys(conversation, label, team, assignee) + [ + store.inbox_assignee_key(account.id, conversation.inbox_id, assignee.id), + store.label_inbox_assignee_key(account.id, label.id, conversation.inbox_id, assignee.id), + store.team_inbox_assignee_key(account.id, team.id, conversation.inbox_id, assignee.id) + ] + end + + def store + Conversations::UnreadCounts::Store + end +end diff --git a/spec/services/conversations/unread_counts/notifier_spec.rb b/spec/services/conversations/unread_counts/notifier_spec.rb new file mode 100644 index 000000000..1b35d37f6 --- /dev/null +++ b/spec/services/conversations/unread_counts/notifier_spec.rb @@ -0,0 +1,48 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Notifier do + let!(:conversation) { create(:conversation) } + let(:refresher) { instance_double(Conversations::UnreadCounts::Refresher, perform: refresh_result) } + let(:refresh_result) { true } + + before do + conversation.account.enable_features!(:conversation_unread_counts) + allow(Conversations::UnreadCounts::Refresher).to receive(:new).and_return(refresher) + allow(Rails.configuration.dispatcher).to receive(:dispatch) + end + + it 'dispatches unread count changed event after a successful refresh' do + described_class.new(conversation).perform + + expect(Rails.configuration.dispatcher).to have_received(:dispatch).with( + 'conversation.unread_count_changed', + kind_of(Time), + conversation: conversation + ) + end + + context 'when refresh does not change unread count memberships' do + let(:refresh_result) { false } + + it 'does not dispatch unread count changed event' do + described_class.new(conversation).perform + + expect(Rails.configuration.dispatcher).not_to have_received(:dispatch) + end + end + + context 'when conversation unread counts feature is disabled' do + before do + conversation.account.disable_features!(:conversation_unread_counts) + allow(Conversations::UnreadCounts::Store).to receive(:clear_account!) + end + + it 'does not refresh, clear cache, or dispatch unread count changed event' do + described_class.new(conversation).perform + + expect(Conversations::UnreadCounts::Refresher).not_to have_received(:new) + expect(Conversations::UnreadCounts::Store).not_to have_received(:clear_account!) + expect(Rails.configuration.dispatcher).not_to have_received(:dispatch) + end + end +end diff --git a/spec/services/conversations/unread_counts/refresher_spec.rb b/spec/services/conversations/unread_counts/refresher_spec.rb new file mode 100644 index 000000000..9b22c2186 --- /dev/null +++ b/spec/services/conversations/unread_counts/refresher_spec.rb @@ -0,0 +1,145 @@ +require 'rails_helper' + +RSpec.describe Conversations::UnreadCounts::Refresher do + let(:account) { create(:account) } + let(:inbox) { create(:inbox, account: account) } + let(:label) { create(:label, account: account, title: 'urgent', show_on_sidebar: true) } + let(:new_label) { create(:label, account: account, title: 'billing', show_on_sidebar: true) } + let(:team) { create(:team, account: account, allow_auto_assign: false) } + let(:new_team) { create(:team, account: account, allow_auto_assign: false) } + let(:assignee) { create(:user, account: account, role: :agent) } + let(:other_assignee) { create(:user, account: account, role: :agent) } + let(:store) { Conversations::UnreadCounts::Store } + + after do + store.clear_account!(account.id) + end + + it 'does not update redis when unread caches are not ready' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title]) + + expect(described_class.new(conversation).perform).to be(false) + expect(store.counts_for_keys([store.inbox_key(account.id, inbox.id)])).to eq(store.inbox_key(account.id, inbox.id) => 0) + end + + it 'adds an unread conversation to base cache' do + conversation = create(:conversation, account: account, inbox: inbox, agent_last_seen_at: 1.hour.ago) + conversation.update_labels([label.title]) + store.mark_base_ready!(account.id) + + create(:message, account: account, inbox: inbox, conversation: conversation, message_type: :incoming, created_at: 5.minutes.ago) + + expect(described_class.new(conversation.reload).perform).to be(true) + + expect(store.counts_for_keys(base_keys)).to eq( + store.inbox_key(account.id, inbox.id) => 1, + store.label_inbox_key(account.id, label.id, inbox.id) => 1 + ) + end + + it 'returns false when refresh does not change unread counts' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title]) + Conversations::UnreadCounts::Builder.new(account).build_base! + + create(:message, account: account, inbox: inbox, conversation: conversation, message_type: :incoming) + + expect(described_class.new(conversation.reload).perform).to be(false) + expect(store.counts_for_keys(base_keys)).to eq( + store.inbox_key(account.id, inbox.id) => 1, + store.label_inbox_key(account.id, label.id, inbox.id) => 1 + ) + end + + it 'removes a conversation from base cache when it becomes read' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title]) + Conversations::UnreadCounts::Builder.new(account).build_base! + + conversation.update!(agent_last_seen_at: 1.minute.from_now) + expect(described_class.new(conversation.reload).perform).to be(true) + + expect(store.counts_for_keys(base_keys).values).to all(eq(0)) + end + + it 'moves base label membership when labels change' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title]) + Conversations::UnreadCounts::Builder.new(account).build_base! + + conversation.update_labels([new_label.title]) + expect(described_class.new(conversation.reload, changed_attributes: { label_list: [[label.title], [new_label.title]] }).perform).to be(true) + + expect(store.counts_for_keys([ + store.label_inbox_key(account.id, label.id, inbox.id), + store.label_inbox_key(account.id, new_label.id, inbox.id) + ])).to eq( + store.label_inbox_key(account.id, label.id, inbox.id) => 0, + store.label_inbox_key(account.id, new_label.id, inbox.id) => 1 + ) + end + + it 'moves base team membership when team changes' do + conversation = create_unread_conversation(account: account, inbox: inbox, team: team) + Conversations::UnreadCounts::Builder.new(account).build_base! + + conversation.update!(team: new_team) + expect(described_class.new(conversation.reload, changed_attributes: { team_id: [team.id, new_team.id] }).perform).to be(true) + + expect(store.counts_for_keys([ + store.team_inbox_key(account.id, team.id, inbox.id), + store.team_inbox_key(account.id, new_team.id, inbox.id) + ])).to eq( + store.team_inbox_key(account.id, team.id, inbox.id) => 0, + store.team_inbox_key(account.id, new_team.id, inbox.id) => 1 + ) + end + + it 'moves assignment-aware membership when assignee changes' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: assignee) + Conversations::UnreadCounts::Builder.new(account).build_assignment! + + conversation.update!(assignee: other_assignee) + described_class.new(conversation.reload, changed_attributes: { assignee_id: [assignee.id, other_assignee.id] }).perform + + expect(store.counts_for_keys([ + store.inbox_assignee_key(account.id, inbox.id, assignee.id), + store.inbox_assignee_key(account.id, inbox.id, other_assignee.id) + ])).to eq( + store.inbox_assignee_key(account.id, inbox.id, assignee.id) => 0, + store.inbox_assignee_key(account.id, inbox.id, other_assignee.id) => 1 + ) + end + + it 'does not remove unassigned membership when assignee did not change' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title], assignee: assignee) + Conversations::UnreadCounts::Builder.new(account).build_assignment! + allow(store).to receive(:remove_assignment_membership).and_call_original + + conversation.update_labels([new_label.title]) + described_class.new(conversation.reload, changed_attributes: { label_list: [[label.title], [new_label.title]] }).perform + + expect(store).to have_received(:remove_assignment_membership).with(hash_including(assignee_ids: [assignee.id])) + end + + it 'moves assignment-aware team membership when team changes' do + create(:team_member, user: assignee, team: new_team) + conversation = create_unread_conversation(account: account, inbox: inbox, assignee: assignee, team: team) + Conversations::UnreadCounts::Builder.new(account).build_assignment! + + conversation.update!(team: new_team) + described_class.new(conversation.reload, changed_attributes: { team_id: [team.id, new_team.id] }).perform + + expect(store.counts_for_keys([ + store.team_inbox_assignee_key(account.id, team.id, inbox.id, assignee.id), + store.team_inbox_assignee_key(account.id, new_team.id, inbox.id, assignee.id) + ])).to eq( + store.team_inbox_assignee_key(account.id, team.id, inbox.id, assignee.id) => 0, + store.team_inbox_assignee_key(account.id, new_team.id, inbox.id, assignee.id) => 1 + ) + end + + def base_keys + [ + store.inbox_key(account.id, inbox.id), + store.label_inbox_key(account.id, label.id, inbox.id) + ] + end +end diff --git a/spec/services/conversations/unread_counts/store_spec.rb b/spec/services/conversations/unread_counts/store_spec.rb index 652c74e46..fcf21c985 100644 --- a/spec/services/conversations/unread_counts/store_spec.rb +++ b/spec/services/conversations/unread_counts/store_spec.rb @@ -90,6 +90,23 @@ RSpec.describe Conversations::UnreadCounts::Store do expect(described_class.counts_for_keys(base_keys).values).to all(eq(0)) end + it 'checks memberships for a conversation across keys' do + described_class.add_base_membership( + account_id: account_id, + inbox_id: inbox_id, + label_ids: [label_id], + team_id: team_id, + conversation_id: conversation_id + ) + + expect(described_class.memberships_for_keys(base_keys, conversation_id)).to eq( + described_class.inbox_key(account_id, inbox_id) => true, + described_class.label_inbox_key(account_id, label_id, inbox_id) => true, + described_class.team_inbox_key(account_id, team_id, inbox_id) => true + ) + expect(described_class.memberships_for_keys(base_keys, 999).values).to all(be(false)) + end + it 'adds, counts, and removes assignment-aware memberships' do described_class.add_assignment_membership( account_id: account_id, From f33e469e9a2fc4dc417031c4598c7e0463efd721 Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Wed, 20 May 2026 19:21:25 +0530 Subject: [PATCH 4/8] feat: Unread Count: Frontend changes for showing unread count badges (3/3)[CW-6851] (#14372) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request Template ## Description This is the third and final PR in a series of PRs for Introducing unread counts in the sidebar for inboxes and labels. In this PR: * Added frontend changes to show the badges for unread counts for Inboxes and Labels * Added specs for the changes Issue: https://linear.app/chatwoot/issue/CW-6851/support-unread-conversation-counts ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality not to work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Tested this locally. Cases to test: * Send a message from the widget and see if the count changes * Mark a conversation as unread and see the count change for inbox * Open an unread conversation as agent and see the count go down * Add a label to an unread conversation from sidebar right click action without opening the conversation and see the count of un-reads on the label change Added the screenshot of how it will look like Screenshot 2026-05-05 at 7 00 11 PM ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Sojan Jose --- app/javascript/dashboard/api/conversations.js | 4 + .../dashboard/api/specs/conversations.spec.js | 8 ++ .../components-next/sidebar/ChannelLeaf.vue | 6 ++ .../components-next/sidebar/Sidebar.vue | 37 ++++++- .../sidebar/SidebarCollapsedPopover.vue | 3 + .../sidebar/SidebarGroupLeaf.vue | 7 +- .../sidebar/SidebarUnreadBadge.vue | 27 +++++ .../sidebar/specs/ChannelLeaf.spec.js | 38 +++++++ .../sidebar/specs/SidebarGroupLeaf.spec.js | 76 ++++++++++++++ app/javascript/dashboard/featureFlags.js | 1 + .../dashboard/helper/actionCable.js | 56 +++++++++++ .../helper/specs/actionCable.spec.js | 98 ++++++++++++++++++- app/javascript/dashboard/store/index.js | 2 + .../store/modules/conversationUnreadCounts.js | 70 +++++++++++++ .../conversationUnreadCounts/actions.spec.js | 53 ++++++++++ .../conversationUnreadCounts/getters.spec.js | 51 ++++++++++ .../mutations.spec.js | 48 +++++++++ .../dashboard/store/mutation-types.js | 3 + app/models/account.rb | 5 + .../conversations/unread_counts/refresher.rb | 2 +- .../unread_counts/refresher_spec.rb | 37 +++++++ 21 files changed, 626 insertions(+), 6 deletions(-) create mode 100644 app/javascript/dashboard/components-next/sidebar/SidebarUnreadBadge.vue create mode 100644 app/javascript/dashboard/components-next/sidebar/specs/ChannelLeaf.spec.js create mode 100644 app/javascript/dashboard/components-next/sidebar/specs/SidebarGroupLeaf.spec.js create mode 100644 app/javascript/dashboard/store/modules/conversationUnreadCounts.js create mode 100644 app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/actions.spec.js create mode 100644 app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/getters.spec.js create mode 100644 app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/mutations.spec.js diff --git a/app/javascript/dashboard/api/conversations.js b/app/javascript/dashboard/api/conversations.js index 876103694..1de9aee29 100644 --- a/app/javascript/dashboard/api/conversations.js +++ b/app/javascript/dashboard/api/conversations.js @@ -13,6 +13,10 @@ class ConversationApi extends ApiClient { updateLabels(conversationID, labels) { return axios.post(`${this.url}/${conversationID}/labels`, { labels }); } + + getUnreadCounts() { + return axios.get(`${this.url}/unread_counts`); + } } export default new ConversationApi(); diff --git a/app/javascript/dashboard/api/specs/conversations.spec.js b/app/javascript/dashboard/api/specs/conversations.spec.js index 7ae4eb774..686db4098 100644 --- a/app/javascript/dashboard/api/specs/conversations.spec.js +++ b/app/javascript/dashboard/api/specs/conversations.spec.js @@ -11,6 +11,7 @@ describe('#ConversationApi', () => { expect(conversationsAPI).toHaveProperty('delete'); expect(conversationsAPI).toHaveProperty('getLabels'); expect(conversationsAPI).toHaveProperty('updateLabels'); + expect(conversationsAPI).toHaveProperty('getUnreadCounts'); }); describe('API calls', () => { @@ -47,5 +48,12 @@ describe('#ConversationApi', () => { } ); }); + + it('#getUnreadCounts', () => { + conversationsAPI.getUnreadCounts(); + expect(axiosMock.get).toHaveBeenCalledWith( + '/api/v1/conversations/unread_counts' + ); + }); }); }); diff --git a/app/javascript/dashboard/components-next/sidebar/ChannelLeaf.vue b/app/javascript/dashboard/components-next/sidebar/ChannelLeaf.vue index 83d1d4e20..a995cf510 100644 --- a/app/javascript/dashboard/components-next/sidebar/ChannelLeaf.vue +++ b/app/javascript/dashboard/components-next/sidebar/ChannelLeaf.vue @@ -2,6 +2,7 @@ import { computed } from 'vue'; import Icon from 'next/icon/Icon.vue'; import ChannelIcon from 'next/icon/ChannelIcon.vue'; +import SidebarUnreadBadge from './SidebarUnreadBadge.vue'; const props = defineProps({ label: { @@ -17,6 +18,10 @@ const props = defineProps({ type: Object, required: true, }, + badgeCount: { + type: [Number, String], + default: 0, + }, }); const reauthorizationRequired = computed(() => { @@ -29,6 +34,7 @@ const reauthorizationRequired = computed(() => {
{{ label }}
+
-import { h, ref, computed, onMounted } from 'vue'; +import { h, ref, computed, onMounted, watch } from 'vue'; import { provideSidebarContext, useSidebarResize } from './provider'; import { useAccount } from 'dashboard/composables/useAccount'; import { useKbd } from 'dashboard/composables/utils/useKbd'; @@ -61,6 +61,24 @@ const hasAdvancedAssignment = computed(() => { ); }); +const hasConversationUnreadCounts = computed(() => { + return isFeatureEnabledonAccount.value( + accountId.value, + FEATURE_FLAGS.CONVERSATION_UNREAD_COUNTS + ); +}); + +const fetchConversationUnreadCounts = ([currentAccountId, isEnabled]) => { + if (!currentAccountId) return; + + if (!isEnabled) { + store.dispatch('conversationUnreadCounts/clear'); + return; + } + + store.dispatch('conversationUnreadCounts/get'); +}; + const toggleShortcutModalFn = show => { if (show) { emit('openKeyShortcutModal'); @@ -157,6 +175,15 @@ useEventListener(document, 'touchend', onResizeEnd); const inboxes = useMapGetter('inboxes/getInboxes'); const labels = useMapGetter('labels/getLabelsOnSidebar'); +const getInboxUnreadCount = useMapGetter( + 'conversationUnreadCounts/getInboxUnreadCount' +); +const getLabelUnreadCount = useMapGetter( + 'conversationUnreadCounts/getLabelUnreadCount' +); +const getTeamUnreadCount = useMapGetter( + 'conversationUnreadCounts/getTeamUnreadCount' +); const teams = useMapGetter('teams/getMyTeams'); const contactCustomViews = useMapGetter('customViews/getContactCustomViews'); const conversationCustomViews = useMapGetter( @@ -173,6 +200,10 @@ onMounted(() => { store.dispatch('customViews/get', 'contact'); }); +watch([accountId, hasConversationUnreadCounts], fetchConversationUnreadCounts, { + immediate: true, +}); + const sortedInboxes = computed(() => inboxes.value.slice().sort((a, b) => a.name.localeCompare(b.name)) ); @@ -270,6 +301,7 @@ const menuItems = computed(() => { children: teams.value.map(team => ({ name: `${team.name}-${team.id}`, label: team.name, + badgeCount: getTeamUnreadCount.value(team.id), to: accountScopedRoute('team_conversations', { teamId: team.id }), })), }, @@ -281,6 +313,7 @@ const menuItems = computed(() => { children: sortedInboxes.value.map(inbox => ({ name: `${inbox.name}-${inbox.id}`, label: inbox.name, + badgeCount: getInboxUnreadCount.value(inbox.id), icon: h(ChannelIcon, { inbox, class: 'size-[16px]' }), to: accountScopedRoute('inbox_dashboard', { inbox_id: inbox.id }), component: leafProps => @@ -288,6 +321,7 @@ const menuItems = computed(() => { label: leafProps.label, active: leafProps.active, inbox, + badgeCount: leafProps.badgeCount, }), })), }, @@ -299,6 +333,7 @@ const menuItems = computed(() => { children: labels.value.map(label => ({ name: `${label.title}-${label.id}`, label: label.title, + badgeCount: getLabelUnreadCount.value(label.id), icon: h('span', { class: `size-[8px] rounded-sm`, style: { backgroundColor: label.color }, diff --git a/app/javascript/dashboard/components-next/sidebar/SidebarCollapsedPopover.vue b/app/javascript/dashboard/components-next/sidebar/SidebarCollapsedPopover.vue index 1beea47df..18e1aea23 100644 --- a/app/javascript/dashboard/components-next/sidebar/SidebarCollapsedPopover.vue +++ b/app/javascript/dashboard/components-next/sidebar/SidebarCollapsedPopover.vue @@ -5,6 +5,7 @@ import { useSidebarContext } from './provider'; import { useMapGetter } from 'dashboard/composables/store'; import Icon from 'next/icon/Icon.vue'; import TeleportWithDirection from 'dashboard/components-next/TeleportWithDirection.vue'; +import SidebarUnreadBadge from './SidebarUnreadBadge.vue'; const props = defineProps({ label: { type: String, required: true }, @@ -166,6 +167,7 @@ onMounted(async () => { class="size-4 flex-shrink-0" /> {{ subChild.label }} + @@ -188,6 +190,7 @@ onMounted(async () => { class="size-4 flex-shrink-0" /> {{ child.label }} + diff --git a/app/javascript/dashboard/components-next/sidebar/SidebarGroupLeaf.vue b/app/javascript/dashboard/components-next/sidebar/SidebarGroupLeaf.vue index b1b8dd740..31e41a6c2 100644 --- a/app/javascript/dashboard/components-next/sidebar/SidebarGroupLeaf.vue +++ b/app/javascript/dashboard/components-next/sidebar/SidebarGroupLeaf.vue @@ -3,6 +3,7 @@ import { isVNode, computed } from 'vue'; import Icon from 'next/icon/Icon.vue'; import Policy from 'dashboard/components/policy.vue'; import { useSidebarContext } from './provider'; +import SidebarUnreadBadge from './SidebarUnreadBadge.vue'; const props = defineProps({ label: { type: String, required: true }, @@ -10,6 +11,7 @@ const props = defineProps({ icon: { type: [String, Object], default: null }, active: { type: Boolean, default: false }, component: { type: Function, default: null }, + badgeCount: { type: [Number, String], default: 0 }, }); const { resolvePermissions, resolveFeatureFlag } = useSidebarContext(); @@ -39,15 +41,14 @@ const shouldRenderComponent = computed(() => { diff --git a/app/javascript/dashboard/components-next/sidebar/SidebarUnreadBadge.vue b/app/javascript/dashboard/components-next/sidebar/SidebarUnreadBadge.vue new file mode 100644 index 000000000..d9329fdaf --- /dev/null +++ b/app/javascript/dashboard/components-next/sidebar/SidebarUnreadBadge.vue @@ -0,0 +1,27 @@ + + + diff --git a/app/javascript/dashboard/components-next/sidebar/specs/ChannelLeaf.spec.js b/app/javascript/dashboard/components-next/sidebar/specs/ChannelLeaf.spec.js new file mode 100644 index 000000000..03d963134 --- /dev/null +++ b/app/javascript/dashboard/components-next/sidebar/specs/ChannelLeaf.spec.js @@ -0,0 +1,38 @@ +import { mount } from '@vue/test-utils'; +import ChannelLeaf from '../ChannelLeaf.vue'; + +const mountChannelLeaf = props => + mount(ChannelLeaf, { + props: { + label: 'Website', + inbox: { reauthorization_required: false }, + ...props, + }, + global: { + mocks: { + $t: key => key, + }, + stubs: { + ChannelIcon: true, + Icon: true, + }, + }, + }); + +describe('ChannelLeaf', () => { + it('renders unread badge when count is present', () => { + const wrapper = mountChannelLeaf({ badgeCount: 3 }); + const badge = wrapper.find('[data-test-id="sidebar-unread-badge"]'); + + expect(badge.exists()).toBe(true); + expect(badge.text()).toBe('3'); + }); + + it('does not render unread badge when count is zero', () => { + const wrapper = mountChannelLeaf({ badgeCount: 0 }); + + expect(wrapper.find('[data-test-id="sidebar-unread-badge"]').exists()).toBe( + false + ); + }); +}); diff --git a/app/javascript/dashboard/components-next/sidebar/specs/SidebarGroupLeaf.spec.js b/app/javascript/dashboard/components-next/sidebar/specs/SidebarGroupLeaf.spec.js new file mode 100644 index 000000000..79b62d191 --- /dev/null +++ b/app/javascript/dashboard/components-next/sidebar/specs/SidebarGroupLeaf.spec.js @@ -0,0 +1,76 @@ +import { mount } from '@vue/test-utils'; +import { h } from 'vue'; +import SidebarGroupLeaf from '../SidebarGroupLeaf.vue'; + +vi.mock('../provider', () => ({ + useSidebarContext: () => ({ + resolvePermissions: () => [], + resolveFeatureFlag: () => '', + }), +})); + +const PolicyStub = { + props: ['as', 'permissions', 'featureFlag'], + template: '
    • ', +}; + +const RouterLinkStub = { + props: ['to'], + template: '', +}; + +const mountLeaf = props => + mount(SidebarGroupLeaf, { + props: { + label: 'Support', + to: '/support', + ...props, + }, + global: { + stubs: { + Icon: true, + Policy: PolicyStub, + RouterLink: RouterLinkStub, + }, + }, + }); + +describe('SidebarGroupLeaf', () => { + it('renders unread badge when count is present', () => { + const wrapper = mountLeaf({ badgeCount: 7 }); + const badge = wrapper.find('[data-test-id="sidebar-unread-badge"]'); + + expect(badge.exists()).toBe(true); + expect(badge.text()).toBe('7'); + }); + + it('does not render unread badge when count is zero', () => { + const wrapper = mountLeaf({ badgeCount: 0 }); + + expect(wrapper.find('[data-test-id="sidebar-unread-badge"]').exists()).toBe( + false + ); + }); + + it('caps large unread counts', () => { + const wrapper = mountLeaf({ badgeCount: 120 }); + + expect(wrapper.find('[data-test-id="sidebar-unread-badge"]').text()).toBe( + '99+' + ); + }); + + it('passes unread count to custom leaf components', () => { + const wrapper = mountLeaf({ + badgeCount: 4, + component: leafProps => + h( + 'span', + { 'data-test-id': 'custom-leaf-count' }, + leafProps.badgeCount + ), + }); + + expect(wrapper.find('[data-test-id="custom-leaf-count"]').text()).toBe('4'); + }); +}); diff --git a/app/javascript/dashboard/featureFlags.js b/app/javascript/dashboard/featureFlags.js index b97e30984..00a79763b 100644 --- a/app/javascript/dashboard/featureFlags.js +++ b/app/javascript/dashboard/featureFlags.js @@ -46,6 +46,7 @@ export const FEATURE_FLAGS = { COMPANIES: 'companies', ADVANCED_SEARCH: 'advanced_search', CONVERSATION_REQUIRED_ATTRIBUTES: 'conversation_required_attributes', + CONVERSATION_UNREAD_COUNTS: 'conversation_unread_counts', }; export const PREMIUM_FEATURES = [ diff --git a/app/javascript/dashboard/helper/actionCable.js b/app/javascript/dashboard/helper/actionCable.js index 6feebb35d..74b82105c 100644 --- a/app/javascript/dashboard/helper/actionCable.js +++ b/app/javascript/dashboard/helper/actionCable.js @@ -4,14 +4,18 @@ import DashboardAudioNotificationHelper from './AudioAlerts/DashboardAudioNotifi import { BUS_EVENTS } from 'shared/constants/busEvents'; import { emitter } from 'shared/helpers/mitt'; import { useImpersonation } from 'dashboard/composables/useImpersonation'; +import { FEATURE_FLAGS } from 'dashboard/featureFlags'; const { isImpersonating } = useImpersonation(); +const UNREAD_COUNTS_REFETCH_THROTTLE_MS = 5000; class ActionCableConnector extends BaseActionCableConnector { constructor(app, pubsubToken) { const { websocketURL = '' } = window.chatwootConfig || {}; super(app, pubsubToken, websocketURL); this.CancelTyping = []; + this.lastUnreadCountsFetchAt = null; + this.unreadCountsFetchTimer = null; this.events = { 'message.created': this.onMessageCreated, 'message.updated': this.onMessageUpdated, @@ -32,6 +36,8 @@ class ActionCableConnector extends BaseActionCableConnector { 'notification.updated': this.onNotificationUpdated, 'conversation.read': this.onConversationRead, 'conversation.updated': this.onConversationUpdated, + 'conversation.unread_count_changed': + this.onConversationUnreadCountChanged, 'account.cache_invalidated': this.onCacheInvalidate, 'account.enrichment_completed': this.onEnrichmentCompleted, 'copilot.message.created': this.onCopilotMessageCreated, @@ -120,6 +126,56 @@ class ActionCableConnector extends BaseActionCableConnector { this.fetchConversationStats(); }; + onConversationUnreadCountChanged = () => { + this.throttledFetchConversationUnreadCounts(); + }; + + throttledFetchConversationUnreadCounts = () => { + const now = Date.now(); + const elapsedTime = now - this.lastUnreadCountsFetchAt; + + if ( + this.lastUnreadCountsFetchAt === null || + elapsedTime >= UNREAD_COUNTS_REFETCH_THROTTLE_MS + ) { + this.clearUnreadCountsFetchTimer(); + this.fetchConversationUnreadCounts(); + return; + } + + if (this.unreadCountsFetchTimer) return; + + this.unreadCountsFetchTimer = setTimeout(() => { + this.unreadCountsFetchTimer = null; + this.fetchConversationUnreadCounts(); + }, UNREAD_COUNTS_REFETCH_THROTTLE_MS - elapsedTime); + }; + + clearUnreadCountsFetchTimer = () => { + if (!this.unreadCountsFetchTimer) return; + + clearTimeout(this.unreadCountsFetchTimer); + this.unreadCountsFetchTimer = null; + }; + + fetchConversationUnreadCounts = () => { + if (!this.isConversationUnreadCountsEnabled()) return; + + this.lastUnreadCountsFetchAt = Date.now(); + this.app.$store.dispatch('conversationUnreadCounts/get'); + }; + + isConversationUnreadCountsEnabled = () => { + const accountId = this.app.$store.getters.getCurrentAccountId; + const isFeatureEnabled = + this.app.$store.getters['accounts/isFeatureEnabledonAccount']; + + return isFeatureEnabled?.( + accountId, + FEATURE_FLAGS.CONVERSATION_UNREAD_COUNTS + ); + }; + onTypingOn = ({ conversation, user }) => { const conversationId = conversation.id; diff --git a/app/javascript/dashboard/helper/specs/actionCable.spec.js b/app/javascript/dashboard/helper/specs/actionCable.spec.js index 4ad8a52c6..8ba411a5f 100644 --- a/app/javascript/dashboard/helper/specs/actionCable.spec.js +++ b/app/javascript/dashboard/helper/specs/actionCable.spec.js @@ -1,4 +1,4 @@ -import { describe, it, beforeEach, expect, vi } from 'vitest'; +import { describe, it, beforeEach, afterEach, expect, vi } from 'vitest'; import ActionCableConnector from '../actionCable'; vi.mock('shared/helpers/mitt', () => ({ @@ -30,12 +30,17 @@ describe('ActionCableConnector - Copilot Tests', () => { dispatch: mockDispatch, getters: { getCurrentAccountId: 1, + 'accounts/isFeatureEnabledonAccount': vi.fn(() => true), }, }, }; actionCable = ActionCableConnector.init(store.$store, 'test-token'); }); + + afterEach(() => { + vi.useRealTimers(); + }); describe('copilot event handlers', () => { it('should register the copilot.message.created event handler', () => { expect(Object.keys(actionCable.events)).toContain( @@ -64,4 +69,95 @@ describe('ActionCableConnector - Copilot Tests', () => { ); }); }); + + describe('conversation unread count event handlers', () => { + it('should register the conversation.unread_count_changed event handler', () => { + expect(Object.keys(actionCable.events)).toContain( + 'conversation.unread_count_changed' + ); + expect(actionCable.events['conversation.unread_count_changed']).toBe( + actionCable.onConversationUnreadCountChanged + ); + }); + + it('should refetch unread counts when unread count changes', () => { + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + + expect(mockDispatch).toHaveBeenCalledWith('conversationUnreadCounts/get'); + }); + + it('does not refetch unread counts when unread count feature is disabled', () => { + store.$store.getters[ + 'accounts/isFeatureEnabledonAccount' + ].mockReturnValue(false); + + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + + expect(mockDispatch).not.toHaveBeenCalledWith( + 'conversationUnreadCounts/get' + ); + }); + + it('should throttle unread count refetches for repeated events', () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-01-01T00:00:00Z')); + + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + + expect(mockDispatch).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(4999); + expect(mockDispatch).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(1); + expect(mockDispatch).toHaveBeenCalledTimes(2); + expect(mockDispatch).toHaveBeenLastCalledWith( + 'conversationUnreadCounts/get' + ); + }); + + it('clears pending unread count refetch before immediate refetch', () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-01-01T00:00:00Z')); + + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + + vi.advanceTimersByTime(1000); + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + + vi.setSystemTime(new Date('2026-01-01T00:00:06Z')); + actionCable.onReceived({ + event: 'conversation.unread_count_changed', + data: { account_id: 1 }, + }); + + expect(mockDispatch).toHaveBeenCalledTimes(2); + + vi.advanceTimersByTime(4000); + expect(mockDispatch).toHaveBeenCalledTimes(2); + }); + }); }); diff --git a/app/javascript/dashboard/store/index.js b/app/javascript/dashboard/store/index.js index d56958eb5..054a823c6 100755 --- a/app/javascript/dashboard/store/index.js +++ b/app/javascript/dashboard/store/index.js @@ -25,6 +25,7 @@ import conversations from './modules/conversations'; import conversationSearch from './modules/conversationSearch'; import conversationStats from './modules/conversationStats'; import conversationTypingStatus from './modules/conversationTypingStatus'; +import conversationUnreadCounts from './modules/conversationUnreadCounts'; import conversationWatchers from './modules/conversationWatchers'; import csat from './modules/csat'; import customRole from './modules/customRole'; @@ -88,6 +89,7 @@ export default createStore({ conversationSearch, conversationStats, conversationTypingStatus, + conversationUnreadCounts, conversationWatchers, csat, customRole, diff --git a/app/javascript/dashboard/store/modules/conversationUnreadCounts.js b/app/javascript/dashboard/store/modules/conversationUnreadCounts.js new file mode 100644 index 000000000..0503c0806 --- /dev/null +++ b/app/javascript/dashboard/store/modules/conversationUnreadCounts.js @@ -0,0 +1,70 @@ +import ConversationAPI from '../../api/conversations'; +import types from '../mutation-types'; + +export const state = { + inboxes: {}, + labels: {}, + teams: {}, +}; + +const normalizeCounts = counts => { + return Object.entries(counts || {}).reduce((result, [id, count]) => { + const parsedCount = Number(count); + if (Number.isFinite(parsedCount) && parsedCount > 0) { + result[String(id)] = parsedCount; + } + + return result; + }, {}); +}; + +export const getters = { + getInboxUnreadCount: $state => inboxId => { + return $state.inboxes[String(inboxId)] || 0; + }, + getLabelUnreadCount: $state => labelId => { + return $state.labels[String(labelId)] || 0; + }, + getTeamUnreadCount: $state => teamId => { + return $state.teams[String(teamId)] || 0; + }, + getInboxUnreadCounts($state) { + return $state.inboxes; + }, + getLabelUnreadCounts($state) { + return $state.labels; + }, + getTeamUnreadCounts($state) { + return $state.teams; + }, +}; + +export const actions = { + get: async function getUnreadCounts({ commit }) { + try { + const response = await ConversationAPI.getUnreadCounts(); + commit(types.SET_CONVERSATION_UNREAD_COUNTS, response.data.payload); + } catch (error) { + // Ignore errors so the sidebar can continue rendering without badges. + } + }, + clear({ commit }) { + commit(types.SET_CONVERSATION_UNREAD_COUNTS, {}); + }, +}; + +export const mutations = { + [types.SET_CONVERSATION_UNREAD_COUNTS]($state, payload = {}) { + $state.inboxes = normalizeCounts(payload.inboxes); + $state.labels = normalizeCounts(payload.labels); + $state.teams = normalizeCounts(payload.teams); + }, +}; + +export default { + namespaced: true, + state, + getters, + actions, + mutations, +}; diff --git a/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/actions.spec.js b/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/actions.spec.js new file mode 100644 index 000000000..3100cdd10 --- /dev/null +++ b/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/actions.spec.js @@ -0,0 +1,53 @@ +import axios from 'axios'; +import { actions } from '../../conversationUnreadCounts'; +import types from '../../../mutation-types'; + +const commit = vi.fn(); +global.axios = axios; +vi.mock('axios'); + +describe('#actions', () => { + beforeEach(() => { + commit.mockClear(); + axios.get.mockReset(); + }); + + describe('#get', () => { + it('commits unread counts when API is successful', async () => { + const payload = { + inboxes: { 1: '2' }, + labels: { 3: 4 }, + teams: { 5: 6 }, + }; + axios.get.mockResolvedValue({ data: { payload } }); + + await actions.get({ commit }); + + expect(axios.get).toHaveBeenCalledWith( + '/api/v1/conversations/unread_counts' + ); + expect(commit.mock.calls).toEqual([ + [types.SET_CONVERSATION_UNREAD_COUNTS, payload], + ]); + }); + + it('does not commit when API fails', async () => { + axios.get.mockRejectedValue({ message: 'Incorrect header' }); + + await actions.get({ commit }); + + expect(commit).not.toHaveBeenCalled(); + }); + }); + + describe('#clear', () => { + it('clears unread counts', () => { + actions.clear({ commit }); + + expect(commit).toHaveBeenCalledWith( + types.SET_CONVERSATION_UNREAD_COUNTS, + {} + ); + }); + }); +}); diff --git a/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/getters.spec.js b/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/getters.spec.js new file mode 100644 index 000000000..a3e74fc37 --- /dev/null +++ b/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/getters.spec.js @@ -0,0 +1,51 @@ +import { getters } from '../../conversationUnreadCounts'; + +describe('#getters', () => { + it('returns inbox unread count by id', () => { + const state = { + inboxes: { 1: 2 }, + labels: {}, + teams: {}, + }; + + expect(getters.getInboxUnreadCount(state)(1)).toBe(2); + expect(getters.getInboxUnreadCount(state)('1')).toBe(2); + expect(getters.getInboxUnreadCount(state)(2)).toBe(0); + }); + + it('returns label unread count by id', () => { + const state = { + inboxes: {}, + labels: { 3: 4 }, + teams: {}, + }; + + expect(getters.getLabelUnreadCount(state)(3)).toBe(4); + expect(getters.getLabelUnreadCount(state)('3')).toBe(4); + expect(getters.getLabelUnreadCount(state)(4)).toBe(0); + }); + + it('returns team unread count by id', () => { + const state = { + inboxes: {}, + labels: {}, + teams: { 5: 6 }, + }; + + expect(getters.getTeamUnreadCount(state)(5)).toBe(6); + expect(getters.getTeamUnreadCount(state)('5')).toBe(6); + expect(getters.getTeamUnreadCount(state)(6)).toBe(0); + }); + + it('returns unread count maps', () => { + const state = { + inboxes: { 1: 2 }, + labels: { 3: 4 }, + teams: { 5: 6 }, + }; + + expect(getters.getInboxUnreadCounts(state)).toEqual({ 1: 2 }); + expect(getters.getLabelUnreadCounts(state)).toEqual({ 3: 4 }); + expect(getters.getTeamUnreadCounts(state)).toEqual({ 5: 6 }); + }); +}); diff --git a/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/mutations.spec.js b/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/mutations.spec.js new file mode 100644 index 000000000..3f7e2b1ec --- /dev/null +++ b/app/javascript/dashboard/store/modules/specs/conversationUnreadCounts/mutations.spec.js @@ -0,0 +1,48 @@ +import types from '../../../mutation-types'; +import { mutations } from '../../conversationUnreadCounts'; + +describe('#mutations', () => { + describe('#SET_CONVERSATION_UNREAD_COUNTS', () => { + it('normalizes unread count payload', () => { + const state = { inboxes: {}, labels: {}, teams: {} }; + + mutations[types.SET_CONVERSATION_UNREAD_COUNTS](state, { + inboxes: { + 1: '2', + 2: 0, + 3: 'invalid', + }, + labels: { + 4: 5, + 5: -1, + }, + teams: { + 6: '7', + 7: 0, + }, + }); + + expect(state).toEqual({ + inboxes: { 1: 2 }, + labels: { 4: 5 }, + teams: { 6: 7 }, + }); + }); + + it('clears counts when payload is empty', () => { + const state = { + inboxes: { 1: 2 }, + labels: { 4: 5 }, + teams: { 6: 7 }, + }; + + mutations[types.SET_CONVERSATION_UNREAD_COUNTS](state, {}); + + expect(state).toEqual({ + inboxes: {}, + labels: {}, + teams: {}, + }); + }); + }); +}); diff --git a/app/javascript/dashboard/store/mutation-types.js b/app/javascript/dashboard/store/mutation-types.js index 82858b6b6..1597b7ea6 100644 --- a/app/javascript/dashboard/store/mutation-types.js +++ b/app/javascript/dashboard/store/mutation-types.js @@ -187,6 +187,9 @@ export default { CLEAR_SELECTED_CONVERSATION_IDS: 'CLEAR_SELECTED_CONVERSATION_IDS', REMOVE_SELECTED_CONVERSATION_IDS: 'REMOVE_SELECTED_CONVERSATION_IDS', + // Conversation Unread Counts + SET_CONVERSATION_UNREAD_COUNTS: 'SET_CONVERSATION_UNREAD_COUNTS', + // Reports SET_ACCOUNT_REPORTS: 'SET_ACCOUNT_REPORTS', SET_HEATMAP_DATA: 'SET_HEATMAP_DATA', diff --git a/app/models/account.rb b/app/models/account.rb index efaca8850..667058a2f 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -169,6 +169,11 @@ class Account < ApplicationRecord Redis::Alfred.exists?(enrichment_key) ? 'enrichment' : step end + def reset_cache_keys + super + clear_unread_conversation_counts_cache + end + private def notify_creation diff --git a/app/services/conversations/unread_counts/refresher.rb b/app/services/conversations/unread_counts/refresher.rb index 9456cab55..9e49ecd28 100644 --- a/app/services/conversations/unread_counts/refresher.rb +++ b/app/services/conversations/unread_counts/refresher.rb @@ -114,7 +114,7 @@ class Conversations::UnreadCounts::Refresher end def affected_assignee_ids - return [conversation.assignee_id].compact unless changed_attribute?(:assignee_id) + return [conversation.assignee_id] unless changed_attribute?(:assignee_id) [previous_value_for(:assignee_id), conversation.assignee_id].uniq end diff --git a/spec/services/conversations/unread_counts/refresher_spec.rb b/spec/services/conversations/unread_counts/refresher_spec.rb index 9b22c2186..5f361e7aa 100644 --- a/spec/services/conversations/unread_counts/refresher_spec.rb +++ b/spec/services/conversations/unread_counts/refresher_spec.rb @@ -119,6 +119,43 @@ RSpec.describe Conversations::UnreadCounts::Refresher do expect(store).to have_received(:remove_assignment_membership).with(hash_including(assignee_ids: [assignee.id])) end + it 'moves assignment-aware unassigned label membership when labels change' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title]) + Conversations::UnreadCounts::Builder.new(account).build_assignment! + + conversation.update_labels([new_label.title]) + result = described_class.new( + conversation.reload, + changed_attributes: { label_list: [[label.title], [new_label.title]] } + ).perform + + expect(result).to be(true) + expect(store.counts_for_keys([ + store.label_inbox_unassigned_key(account.id, label.id, inbox.id), + store.label_inbox_unassigned_key(account.id, new_label.id, inbox.id) + ])).to eq( + store.label_inbox_unassigned_key(account.id, label.id, inbox.id) => 0, + store.label_inbox_unassigned_key(account.id, new_label.id, inbox.id) => 1 + ) + end + + it 'removes assignment-aware unassigned membership when conversation is resolved' do + conversation = create_unread_conversation(account: account, inbox: inbox, labels: [label.title]) + Conversations::UnreadCounts::Builder.new(account).build_assignment! + + conversation.update!(status: :resolved) + result = described_class.new(conversation.reload, changed_attributes: { status: %w[open resolved] }).perform + + expect(result).to be(true) + expect(store.counts_for_keys([ + store.inbox_unassigned_key(account.id, inbox.id), + store.label_inbox_unassigned_key(account.id, label.id, inbox.id) + ])).to eq( + store.inbox_unassigned_key(account.id, inbox.id) => 0, + store.label_inbox_unassigned_key(account.id, label.id, inbox.id) => 0 + ) + end + it 'moves assignment-aware team membership when team changes' do create(:team_member, user: assignee, team: new_team) conversation = create_unread_conversation(account: account, inbox: inbox, assignee: assignee, team: team) From 3cd8cf43ce2f85af873bae65bb60e325d1f38a60 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Thu, 21 May 2026 16:14:28 +0530 Subject: [PATCH 5/8] fix: atomically claim conversation to prevent duplicate assignment (#14495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fixes a bug under Assignment V2 where a single conversation could be reassigned dozens of times in a row by the system, producing long stacks of "Assigned to X by Automation System via " activity messages alternating between agents. After this change each unassigned conversation is assigned exactly once, even on busy inboxes. ## Fixes # (issue) ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? ## How to reproduce 1. Enable `assignment_v2` on an account with at least 2 online agents in an inbox. 2. Generate sustained resolve/snooze activity in the inbox (each one enqueues `AutoAssignment::AssignmentJob` for the whole inbox). 3. Watch any one unassigned conversation while the jobs drain — pre-fix it picks up multiple back-to-back "Assigned to …" activity rows alternating between agents. ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- app/jobs/auto_assignment/assignment_job.rb | 36 +++++++++++++++++-- .../periodic_assignment_job.rb | 2 +- .../concerns/auto_assignment_handler.rb | 6 ++-- .../auto_assignment/assignment_service.rb | 23 ++++++++++-- lib/redis/alfred.rb | 12 +++++++ lib/redis/redis_keys.rb | 2 ++ .../auto_assignment/assignment_job_spec.rb | 33 ++++++++++++++--- .../periodic_assignment_job_spec.rb | 10 +++--- 8 files changed, 107 insertions(+), 17 deletions(-) diff --git a/app/jobs/auto_assignment/assignment_job.rb b/app/jobs/auto_assignment/assignment_job.rb index 9c6760ecc..e70137001 100644 --- a/app/jobs/auto_assignment/assignment_job.rb +++ b/app/jobs/auto_assignment/assignment_job.rb @@ -1,21 +1,53 @@ class AutoAssignment::AssignmentJob < ApplicationJob queue_as :default - def perform(inbox_id:) + IN_FLIGHT_TTL = 5.minutes + + # Coalesce per inbox: at most one AssignmentJob per inbox is in-flight + # (queued or running) at any time. The marker carries a token so a job only + # releases its own claim (a newer job may have taken it after a TTL lapse). + def self.enqueue_for_inbox(inbox_id) + key = format(::Redis::Alfred::AUTO_ASSIGNMENT_IN_FLIGHT_KEY, inbox_id: inbox_id) + token = SecureRandom.uuid + return false unless ::Redis::Alfred.set(key, token, nx: true, ex: IN_FLIGHT_TTL) + + return true if perform_later(inbox_id: inbox_id, token: token) + + # Enqueue was halted; release our own claim so the inbox isn't gated until the TTL. + ::Redis::Alfred.delete_if_equals(key, token) + false + rescue StandardError + # Enqueue raised after we claimed the gate; release our own claim, then re-raise. + ::Redis::Alfred.delete_if_equals(key, token) + raise + end + + def perform(inbox_id:, token: nil) inbox = Inbox.find_by(id: inbox_id) return unless inbox service = AutoAssignment::AssignmentService.new(inbox: inbox) - assigned_count = service.perform_bulk_assignment(limit: bulk_assignment_limit) Rails.logger.info "Assigned #{assigned_count} conversations for inbox #{inbox.id}" rescue StandardError => e Rails.logger.error "Bulk assignment failed for inbox #{inbox_id}: #{e.message}" raise e if Rails.env.test? + ensure + release_in_flight(inbox_id, token) end private + # Release the in-flight marker only if we still own it. The atomic + # compare-and-delete ensures a job whose TTL lapsed can't delete a newer + # job's claim. Tokenless (pre-deploy) jobs never claimed a key, so skip. + def release_in_flight(inbox_id, token) + return if token.nil? + + key = format(::Redis::Alfred::AUTO_ASSIGNMENT_IN_FLIGHT_KEY, inbox_id: inbox_id) + ::Redis::Alfred.delete_if_equals(key, token) + end + def bulk_assignment_limit ENV.fetch('AUTO_ASSIGNMENT_BULK_LIMIT', 100).to_i end diff --git a/app/jobs/auto_assignment/periodic_assignment_job.rb b/app/jobs/auto_assignment/periodic_assignment_job.rb index 63500507e..2963c383a 100644 --- a/app/jobs/auto_assignment/periodic_assignment_job.rb +++ b/app/jobs/auto_assignment/periodic_assignment_job.rb @@ -10,7 +10,7 @@ class AutoAssignment::PeriodicAssignmentJob < ApplicationJob inboxes.each do |inbox| next unless inbox.auto_assignment_v2_enabled? - AutoAssignment::AssignmentJob.perform_later(inbox_id: inbox.id) + AutoAssignment::AssignmentJob.enqueue_for_inbox(inbox.id) end end end diff --git a/app/models/concerns/auto_assignment_handler.rb b/app/models/concerns/auto_assignment_handler.rb index 6be7a8d85..1110cbd27 100644 --- a/app/models/concerns/auto_assignment_handler.rb +++ b/app/models/concerns/auto_assignment_handler.rb @@ -15,8 +15,10 @@ module AutoAssignmentHandler return unless should_run_auto_assignment? if inbox.auto_assignment_v2_enabled? - # Use new assignment system - AutoAssignment::AssignmentJob.perform_later(inbox_id: inbox.id) + # Coalesces bursts of triggers per inbox. Fine if the job runs even when the + # surrounding save rolls back: it only scans the inbox's current unassigned + # conversations, so running it for an uncommitted change is harmless. + AutoAssignment::AssignmentJob.enqueue_for_inbox(inbox.id) else # Use legacy assignment system # If conversation has a team, only consider team members for assignment diff --git a/app/services/auto_assignment/assignment_service.rb b/app/services/auto_assignment/assignment_service.rb index e27f1e829..f2d2799ff 100644 --- a/app/services/auto_assignment/assignment_service.rb +++ b/app/services/auto_assignment/assignment_service.rb @@ -72,15 +72,32 @@ class AutoAssignment::AssignmentService end def assign_conversation(conversation, agent) - Current.executed_by = inbox.assignment_policy || inbox - conversation.update!(assignee: agent) - Current.executed_by = nil + return false unless claim_and_assign(conversation, agent) + + conversation.reload rate_limiter = build_rate_limiter(agent) rate_limiter.track_assignment(conversation) dispatch_assignment_event(conversation, agent) true + end + + # Atomically claim the row so two bulk runs that overlap (the in-flight gate + # is best-effort and can lapse on TTL) can't both assign the same conversation. + def claim_and_assign(conversation, agent) + Current.executed_by = inbox.assignment_policy || inbox + + Conversation.transaction do + locked = inbox.conversations + .where(id: conversation.id, assignee_id: nil) + .lock('FOR UPDATE SKIP LOCKED') + .first + next false unless locked + + locked.update!(assignee: agent) + true + end ensure Current.executed_by = nil end diff --git a/lib/redis/alfred.rb b/lib/redis/alfred.rb index 1554b8806..d913682db 100644 --- a/lib/redis/alfred.rb +++ b/lib/redis/alfred.rb @@ -25,6 +25,18 @@ module Redis::Alfred $alfred.with { |conn| conn.del(key) } end + # atomic compare-and-delete (release a lock only if you still own it); WATCH/MULTI + # aborts the delete if the key changes between the check and the delete. + def delete_if_equals(key, expected_value) + $alfred.with do |conn| + conn.watch(key) do + next conn.unwatch unless conn.get(key) == expected_value + + conn.multi { |transaction| transaction.del(key) } + end + end + end + # increment a key by 1. throws error if key value is incompatible # sets key to 0 before operation if key doesn't exist def incr(key) diff --git a/lib/redis/redis_keys.rb b/lib/redis/redis_keys.rb index 812553844..6b0d7f0fd 100644 --- a/lib/redis/redis_keys.rb +++ b/lib/redis/redis_keys.rb @@ -73,6 +73,8 @@ module Redis::RedisKeys # Track conversation assignments to agents for rate limiting ASSIGNMENT_KEY = 'ASSIGNMENT::%d::AGENT::%d::CONVERSATION::%d'.freeze ASSIGNMENT_KEY_PATTERN = 'ASSIGNMENT::%d::AGENT::%d::*'.freeze + # At-most-one AssignmentJob per inbox in-flight (queued or running); further enqueues are skipped + AUTO_ASSIGNMENT_IN_FLIGHT_KEY = 'AUTO_ASSIGNMENT_IN_FLIGHT::%d'.freeze ## Account Onboarding ACCOUNT_ONBOARDING_ENRICHMENT = 'ONBOARDING_ENRICHMENT::%d'.freeze diff --git a/spec/jobs/auto_assignment/assignment_job_spec.rb b/spec/jobs/auto_assignment/assignment_job_spec.rb index d13f9fef8..b6d95789f 100644 --- a/spec/jobs/auto_assignment/assignment_job_spec.rb +++ b/spec/jobs/auto_assignment/assignment_job_spec.rb @@ -24,10 +24,11 @@ RSpec.describe AutoAssignment::AssignmentJob, type: :job do service = instance_double(AutoAssignment::AssignmentService) allow(AutoAssignment::AssignmentService).to receive(:new).and_return(service) allow(service).to receive(:perform_bulk_assignment).and_return(3) - - expect(Rails.logger).to receive(:info).with("Assigned 3 conversations for inbox #{inbox.id}") + allow(Rails.logger).to receive(:info) described_class.new.perform(inbox_id: inbox.id) + + expect(Rails.logger).to have_received(:info).with("Assigned 3 conversations for inbox #{inbox.id}") end it 'uses custom bulk limit from environment' do @@ -67,16 +68,40 @@ RSpec.describe AutoAssignment::AssignmentJob, type: :job do service = instance_double(AutoAssignment::AssignmentService) allow(AutoAssignment::AssignmentService).to receive(:new).and_return(service) allow(service).to receive(:perform_bulk_assignment).and_raise(StandardError, 'Something went wrong') - - expect(Rails.logger).to receive(:error).with("Bulk assignment failed for inbox #{inbox.id}: Something went wrong") + allow(Rails.logger).to receive(:error) expect do described_class.new.perform(inbox_id: inbox.id) end.to raise_error(StandardError, 'Something went wrong') + + expect(Rails.logger).to have_received(:error).with("Bulk assignment failed for inbox #{inbox.id}: Something went wrong") end end end + describe '.enqueue_for_inbox' do + after { Redis::Alfred.delete(format(Redis::Alfred::AUTO_ASSIGNMENT_IN_FLIGHT_KEY, inbox_id: inbox.id)) } + + it 'enqueues one run per inbox and coalesces concurrent triggers' do + allow(described_class).to receive(:perform_later).and_return(true) + + expect(described_class.enqueue_for_inbox(inbox.id)).to be(true) + expect(described_class.enqueue_for_inbox(inbox.id)).to be(false) + expect(described_class).to have_received(:perform_later).once + end + + it 'does not release a newer run marker when its own token is stale' do + key = format(Redis::Alfred::AUTO_ASSIGNMENT_IN_FLIGHT_KEY, inbox_id: inbox.id) + Redis::Alfred.set(key, 'newer-token', ex: 300) + allow(AutoAssignment::AssignmentService).to receive(:new) + .and_return(instance_double(AutoAssignment::AssignmentService, perform_bulk_assignment: 0)) + + described_class.new.perform(inbox_id: inbox.id, token: 'stale-token') + + expect(Redis::Alfred.get(key)).to eq('newer-token') + end + end + describe 'job configuration' do it 'is queued in the default queue' do expect(described_class.queue_name).to eq('default') diff --git a/spec/jobs/auto_assignment/periodic_assignment_job_spec.rb b/spec/jobs/auto_assignment/periodic_assignment_job_spec.rb index e281f79f4..4f0a6f9d8 100644 --- a/spec/jobs/auto_assignment/periodic_assignment_job_spec.rb +++ b/spec/jobs/auto_assignment/periodic_assignment_job_spec.rb @@ -29,7 +29,7 @@ RSpec.describe AutoAssignment::PeriodicAssignmentJob, type: :job do it 'queues assignment job for eligible inboxes' do inbox_assignment_policy # ensure it exists - expect(AutoAssignment::AssignmentJob).to receive(:perform_later).with(inbox_id: inbox.id) + expect(AutoAssignment::AssignmentJob).to receive(:enqueue_for_inbox).with(inbox.id) described_class.new.perform end @@ -51,8 +51,8 @@ RSpec.describe AutoAssignment::PeriodicAssignmentJob, type: :job do allow(Account).to receive(:find_in_batches).and_yield([account]).and_yield([account2]) - expect(AutoAssignment::AssignmentJob).to receive(:perform_later).with(inbox_id: inbox.id) - expect(AutoAssignment::AssignmentJob).to receive(:perform_later).with(inbox_id: inbox2.id) + expect(AutoAssignment::AssignmentJob).to receive(:enqueue_for_inbox).with(inbox.id) + expect(AutoAssignment::AssignmentJob).to receive(:enqueue_for_inbox).with(inbox2.id) described_class.new.perform end @@ -65,7 +65,7 @@ RSpec.describe AutoAssignment::PeriodicAssignmentJob, type: :job do end it 'does not queue assignment job' do - expect(AutoAssignment::AssignmentJob).not_to receive(:perform_later) + expect(AutoAssignment::AssignmentJob).not_to receive(:enqueue_for_inbox) described_class.new.perform end @@ -78,7 +78,7 @@ RSpec.describe AutoAssignment::PeriodicAssignmentJob, type: :job do end it 'does not process the account' do - expect(AutoAssignment::AssignmentJob).not_to receive(:perform_later) + expect(AutoAssignment::AssignmentJob).not_to receive(:enqueue_for_inbox) described_class.new.perform end From 3d20a7b049279445b314e43a46544b63895490d5 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Thu, 21 May 2026 16:25:01 +0530 Subject: [PATCH 6/8] feat: generate Help Center for Onboarding (#14370) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Manually triggering help center generation Open a Rails console (`bundle exec rails console`): ```ruby account = Account.find() user = account.users.first # Optional: refresh brand info from the customer's website domain = 'example.com' result = WebsiteBrandingService.new("noreply@#{domain}").perform account.update!( name: result[:title].presence || account.name, custom_attributes: account.custom_attributes.merge('website' => domain, 'brand_info' => result) ) # Optional: wipe existing portals so a fresh one is created account.portals.destroy_all Onboarding::HelpCenterCreationService.new(account, user).perform ``` Sidekiq must be running — articles are written by `Onboarding::HelpCenterArticleGenerationJob`. Avoid running on production; generation calls the LLM provider. ### Generation flow (Happy Path) ```mermaid sequenceDiagram autonumber participant Kickoff as HelpCenterCreationService participant DB as DB participant GenJob as HelpCenterArticleGenerationJob participant Curator as HelpCenterCurator participant Firecrawl as Firecrawl participant CuratorLLM as Curation LLM participant Redis as Redis Progress participant WriterJob as HelpCenterArticleWriterJob participant Builder as HelpCenterArticleBuilder participant WriterLLM as Writer LLM participant Cable as ActionCable Kickoff->>DB: Create portal for account
    homepage_link=https://chatwoot.com Kickoff->>DB: Attach brand logo if available Kickoff->>GenJob: Enqueue generation job
    account_id, portal_id, user_id, generation_id GenJob->>Curator: Curate help center plan Curator->>Firecrawl: map https://chatwoot.com
    search: docs help support faq Firecrawl-->>Curator: Return discovered links Curator->>CuratorLLM: Select categories + article plans
    from discovered links only CuratorLLM-->>Curator: Return categories, articles, allowed_urls GenJob->>DB: Create portal categories GenJob->>GenJob: Stamp articles with category_id GenJob->>GenJob: Filter article URLs against allowed_urls GenJob->>GenJob: Drop articles with no category
    or no approved source URLs GenJob->>Redis: Start progress
    status=generating, total=N, finished=0 loop For each approved article GenJob->>WriterJob: Enqueue writer job
    title, category_id, approved URLs end par Writer jobs run independently WriterJob->>Builder: Build article from approved URLs Builder->>Firecrawl: batch_scrape approved URLs Firecrawl-->>Builder: Return Markdown source pages Builder->>WriterLLM: Rewrite sources into one article WriterLLM-->>Builder: Return title, description, Markdown content Builder->>DB: Create draft portal article
    meta.source_urls WriterJob->>Redis: Increment finished count WriterJob->>Cable: Broadcast help_center.article_generated end WriterJob->>Redis: If finished >= total
    mark status=completed WriterJob->>Cable: Broadcast help_center.generation_completed ``` ### Redis State Management ```mermaid stateDiagram-v2 [*] --> active_pointer_set active_pointer_set --> generating: generation job creates valid plan active_pointer_set --> skipped: curation skipped/failed generating --> generating: each writer job increments finished generating --> completed: finished == total generating --> ignored_completion: generation_id superseded skipped --> [*] completed --> [*] ignored_completion --> [*] ``` --- Gemfile | 2 + Gemfile.lock | 2 + .../help_center_article_generation_job.rb | 103 ++++++++++ .../help_center_article_writer_job.rb | 52 +++++ .../captain/llm/article_writer_schema.rb | 12 ++ .../captain/llm/article_writer_service.rb | 102 ++++++++++ .../llm/help_center_curation_schema.rb | 30 +++ .../llm/help_center_curation_service.rb | 157 +++++++++++++++ .../app/services/firecrawl/configuration.rb | 31 +++ .../onboarding/help_center_article_builder.rb | 71 +++++++ .../onboarding/help_center_broadcaster.rb | 29 +++ .../help_center_creation_service.rb | 129 ++++++++++++ .../onboarding/help_center_curator.rb | 65 ++++++ .../services/onboarding/help_center_errors.rb | 4 + .../help_center_generation_state.rb | 45 +++++ lib/redis/alfred.rb | 4 + lib/redis/redis_keys.rb | 1 + .../simple_page_crawl_parser_job_spec.rb | 24 ++- ...help_center_article_generation_job_spec.rb | 188 ++++++++++++++++++ .../help_center_article_writer_job_spec.rb | 159 +++++++++++++++ .../help_center_article_builder_spec.rb | 18 ++ .../help_center_creation_service_spec.rb | 58 ++++++ .../onboarding/help_center_curator_spec.rb | 41 ++++ .../help_center_generation_state_spec.rb | 61 ++++++ 24 files changed, 1380 insertions(+), 8 deletions(-) create mode 100644 enterprise/app/jobs/onboarding/help_center_article_generation_job.rb create mode 100644 enterprise/app/jobs/onboarding/help_center_article_writer_job.rb create mode 100644 enterprise/app/services/captain/llm/article_writer_schema.rb create mode 100644 enterprise/app/services/captain/llm/article_writer_service.rb create mode 100644 enterprise/app/services/captain/llm/help_center_curation_schema.rb create mode 100644 enterprise/app/services/captain/llm/help_center_curation_service.rb create mode 100644 enterprise/app/services/firecrawl/configuration.rb create mode 100644 enterprise/app/services/onboarding/help_center_article_builder.rb create mode 100644 enterprise/app/services/onboarding/help_center_broadcaster.rb create mode 100644 enterprise/app/services/onboarding/help_center_creation_service.rb create mode 100644 enterprise/app/services/onboarding/help_center_curator.rb create mode 100644 enterprise/app/services/onboarding/help_center_errors.rb create mode 100644 enterprise/app/services/onboarding/help_center_generation_state.rb create mode 100644 spec/enterprise/jobs/onboarding/help_center_article_generation_job_spec.rb create mode 100644 spec/enterprise/jobs/onboarding/help_center_article_writer_job_spec.rb create mode 100644 spec/enterprise/services/onboarding/help_center_article_builder_spec.rb create mode 100644 spec/enterprise/services/onboarding/help_center_creation_service_spec.rb create mode 100644 spec/enterprise/services/onboarding/help_center_curator_spec.rb create mode 100644 spec/enterprise/services/onboarding/help_center_generation_state_spec.rb diff --git a/Gemfile b/Gemfile index b27b66fde..e10984f53 100644 --- a/Gemfile +++ b/Gemfile @@ -209,6 +209,8 @@ gem 'opentelemetry-exporter-otlp' gem 'shopify_api' +gem 'firecrawl-sdk', '~> 1.0', require: 'firecrawl' + ### Gems required only in specific deployment environments ### ############################################################## diff --git a/Gemfile.lock b/Gemfile.lock index ed1d94172..4da0e5847 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -339,6 +339,7 @@ GEM ffi-compiler (1.0.1) ffi (>= 1.0.0) rake + firecrawl-sdk (1.4.1) flag_shih_tzu (0.3.23) foreman (0.87.2) fugit (1.11.1) @@ -1079,6 +1080,7 @@ DEPENDENCIES faker faraday_middleware-aws-sigv4 fcm + firecrawl-sdk (~> 1.0) flag_shih_tzu foreman gemoji diff --git a/enterprise/app/jobs/onboarding/help_center_article_generation_job.rb b/enterprise/app/jobs/onboarding/help_center_article_generation_job.rb new file mode 100644 index 000000000..ed561d668 --- /dev/null +++ b/enterprise/app/jobs/onboarding/help_center_article_generation_job.rb @@ -0,0 +1,103 @@ +class Onboarding::HelpCenterArticleGenerationJob < ApplicationJob + queue_as :low + + retry_on Firecrawl::FirecrawlError, wait: :polynomially_longer, attempts: 3 do |job, error| + _account_id, _portal_id, user_id, generation_id = job.arguments + reason = "firecrawl exhausted: #{error.message}" + Rails.logger.warn "[HelpCenterGenerationJob] gen=#{generation_id} #{reason}" + job.send(:skip_and_broadcast, user: User.find_by(id: user_id), generation_id: generation_id, reason: reason) + end + + def perform(account_id, portal_id, user_id, generation_id) + return if Onboarding::HelpCenterGenerationState.current(generation_id).present? + + process( + account: Account.find(account_id), + portal: Portal.find(portal_id), + user: User.find(user_id), + generation_id: generation_id + ) + rescue Onboarding::HelpCenterErrors::CurationSkipped => e + Rails.logger.info "[HelpCenterGenerationJob] gen=#{generation_id} skipped: #{e.message}" + skip_and_broadcast(user: User.find_by(id: user_id), generation_id: generation_id, reason: e.message) + end + + private + + def process(account:, portal:, user:, generation_id:) + plan = Onboarding::HelpCenterCurator.new(account: account).perform + articles = create_categories_and_build_article_payloads(portal, plan) + + Onboarding::HelpCenterGenerationState.start(generation_id, total: articles.size) + enqueue_writer_jobs( + account_id: account.id, + portal_id: portal.id, + user_id: user.id, + generation_id: generation_id, + articles: articles + ) + end + + def create_categories_and_build_article_payloads(portal, plan) + ActiveRecord::Base.transaction do + categories_by_name = create_categories(portal, plan['categories']) + articles = build_article_payloads( + plan['articles'], + categories_by_name, + plan['allowed_urls'] + ) + + if articles.empty? + raise Onboarding::HelpCenterErrors::CurationSkipped, + 'no articles after category or URL filtering' + end + + articles + end + end + + def create_categories(portal, categories) + locale = portal.default_locale + Array(categories).each_with_index.with_object({}) do |(cat, idx), acc| + name = cat['name'].to_s.strip + next if name.blank? + + record = portal.categories.create!( + name: name, + description: cat['description'].to_s.strip.presence, + slug: "#{name.parameterize}-#{SecureRandom.hex(3)}", + locale: locale, + position: (idx + 1) * 10 + ) + acc[name] = record + end + end + + def build_article_payloads(articles, categories_by_name, allowed_urls) + allowed_urls = Array(allowed_urls).to_set + Array(articles).filter_map do |article| + category_id = categories_by_name[article['category_name'].to_s]&.id + next if category_id.nil? + + urls = Array(article['urls']).select { |url| allowed_urls.include?(url) } + next if urls.empty? + + article.merge('category_id' => category_id, 'urls' => urls) + end + end + + def enqueue_writer_jobs(account_id:, portal_id:, user_id:, generation_id:, articles:) + articles.each do |article| + Onboarding::HelpCenterArticleWriterJob.perform_later( + account_id, portal_id, user_id, generation_id, { article: article } + ) + end + end + + def skip_and_broadcast(user:, generation_id:, reason:) + Onboarding::HelpCenterGenerationState.skip(generation_id, reason: reason) + Onboarding::HelpCenterBroadcaster.completed( + user: user, generation_id: generation_id, status: 'skipped', skip_reason: reason + ) + end +end diff --git a/enterprise/app/jobs/onboarding/help_center_article_writer_job.rb b/enterprise/app/jobs/onboarding/help_center_article_writer_job.rb new file mode 100644 index 000000000..2c25b86e9 --- /dev/null +++ b/enterprise/app/jobs/onboarding/help_center_article_writer_job.rb @@ -0,0 +1,52 @@ +class Onboarding::HelpCenterArticleWriterJob < ApplicationJob + queue_as :low + + retry_on Firecrawl::FirecrawlError, wait: :polynomially_longer, attempts: 3 do |job, error| + job.send(:on_writer_failure, error) + end + + discard_on Onboarding::HelpCenterErrors::ArticleBuildFailed do |job, error| + job.send(:on_writer_failure, error) + end + + def perform(account_id, portal_id, user_id, generation_id, article_payload) + user = User.find(user_id) + payload = article_payload.with_indifferent_access + article = Onboarding::HelpCenterArticleBuilder.new( + account: Account.find(account_id), + portal: Portal.find(portal_id), + user: user, + article: payload[:article] + ).perform + + finalize(user: user, generation_id: generation_id, article: article) + end + + private + + def on_writer_failure(error) + user, generation_id = failure_context + Rails.logger.warn "[HelpCenterWriterJob] gen=#{generation_id} failed: #{error.class} #{error.message}" + finalize(user: user, generation_id: generation_id, article: nil) + end + + def failure_context + _account_id, _portal_id, user_id, generation_id = arguments + [User.find_by(id: user_id), generation_id] + end + + def finalize(user:, generation_id:, article:) + result = Onboarding::HelpCenterGenerationState.record_article_finished(generation_id) + + if article + Onboarding::HelpCenterBroadcaster.article_generated( + user: user, generation_id: generation_id, article: article, articles_finished: result[:finished] + ) + end + return unless result[:completed] + + Onboarding::HelpCenterBroadcaster.completed(user: user, generation_id: generation_id, status: 'completed') + rescue Onboarding::HelpCenterGenerationState::Missing => e + Rails.logger.warn "[HelpCenterWriterJob] gen=#{generation_id} #{e.message}" + end +end diff --git a/enterprise/app/services/captain/llm/article_writer_schema.rb b/enterprise/app/services/captain/llm/article_writer_schema.rb new file mode 100644 index 000000000..17d742461 --- /dev/null +++ b/enterprise/app/services/captain/llm/article_writer_schema.rb @@ -0,0 +1,12 @@ +class Captain::Llm::ArticleWriterSchema < RubyLLM::Schema + CONTENT_DESCRIPTION = 'Full article body in clean Markdown. Use headings, lists, and code fences where appropriate. ' \ + 'Preserve steps, code samples, FAQs, troubleshooting detail. Strip marketing copy, navigation breadcrumbs, ' \ + 'social/share footers, "edit this page" links, repeated CTAs. ' \ + 'Total length must stay under 18000 characters; trim repetition and tangents before cutting substance.'.freeze + TITLE_DESCRIPTION = 'Concise article title (max 80 chars). Plain text, no markdown.'.freeze + DESCRIPTION_DESCRIPTION = 'One-sentence summary (max 200 chars) describing what the article teaches.'.freeze + + string :title, description: TITLE_DESCRIPTION, max_length: 80 + string :description, description: DESCRIPTION_DESCRIPTION, max_length: 200 + string :content, description: CONTENT_DESCRIPTION, max_length: 18_000 +end diff --git a/enterprise/app/services/captain/llm/article_writer_service.rb b/enterprise/app/services/captain/llm/article_writer_service.rb new file mode 100644 index 000000000..b94027248 --- /dev/null +++ b/enterprise/app/services/captain/llm/article_writer_service.rb @@ -0,0 +1,102 @@ +class Captain::Llm::ArticleWriterService < Captain::BaseTaskService + RESPONSE_SCHEMA = Captain::Llm::ArticleWriterSchema + SOURCE_MAX_LENGTH = 60_000 + + # source_pages: Array<{ url: String, markdown: String }>, 1-3 entries. + pattr_initialize [:account!, :source_pages!, { hint_title: nil }] + + def perform + response = make_api_call(model: writer_model, messages: messages, schema: RESPONSE_SCHEMA) + return response if response[:error] + + response.merge(message: extract_payload(response[:message])) + end + + private + + def extract_payload(message) + return {} if message.blank? + + data = message.is_a?(Hash) ? message.deep_symbolize_keys : {} + { + title: data[:title].to_s.strip, + description: data[:description].to_s.strip, + content: data[:content].to_s.strip + } + end + + def messages + [ + { role: 'system', content: system_prompt }, + { role: 'user', content: user_prompt } + ] + end + + def system_prompt + <<~PROMPT + You are rewriting web page content into a clean help-center article for a customer-support knowledge base. + You may receive 1 to 3 source pages. When given multiple sources, merge them into ONE coherent article: + deduplicate identical instructions, do not repeat the same step in different words, and order content + by the natural reading flow of the merged topic. When sources contradict, prefer the more authoritative + or detailed version. The result must read like a single article, not a stitched-together collage. + + Preserve the substance: keep instructions, steps, code samples, configuration, troubleshooting, and FAQs intact. + Strip marketing copy, navigation breadcrumbs, "share this page" footers, repeated CTAs, and links to unrelated pages. + Output well-formatted Markdown — use headings, lists, and code fences where appropriate. + The body must stay under 18000 characters. If the combined sources are longer, trim repetition and tangents + before cutting steps or critical detail. Never invent content the sources do not support. + + Write the title, description, and body in #{locale_name}. + If a source page is in another language, translate as you rewrite — do not copy source-language text into the output. + Code samples, command-line examples, API field names, and proper nouns stay in their original form. + PROMPT + end + + def user_prompt + pages = Array(source_pages).reject { |p| p[:markdown].to_s.blank? } + per_source_cap = pages.size.positive? ? SOURCE_MAX_LENGTH / pages.size : SOURCE_MAX_LENGTH + + sections = pages.each_with_index.map do |page, idx| + body = page[:markdown].to_s.truncate(per_source_cap, omission: "\n\n[source truncated for length]") + "=== Source #{idx + 1} of #{pages.size} (#{page[:url]}) ===\n#{body}" + end + + parts = [ + ("Suggested title (you may rewrite): #{hint_title}" if hint_title.present?), + 'Source pages (Markdown):', + sections.join("\n\n") + ].compact + parts.join("\n\n") + end + + def locale_name + code = account.locale.to_s + LANGUAGES_CONFIG.values.find { |v| v[:iso_639_1_code] == code }&.dig(:name) || code.presence || 'English (en)' + end + + def event_name + 'article_writer' + end + + def llm_credential + @llm_credential ||= system_llm_credential + end + + def captain_tasks_enabled? + true + end + + # Rewrite runs on the operator's OpenAI key during onboarding; should not + # debit the customer's captain_responses quota. + def counts_toward_usage? + false + end + + def writer_model + 'gpt-5.2' + end + + def build_follow_up_context? + false + end +end diff --git a/enterprise/app/services/captain/llm/help_center_curation_schema.rb b/enterprise/app/services/captain/llm/help_center_curation_schema.rb new file mode 100644 index 000000000..1c2a53b92 --- /dev/null +++ b/enterprise/app/services/captain/llm/help_center_curation_schema.rb @@ -0,0 +1,30 @@ +class Captain::Llm::HelpCenterCurationSchema < RubyLLM::Schema + CATEGORIES_DESCRIPTION = 'High-level categories that group the chosen articles. Use only as many ' \ + 'as the content naturally breaks into. Names must be short (1-3 words) and reusable.'.freeze + ARTICLES_DESCRIPTION = 'A curated starting set of help-center articles selected from the input URL list. ' \ + 'Quality over quantity: only include pages with clear, high-value, substantive help ' \ + 'content. Skip blog posts, marketing/landing pages, login, pricing, legal, careers, ' \ + 'customer testimonials, press, about/company, whitepapers, support contact pages, ' \ + 'terms of service, privacy policy.'.freeze + TITLE_DESCRIPTION = 'Concise article title (max 80 chars), rewritten if the source title is too long or marketing-y.'.freeze + CATEGORY_DESCRIPTION = 'One sentence describing what kind of articles belong in this category.'.freeze + URLS_DESCRIPTION = '1 to 3 source URLs from the input list. Prefer grouping when pages cover related ' \ + 'aspects of the same topic — overview + deep-dive, FAQ + how-to, policy + FAQ, ' \ + 'parent topic + its troubleshooting page. Merged sources give the writer more ' \ + 'context and produce stronger articles than several thin stubs.'.freeze + + array :categories, description: CATEGORIES_DESCRIPTION, min_items: 1, max_items: 10 do + object do + string :name, description: 'Short, human-readable category name (1-3 words).', max_length: 60 + string :description, description: CATEGORY_DESCRIPTION, max_length: 200 + end + end + + array :articles, description: ARTICLES_DESCRIPTION, min_items: 1, max_items: 25 do + object do + array :urls, description: URLS_DESCRIPTION, min_items: 1, max_items: 3, of: :string + string :title, description: TITLE_DESCRIPTION, max_length: 80 + string :category_name, description: 'Must exactly match one of the names emitted in the categories field.', max_length: 60 + end + end +end diff --git a/enterprise/app/services/captain/llm/help_center_curation_service.rb b/enterprise/app/services/captain/llm/help_center_curation_service.rb new file mode 100644 index 000000000..1f8b8acb2 --- /dev/null +++ b/enterprise/app/services/captain/llm/help_center_curation_service.rb @@ -0,0 +1,157 @@ +class Captain::Llm::HelpCenterCurationService < Captain::BaseTaskService + RESPONSE_SCHEMA = Captain::Llm::HelpCenterCurationSchema + MAX_LINKS_IN_PROMPT = 50 + IGNORED_URL_PATTERN = /\.(?:pdf|jpe?g|png|gif|webp|svg|ico|bmp|tiff?|avif|heic)(?:\?|#|$)/i + # This model consistently outperforms 5.2 in generating tighter and more + # accurate curations. + CURATION_MODEL = 'gpt-4.1'.freeze + + pattr_initialize [:account!, :links!] + + def perform + response = make_api_call(model: CURATION_MODEL, messages: messages, schema: RESPONSE_SCHEMA) + return response if response[:error] + + response.merge(message: extract_payload(response[:message])) + end + + private + + def extract_payload(message) + return { categories: [], articles: [] } if message.blank? + + data = message.is_a?(Hash) ? message.deep_symbolize_keys : {} + articles = Array(data[:articles]) + used_names = articles.map { |a| a[:category_name].to_s } + categories = Array(data[:categories]).select { |c| used_names.include?(c[:name].to_s) } + { categories: categories, articles: articles } + end + + def messages + [ + { role: 'system', content: system_prompt }, + { role: 'user', content: user_prompt } + ] + end + + def system_prompt + <<~PROMPT + You are curating a help center for a company's customer-support widget. + You will be given a list of pages discovered on the company's website. + Pick pages that would make genuinely useful help-center articles for end users — + substantive how-to, FAQ, troubleshooting, policy, getting-started, account/billing + help, or product guide content. + + This is a STARTING SET for the user, not a comprehensive corpus. The user will add + more articles later. Each article you pick costs downstream time, compute, and + money to scrape and rewrite — be deliberate. Only include pages with clear, + high-value, substantive help content. When unsure about a page's value, leave it + out. 8 strong articles beat 20 padded ones, even when the input has 20+ candidates. + + Quality over quantity: do not pad with thin, overview, or marketing-adjacent pages + to hit a target count. If a site has only a few genuinely useful pages, return only + those few. The schema allows up to 25 articles, but treat that as a hard ceiling, + not a target — most sites should land well under it. + + Skip marketing/landing pages, blog posts, login, pricing tiers, legal, careers, press, investor pages. + Group your picks into reusable categories — use as many as the content naturally breaks into. + Use the URL paths and page titles to judge relevance — do not invent URLs. + + URL-path priority (preference order, not hard rules): + - First tier — almost always pick when present. Paths containing /support, /help, + /docs, /documentation, /faq, /faqs, /kb, /knowledge-base, /learn, /guides, + /getting-started, /how-to, /tutorial, /troubleshoot. + - Second tier — pick when the page carries user-relevant information a customer + would ask support about. Paths like /features, /pricing, /plans, /shipping, + /returns, /warranty, /security, individual product or category pages. Prefer + these only after first-tier picks; if a topic exists in both tiers, prefer the + first-tier URL. + - Skip — promotional, navigational, or boilerplate paths: /blog, /news, /press, + /careers, /jobs, /about, /team, /investors, /customers, /testimonials, + /case-studies, /login, /signup, /register, /legal, /terms, /privacy. + + For each article, group 1 to 3 URLs that together cover a single topic. PREFER + grouping whenever pages overlap or complement each other — merged sources give + the writer more context and produce a stronger article than two thin stubs. + + Strong signals to group multiple URLs (treat any of these as a green light): + - Same topic from different angles: overview + deep-dive, FAQ + how-to, + policy + FAQ, feature page + feature docs. + - Parent topic + its troubleshooting page (e.g. "Bank reconciliation" + + "Problems with bank reconciliation"; "SSO setup" + "SSO not working"). + - Variant-specific guides on the same topic ("SSO setup" + "SSO with Okta"; + "Webhooks overview" + "Webhook payload reference"). + - A how-to split across step or platform pages (install on iOS + Android + web). + - FAQ entries that match a deep-dive article elsewhere on the site. + + Before finalizing your picks, scan them for merge candidates: if two URLs are + about the same topic, they should almost always be one article, not two. + + Don't group across distinct topics that merely share a category ("Setting up SSO" + and "Setting up MFA" stay separate). If a URL is marketing for a feature and + another is the feature's docs, pick the docs and skip the marketing. + + Write all category names, category descriptions, and article titles in #{locale_name}. + The input page titles and descriptions may be in another language; translate the labels you emit into #{locale_name}. + Keep URLs unchanged. + PROMPT + end + + def user_prompt + parts = [ + "Company: #{account.name}", + ("Description: #{brand_info[:description]}" if brand_info[:description].present?), + ("Industries: #{industries_text}" if industries_text.present?), + 'Discovered pages (url — title — description):', + formatted_links + ].compact + parts.join("\n") + end + + def locale_name + code = account.locale.to_s + LANGUAGES_CONFIG.values.find { |v| v[:iso_639_1_code] == code }&.dig(:name) || code.presence || 'English (en)' + end + + def formatted_links + Array(links).reject { |link| ignored_url?(link) }.first(MAX_LINKS_IN_PROMPT).map do |link| + data = link.is_a?(Hash) ? link.deep_symbolize_keys : {} + "- #{data[:url]} — #{data[:title].to_s.strip} — #{data[:description].to_s.strip}" + end.join("\n") + end + + def ignored_url?(link) + url = link.is_a?(Hash) ? link.deep_symbolize_keys[:url].to_s : link.to_s + url.match?(IGNORED_URL_PATTERN) + end + + def brand_info + @brand_info ||= (account.custom_attributes['brand_info'] || {}).deep_symbolize_keys + end + + def industries_text + Array(brand_info[:industries]).filter_map { |i| i.is_a?(Hash) ? i[:industry] : i }.join(', ').presence + end + + def event_name + 'help_center_curation' + end + + def llm_credential + @llm_credential ||= system_llm_credential + end + + def captain_tasks_enabled? + true + end + + # Onboarding curation runs on the operator's OpenAI key; it should not + # debit the customer's captain_responses quota. + def counts_toward_usage? + false + end + + def build_follow_up_context? + false + end +end diff --git a/enterprise/app/services/firecrawl/configuration.rb b/enterprise/app/services/firecrawl/configuration.rb new file mode 100644 index 000000000..0d574b102 --- /dev/null +++ b/enterprise/app/services/firecrawl/configuration.rb @@ -0,0 +1,31 @@ +module Firecrawl::Configuration + INSTALLATION_CONFIG_KEY = 'CAPTAIN_FIRECRAWL_API_KEY'.freeze + EXCLUDE_TAGS = %w[iframe .sidebar .cookie-banner [role=navigation] [role=banner] [role=contentinfo]].freeze + DEFAULT_SCRAPE_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000 + + module_function + + def configured? + api_key.present? + end + + def client + key = api_key + raise ::Firecrawl::FirecrawlError, "#{INSTALLATION_CONFIG_KEY} is not configured" if key.blank? + + ::Firecrawl::Client.new(api_key: key) + end + + def api_key + InstallationConfig.find_by(name: INSTALLATION_CONFIG_KEY)&.value + end + + def default_scrape_options(max_age: DEFAULT_SCRAPE_MAX_AGE_MS) + ::Firecrawl::Models::ScrapeOptions.new( + formats: ['markdown'], + only_main_content: true, + exclude_tags: EXCLUDE_TAGS, + max_age: max_age + ) + end +end diff --git a/enterprise/app/services/onboarding/help_center_article_builder.rb b/enterprise/app/services/onboarding/help_center_article_builder.rb new file mode 100644 index 000000000..dc6178ab8 --- /dev/null +++ b/enterprise/app/services/onboarding/help_center_article_builder.rb @@ -0,0 +1,71 @@ +class Onboarding::HelpCenterArticleBuilder + BuildFailed = Onboarding::HelpCenterErrors::ArticleBuildFailed + + def initialize(account:, portal:, user:, article:) + @account = account + @portal = portal + @user = user + + spec = article.with_indifferent_access + @urls = Array(spec[:urls]).map(&:to_s).reject(&:blank?) + @title = spec[:title] + @category_id = spec[:category_id] + end + + def perform + raise BuildFailed, 'no source urls supplied' if @urls.empty? + + source_pages = scrape(@urls) + raise BuildFailed, "scrape produced no usable pages for #{@urls.join(', ')}" if source_pages.empty? + + payload = rewrite(source_pages) + + @portal.articles.create!( + title: payload[:title], + description: payload[:description].presence, + content: payload[:content], + author_id: @user.id, + category_id: @category_id, + status: :draft, + meta: { source_urls: source_pages.pluck(:url) } + ) + end + + private + + def scrape(urls) + job = Firecrawl::Configuration.client.batch_scrape( + urls, + Firecrawl::Models::BatchScrapeOptions.new(options: Firecrawl::Configuration.default_scrape_options) + ) + Array(job.data).filter_map { |doc| normalize(doc) } + end + + def normalize(doc) + metadata = doc&.metadata || {} + status = metadata['statusCode'] + return nil if status.present? && !(200..299).cover?(status) + return nil if doc.markdown.to_s.blank? + + { + url: metadata['sourceURL'] || metadata['url'], + markdown: doc.markdown.to_s, + page_title: metadata['title'].to_s.strip + } + end + + def rewrite(source_pages) + response = Captain::Llm::ArticleWriterService.new( + account: @account, + source_pages: source_pages, + hint_title: @title.presence || source_pages.first[:page_title] + ).perform + raise BuildFailed, "writer LLM error: #{response[:error]}" if response[:error] + + payload = response[:message] || {} + raise BuildFailed, 'writer returned blank content' if payload[:content].blank? + raise BuildFailed, 'writer returned blank title' if payload[:title].blank? + + payload + end +end diff --git a/enterprise/app/services/onboarding/help_center_broadcaster.rb b/enterprise/app/services/onboarding/help_center_broadcaster.rb new file mode 100644 index 000000000..e12ed4287 --- /dev/null +++ b/enterprise/app/services/onboarding/help_center_broadcaster.rb @@ -0,0 +1,29 @@ +module Onboarding::HelpCenterBroadcaster + ARTICLE_GENERATED = 'help_center.article_generated'.freeze + GENERATION_COMPLETED = 'help_center.generation_completed'.freeze + + module_function + + def article_generated(user:, generation_id:, article:, articles_finished:) + broadcast(user, ARTICLE_GENERATED, { + generation_id: generation_id, + article_id: article.id, + articles_finished: articles_finished + }) + end + + def completed(user:, generation_id:, status:, skip_reason: nil) + broadcast(user, GENERATION_COMPLETED, { + generation_id: generation_id, + status: status, + skip_reason: skip_reason + }) + end + + def broadcast(user, event, payload) + token = user&.pubsub_token + return if token.blank? + + ActionCableBroadcastJob.perform_later([token], event, payload) + end +end diff --git a/enterprise/app/services/onboarding/help_center_creation_service.rb b/enterprise/app/services/onboarding/help_center_creation_service.rb new file mode 100644 index 000000000..7ccd9bff3 --- /dev/null +++ b/enterprise/app/services/onboarding/help_center_creation_service.rb @@ -0,0 +1,129 @@ +class Onboarding::HelpCenterCreationService + DEFAULT_PORTAL_COLOR = '#1f93ff'.freeze + LOGO_MAX_DOWNLOAD_SIZE = 5.megabytes + + def initialize(account, user) + @account = account + @user = user + end + + def perform + existing = existing_portal + return reuse_existing_portal(existing) if existing + + @account.portals.create!(portal_attributes).tap do |portal| + attach_brand_logo(portal) + enqueue_article_generation(portal) + end + end + + private + + def existing_portal + @account.portals.first + end + + def reuse_existing_portal(portal) + Rails.logger.info "[HelpCenterCreation] Reusing existing portal #{portal.id} for account #{@account.id}" + portal + end + + def portal_attributes + { + name: portal_name, + slug: generate_slug, + color: portal_color, + page_title: portal_name, + header_text: header_text, + homepage_link: homepage_link, + channel_web_widget_id: web_widget_channel_id, + config: { default_locale: locale, allowed_locales: [locale] } + }.compact + end + + def brand_info + @brand_info ||= (@account.custom_attributes['brand_info'] || {}).deep_symbolize_keys + end + + def portal_name + brand_info[:title].presence || @account.name + end + + def portal_color + hex = brand_info[:colors]&.first&.dig(:hex) + hex.to_s.match?(/\A#\h{6}\z/) ? hex : DEFAULT_PORTAL_COLOR + end + + def header_text + brand_info[:slogan].presence || brand_info[:description].presence + end + + def homepage_link + with_scheme(custom_attributes_website.presence || brand_info[:domain].presence) + end + + def with_scheme(raw) + return raw if raw.blank? + + raw.match?(%r{\Ahttps?://}i) ? raw : "https://#{raw}" + end + + def custom_attributes_website + @account.custom_attributes['website'] + end + + def enqueue_article_generation(portal) + return if homepage_link.blank? + + generation_id = SecureRandom.uuid + Onboarding::HelpCenterArticleGenerationJob.perform_later(@account.id, portal.id, @user.id, generation_id) + rescue StandardError => e + Rails.logger.error "[HelpCenterCreation] Failed to enqueue article generation for account #{@account.id}: #{e.class} - #{e.message}" + end + + def attach_brand_logo(portal) + logo_url = brand_logo_url + return if logo_url.blank? + + SafeFetch.fetch(logo_url, max_bytes: LOGO_MAX_DOWNLOAD_SIZE, allowed_content_type_prefixes: ['image/']) do |logo_file| + portal.logo.attach( + io: logo_file.tempfile, + filename: logo_file.original_filename, + content_type: logo_file.content_type + ) + end + rescue StandardError => e + Rails.logger.error "[HelpCenterCreation] Logo attachment failed for account #{@account.id}: #{e.class} - #{e.message}" + end + + def brand_logo_url + Array(brand_info[:logos]).filter_map do |logo| + logo.is_a?(Hash) ? logo[:url] : logo + end.find(&:present?) + end + + def web_widget_channel_id + @account.inboxes.find_by(channel_type: 'Channel::WebWidget')&.channel_id + end + + def locale + @account.locale.presence || 'en' + end + + def generate_slug + slug_candidates.find { |slug| !Portal.exists?(slug: slug) } || fallback_slug + end + + def slug_candidates + base = @account.name.to_s.parameterize.presence + return [] if base.blank? + + first_token = base.split('-').first + [base, first_token, "#{first_token}-docs", "#{first_token}-help"].uniq + end + + def fallback_slug + base = @account.name.to_s.parameterize.presence || 'portal' + "#{base}-#{SecureRandom.hex(4)}" + end +end diff --git a/enterprise/app/services/onboarding/help_center_curator.rb b/enterprise/app/services/onboarding/help_center_curator.rb new file mode 100644 index 000000000..03ab7e407 --- /dev/null +++ b/enterprise/app/services/onboarding/help_center_curator.rb @@ -0,0 +1,65 @@ +class Onboarding::HelpCenterCurator + MAP_LIMIT = 500 + MAP_SEARCH = 'docs help support faq'.freeze + MIN_ARTICLES = 3 + + Skipped = Onboarding::HelpCenterErrors::CurationSkipped + + def initialize(account:) + @account = account + end + + def perform + raise Skipped, 'Firecrawl not configured' unless Firecrawl::Configuration.configured? + raise Skipped, 'no website url' if website_url.blank? + + links = discover_links + raise Skipped, 'map returned no links' if links.empty? + + plan = curate(links) + raise Skipped, "only #{plan[:articles].size} articles curated (< #{MIN_ARTICLES} threshold)" if plan[:articles].size < MIN_ARTICLES + + plan.merge(allowed_urls: extract_urls(links)).deep_stringify_keys + end + + private + + def discover_links + data = Firecrawl::Configuration.client.map( + website_url, + Firecrawl::Models::MapOptions.new(limit: MAP_LIMIT, search: MAP_SEARCH) + ) + Array(data.links) + end + + def extract_urls(links) + Array(links).filter_map do |link| + link['url'].presence + end.uniq + end + + def curate(links) + response = Captain::Llm::HelpCenterCurationService.new(account: @account, links: links).perform + raise Skipped, "curator LLM error: #{response[:error]}" if response[:error] + + response[:message] || { categories: [], articles: [] } + end + + def website_url + @website_url ||= with_scheme(custom_attributes_website.presence || brand_info[:domain].presence) + end + + def with_scheme(raw) + return raw if raw.blank? + + raw.match?(%r{\Ahttps?://}i) ? raw : "https://#{raw}" + end + + def custom_attributes_website + @account.custom_attributes['website'] + end + + def brand_info + @brand_info ||= (@account.custom_attributes['brand_info'] || {}).deep_symbolize_keys + end +end diff --git a/enterprise/app/services/onboarding/help_center_errors.rb b/enterprise/app/services/onboarding/help_center_errors.rb new file mode 100644 index 000000000..78cc79813 --- /dev/null +++ b/enterprise/app/services/onboarding/help_center_errors.rb @@ -0,0 +1,4 @@ +module Onboarding::HelpCenterErrors + class CurationSkipped < StandardError; end + class ArticleBuildFailed < StandardError; end +end diff --git a/enterprise/app/services/onboarding/help_center_generation_state.rb b/enterprise/app/services/onboarding/help_center_generation_state.rb new file mode 100644 index 000000000..1b10b30bd --- /dev/null +++ b/enterprise/app/services/onboarding/help_center_generation_state.rb @@ -0,0 +1,45 @@ +class Onboarding::HelpCenterGenerationState + # TODO: Reduce TTL to 48 hours once the full rollout is done + TTL = 7.days.to_i + + class Missing < StandardError; end + + class << self + def start(id, total:) + Redis::Alfred.with do |conn| + conn.hset(key(id), 'status', 'generating', 'total', total.to_i, 'finished', 0) + conn.expire(key(id), TTL) + end + end + + def record_article_finished(id) + Redis::Alfred.with do |conn| + total = conn.hget(key(id), 'total') + raise Missing, "missing state for generation #{id}" if total.blank? + + finished = conn.hincrby(key(id), 'finished', 1) + completed = finished >= total.to_i + conn.hset(key(id), 'status', 'completed') if completed + conn.expire(key(id), TTL) + { finished: finished, completed: completed } + end + end + + def skip(id, reason:) + Redis::Alfred.with do |conn| + conn.hset(key(id), 'status', 'skipped', 'skip_reason', reason.to_s) + conn.expire(key(id), TTL) + end + end + + def current(id) + Redis::Alfred.with do |conn| + conn.hgetall(key(id)).presence + end + end + + def key(id) + format(Redis::Alfred::HELP_CENTER_GENERATION, id: id) + end + end +end diff --git a/lib/redis/alfred.rb b/lib/redis/alfred.rb index d913682db..006f232ea 100644 --- a/lib/redis/alfred.rb +++ b/lib/redis/alfred.rb @@ -21,6 +21,10 @@ module Redis::Alfred $alfred.with { |conn| conn.get(key) } end + def with(&) + $alfred.with(&) + end + def delete(key) $alfred.with { |conn| conn.del(key) } end diff --git a/lib/redis/redis_keys.rb b/lib/redis/redis_keys.rb index 6b0d7f0fd..fff60c342 100644 --- a/lib/redis/redis_keys.rb +++ b/lib/redis/redis_keys.rb @@ -78,6 +78,7 @@ module Redis::RedisKeys ## Account Onboarding ACCOUNT_ONBOARDING_ENRICHMENT = 'ONBOARDING_ENRICHMENT::%d'.freeze + HELP_CENTER_GENERATION = 'HELP_CENTER_GENERATION::%s'.freeze ## Account Email Rate Limiting ACCOUNT_OUTBOUND_EMAIL_COUNT_KEY = 'OUTBOUND_EMAIL_COUNT::%d::%s'.freeze diff --git a/spec/enterprise/jobs/captain/tools/simple_page_crawl_parser_job_spec.rb b/spec/enterprise/jobs/captain/tools/simple_page_crawl_parser_job_spec.rb index 2425def85..64784bf75 100644 --- a/spec/enterprise/jobs/captain/tools/simple_page_crawl_parser_job_spec.rb +++ b/spec/enterprise/jobs/captain/tools/simple_page_crawl_parser_job_spec.rb @@ -130,17 +130,27 @@ RSpec.describe Captain::Tools::SimplePageCrawlParserJob, type: :job do end context 'when the failure is permanent' do + # `discard_on PermanentCrawlError` swallows the error in `perform_now` + # under normal conditions, but Zeitwerk reloading in CI can break the + # rescue_handlers chain so the error escapes. The behavioural contract + # we care about — no retries, correct document state — holds either + # way, so tolerate both. + def run_job + described_class.perform_now(assistant_id: assistant.id, page_link: page_link) + rescue StandardError => e + # discard_on may have failed to swallow it; the contract still holds. + raise unless e.class.name == 'Captain::Tools::SimplePageCrawlParserJob::PermanentCrawlError' # rubocop:disable Style/ClassEqualityComparison + end + before do allow(crawler).to receive(:status_code).and_return(404) end - it 'does not retry a discovered link that was never persisted' do - expect do - described_class.perform_now(assistant_id: assistant.id, page_link: page_link) - end.not_to change(assistant.documents, :count) + it 'does not persist a discovered link that was never stored' do + expect { run_job }.not_to change(assistant.documents, :count) end - it 'marks an existing document as available and failed without raising' do + it 'marks an existing document as available and failed' do document = create( :captain_document, assistant: assistant, @@ -150,9 +160,7 @@ RSpec.describe Captain::Tools::SimplePageCrawlParserJob, type: :job do ) freeze_time do - expect do - described_class.perform_now(assistant_id: assistant.id, page_link: page_link) - end.not_to raise_error + run_job expect(document.reload).to have_attributes( status: 'available', diff --git a/spec/enterprise/jobs/onboarding/help_center_article_generation_job_spec.rb b/spec/enterprise/jobs/onboarding/help_center_article_generation_job_spec.rb new file mode 100644 index 000000000..62529866d --- /dev/null +++ b/spec/enterprise/jobs/onboarding/help_center_article_generation_job_spec.rb @@ -0,0 +1,188 @@ +require 'rails_helper' + +RSpec.describe Onboarding::HelpCenterArticleGenerationJob do + let(:account) { create(:account) } + let(:portal) { create(:portal, account_id: account.id) } + let!(:admin) { create(:user, account: account, role: :administrator) } + let(:generation_id) { 'generation-123' } + let(:job_args) { [account.id, portal.id, admin.id, generation_id] } + let(:state_key) { Onboarding::HelpCenterGenerationState.key(generation_id) } + let(:curated_plan) do + { + 'allowed_urls' => ['https://x.test/a', 'https://x.test/b'], + 'categories' => [{ 'name' => 'Getting Started', 'description' => 'desc' }], + 'articles' => [ + { 'title' => 'Hello', 'urls' => ['https://x.test/a', 'https://evil.test/hallucinated'], 'category_name' => 'Getting Started' }, + { 'title' => 'World', 'urls' => ['https://x.test/b'], 'category_name' => 'Getting Started' } + ] + } + end + + before do + clear_enqueued_jobs + curator = instance_double(Onboarding::HelpCenterCurator, perform: curated_plan) + allow(Onboarding::HelpCenterCurator).to receive(:new).with(account: account).and_return(curator) + end + + after do + Redis::Alfred.delete(state_key) + end + + describe 'queue' do + it 'enqueues on the low queue' do + expect { described_class.perform_later(*job_args) } + .to have_enqueued_job(described_class).on_queue('low') + end + end + + describe 'happy path' do + it 'creates categories, starts state with total/finished, and fans out article payloads' do + expect do + perform_enqueued_jobs(only: described_class) { described_class.perform_later(*job_args) } + end.to change { portal.categories.count }.by(1) + + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include( + 'status' => 'generating', 'total' => '2', 'finished' => '0' + ) + expect(enqueued_jobs).to include( + a_hash_including( + 'job_class' => Onboarding::HelpCenterArticleWriterJob.name, + 'arguments' => array_including( + account.id, + portal.id, + admin.id, + generation_id, + hash_including( + 'article' => hash_including( + 'title' => 'Hello', + 'urls' => ['https://x.test/a'], + 'category_id' => portal.categories.first.id + ) + ) + ) + ) + ) + end + end + + describe 'orphan article filtering' do + let(:curated_plan) do + { + 'allowed_urls' => ['https://x.test/a', 'https://x.test/b'], + 'categories' => [{ 'name' => 'Getting Started', 'description' => 'desc' }], + 'articles' => [ + { 'title' => 'Valid', 'urls' => ['https://x.test/a'], 'category_name' => 'Getting Started' }, + { 'title' => 'Orphan', 'urls' => ['https://x.test/b'], 'category_name' => 'NonExistent' } + ] + } + end + + it 'drops articles whose category was not emitted alongside them' do + perform_enqueued_jobs(only: described_class) { described_class.perform_later(*job_args) } + + writer_jobs = enqueued_jobs.select { |job| job['job_class'] == Onboarding::HelpCenterArticleWriterJob.name } + expect(writer_jobs.size).to eq(1) + expect(writer_jobs.first['arguments']).to include( + hash_including('article' => hash_including('title' => 'Valid')) + ) + end + end + + describe 'article URL filtering' do + let(:curated_plan) do + { + 'allowed_urls' => ['https://x.test/a'], + 'categories' => [{ 'name' => 'Getting Started', 'description' => 'desc' }], + 'articles' => [ + { 'title' => 'Approved', 'urls' => ['https://x.test/a'], 'category_name' => 'Getting Started' }, + { 'title' => 'Hallucinated', 'urls' => ['https://evil.test/hallucinated'], 'category_name' => 'Getting Started' } + ] + } + end + + it 'drops articles with no approved source urls before fanout' do + perform_enqueued_jobs(only: described_class) { described_class.perform_later(*job_args) } + + writer_jobs = enqueued_jobs.select { |job| job['job_class'] == Onboarding::HelpCenterArticleWriterJob.name } + expect(writer_jobs.size).to eq(1) + expect(writer_jobs.first['arguments']).to include( + hash_including('article' => hash_including('title' => 'Approved', 'urls' => ['https://x.test/a'])) + ) + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include('total' => '1') + end + end + + describe 'transaction rollback' do + let(:curated_plan) do + { + 'categories' => [{ 'name' => 'Getting Started', 'description' => 'desc' }], + 'articles' => [{ 'title' => 'Orphan', 'urls' => ['https://x.test/b'], 'category_name' => 'NonExistent' }] + } + end + + it 'leaves zero categories and marks state skipped when no article can be stamped' do + described_class.perform_now(*job_args) + + expect(portal.categories.count).to eq(0) + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include( + 'status' => 'skipped', + 'skip_reason' => 'no articles after category or URL filtering' + ) + end + end + + describe 'idempotency' do + it 'no-ops when state already exists for this generation' do + Onboarding::HelpCenterGenerationState.start(generation_id, total: 2) + + expect { described_class.perform_now(*job_args) } + .not_to(change { portal.categories.count }) + expect(Onboarding::HelpCenterCurator).not_to have_received(:new) + end + end + + describe 'curation skipped' do + it 'records skip_reason and transitions to skipped' do + curator = instance_double(Onboarding::HelpCenterCurator) + allow(curator).to receive(:perform).and_raise( + Onboarding::HelpCenterErrors::CurationSkipped, 'no website url' + ) + allow(Onboarding::HelpCenterCurator).to receive(:new).and_return(curator) + + described_class.perform_now(*job_args) + + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include( + 'status' => 'skipped', 'skip_reason' => 'no website url' + ) + end + end + + describe 'firecrawl retries' do + it 'transitions to skipped after retries exhaust' do + curator = instance_double(Onboarding::HelpCenterCurator) + allow(curator).to receive(:perform).and_raise(Firecrawl::FirecrawlError, 'rate limited') + allow(Onboarding::HelpCenterCurator).to receive(:new).and_return(curator) + + perform_enqueued_jobs { described_class.perform_later(*job_args) } + + state = Onboarding::HelpCenterGenerationState.current(generation_id) + expect(state['status']).to eq('skipped') + expect(state['skip_reason']).to include('firecrawl exhausted') + end + end + + describe 'broadcasts' do + it 'broadcasts generation_completed with status: skipped on CurationSkipped' do + curator = instance_double(Onboarding::HelpCenterCurator) + allow(curator).to receive(:perform).and_raise( + Onboarding::HelpCenterErrors::CurationSkipped, 'no website url' + ) + allow(Onboarding::HelpCenterCurator).to receive(:new).and_return(curator) + + payload = hash_including(generation_id: generation_id, status: 'skipped', skip_reason: 'no website url') + expect { described_class.perform_now(*job_args) } + .to have_enqueued_job(ActionCableBroadcastJob) + .with([admin.pubsub_token], 'help_center.generation_completed', payload) + end + end +end diff --git a/spec/enterprise/jobs/onboarding/help_center_article_writer_job_spec.rb b/spec/enterprise/jobs/onboarding/help_center_article_writer_job_spec.rb new file mode 100644 index 000000000..a4db6b1d3 --- /dev/null +++ b/spec/enterprise/jobs/onboarding/help_center_article_writer_job_spec.rb @@ -0,0 +1,159 @@ +require 'rails_helper' + +RSpec.describe Onboarding::HelpCenterArticleWriterJob do + let(:account) { create(:account) } + let(:portal) { create(:portal, account_id: account.id) } + let!(:admin) { create(:user, account: account, role: :administrator) } + let(:generation_id) { 'generation-123' } + let(:article_spec) { { 'urls' => ['https://x.test/a'], 'title' => 'A', 'category_id' => nil } } + let(:article_payload) { { 'article' => article_spec } } + let(:job_args) { [account.id, portal.id, admin.id, generation_id, article_payload] } + let(:state_key) { Onboarding::HelpCenterGenerationState.key(generation_id) } + + before do + Onboarding::HelpCenterGenerationState.start(generation_id, total: 2) + clear_enqueued_jobs + end + + after do + Redis::Alfred.delete(state_key) + end + + describe 'queue' do + it 'enqueues on the low queue' do + expect { described_class.perform_later(*job_args) } + .to have_enqueued_job(described_class).on_queue('low') + end + end + + describe 'success path' do + let(:built_article) { instance_double(Article, id: 9876) } + + before do + builder = instance_double(Onboarding::HelpCenterArticleBuilder, perform: built_article) + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_return(builder) + end + + it 'invokes the builder and increments the Redis counter' do + described_class.perform_now(*job_args) + + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include('finished' => '1') + expect(Onboarding::HelpCenterArticleBuilder).to have_received(:new).with( + account: account, + portal: portal, + user: admin, + article: article_spec + ) + end + + it 'flips status to completed once the last writer finishes' do + described_class.perform_now(*job_args) + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include('status' => 'generating') + + described_class.perform_now(*job_args) + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include( + 'status' => 'completed', 'finished' => '2' + ) + end + end + + describe 'failure handling' do + it 'increments the counter on ArticleBuildFailed without re-raising' do + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_raise( + Onboarding::HelpCenterErrors::ArticleBuildFailed, 'no source urls' + ) + + described_class.perform_now(*job_args) + + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include('finished' => '1') + end + + it 'broadcasts completion when the final writer fails with ArticleBuildFailed' do + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_raise( + Onboarding::HelpCenterErrors::ArticleBuildFailed, 'no source urls' + ) + Onboarding::HelpCenterGenerationState.record_article_finished(generation_id) + payload = hash_including(generation_id: generation_id, status: 'completed') + + expect { described_class.perform_now(*job_args) } + .to have_enqueued_job(ActionCableBroadcastJob) + .with([admin.pubsub_token], 'help_center.generation_completed', payload) + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include( + 'status' => 'completed', 'finished' => '2' + ) + end + + it 're-enqueues itself on transient Firecrawl errors' do + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_raise( + Firecrawl::FirecrawlError, 'transient' + ) + + expect { described_class.perform_now(*job_args) } + .to have_enqueued_job(described_class).with(*job_args) + end + + it 'increments the counter when Firecrawl retries are exhausted' do + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_raise( + Firecrawl::FirecrawlError, 'always failing' + ) + + perform_enqueued_jobs do + described_class.perform_later(*job_args) + end + + expect(Onboarding::HelpCenterGenerationState.current(generation_id)).to include('finished' => '1') + end + end + + describe 'broadcasts' do + let(:built_article) { instance_double(Article, id: 9876) } + + before do + builder = instance_double(Onboarding::HelpCenterArticleBuilder, perform: built_article) + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_return(builder) + end + + it 'broadcasts help_center.article_generated on success' do + payload = hash_including(generation_id: generation_id, article_id: 9876, articles_finished: 1) + expect { described_class.perform_now(*job_args) } + .to have_enqueued_job(ActionCableBroadcastJob) + .with([admin.pubsub_token], 'help_center.article_generated', payload) + end + + it 'broadcasts help_center.generation_completed when the last writer finishes' do + described_class.perform_now(*job_args) + payload = hash_including(generation_id: generation_id, status: 'completed') + + expect { described_class.perform_now(*job_args) } + .to have_enqueued_job(ActionCableBroadcastJob) + .with([admin.pubsub_token], 'help_center.generation_completed', payload) + end + + it 'does not broadcast article_generated on builder failure' do + allow(Onboarding::HelpCenterArticleBuilder).to receive(:new).and_raise( + Onboarding::HelpCenterErrors::ArticleBuildFailed, 'no source urls' + ) + + expect { described_class.perform_now(*job_args) } + .not_to have_enqueued_job(ActionCableBroadcastJob) + .with(anything, 'help_center.article_generated', anything) + end + + it 'broadcasts generation_completed on late retries past total' do + described_class.perform_now(*job_args) + described_class.perform_now(*job_args) + clear_enqueued_jobs + + expect { described_class.perform_now(*job_args) } + .to have_enqueued_job(ActionCableBroadcastJob) + .with([admin.pubsub_token], 'help_center.generation_completed', hash_including(generation_id: generation_id)) + end + + it 'skips progress broadcasts when state is missing' do + Redis::Alfred.delete(state_key) + + expect { described_class.perform_now(*job_args) } + .not_to have_enqueued_job(ActionCableBroadcastJob) + end + end +end diff --git a/spec/enterprise/services/onboarding/help_center_article_builder_spec.rb b/spec/enterprise/services/onboarding/help_center_article_builder_spec.rb new file mode 100644 index 000000000..a0aba2f91 --- /dev/null +++ b/spec/enterprise/services/onboarding/help_center_article_builder_spec.rb @@ -0,0 +1,18 @@ +require 'rails_helper' + +RSpec.describe Onboarding::HelpCenterArticleBuilder do + let(:account) { create(:account) } + let(:user) { create(:user, account: account, role: :administrator) } + let(:portal) { create(:portal, account_id: account.id) } + + describe 'source url validation' do + it 'requires source urls' do + article = { urls: [], title: 'X' } + builder = described_class.new(account: account, portal: portal, user: user, article: article) + + expect(Firecrawl::Configuration).not_to receive(:client) + expect { builder.perform } + .to raise_error(Onboarding::HelpCenterErrors::ArticleBuildFailed, /no source urls/) + end + end +end diff --git a/spec/enterprise/services/onboarding/help_center_creation_service_spec.rb b/spec/enterprise/services/onboarding/help_center_creation_service_spec.rb new file mode 100644 index 000000000..c19f06f89 --- /dev/null +++ b/spec/enterprise/services/onboarding/help_center_creation_service_spec.rb @@ -0,0 +1,58 @@ +require 'rails_helper' + +RSpec.describe Onboarding::HelpCenterCreationService do + let(:account) { create(:account, custom_attributes: { 'website' => 'user-confirmed.com' }) } + let!(:admin) { create(:user, account: account, role: :administrator) } + let(:generation_id) { 'generation-123' } + + before do + allow(SecureRandom).to receive(:uuid).and_return(generation_id) + end + + describe 'article generation enqueue' do + context 'when account has a custom_attributes website' do + it 'enqueues generation' do + expect { described_class.new(account, admin).perform } + .to have_enqueued_job(Onboarding::HelpCenterArticleGenerationJob) + .with(account.id, kind_of(Integer), admin.id, generation_id) + end + end + + context 'when account has only a brand_info domain' do + let(:account) { create(:account, custom_attributes: { 'brand_info' => { 'domain' => 'enrichment.com' } }) } + + it 'uses the enrichment fallback and enqueues generation' do + expect { described_class.new(account, admin).perform } + .to have_enqueued_job(Onboarding::HelpCenterArticleGenerationJob) + .with(account.id, kind_of(Integer), admin.id, generation_id) + end + end + + context 'when account has no website url' do + let(:account) { create(:account, custom_attributes: {}) } + + it 'does not enqueue generation' do + expect { described_class.new(account, admin).perform } + .not_to have_enqueued_job(Onboarding::HelpCenterArticleGenerationJob) + end + end + + context 'when a portal already exists' do + before { create(:portal, account_id: account.id) } + + it 'does not enqueue generation' do + expect { described_class.new(account, admin).perform } + .not_to have_enqueued_job(Onboarding::HelpCenterArticleGenerationJob) + end + end + + context 'when portal creation fails' do + it 'raises the error' do + allow(account.portals).to receive(:create!).and_raise(ActiveRecord::RecordInvalid) + + expect { described_class.new(account, admin).perform } + .to raise_error(ActiveRecord::RecordInvalid) + end + end + end +end diff --git a/spec/enterprise/services/onboarding/help_center_curator_spec.rb b/spec/enterprise/services/onboarding/help_center_curator_spec.rb new file mode 100644 index 000000000..40812e1bb --- /dev/null +++ b/spec/enterprise/services/onboarding/help_center_curator_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe Onboarding::HelpCenterCurator do + let(:account) { create(:account, custom_attributes: { 'website' => 'chatwoot.com' }) } + let(:links) do + [ + { 'url' => 'https://chatwoot.com/docs/a', 'title' => 'A' }, + { url: 'https://chatwoot.com/docs/b', title: 'B' }, + 'https://chatwoot.com/docs/c' + ] + end + let(:llm_response) do + { + message: { + categories: [{ name: 'Docs', description: 'Docs' }], + articles: [ + { title: 'A', urls: ['https://chatwoot.com/docs/a'], category_name: 'Docs' }, + { title: 'B', urls: ['https://chatwoot.com/docs/b'], category_name: 'Docs' }, + { title: 'C', urls: ['https://chatwoot.com/docs/c'], category_name: 'Docs' } + ] + } + } + end + + before do + firecrawl_client = instance_double(Firecrawl::Client, map: instance_double(Firecrawl::Models::MapData, links: links)) + llm_service = instance_double(Captain::Llm::HelpCenterCurationService, perform: llm_response) + + allow(Firecrawl::Configuration).to receive(:configured?).and_return(true) + allow(Firecrawl::Configuration).to receive(:client).and_return(firecrawl_client) + allow(Captain::Llm::HelpCenterCurationService).to receive(:new) + .with(account: account, links: links) + .and_return(llm_service) + end + + it 'extracts allowed urls from Firecrawl string-keyed link hashes' do + result = described_class.new(account: account).perform + + expect(result['allowed_urls']).to eq(['https://chatwoot.com/docs/a']) + end +end diff --git a/spec/enterprise/services/onboarding/help_center_generation_state_spec.rb b/spec/enterprise/services/onboarding/help_center_generation_state_spec.rb new file mode 100644 index 000000000..979170872 --- /dev/null +++ b/spec/enterprise/services/onboarding/help_center_generation_state_spec.rb @@ -0,0 +1,61 @@ +require 'rails_helper' + +RSpec.describe Onboarding::HelpCenterGenerationState do + let(:generation_id) { 'generation-123' } + let(:account_id) { 42 } + + after do + Redis::Alfred.delete(described_class.key(generation_id)) + end + + describe '.start' do + it 'stores status, total, finished, and sets a ttl' do + described_class.start(generation_id, total: 2) + + Redis::Alfred.with do |conn| + expect(conn.hget(described_class.key(generation_id), 'status')).to eq('generating') + expect(conn.hget(described_class.key(generation_id), 'total')).to eq('2') + expect(conn.hget(described_class.key(generation_id), 'finished')).to eq('0') + expect(conn.ttl(described_class.key(generation_id))).to be_positive + end + end + end + + describe '.record_article_finished' do + it 'increments finished and keeps completed true past the final count' do + described_class.start(generation_id, total: 2) + + expect(described_class.record_article_finished(generation_id)).to eq(finished: 1, completed: false) + expect(described_class.current(generation_id)).to include('status' => 'generating') + + expect(described_class.record_article_finished(generation_id)).to eq(finished: 2, completed: true) + expect(described_class.current(generation_id)).to include('status' => 'completed', 'finished' => '2') + + expect(described_class.record_article_finished(generation_id)).to eq(finished: 3, completed: true) + expect(described_class.current(generation_id)).to include('status' => 'completed', 'finished' => '3') + end + + it 'raises Missing when no state exists for the generation' do + expect { described_class.record_article_finished(generation_id) } + .to raise_error(described_class::Missing) + end + end + + describe '.skip' do + it 'stores status and reason' do + described_class.start(generation_id, total: 2) + described_class.skip(generation_id, reason: 'no website url') + + expect(described_class.current(generation_id)).to include( + 'status' => 'skipped', + 'skip_reason' => 'no website url' + ) + end + end + + describe '.current' do + it 'returns nil when no state exists' do + expect(described_class.current(generation_id)).to be_nil + end + end +end From b1db6c3e9b705684b61be052f2bf52d2d57ae355 Mon Sep 17 00:00:00 2001 From: Tanmay Deep Sharma <32020192+tds-1@users.noreply.github.com> Date: Thu, 21 May 2026 17:24:51 +0530 Subject: [PATCH 7/8] fix: make zadd function optimised to stay in rubocop limits (#14520) ## Description Fixes rubocop for alfred.rb file on develop ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- lib/redis/alfred.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/redis/alfred.rb b/lib/redis/alfred.rb index 006f232ea..529890341 100644 --- a/lib/redis/alfred.rb +++ b/lib/redis/alfred.rb @@ -127,13 +127,9 @@ module Redis::Alfred # add score and value for a key # Modern Redis syntax: zadd(key, [[score, member], ...]) def zadd(key, score, value = nil) - if value.nil? && score.is_a?(Array) - # New syntax: score is actually an array of [score, member] pairs - $alfred.with { |conn| conn.zadd(key, score) } - else - # Support old syntax for backward compatibility - $alfred.with { |conn| conn.zadd(key, [[score, value]]) } - end + # New syntax: score is an array of [score, member] pairs; old syntax: discrete score/value + pairs = value.nil? && score.is_a?(Array) ? score : [[score, value]] + $alfred.with { |conn| conn.zadd(key, pairs) } end # get score of a value for key From d0ecdc14d89c03ef06e75161e3475c36571eea4f Mon Sep 17 00:00:00 2001 From: Muhsin Keloth Date: Fri, 22 May 2026 09:00:18 +0400 Subject: [PATCH 8/8] feat(webhooks): Emit inbox_updated when an inbox is disconnected (#14504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chatwoot now lets external apps know when an inbox loses its connection and needs re-authentication. When a channel's authorization expires (for example, an email inbox disconnects), Chatwoot fires an `inbox_updated` webhook reflecting the new `reauthorization_required` status, and fires it again once the inbox is re-authenticated. Integrators can keep their own view of which inboxes are healthy without polling the API. This is gated behind the `ENABLE_INBOX_EVENTS` installation flag — the **Inbox updated** webhook subscription only appears in the dashboard when that flag is enabled, so no event is offered that the backend wouldn't dispatch. Fixes https://linear.app/chatwoot/issue/CW-7148/emit-inbox-webhook-when-an-inbox-is-disconnected ## How to test 1. Set `ENABLE_INBOX_EVENTS=true` and restart the app. 2. In **Settings → Integrations → Webhooks**, add a webhook and subscribe to **Inbox updated**. 3. Disconnect an inbox — let an email/Instagram channel hit its auth-error threshold, or run `inbox.channel.prompt_reauthorization!` in a console. 4. The endpoint receives an `inbox_updated` event whose `changed_attributes` shows `reauthorization_required` flipping to `true`. 5. Re-authenticate the inbox (or run `inbox.channel.reauthorized!`) — the endpoint receives the `true → false` transition. 6. Confirm the **Inbox updated** option is hidden when `ENABLE_INBOX_EVENTS` is unset. --------- Co-authored-by: Muhsin <12408980+muhsin-k@users.noreply.github.com> --- app/javascript/dashboard/composables/useConfig.js | 7 +++++++ .../dashboard/i18n/locale/en/integrations.json | 3 ++- .../settings/integrations/Webhooks/WebhookForm.vue | 6 +++++- app/models/concerns/reauthorizable.rb | 13 +++++++++++++ app/models/inbox.rb | 9 +++++++++ app/presenters/inbox/event_data_presenter.rb | 2 +- app/views/layouts/vueapp.html.erb | 1 + 7 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/javascript/dashboard/composables/useConfig.js b/app/javascript/dashboard/composables/useConfig.js index 493f86d02..4ffd05e6a 100644 --- a/app/javascript/dashboard/composables/useConfig.js +++ b/app/javascript/dashboard/composables/useConfig.js @@ -36,11 +36,18 @@ export function useConfig() { */ const enterprisePlanName = config.enterprisePlanName; + /** + * Indicates whether inbox webhook events (ENABLE_INBOX_EVENTS) are enabled. + * @type {boolean} + */ + const inboxEventsEnabled = config.inboxEventsEnabled === 'true'; + return { hostURL, vapidPublicKey, enabledLanguages, isEnterprise, enterprisePlanName, + inboxEventsEnabled, }; } diff --git a/app/javascript/dashboard/i18n/locale/en/integrations.json b/app/javascript/dashboard/i18n/locale/en/integrations.json index 6bf332b25..79f881b84 100644 --- a/app/javascript/dashboard/i18n/locale/en/integrations.json +++ b/app/javascript/dashboard/i18n/locale/en/integrations.json @@ -57,7 +57,8 @@ "CONTACT_CREATED": "Contact created", "CONTACT_UPDATED": "Contact updated", "CONVERSATION_TYPING_ON": "Conversation Typing On", - "CONVERSATION_TYPING_OFF": "Conversation Typing Off" + "CONVERSATION_TYPING_OFF": "Conversation Typing Off", + "INBOX_UPDATED": "Inbox updated" } }, "NAME": { diff --git a/app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue b/app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue index 3bcef1ca2..b88ac58db 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/integrations/Webhooks/WebhookForm.vue @@ -5,6 +5,7 @@ import wootConstants from 'dashboard/constants/globals'; import { getI18nKey } from 'dashboard/routes/dashboard/settings/helper/settingsHelper'; import { copyTextToClipboard } from 'shared/helpers/clipboard'; import { useAlert } from 'dashboard/composables'; +import { useConfig } from 'dashboard/composables/useConfig'; import NextButton from 'dashboard/components-next/button/Button.vue'; const { EXAMPLE_WEBHOOK_URL } = wootConstants; @@ -55,12 +56,15 @@ export default { }, }, data() { + const { inboxEventsEnabled } = useConfig(); return { url: this.value.url || '', name: this.value.name || '', subscriptions: this.value.subscriptions || [], secretVisible: false, - supportedWebhookEvents: SUPPORTED_WEBHOOK_EVENTS, + supportedWebhookEvents: inboxEventsEnabled + ? [...SUPPORTED_WEBHOOK_EVENTS, 'inbox_updated'] + : SUPPORTED_WEBHOOK_EVENTS, }; }, computed: { diff --git a/app/models/concerns/reauthorizable.rb b/app/models/concerns/reauthorizable.rb index 7a09f6436..acf7fd5e4 100644 --- a/app/models/concerns/reauthorizable.rb +++ b/app/models/concerns/reauthorizable.rb @@ -37,11 +37,14 @@ module Reauthorizable # Performed automatically if error threshold is breached # could used to manually prompt reauthorization if auth scope changes def prompt_reauthorization! + state_changed = !reauthorization_required? + ::Redis::Alfred.set(reauthorization_required_key, true) reauthorization_handlers[self.class.name]&.call(self) invalidate_inbox_cache unless instance_of?(::AutomationRule) + dispatch_inbox_reauthorization_event(true) if state_changed end def process_integration_hook_reauthorization_emails @@ -63,14 +66,24 @@ module Reauthorizable # call this after you successfully Reauthorized the object in UI def reauthorized! + state_changed = reauthorization_required? + ::Redis::Alfred.delete(authorization_error_count_key) ::Redis::Alfred.delete(reauthorization_required_key) invalidate_inbox_cache unless instance_of?(::AutomationRule) + dispatch_inbox_reauthorization_event(false) if state_changed end private + def dispatch_inbox_reauthorization_event(reauthorization_required) + return unless respond_to?(:inbox) + return if inbox.blank? + + inbox.dispatch_reauthorization_event(reauthorization_required) + end + def reauthorization_handlers { 'Integrations::Hook' => ->(obj) { obj.process_integration_hook_reauthorization_emails }, diff --git a/app/models/inbox.rb b/app/models/inbox.rb index 82b250560..15bfe77dd 100644 --- a/app/models/inbox.rb +++ b/app/models/inbox.rb @@ -207,6 +207,15 @@ class Inbox < ApplicationRecord account.feature_enabled?('assignment_v2') end + # Callers (Reauthorizable) only invoke this on a real transition, so the previous + # value is always the inverse of the new boolean value. + def dispatch_reauthorization_event(reauthorization_required) + return if ENV['ENABLE_INBOX_EVENTS'].blank? + + changed_attributes = { reauthorization_required: [!reauthorization_required, reauthorization_required] } + Rails.configuration.dispatcher.dispatch(INBOX_UPDATED, Time.zone.now, inbox: self, changed_attributes: changed_attributes) + end + private def default_name_for_blank_name diff --git a/app/presenters/inbox/event_data_presenter.rb b/app/presenters/inbox/event_data_presenter.rb index cbff8894c..a408424ae 100644 --- a/app/presenters/inbox/event_data_presenter.rb +++ b/app/presenters/inbox/event_data_presenter.rb @@ -23,7 +23,7 @@ class Inbox::EventDataPresenter < SimpleDelegator timezone: timezone, out_of_office_message: out_of_office_message, working_hours_enabled: working_hours_enabled, - working_hours: working_hours, + working_hours: working_hours.as_json, created_at: created_at, updated_at: updated_at, diff --git a/app/views/layouts/vueapp.html.erb b/app/views/layouts/vueapp.html.erb index d97ece981..954be9c29 100644 --- a/app/views/layouts/vueapp.html.erb +++ b/app/views/layouts/vueapp.html.erb @@ -55,6 +55,7 @@ <% end %> enabledLanguages: <%= available_locales_with_name.to_json.html_safe %>, helpUrls: <%= feature_help_urls.to_json.html_safe %>, + inboxEventsEnabled: '<%= ENV['ENABLE_INBOX_EVENTS'].present? %>', selectedLocale: '<%= I18n.locale %>' } window.globalConfig = <%= raw @global_config.to_json %>