fix: Unknown contacts able to access outbound message #36992
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughUpdates outbound messaging and contact info flows: unknown contacts are treated as “not found” in RecipientForm, and several ContactInfo components now pass full contact objects instead of IDs. UI elements related to outbound messages become disabled with tooltips for unknown contacts. Tests add coverage for the unknown-contact scenario. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant RF as RecipientForm
participant API as getContact endpoint
U->>RF: Open Outbound Message (with contactId)
RF->>API: getContact({ contactId })
API-->>RF: { contact: { _id, unknown: true } }
RF->>RF: Throw ContactNotFoundError
RF-->>U: Show error state + Retry
Note over RF,U: Unknown contacts treated as "not found"
sequenceDiagram
autonumber
actor U as User
participant CI as ContactInfo*
participant Btn as Outbound Message Button
U->>CI: View Contact Info (contact passed to children)
CI->>Btn: Render with contact { _id, unknown }
alt contact.unknown == true
Btn-->>U: Disabled + tooltip "error-unknown-contact"
else contact.unknown == false
U->>Btn: Click
Btn-->>U: Open Outbound Message modal (prefilled)
end
Note over CI: Applies to ChannelsItem and PhoneEntry actions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36992 +/- ##
========================================
Coverage 66.34% 66.35%
========================================
Files 3391 3391
Lines 115300 115313 +13
Branches 21131 21139 +8
========================================
+ Hits 76499 76512 +13
+ Misses 36188 36185 -3
- Partials 2613 2616 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/omnichannel/contactInfo/ContactInfo/ContactInfo.tsx (1)
37-41: hasConflicts can infer non-boolean typeThe expression may resolve to string[] | boolean, which can trip TS when used in disabled props. Prefer a boolean-only expression.
Apply:
- const hasConflicts = conflictingFields && conflictingFields?.length > 0; + const hasConflicts = (conflictingFields?.length ?? 0) > 0;apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx (1)
1-18: Make ILivechatContact import consistent: use @rocket.chat/core-typingsContactInfoPhoneEntry.tsx imports ILivechatContact from '@rocket.chat/apps-engine/definition/livechat' while the rest of contactInfo views import it from '@rocket.chat/core-typings' — change the import to: import type { ILivechatContact } from '@rocket.chat/core-typings'. File: apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx:1
🧹 Nitpick comments (10)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm.spec.tsx (1)
175-185: Test covers unknown-contact path; guard against react-query retry flakinessIf the QueryClient retries failed queries, a single
mockImplementationOncecan cause the second attempt to succeed and skip the error UI. Either:
- Disable retries for the contact query (preferred; see suggested diff in RecipientForm.tsx), or
- Make the mock return an unknown contact for all attempts in this test and reset the mock after the assertion.
Also consider asserting that “To*” and “From*” remain disabled in this state to harden UX expectations.
Would you confirm whether the test harness sets
retry: falseglobally for TanStack Query? If not, I recommend adjusting the query’s retry policy as suggested below.apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx (2)
81-90: Avoid retry loops for deterministic unknown contactsThrowing
ContactNotFoundErroris correct. However, TanStack Query retries by default; this will spam the endpoint and can cause UI flicker. Add a retry predicate that skips retries forContactNotFoundError.Apply this diff inside the contact
useQueryoptions:} = useQuery({ queryKey: omnichannelQueryKeys.contact(contactId), queryFn: async () => { const data = await getContact({ contactId }); // TODO: Can be safely removed once unknown contacts handling is added to the endpoint if (data?.contact && data.contact.unknown) { throw new ContactNotFoundError(); } return data; }, staleTime: 5 * 60 * 1000, select: (data) => data?.contact || undefined, enabled: !!contactId, + retry: (failureCount, error) => !(error instanceof ContactNotFoundError) && failureCount < 3, });
199-206: Gate channel selection on successful contact loadCurrently
ChannelFieldis only disabled when!contactId. With an unknown or failed contact fetch, users can still change channels even though the flow can’t proceed. Disable the channel field when the contact isn’t successfully loaded.Apply this diff:
<ChannelField control={control} contact={contact} - disabled={!contactId} + disabled={!contactId || isErrorContact || isContactNotFound || !contact} isFetching={isFetchingProvider} isError={isErrorProvider || isProviderNotFound} onRetry={refetchProvider} />apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx (2)
26-34: Avoid trailing space and improve a11y labelCurrent string interpolation leaves a trailing space when no title. Also add an explicit aria-label for SRs.
Apply:
- title={`${t('Outbound_message')} ${title ? `(${title})` : ''}`} + title={title ? `${t('Outbound_message')} (${title})` : t('Outbound_message')} + aria-label={title ? `${t('Outbound_message')} (${title})` : t('Outbound_message')}
10-12: Prop naming clarity"title" here represents a tooltip/reason. Consider renaming to "tooltip" for intent clarity across callers. Non-blocking.
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx (1)
30-35: Guard against missing contactIf contact is ever undefined, defaultValues would pass an undefined contactId. Disable the action in that case.
Apply:
- <ContactInfoOutboundMessageButton + <ContactInfoOutboundMessageButton key={`${value}-outbound-message`} title={contact?.unknown ? t('error-unknown-contact') : undefined} - disabled={contact?.unknown} - defaultValues={{ contactId: contact?._id, recipient: value }} + disabled={contact?.unknown || !contact?._id} + defaultValues={contact?._id ? { contactId: contact._id, recipient: value } : undefined} />apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsx (2)
54-62: Also gate menu item by license/permissionElse users without permission may still see the option (even if the modal later blocks). Mirror the button’s gating for consistent UX.
Apply inside the unshift block:
- items.unshift({ + items.unshift({ id: 'outbound-message', icon: 'send', disabled: contact?.unknown, tooltip: contact?.unknown ? t('error-unknown-contact') : undefined, content: t('Outbound_message'), onClick: () => outboundMessageModal.open({ contactId: contact?._id, providerId: details.id }), });And above, compute license/permission and fold into the if:
+ // optionally import/use the same hooks used elsewhere: + // const hasLicense = useHasLicenseModule('livechat-enterprise') === true; + // const hasPermission = usePermission('outbound.send-messages'); - if (canSendOutboundMessage) { + if (canSendOutboundMessage /* && hasLicense && hasPermission */) {If you prefer not to add hooks here, pass a precomputed canSendOutboundMessage from the parent that already accounts for license/permission.
65-66: Memo deps completenessIf you add license/permission gating as suggested, include them in deps to keep memoization correct.
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannels.tsx (2)
26-28: Fold license/permission into canSendOutboundMessage at the sourceCompute canSendOutboundMessage in this container so items don’t need to know about permissions.
Illustrative changes:
+import { useHasLicenseModule } from '../../../../../hooks/useHasLicenseModule'; +import { usePermission } from '@rocket.chat/ui-contexts'; ... + const hasLicense = useHasLicenseModule('livechat-enterprise') === true; + const hasPermission = usePermission('outbound.send-messages'); ... - canSendOutboundMessage={data.details.id ? providers.includes(data.details.id) : false} + canSendOutboundMessage={ + !!(data.details.id && providers.includes(data.details.id) && hasLicense && hasPermission) + }
68-74: Key stabilityUsing index as key can cause UI glitches on reordering. Prefer a stable id if available (e.g., data._id).
- key={index} + key={data._id ?? index}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm.spec.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx(1 hunks)apps/meteor/client/views/omnichannel/contactInfo/ContactInfo/ContactInfo.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannels.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsx(3 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoDetails.tsx(4 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm.spec.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createFakeContactWithManagerData(352-366)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/modals/OutboundMessageModal/OutboundMessageModal.tsx (1)
OutboundMessageModalProps(11-14)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx (1)
packages/apps-engine/src/definition/livechat/index.ts (1)
ILivechatContact(29-29)
🔇 Additional comments (5)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm.spec.tsx (1)
27-27: Mocks: explicitunknown: falseis goodBeing explicit aligns the test fixtures with the new runtime shape and avoids accidental truthy/undefined behavior.
Also applies to: 40-40
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx (1)
81-90: Type-safety OK —unknown?: booleanis declared on ILivechatContactDeclaration found at packages/core-typings/src/ILivechatContact.ts (unknown?: boolean); the runtime guard is type-safe and can remain or be removed later when the endpoint handles unknown contacts.
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx (1)
22-24: Permission/license gating: good callEarly return on missing license/permission avoids dangling UI states. LGTM.
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoDetails.tsx (2)
41-47: Contact handed down to phone entries: LGTMEnsures unknown-contact awareness in per-phone actions.
56-59: A11y: good use of labelled-byLabel associations for created-at improve SR navigation. LGTM.
apps/meteor/client/views/omnichannel/contactInfo/ContactInfo/ContactInfo.tsx
Show resolved
Hide resolved
...eteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx (1)
1-1: Resolved: ILivechatContact import aligned to core-typingsGood fix; matches the rest of the feature and avoids type drift.
🧹 Nitpick comments (1)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx (1)
14-15: Prop allows undefined contact; current gating only checksunknownIf
contactis omitted, the Outbound button won’t be disabled (sincecontact?.unknownis falsy). Either ensurecontactis always provided, or treat missing contact as unknown in consumers.Would you confirm whether
contactis guaranteed at all call sites after this refactor? If not, the change suggested below on Lines 30–35 will harden the gating without widening types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx(3 hunks)
🔇 Additional comments (1)
apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx (1)
17-21: LGTM on signature change and hooks usageDestructuring
contactand keepingvalueflow intact looks good.
...eteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx
Show resolved
Hide resolved
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
aleksandernsilva
removed
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: ready to merge
aleksandernsilva
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
aleksandernsilva
force-pushed
the
fix/outbound-unknown-contact
branch
from
517602d to
832fb28
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx (2)
33-34: Mirror change on contactTwo — OK; consider centralizing default.Optional: set
unknown: falseas a default in the test factory to avoid repeating in every mock.For example, in
createFakeContact(orcreateFakeContactWithManagerData) default the field when absent:export function createFakeContactWithManagerData(overrides?: Partial<Serialized<ILivechatContactWithManagerData>>) { const { contactManager: contactManagerOverwrites, ...contactOverwrites } = overrides || {}; const contact = createFakeContact({ unknown: false, ...contactOverwrites }); return { ...contact, contactManager: { _id: faker.string.uuid(), name: faker.person.fullName(), username: faker.internet.userName(), ...contactManagerOverwrites, }, }; }
168-177: Great coverage for the unknown‑contact path; add state assertions and retry recovery.Strengthen by asserting dependent fields are disabled and that Retry clears the error once the contact becomes known.
Apply this diff within this test case:
it('should show retry button when contact is unknown', async () => { getContactMock.mockImplementationOnce(() => ({ contact: createFakeContactWithManagerData({ _id: 'contact-1', unknown: true }) })); render(<RecipientForm defaultValues={{ contactId: 'contact-1' }} {...defaultProps} />, { wrapper: appRoot.build() }); await waitFor(() => expect(screen.getByLabelText('Contact*')).toHaveAccessibleDescription('Error loading contact information')); const retryButton = screen.getByRole('button', { name: 'Retry' }); expect(retryButton).toBeInTheDocument(); + + // Dependent fields should be disabled when contact is unknown + expect(screen.getByLabelText('To*')).toHaveClass('disabled'); + expect(screen.getByLabelText('From*')).toHaveClass('disabled'); + + // On retry, if contact becomes known, error should clear + getContactMock.mockImplementationOnce(() => ({ contact: contactOneMock })); + await userEvent.click(retryButton); + await waitFor(() => expect(screen.queryByText('Error loading contact information')).not.toBeInTheDocument()); });Additionally, keep mock return shapes consistently async to reduce flakiness:
// Prefer this shape (apply similarly to other GET mocks): const getContactMock = jest.fn().mockResolvedValue({ contact: contactOneMock });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx(3 hunks)apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx(1 hunks)apps/meteor/client/views/omnichannel/contactInfo/ContactInfo/ContactInfo.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannels.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsx(3 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoDetails.tsx(4 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx(2 hunks)apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.tsx
- apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoPhoneEntry.tsx
- apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoOutboundMessageButton.tsx
- apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoDetails/ContactInfoDetails.tsx
- apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannelsItem.tsx
- apps/meteor/client/views/omnichannel/contactInfo/ContactInfo/ContactInfo.tsx
- apps/meteor/client/views/omnichannel/contactInfo/tabs/ContactInfoChannels/ContactInfoChannels.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createFakeContactWithManagerData(352-366)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/RecipientForm/RecipientForm.spec.tsx (1)
20-21: Explicitunknown: falseon contactOne mock — LGTM; typings includeunknown?: boolean.
Confirmed ILivechatContact / ILivechatContactWithManagerData declareunknown?: boolean.
Proposed changes (including videos or screenshots)
This PR restricts shortcut access to the outbound message flow for unknown contacts.
Issue(s)
CTZ-341
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Security/Permissions