From 4beeaae087bda13e52f2100a0c9e65f7c874abc9 Mon Sep 17 00:00:00 2001 From: Mohamed Marrouchi Date: Mon, 9 Jun 2025 16:11:01 +0100 Subject: [PATCH] feat: disallow multiple matches when local fallback is enabled --- api/src/chat/services/block.service.spec.ts | 38 +++- api/src/chat/services/block.service.ts | 219 +++++++++++--------- api/src/chat/services/bot.service.spec.ts | 2 +- api/src/chat/services/bot.service.ts | 13 +- api/src/utils/test/mocks/block.ts | 8 + 5 files changed, 168 insertions(+), 112 deletions(-) diff --git a/api/src/chat/services/block.service.spec.ts b/api/src/chat/services/block.service.spec.ts index f4d45440..0988e6de 100644 --- a/api/src/chat/services/block.service.spec.ts +++ b/api/src/chat/services/block.service.spec.ts @@ -65,6 +65,7 @@ import { mockNlpGreetingNamePatterns, mockNlpGreetingPatterns, mockNlpGreetingWrongNamePatterns, + mockWebChannelData, } from '@/utils/test/mocks/block'; import { contextBlankInstance, @@ -288,11 +289,7 @@ describe('BlockService', () => { text: 'Hello', }, }, - { - isSocket: true, - ipAddress: '1.1.1.1', - agent: 'Chromium', - }, + mockWebChannelData, ); const webEventGetStarted = new WebEventWrapper( handlerMock, @@ -303,11 +300,18 @@ describe('BlockService', () => { payload: 'GET_STARTED', }, }, + mockWebChannelData, + ); + + const webEventAmbiguous = new WebEventWrapper( + handlerMock, { - isSocket: true, - ipAddress: '1.1.1.1', - agent: 'Chromium', + type: Web.IncomingMessageType.text, + data: { + text: "It's not a yes or no answer!", + }, }, + mockWebChannelData, ); it('should return undefined when no blocks are provided', async () => { @@ -332,6 +336,24 @@ describe('BlockService', () => { expect(result).toEqual(blockGetStarted); }); + it('should return undefined when multiple matches are not allowed', async () => { + const result = await blockService.match( + [ + { + ...blockEmpty, + patterns: ['/yes/'], + }, + { + ...blockEmpty, + patterns: ['/no/'], + }, + ], + webEventAmbiguous, + false, + ); + expect(result).toEqual(undefined); + }); + it('should match block with payload', async () => { webEventGetStarted.setSender(subscriberWithLabels); const result = await blockService.match(blocks, webEventGetStarted); diff --git a/api/src/chat/services/block.service.ts b/api/src/chat/services/block.service.ts index fe88a274..d85b7bc1 100644 --- a/api/src/chat/services/block.service.ts +++ b/api/src/chat/services/block.service.ts @@ -64,68 +64,66 @@ export class BlockService extends BaseService< } /** - * Filters an array of blocks based on the specified channel. + * Checks if block is supported on the specified channel. * - * This function ensures that only blocks that are either: - * - Not restricted to specific trigger channels (`trigger_channels` is undefined or empty), or - * - Explicitly allow the given channel - * - * are included in the returned array. - * - * @param blocks - The list of blocks to be filtered. + * @param block - The block * @param channel - The name of the channel to filter blocks by. * - * @returns The filtered array of blocks that are allowed for the given channel. + * @returns Whether the block is supported on the given channel. */ - filterBlocksByChannel( - blocks: B[], + isChannelSupported( + block: B, channel: ChannelName, ) { - return blocks.filter((b) => { - return ( - !b.trigger_channels || - b.trigger_channels.length === 0 || - b.trigger_channels.includes(channel) - ); - }); + return ( + !block.trigger_channels || + block.trigger_channels.length === 0 || + block.trigger_channels.includes(channel) + ); } /** - * Filters an array of blocks based on subscriber labels. + * Checks if the block matches the subscriber labels, allowing for two scenarios: + * - Has no trigger labels (making it applicable to all subscribers), or + * - Contains at least one trigger label that matches a label from the provided list. * - * This function selects blocks that either: - * - Have no trigger labels (making them applicable to all subscribers), or - * - Contain at least one trigger label that matches a label from the provided list. - * - * The filtered blocks are then **sorted** in descending order by the number of trigger labels, - * ensuring that blocks with more specific targeting (more trigger labels) are prioritized. - * - * @param blocks - The list of blocks to be filtered. + * @param block - The block to check. * @param labels - The list of subscriber labels to match against. - * @returns The filtered and sorted list of blocks. + * @returns True if the block matches the subscriber labels, false otherwise. */ - filterBlocksBySubscriberLabels( - blocks: B[], - profile?: Subscriber, + matchesSubscriberLabels( + block: B, + subscriber?: Subscriber, ) { - if (!profile) { - return blocks; + if (!subscriber) { + return block; } - return ( - blocks - .filter((b) => { - const triggerLabels = b.trigger_labels.map((l) => - typeof l === 'string' ? l : l.id, - ); - return ( - triggerLabels.length === 0 || - triggerLabels.some((l) => profile.labels.includes(l)) - ); - }) - // Priority goes to block who target users with labels - .sort((a, b) => b.trigger_labels.length - a.trigger_labels.length) + const triggerLabels = block.trigger_labels.map((l: string | Label) => + typeof l === 'string' ? l : l.id, ); + return ( + triggerLabels.length === 0 || + triggerLabels.some((l) => subscriber.labels.includes(l)) + ); + } + + /** + * Retrieves the configured NLU penalty factor from settings, or falls back to a default value. + * + * @returns The NLU penalty factor as a number. + */ + private async getPenaltyFactor(): Promise { + const settings = await this.settingService.getSettings(); + const configured = settings.chatbot_settings?.default_nlu_penalty_factor; + + if (configured == null) { + this.logger.warn( + 'Using fallback NLU penalty factor value: %s', + FALLBACK_DEFAULT_NLU_PENALTY_FACTOR, + ); + } + return configured ?? FALLBACK_DEFAULT_NLU_PENALTY_FACTOR; } /** @@ -133,75 +131,88 @@ export class BlockService extends BaseService< * * @param filteredBlocks blocks Starting/Next blocks in the conversation flow * @param event Received channel's message + * @param canHaveMultipleMatches Whether to allow multiple matches for the same event + * (eg. Yes/No question to which the answer is ambiguous "Sometimes yes, sometimes no") * * @returns The block that matches */ async match( blocks: BlockFull[], event: EventWrapper, + canHaveMultipleMatches = true, ): Promise { if (!blocks.length) { return undefined; } - // Search for block matching a given event - let block: BlockFull | undefined = undefined; - const payload = event.getPayload(); + // Narrow the search space + const channelName = event.getHandler().getName(); + const sender = event.getSender(); + const candidates = blocks.filter( + (b) => + this.isChannelSupported(b, channelName) && + this.matchesSubscriberLabels(b, sender), + ); - // Perform a filter to get the candidates blocks - const filteredBlocks = this.filterBlocksBySubscriberLabels( - this.filterBlocksByChannel(blocks, event.getHandler().getName()), - event.getSender(), + if (!candidates.length) { + return undefined; + } + + // Priority goes to block who target users with labels + const prioritizedCandidates = candidates.sort( + (a, b) => b.trigger_labels.length - a.trigger_labels.length, ); // Perform a payload match & pick last createdAt + const payload = event.getPayload(); if (payload) { - block = filteredBlocks - .filter((b) => { - return this.matchPayload(payload, b); - }) - .shift(); - } - - if (!block) { - // Perform a text match (Text or Quick reply) - const text = event.getText().trim(); - - // Perform a text pattern match - block = filteredBlocks - .filter((b) => { - return this.matchText(text, b); - }) - .shift(); - - // Perform an NLP Match - const nlp = event.getNLP(); - if (!block && nlp) { - const scoredEntities = - await this.nlpService.computePredictionScore(nlp); - - const settings = await this.settingService.getSettings(); - let penaltyFactor = - settings.chatbot_settings?.default_nlu_penalty_factor; - if (!penaltyFactor) { - this.logger.warn( - 'Using fallback NLU penalty factor value: %s', - FALLBACK_DEFAULT_NLU_PENALTY_FACTOR, - ); - penaltyFactor = FALLBACK_DEFAULT_NLU_PENALTY_FACTOR; - } - - if (scoredEntities.entities.length > 0) { - block = this.matchBestNLP( - filteredBlocks, - scoredEntities, - penaltyFactor, - ); - } + const payloadMatches = prioritizedCandidates.filter((b) => { + return this.matchPayload(payload, b); + }); + if (payloadMatches.length > 1 && !canHaveMultipleMatches) { + // If the payload matches multiple blocks , + // we return undefined so that we trigger the local fallback + return undefined; + } else if (payloadMatches.length > 0) { + // If we have a payload match, we return the first one + // (which is the most recent one due to the sort) + // and we don't check for text or NLP matches + return payloadMatches[0]; } } - return block; + // Perform a text match (Text or Quick reply) + const text = event.getText().trim(); + if (text) { + const textMatches = prioritizedCandidates.filter((b) => { + return this.matchText(text, b); + }); + + if (textMatches.length > 1 && !canHaveMultipleMatches) { + // If the text matches multiple blocks (especially regex), + // we return undefined so that we trigger the local fallback + return undefined; + } else if (textMatches.length > 0) { + return textMatches[0]; + } + } + + // Perform an NLP Match + const nlp = event.getNLP(); + if (nlp) { + const scoredEntities = await this.nlpService.computePredictionScore(nlp); + + if (scoredEntities.entities.length) { + const penaltyFactor = await this.getPenaltyFactor(); + return this.matchBestNLP( + prioritizedCandidates, + scoredEntities, + penaltyFactor, + ); + } + } + + return undefined; } /** @@ -500,11 +511,19 @@ export class BlockService extends BaseService< envelope: StdOutgoingSystemEnvelope, ) { // Perform a filter to get the candidates blocks - const filteredBlocks = this.filterBlocksBySubscriberLabels( - this.filterBlocksByChannel(blocks, event.getHandler().getName()), - event.getSender(), + const handlerName = event.getHandler().getName(); + const sender = event.getSender(); + const candidates = blocks.filter( + (b) => + this.isChannelSupported(b, handlerName) && + this.matchesSubscriberLabels(b, sender), ); - return filteredBlocks.find((b) => { + + if (!candidates.length) { + return undefined; + } + + return candidates.find((b) => { return b.patterns .filter( (p) => typeof p === 'object' && 'type' in p && p.type === 'outcome', diff --git a/api/src/chat/services/bot.service.spec.ts b/api/src/chat/services/bot.service.spec.ts index 93814633..d8b61596 100644 --- a/api/src/chat/services/bot.service.spec.ts +++ b/api/src/chat/services/bot.service.spec.ts @@ -293,7 +293,7 @@ describe('BotService', () => { event.setSender(webSubscriber); const clearMock = jest - .spyOn(botService, 'handleIncomingMessage') + .spyOn(botService, 'handleOngoingConversationMessage') .mockImplementation( async ( actualConversation: ConversationFull, diff --git a/api/src/chat/services/bot.service.ts b/api/src/chat/services/bot.service.ts index a5b9dbb3..ae3dac76 100644 --- a/api/src/chat/services/bot.service.ts +++ b/api/src/chat/services/bot.service.ts @@ -253,7 +253,7 @@ export class BotService { * * @returns A promise that resolves with a boolean indicating whether the conversation is active and a matching block was found. */ - async handleIncomingMessage( + async handleOngoingConversationMessage( convo: ConversationFull, event: EventWrapper, ) { @@ -272,8 +272,15 @@ export class BotService { max_attempts: 0, }; + // We will avoid having multiple matches when we are not at the start of a conversation + // and only if local fallback is enabled + const canHaveMultipleMatches = !fallbackOptions.active; // Find the next block that matches - const matchedBlock = await this.blockService.match(nextBlocks, event); + const matchedBlock = await this.blockService.match( + nextBlocks, + event, + canHaveMultipleMatches, + ); // If there is no match in next block then loopback (current fallback) // This applies only to text messages + there's a max attempt to be specified let fallbackBlock: BlockFull | undefined; @@ -376,7 +383,7 @@ export class BotService { 'Existing conversations', ); this.logger.debug('Conversation has been captured! Responding ...'); - return await this.handleIncomingMessage(conversation, event); + return await this.handleOngoingConversationMessage(conversation, event); } catch (err) { this.logger.error( 'An error occurred when searching for a conversation ', diff --git a/api/src/utils/test/mocks/block.ts b/api/src/utils/test/mocks/block.ts index 32a26f70..0de1196f 100644 --- a/api/src/utils/test/mocks/block.ts +++ b/api/src/utils/test/mocks/block.ts @@ -18,6 +18,7 @@ import { OutgoingMessageFormat } from '@/chat/schemas/types/message'; import { BlockOptions, ContentOptions } from '@/chat/schemas/types/options'; import { NlpPattern, Pattern } from '@/chat/schemas/types/pattern'; import { QuickReplyType } from '@/chat/schemas/types/quick-reply'; +import { WEB_CHANNEL_NAME } from '@/extensions/channels/web/settings'; import { modelInstance } from './misc'; @@ -391,3 +392,10 @@ export const blockCarouselMock = { } as unknown as BlockFull; export const blocks: BlockFull[] = [blockGetStarted, blockEmpty]; + +export const mockWebChannelData: SubscriberChannelDict[typeof WEB_CHANNEL_NAME] = + { + isSocket: true, + ipAddress: '1.1.1.1', + agent: 'Chromium', + };