feat(whatsapp): add an identity group for BSUID and phone contact inboxes - #15533
feat(whatsapp): add an identity group for BSUID and phone contact inboxes#15533marcoazcabral wants to merge 2 commits into
Conversation
… identity Meta answers the question of which source ids belong to the same person per event and never sends a stable group identifier, so the answer only exists at the moment a payload arrives. Chatwoot has nowhere to keep it, and each call site that needs it reinvents the rule, usually by treating same contact as same identity. That proxy holds for coexistence aliases and fails after a dashboard merge, since ContactMergeAction reassigns contact_id while leaving each conversation bound to its original contact inbox. A nullable identity_group_id on contact_inboxes keeps the evidence instead. The identifiers carried by one payload join the same group, an existing group is never overwritten, and a merge never assigns one, so rows an agent brought together stay distinct by construction. Joining two existing groups is left out on purpose: it needs a deliberate path, and it is the one place where being wrong recreates the routing bug this exists to avoid. The backfill gives every existing row a group of its own, which guesses nothing and degrades to the behaviour that predates coexistence, where one source id was one identity. Groups then form as payloads arrive. Nothing reads the column yet. The first intended consumer is the rotation path, which is not upstream, and the call sites that are upstream were narrowed on purpose in the direction agreed in chatwoot#14931.
|
One cost worth raising before anyone has to ask about it. The backfill writes every row of The alternative is to drop the backfill entirely and let It is a real trade rather than a clear win. The backfill buys query simplicity: I left the backfill in because it is what I described in #13837, but I would rather ship the one you would actually be willing to run. Say which and I will change it, or drop the migration to the column alone and leave the grouping of existing rows out of scope. |
|
I built the conditional consumer locally and measured it, so the offer above is no longer hypothetical. First a correction to the description, which I got wrong: The consumer is one method: def conversations_in_identity_group
group_id = @contact_inbox.identity_group_id
return @contact_inbox.conversations if group_id.blank?
Conversation.where(contact_inbox_id: @inbox.contact_inboxes.where(identity_group_id: group_id).select(:id))
endTwo measurements, same database, nothing different but that method. No regression. The case from #15359 does tell them apart. Two people write in, an agent merges the two contacts by mistake, and then the first person writes again. Its thread is not the most recent one, which is where a contact-wide it 'does not answer one identity through the source id of another' do
deliver(from: '5511900000001', wamid: 'wamid.ig-a1')
deliver(from: '5511900000002', wamid: 'wamid.ig-b1')
first = inbox.contact_inboxes.find_by(source_id: '5511900000001')
second = inbox.contact_inboxes.find_by(source_id: '5511900000002')
first_conversation = first.conversations.last
second_conversation = second.conversations.last
expect(first_conversation).not_to eq(second_conversation)
ContactMergeAction.new(account: account, base_contact: first.contact, mergee_contact: second.contact).perform
deliver(from: '5511900000001', wamid: 'wamid.ig-a2')
message = Message.find_by(source_id: 'wamid.ig-a2')
expect(message.conversation).to eq(first_conversation)
endOn develop that fails: With the group it passes. A person wrote in and the message landed in someone else's thread, which is the same defect #15359 reports, reached through the message path rather than through a reply. Worth noting what this does not do: it fixes which conversation is found, not which address the reply goes out to. That is still It also removes the need for the provider conditional in #15175. The lookup stops depending on which row the message arrived on, so 360Dialog needs no special case to keep landing on a thread it can answer. None of this is pushed to this PR. Say which shape you want and I will push it, either here or separately. |
This is a draft, and it is not a request to merge. In #13837 I said I was posting the identity group as a proposal rather than a PR on purpose, because it touches shared routing code across CE and Enterprise and the migration is yours to weigh. That is still true. What changed is that a proposal has been easier to agree with than to react to, so this puts the smallest useful amount of code behind it to make the separate evaluation you mentioned concrete.
The decision is yours in both directions: take it forward, or keep it open as the place where the approach gets discussed and close it once a direction exists. Either outcome works for me.
What this assumes
That the direction settled in #14931 holds: resolve the exact BSUID
ContactInbox, reuse or create a conversation on it, no contact-wide lookup, no repointing. Nothing here reopens that, and nothing here changes any behaviour at all.If #15175 lands on a different shape, this should be closed rather than adapted.
Why this does not ask you to revert anything
Each step in this area fixed the cost of the one before it:
The group is the third step, not a return to the first. A merge never assigns one, so the safety the exact-contact-inbox decision bought is preserved by construction rather than traded away. Adopting groups later does not require reopening anything settled in #14931.
What is in the diff
identity_group_idoncontact_inboxesWhatsapp::IdentifierSyncService, which is the one place that already sees every co-occurrenceThe rules are the ones from the proposal. Identifiers carried by one payload join the same group. An existing group is never overwritten. A merge never assigns one, which holds by construction rather than by a check, since the write only runs from an inbound payload. Two identifiers that arrive together while already belonging to different groups are left alone: joining groups needs a deliberate path, and it is the one place where being wrong recreates the routing bug this exists to avoid.
The backfill guesses nothing. One group per row degrades to the behaviour that predates #15098, where one source id was one identity by construction, and groups then form as payloads arrive.
Nothing reads the column yet, and that is the honest state
The consumer I wanted to include is the rotation path, where the group forms without any inference:
user_id_updatenames the previous and the current identifier in the same payload, so the evidence and the grouping arrive together. That path is not upstream. It is #15357, still open.The call sites that are upstream are
set_conversationandVoice::InboundCallBuilder. The voice builder already scopes to the exact contact inbox.set_conversationon develop is still contact-wide, since the narrowing agreed in #14931 lives in #15175 and has not landed yet. I left both alone because converting them changes behaviour, and that belongs with whoever owns the decision rather than with a draft meant to stay inert.That leaves a schema change with no reader, which I would normally not send. I am sending it because the alternative was another paragraph of prose, and because the shape is easier to argue with than the description was.
The two cases this is really about
#15359.
ContactMergeAction#merge_contact_inboxesreassignscontact_idand preserves every row, whilemerge_conversationsleaves each conversation'scontact_inbox_iduntouched. Any rule that treats same contact as same identity inherits that, which is why the merged-contact routing test added to #15175 has to exist. With groups, an agent merge never assigns one, so rows brought together stay distinct by construction.#15357. A rotation is the opposite case: two identifiers that genuinely are one person, proven by the event itself. The group survives the change, and the conversation does not need to be moved for the reply to reach the right place.
A conditional follow-up, if you want the demonstration
If you would rather see this do something before evaluating it, there is one consumer available upstream today: scoping
set_conversationby group instead of by contact inbox. That sits between the two extremes already tried, since contact-wide is unsafe after a merge and contact-inbox-only splits a coexistence contact into two conversations.I did not include it because it changes a user-visible outcome you settled deliberately. Say the word and I will add it as a separate commit here, or as its own PR, whichever is easier to review.
Open questions, unchanged from the proposal
Verification
RuboCop is clean at the version the CI pins. The new spec passes, and
spec/services/whatsapp,spec/models/contact_inbox_spec.rbandspec/actions/contact_merge_action_spec.rbpass together with it, which is the surface the write path touches. I have not run the full suite or anything underspec/enterprise.Credit
The term is @jochenstu's, from #15359. The proposal built on it is mine, and this draft is the code behind it.