fix: unable to clear contact manager field #38265
Conversation
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 264eaa1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces contact update flow with a patch-style API that applies Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI/API)
participant Server as Omnichannel API
participant Model as LivechatContacts.model
participant DB as MongoDB
participant Post as Post-update flows
rect rgba(200,220,255,0.5)
Client->>Server: POST omnichannel/contacts.update (contactId, contactManager: "")
end
rect rgba(200,255,200,0.5)
Server->>Model: patchContact(contactId, { set: {...}, unset: ['contactManager']? })
Model->>DB: findOneAndUpdate({ _id, enabled: { $ne: false } }, { $set, $unset? })
DB-->>Model: updatedContact | null
Model-->>Server: updatedContact | null
end
rect rgba(255,230,200,0.5)
Server->>Post: trigger name/room/subscription/inquiry updates using updatedContact
Server-->>Client: HTTP 200 { success: true, contact: updatedContact }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
abhinavkrin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/LivechatContacts.ts (1)
117-133: Remove the inline comment on line 122 to follow coding guidelines.Per the coding guidelines for TypeScript files, avoid code comments in implementation. The comment explaining the contactManager unsetting intent is unnecessary—the code logic is clear from the conditional check and operation.
The MongoDB behavior regarding empty
$unsetobjects is handled correctly for the driver versions in use (6.10.0+). However, the main issue is the inline comment that violates the stated guideline.
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/lib/contacts/updateContact.ts (1)
74-82: Logic is correct for enabling contactManager clearing.The use of
'contactManager' in paramscorrectly detects key presence regardless of value, enabling the model layer to handle empty strings as unset operations. The validation at lines 39-41 appropriately only triggers for truthy values.However, as per coding guidelines for TypeScript files, consider removing the inline comment on line 78. The behavior is self-documenting through the
'in'operator usage, and the model layer's handling can be documented in the model itself if needed.🔧 Suggested change
const updatedContact = await LivechatContacts.updateContact(contactId, { name, ...(emails && { emails: emails?.map((address) => ({ address })) }), ...(phones && { phones: phones?.map((phoneNumber) => ({ phoneNumber })) }), - ...('contactManager' in params && { contactManager }), // A contactManager of empty string or undefined will be unset in the model method + ...('contactManager' in params && { contactManager }), ...(channels && { channels }), ...(customFieldsToUpdate && { customFields: customFieldsToUpdate }), ...(wipeConflicts && { conflictingFields: [] }), });
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38265 +/- ##
===========================================
+ Coverage 70.70% 70.76% +0.05%
===========================================
Files 3139 3142 +3
Lines 108744 108926 +182
Branches 19560 19622 +62
===========================================
+ Hits 76887 77078 +191
+ Misses 29856 29847 -9
Partials 2001 2001
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/22 17:42 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 22 Jan 2026 17:42:14 GMT |
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
Also, might the same issue exist in resolveContactConflicts.ts for the omnichannel/contacts.conflicts endpoint? It also uses a truthy check which would prevent clearing the contactManager field.
| const $set = { ...data, unknown: false, ...(data.channels && { preRegistration: !data.channels.length }) }; | ||
| const $unset: Partial<Record<keyof ILivechatContact, ''>> = {}; |
There was a problem hiding this comment.
WDYT on moving the set/unset decision logic from the model layer (LivechatContacts.ts) to the business layer (updateContact.ts)? The model should act as a pure database repository, receiving explicit $set and $unset data, while the business layer decides what to set/unset based on rules like contactManager === null || ''.
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/livechat/server/lib/contacts/resolveContactConflicts.ts (1)
46-69: Contact-manager conflicts won’t clear when contactManager is emptied.Clearing contactManager now uses
unset, butfieldsToRemoveonly removesmanagerwhencontactManageris truthy. With an empty string, the conflict remains. Align this with the same condition used forunset.🐛 Proposed fix
- const fieldsToRemove = new Set<string>( - [ - name && 'name', - contactManager && 'manager', + const fieldsToRemove = new Set<string>( + [ + name && 'name', + ('contactManager' in params) && 'manager', ...(customFields ? Object.keys(customFields).map((key) => `customFields.${key}`) : []), ].filter((field): field is string => !!field), );
🤖 Fix all issues with AI agents
In `@apps/meteor/app/livechat/server/lib/contacts/updateContact.ts`:
- Around line 74-85: The patchContact call is including name even when
undefined, which can overwrite existing names; update the
LivechatContacts.patchContact invocation in updateContact.ts to only add name to
the set object when params.name is explicitly provided (e.g., check for
params.hasOwnProperty('name') or typeof name !== 'undefined') so that the set
payload omits name when not present; keep the other conditional spreads (emails,
phones, contactManager, channels, customFieldsToUpdate, wipeConflicts) as-is and
retain the existing unset logic for contactManager.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/livechat/contacts.ts (1)
795-836: Drop inline comments in test body.Guideline asks to avoid code comments in implementation; these can be removed without losing intent.
As per coding guidelines, avoid code comments in implementation.♻️ Proposed cleanup
- // Create a conflict await request.post(api('livechat/custom.field')).send({ token, key: 'cf1', value: '111', overwrite: true }).expect(200); await request.post(api('livechat/custom.field')).send({ token, key: 'cf1', value: '222', overwrite: false }).expect(200); - // Verify the contact has a contact manager and conflicts const contactBefore = await request.get(api('omnichannel/contacts.get')).set(credentials).query({ contactId }).expect(200);
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/livechat/server/lib/contacts/updateContact.ts">
<violation number="1" location="apps/meteor/app/livechat/server/lib/contacts/updateContact.ts:76">
P2: Using a truthy check here skips updates when `name` is an empty string, but downstream logic still treats `name` as a change and updates rooms/subscriptions. This can leave the contact name unchanged while related data is cleared. Prefer checking for undefined so empty strings are persisted consistently.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const $set = { | ||
| ...set, | ||
| unknown: false, |
There was a problem hiding this comment.
even if set is empty (no updates) this will issue a query to make the contact known (and if the contact is known, then it would issue a no-op query)
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
| if (Object.keys(set).length === 0 && unset.length === 0) { | ||
| return this.findOne({ _id: contactId, enabled: { $ne: false } }); | ||
| } |
There was a problem hiding this comment.
I'd suggest doing this before calling patchContact 👀
Proposed changes (including videos or screenshots)
This PR fixes an issue where the Contact Manager field could not be cleared (set to empty) for a contact, either via the UI or through the API.
The PR brings changes which will allow clearing the contactManager field by broviding an empty string for the 'contactManager' field in the payload for the endpoint
omnichannel/contacts.update.Issue(s)
Steps to test or reproduce
Further comments
SUP-954
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.