diff --git a/app/services/whatsapp/contact_inbox_consolidation_service.rb b/app/services/whatsapp/contact_inbox_consolidation_service.rb index ace73bc93..218b2389c 100644 --- a/app/services/whatsapp/contact_inbox_consolidation_service.rb +++ b/app/services/whatsapp/contact_inbox_consolidation_service.rb @@ -67,11 +67,8 @@ class Whatsapp::ContactInboxConsolidationService lid_contact = lid_contact_inbox.contact ActiveRecord::Base.transaction do - # Clear identifier and phone from LID contact to avoid uniqueness conflicts when updating the canonical contact - lid_cleanup = {} - lid_cleanup[:identifier] = nil if lid_contact.identifier == @identifier - lid_cleanup[:phone_number] = nil if lid_contact.phone_number == "+#{@phone}" - lid_contact.update!(lid_cleanup) if lid_cleanup.present? + moved_conversation_ids = lid_contact_inbox.conversations.pluck(:id) + lid_contact.update!(phone_number: nil) if lid_contact.phone_number == "+#{@phone}" # Move conversations from LID contact_inbox to the phone contact lid_contact_inbox.conversations.find_each do |conversation| @@ -79,11 +76,10 @@ class Whatsapp::ContactInboxConsolidationService end lid_contact_inbox.destroy! + reassign_sender_and_destroy(lid_contact, phone_contact, conversation_ids: moved_conversation_ids) - # Clean up orphaned LID contact if it has no remaining contact_inboxes - lid_contact.destroy! if lid_contact.contact_inboxes.reload.empty? - - # Update phone contact_inbox to use LID as source_id and set identifier on the canonical contact + # Resolve identifier conflicts account-wide, then update the canonical contact + transfer_identifier_to(phone_contact) phone_contact_inbox.update!(source_id: @lid) phone_contact.update!(identifier: @identifier, phone_number: "+#{@phone}") end @@ -92,10 +88,10 @@ class Whatsapp::ContactInboxConsolidationService def migrate_phone_to_lid(phone_contact_inbox) existing_contact = phone_contact_inbox.contact - return if identifier_conflict?(existing_contact) return if phone_conflict?(existing_contact) ActiveRecord::Base.transaction do + transfer_identifier_to(existing_contact) phone_contact_inbox.update!(source_id: @lid) existing_contact.update!(identifier: @identifier, phone_number: "+#{@phone}") end @@ -108,7 +104,14 @@ class Whatsapp::ContactInboxConsolidationService return unless existing_contact existing_contact_inbox = existing_contact.contact_inboxes.find_by(inbox_id: @inbox.id) - return unless existing_contact_inbox + + unless existing_contact_inbox + # Contact exists by phone but has no contact_inbox in this inbox (e.g., after provider conversion). + # Must still resolve identifier conflicts so the builder finds the phone-based contact + # instead of a stale LID-only contact, which would cause "Phone number has already been taken". + adopt_or_resolve_lid_contact(existing_contact) + return + end # If a LID contact_inbox already exists, route into the merge logic instead of early-returning lid_contact_inbox = find_lid_contact_inbox @@ -117,14 +120,59 @@ class Whatsapp::ContactInboxConsolidationService return consolidate_different_contacts(existing_contact_inbox, lid_contact_inbox) end - return if identifier_conflict?(existing_contact) - ActiveRecord::Base.transaction do + transfer_identifier_to(existing_contact) existing_contact.update!(identifier: @identifier) existing_contact_inbox.update!(source_id: @lid) end end + # When the phone-based contact has no contact_inbox in this inbox, handle + # any conflicting LID contact that would otherwise intercept the builder lookup. + def adopt_or_resolve_lid_contact(phone_contact) + lid_contact_inbox = find_lid_contact_inbox + + if lid_contact_inbox && lid_contact_inbox.contact_id != phone_contact.id + adopt_lid_contact_inbox(phone_contact, lid_contact_inbox) + else + transfer_identifier_to(phone_contact) + end + end + + # Transfer a LID contact_inbox (and its conversations) from the LID contact to the phone contact. + def adopt_lid_contact_inbox(phone_contact, lid_ci) + lid_contact = lid_ci.contact + + ActiveRecord::Base.transaction do + moved_conversation_ids = lid_ci.conversations.pluck(:id) + lid_contact.update!(phone_number: nil) if lid_contact.phone_number == "+#{@phone}" + + lid_ci.conversations.find_each do |conversation| + conversation.update!(contact_id: phone_contact.id) + end + lid_ci.update!(contact_id: phone_contact.id) + + # Resolve identifier conflicts account-wide, not just with lid_contact + transfer_identifier_to(phone_contact) + phone_contact.update!(identifier: @identifier) + + reassign_sender_and_destroy(lid_contact, phone_contact, conversation_ids: moved_conversation_ids) + end + end + + # Resolve identifier conflict by transferring the identifier to the phone-based contact. + def transfer_identifier_to(target_contact) + return if target_contact.identifier == @identifier + + conflicting = @inbox.account.contacts.find_by(identifier: @identifier) + return unless conflicting && conflicting.id != target_contact.id + + ActiveRecord::Base.transaction do + conflicting.update!(identifier: nil) + target_contact.update!(identifier: @identifier) + end + end + def identifier_conflict?(existing_contact) conflicting = @inbox.account.contacts.find_by(identifier: @identifier) conflicting.present? && conflicting.id != existing_contact.id @@ -134,4 +182,13 @@ class Whatsapp::ContactInboxConsolidationService conflicting = @inbox.account.contacts.find_by(phone_number: "+#{@phone}") conflicting.present? && conflicting.id != existing_contact.id end + + # Reassign message sender references for moved conversations, then destroy source contact if orphaned. + # Scoped to conversation_ids to avoid touching messages in other inboxes the source contact may still own. + # Prevents dependent: :destroy_async on Contact#messages from deleting message history. + def reassign_sender_and_destroy(source_contact, target_contact, conversation_ids:) + Message.where(sender: source_contact, conversation_id: conversation_ids) + .update_all(sender_id: target_contact.id) # rubocop:disable Rails/SkipsModelValidations + source_contact.destroy! if source_contact.contact_inboxes.reload.empty? + end end diff --git a/spec/services/whatsapp/baileys_handlers/messages_upsert_spec.rb b/spec/services/whatsapp/baileys_handlers/messages_upsert_spec.rb index 0f7ba8221..6eb525275 100644 --- a/spec/services/whatsapp/baileys_handlers/messages_upsert_spec.rb +++ b/spec/services/whatsapp/baileys_handlers/messages_upsert_spec.rb @@ -157,6 +157,39 @@ describe Whatsapp::BaileysHandlers::MessagesUpsert do end end + context 'when phone contact has contact_inbox with phone source_id and a separate LID contact exists (provider conversion)' do + it 'resolves the identifier conflict and uses the phone contact' do + phone_contact = create(:contact, account: inbox.account, phone_number: "+#{phone}", identifier: nil, name: 'Phone Contact') + phone_ci = create(:contact_inbox, inbox: inbox, contact: phone_contact, source_id: phone) + create(:contact, account: inbox.account, identifier: identifier, phone_number: nil, name: 'LID Contact') + + raw_message = { + key: { id: 'msg_conv_123', remoteJid: "#{lid}@lid", remoteJidAlt: "#{phone}@s.whatsapp.net", fromMe: false, + addressingMode: 'lid' }, + pushName: 'Phone Contact', + messageTimestamp: timestamp, + message: { conversation: 'Hello after conversion' } + } + params = { + webhookVerifyToken: webhook_verify_token, + event: 'messages.upsert', + data: { type: 'notify', messages: [raw_message] } + } + + expect do + Whatsapp::IncomingMessageBaileysService.new(inbox: inbox, params: params).perform + end.not_to raise_error + + expect(phone_ci.reload.source_id).to eq(source_id) + expect(phone_contact.reload.identifier).to eq(identifier) + expect(phone_contact.phone_number).to eq("+#{phone}") + + message = inbox.messages.find_by(source_id: 'msg_conv_123') + expect(message).to be_present + expect(message.sender).to eq(phone_contact) + end + end + context 'when updating the same contact (no conflict)' do it 'successfully updates the contact' do contact = create(:contact, account: inbox.account, phone_number: "+#{phone}", identifier: nil) diff --git a/spec/services/whatsapp/contact_inbox_consolidation_service_spec.rb b/spec/services/whatsapp/contact_inbox_consolidation_service_spec.rb index b7a5dbb12..c17986f0f 100644 --- a/spec/services/whatsapp/contact_inbox_consolidation_service_spec.rb +++ b/spec/services/whatsapp/contact_inbox_consolidation_service_spec.rb @@ -38,13 +38,57 @@ describe Whatsapp::ContactInboxConsolidationService do end context 'when no phone-based contact_inbox exists' do - it 'does nothing' do + it 'does nothing when no contacts exist' do service = described_class.new(inbox: inbox, phone: phone, lid: lid, identifier: identifier) expect { service.perform }.not_to change(ContactInbox, :count) end end + context 'when contact exists by phone but has no contact_inbox in this inbox' do + let!(:phone_contact) { create(:contact, account: inbox.account, phone_number: "+#{phone}") } + + context 'when another contact has the target identifier (provider conversion scenario)' do + let!(:lid_contact) { create(:contact, account: inbox.account, identifier: identifier) } + + it 'transfers identifier from the conflicting contact to the phone contact' do + service = described_class.new(inbox: inbox, phone: phone, lid: lid, identifier: identifier) + service.perform + + expect(phone_contact.reload.identifier).to eq(identifier) + expect(lid_contact.reload.identifier).to be_nil + end + end + + context 'when another contact has the identifier AND a lid contact_inbox in this inbox' do + let!(:lid_contact) { create(:contact, account: inbox.account, identifier: identifier) } + let!(:lid_contact_inbox) { create(:contact_inbox, inbox: inbox, contact: lid_contact, source_id: lid) } + + it 'adopts the lid contact_inbox, reassigns messages, and transfers it to the phone contact' do + lid_conversation = create(:conversation, inbox: inbox, contact: lid_contact, contact_inbox: lid_contact_inbox) + lid_message = create(:message, conversation: lid_conversation, sender: lid_contact, inbox: inbox, account: inbox.account) + + service = described_class.new(inbox: inbox, phone: phone, lid: lid, identifier: identifier) + service.perform + + expect(lid_contact_inbox.reload.contact_id).to eq(phone_contact.id) + expect(phone_contact.reload.identifier).to eq(identifier) + expect(lid_conversation.reload.contact_id).to eq(phone_contact.id) + expect(lid_message.reload.sender).to eq(phone_contact) + expect(Contact.exists?(lid_contact.id)).to be(false) + end + end + + context 'when no identifier conflict exists' do + it 'does not change contacts' do + service = described_class.new(inbox: inbox, phone: phone, lid: lid, identifier: identifier) + service.perform + + expect(phone_contact.reload.identifier).to be_nil + end + end + end + context 'when only phone-based contact_inbox exists' do let!(:contact) { create(:contact, account: inbox.account, phone_number: "+#{phone}") } let!(:phone_contact_inbox) { create(:contact_inbox, inbox: inbox, contact: contact, source_id: phone) } @@ -59,13 +103,15 @@ describe Whatsapp::ContactInboxConsolidationService do end context 'when there is an identifier conflict with a different contact' do - let!(:conflicting_contact) { create(:contact, account: inbox.account, identifier: identifier) } # rubocop:disable RSpec/LetSetup + let!(:conflicting_contact) { create(:contact, account: inbox.account, identifier: identifier) } - it 'does not migrate' do + it 'resolves the conflict and migrates' do service = described_class.new(inbox: inbox, phone: phone, lid: lid, identifier: identifier) service.perform - expect(phone_contact_inbox.reload.source_id).to eq(phone) + expect(phone_contact_inbox.reload.source_id).to eq(lid) + expect(contact.reload.identifier).to eq(identifier) + expect(conflicting_contact.reload.identifier).to be_nil end end @@ -214,14 +260,15 @@ describe Whatsapp::ContactInboxConsolidationService do end context 'when another contact already has the same identifier' do - let!(:conflicting_contact) { create(:contact, account: inbox.account, identifier: identifier) } # rubocop:disable RSpec/LetSetup + let!(:conflicting_contact) { create(:contact, account: inbox.account, identifier: identifier) } - it 'does not update to avoid identifier conflict' do + it 'resolves the conflict and updates' do service = described_class.new(inbox: inbox, phone: phone, lid: lid, identifier: identifier) service.perform - expect(old_contact_inbox.reload.source_id).to eq('999999999') - expect(contact.reload.identifier).not_to eq(identifier) + expect(old_contact_inbox.reload.source_id).to eq(lid) + expect(contact.reload.identifier).to eq(identifier) + expect(conflicting_contact.reload.identifier).to be_nil end end end diff --git a/spec/services/whatsapp/zapi_handlers/received_callback_spec.rb b/spec/services/whatsapp/zapi_handlers/received_callback_spec.rb index 4bd7b1ed5..3a64d3461 100644 --- a/spec/services/whatsapp/zapi_handlers/received_callback_spec.rb +++ b/spec/services/whatsapp/zapi_handlers/received_callback_spec.rb @@ -428,6 +428,122 @@ describe Whatsapp::ZapiHandlers::ReceivedCallback do end end + context 'when phone contact exists without contact_inbox and a separate LID contact exists (provider conversion)' do + let!(:phone_contact) do + create(:contact, + account: inbox.account, + identifier: 'old_baileys_lid@lid', + phone_number: '+5511987654321', + name: 'Phone Contact') + end + let!(:lid_contact) do + create(:contact, + account: inbox.account, + identifier: '123456789@lid', + phone_number: nil, + name: 'LID Contact') + end + let(:params) do + base_params.merge( + phone: '5511987654321', + chatLid: '123456789@lid' + ) + end + + it 'resolves the conflict and uses the phone contact' do + service.perform + + expect(phone_contact.reload.identifier).to eq('123456789@lid') + message = Message.last + expect(message.sender).to eq(phone_contact) + end + + context 'when the LID contact has a contact_inbox in this inbox' do + let!(:lid_contact_inbox) do + create(:contact_inbox, inbox: inbox, contact: lid_contact, source_id: '123456789') + end + + it 'adopts the contact_inbox and uses the phone contact for the message' do + service.perform + + expect(lid_contact_inbox.reload.contact_id).to eq(phone_contact.id) + expect(phone_contact.reload.identifier).to eq('123456789@lid') + message = Message.last + expect(message.sender).to eq(phone_contact) + end + end + end + + context 'when phone contact has contact_inbox with phone as source_id and a separate LID contact exists' do + let!(:phone_contact) do + create(:contact, + account: inbox.account, + identifier: nil, + phone_number: '+5511987654321', + name: 'Phone Contact') + end + let!(:phone_contact_inbox) do + create(:contact_inbox, inbox: inbox, contact: phone_contact, source_id: '5511987654321') + end + let!(:lid_contact) do # rubocop:disable RSpec/LetSetup + create(:contact, + account: inbox.account, + identifier: '123456789@lid', + phone_number: nil, + name: 'LID Contact') + end + let(:params) do + base_params.merge( + phone: '5511987654321', + chatLid: '123456789@lid' + ) + end + + it 'resolves the identifier conflict, migrates the contact_inbox, and uses the phone contact' do + service.perform + + expect(phone_contact_inbox.reload.source_id).to eq('123456789') + expect(phone_contact.reload.identifier).to eq('123456789@lid') + message = Message.last + expect(message.sender).to eq(phone_contact) + end + end + + context 'when phone contact has contact_inbox with old source_id and a separate LID contact exists' do + let!(:phone_contact) do + create(:contact, + account: inbox.account, + identifier: 'old_lid@lid', + phone_number: '+5511987654321', + name: 'Phone Contact') + end + let!(:old_contact_inbox) do + create(:contact_inbox, inbox: inbox, contact: phone_contact, source_id: '999999999') + end + let!(:lid_contact) do # rubocop:disable RSpec/LetSetup + create(:contact, + account: inbox.account, + identifier: '123456789@lid', + phone_number: nil, + name: 'LID Contact') + end + let(:params) do + base_params.merge( + phone: '5511987654321', + chatLid: '123456789@lid' + ) + end + + it 'resolves the identifier conflict, updates the contact_inbox, and uses the phone contact' do + service.perform + + expect(old_contact_inbox.reload.source_id).to eq('123456789') + expect(phone_contact.reload.identifier).to eq('123456789@lid') + message = Message.last + expect(message.sender).to eq(phone_contact) + end + end + context 'when handling avatar' do let(:params) do base_params.merge(