From e0fb926fd6c0a1b51d7bfb0f1ce2498e672ad938 Mon Sep 17 00:00:00 2001 From: Umberto Sgueglia Date: Fri, 16 Oct 2026 15:16:20 +0200 Subject: [PATCH] fix: name the lfx insights project in the review card and shorten the hand-off reason Refs linuxfoundation/lfx-cdp-insights-ops#822 Signed-off-by: Umberto Sgueglia --- .../slack/answerRequestCommand.test.ts | 3 +- .../services/slack/answerRequestCommand.ts | 12 ++--- .../src/project-catalog/types.ts | 1 + .../src/botThreadReply.test.ts | 4 +- .../src/classifyCatalogRow.test.ts | 15 ++++++ .../src/classifyCatalogRow.ts | 2 +- .../src/requestQuestion.test.ts | 51 ++++++++++++++++++- .../project-onboarding/src/requestQuestion.ts | 28 +++++++++- .../src/requestResolver.test.ts | 1 + .../project-onboarding/src/requestResolver.ts | 7 ++- 10 files changed, 108 insertions(+), 16 deletions(-) diff --git a/backend/src/services/slack/answerRequestCommand.test.ts b/backend/src/services/slack/answerRequestCommand.test.ts index 849a0cc648..9f760259f9 100644 --- a/backend/src/services/slack/answerRequestCommand.test.ts +++ b/backend/src/services/slack/answerRequestCommand.test.ts @@ -49,6 +49,7 @@ const row = { sourceUrl: 'https://acme.slack.com/archives/C1/p1', action: 'ask', requestedBy: 'U-REQUESTER', + projectName: 'Acme Widgets', requestQuestion: question, } as any @@ -110,7 +111,7 @@ describe('runAnswerRequestCommand', () => { CATALOG_ID, expect.objectContaining({ action: 'review', - reviewReason: 'the requester couldn\'t answer "Did you mean Acme?" (No, ask the CDP team)', + reviewReason: "the requester said Acme Widgets isn't Acme", proposedAction: 'human_review', }), ) diff --git a/backend/src/services/slack/answerRequestCommand.ts b/backend/src/services/slack/answerRequestCommand.ts index 80451b86b3..54268c684c 100644 --- a/backend/src/services/slack/answerRequestCommand.ts +++ b/backend/src/services/slack/answerRequestCommand.ts @@ -1,14 +1,12 @@ import { answerProjectCatalogQuestion, findProjectCatalogById } from '@crowd/data-access-layer' -import { - IDbProjectCatalog, - IRequestQuestionOption, -} from '@crowd/data-access-layer/src/project-catalog/types' +import { IDbProjectCatalog } from '@crowd/data-access-layer/src/project-catalog/types' import { applyRequestStatus, buildBotAnsweredContent, decodeAnswerValue, describeAnsweredQuestion, findQuestionOption, + handOffReason, refreshRequestStatusMessage, } from '@crowd/project-onboarding' import { getSlackBotConfig } from '@crowd/slack' @@ -46,10 +44,6 @@ function canAnswer(row: IDbProjectCatalog, actorId: string): boolean { return row.requestedBy === actorId || getSlackBotConfig().reviewerUserIds.includes(actorId) } -function handOffReason(question: string, option: IRequestQuestionOption): string { - return `the requester couldn't answer "${question}" (${option.label})` -} - async function startAnswerEvaluation( temporal: OnboardingRequestTemporal | null, catalogId: string, @@ -109,7 +103,7 @@ export async function runAnswerRequestCommand({ answeredBy: actorId, hints: option.hints ?? null, action: option.handOff ? 'review' : 'evaluate', - reviewReason: option.handOff ? handOffReason(row.requestQuestion.text, option) : null, + reviewReason: option.handOff ? handOffReason(row, row.requestQuestion) : null, proposedAction: option.handOff ? 'human_review' : null, }) if (!answered) { diff --git a/services/libs/data-access-layer/src/project-catalog/types.ts b/services/libs/data-access-layer/src/project-catalog/types.ts index 62dd75e8c5..9729ab94a7 100644 --- a/services/libs/data-access-layer/src/project-catalog/types.ts +++ b/services/libs/data-access-layer/src/project-catalog/types.ts @@ -71,6 +71,7 @@ export interface IRequestQuestionOption { label: string hints?: IRequestHints handOff?: boolean + suggested?: boolean } export interface IRequestQuestion { diff --git a/services/libs/project-onboarding/src/botThreadReply.test.ts b/services/libs/project-onboarding/src/botThreadReply.test.ts index 0d1a2d2ab5..40a6d0bcb8 100644 --- a/services/libs/project-onboarding/src/botThreadReply.test.ts +++ b/services/libs/project-onboarding/src/botThreadReply.test.ts @@ -124,10 +124,10 @@ describe('botThreadReply', () => { ...project, classificationKind: 'ambiguous', proposedAction: 'human_review', - reviewReason: 'the requester couldn\'t answer "Did you mean Acme?" (No, ask the CDP team)', + reviewReason: "the requester said Acme Widgets isn't Acme", }), ).toBe( - `๐Ÿ‘€ ${REPO_LABEL}: the requester couldn't answer "Did you mean Acme?" (No, ask the CDP team). I've asked the CDP team to pick the right project and add it by hand; they'll confirm here.`, + `๐Ÿ‘€ ${REPO_LABEL}: the requester said Acme Widgets isn't Acme. I've asked the CDP team to pick the right project and add it by hand; they'll confirm here.`, ) expect( buildBotReviewReply({ diff --git a/services/libs/project-onboarding/src/classifyCatalogRow.test.ts b/services/libs/project-onboarding/src/classifyCatalogRow.test.ts index 329ed149b4..00770dffba 100644 --- a/services/libs/project-onboarding/src/classifyCatalogRow.test.ts +++ b/services/libs/project-onboarding/src/classifyCatalogRow.test.ts @@ -278,6 +278,21 @@ describe('classifyCatalogRow', () => { }) }) + it('names the LFX Insights project when it differs from the PCC node that owns it', async () => { + const parent = { ...pccProject, name: 'Software Supply Chain Security Project', isLeaf: false } + const result = await classifyCatalogRow( + row({ projectName: 'Uptane' }), + deps({ candidates: [parent], cdpSegment: { ...segment, name: 'Uptane' } }), + ) + + expect(result.classification).toMatchObject({ + classificationKind: 'lf_in_cdp', + pccProjectId: 'pcc-1', + matchedProjectName: 'Uptane', + matchedSegmentId: 'seg-1', + }) + }) + it('routes an LF project in CDP to review when auto apply is off', async () => { const result = await classifyCatalogRow( row(), diff --git a/services/libs/project-onboarding/src/classifyCatalogRow.ts b/services/libs/project-onboarding/src/classifyCatalogRow.ts index efb854979b..8aeb3c0d65 100644 --- a/services/libs/project-onboarding/src/classifyCatalogRow.ts +++ b/services/libs/project-onboarding/src/classifyCatalogRow.ts @@ -118,7 +118,7 @@ function toProposal(resolution: OnboardingResolution): IProposal { classificationKind: resolution.kind, proposedAction: resolution.action, pccProjectId: resolution.pccProject.projectId, - matchedProjectName: resolution.pccProject.name, + matchedProjectName: resolution.segment.name, matchedSegmentId: resolution.segment.segmentId, } case 'lf_owner_in_cdp': diff --git a/services/libs/project-onboarding/src/requestQuestion.test.ts b/services/libs/project-onboarding/src/requestQuestion.test.ts index bb13f39ade..10af348649 100644 --- a/services/libs/project-onboarding/src/requestQuestion.test.ts +++ b/services/libs/project-onboarding/src/requestQuestion.test.ts @@ -7,6 +7,7 @@ import { describeAnsweredQuestion, encodeAnswerValue, findQuestionOption, + handOffReason, } from './requestQuestion' const question = { @@ -17,10 +18,33 @@ const question = { id: 'yes', label: 'Yes, Acme', hints: { pccProject: { projectId: 'p', name: 'Acme', slug: 'a', isLeaf: true } }, + suggested: true, }, { id: 'no', label: 'No, ask the CDP team', handOff: true }, ], } +const pickQuestion = { + kind: 'pick_project' as const, + text: 'There are 2 Acme projects in LFX Insights. Which one should this repo go in?', + options: [ + { id: 'seg-1', label: 'Acme ยท 28 repos', hints: { segmentId: 'seg-1' } }, + { id: 'seg-2', label: 'Acme ยท no repos yet', hints: { segmentId: 'seg-2' } }, + { id: 'not_sure', label: 'Not sure, ask the CDP team', handOff: true }, + ], +} +const conflictQuestion = { + kind: 'lf_conflict' as const, + text: "You said Acme isn't an LF project, but I found an LF project called Acme Labs. Which is right?", + options: [ + { + id: 'use_lf', + label: 'Use the LF project Acme Labs', + hints: { pccProject: { projectId: 'p', name: 'Acme Labs', slug: 'a', isLeaf: true } }, + }, + { id: 'not_lf', label: "It's not LF, treat it as new", hints: { treatAsNew: true } }, + { id: 'not_sure', label: 'Not sure, ask the CDP team', handOff: true }, + ], +} const row = { id: '8f1c2a44-5b7e-4d2f-9c1a-3e6f7a8b9c0d', repoUrl: 'https://github.com/acme/widget', @@ -28,7 +52,7 @@ const row = { } describe('buildBotQuestionContent', () => { - it('posts the question with one button per option, the first one primary', () => { + it('posts the question with one button per option, the suggested one primary', () => { const content = buildBotQuestionContent(row) expect(content?.text).toBe( @@ -56,6 +80,14 @@ describe('buildBotQuestionContent', () => { expect(buildBotQuestionContent({ ...row, requestQuestion: null })).toBeNull() }) + it('leaves every button plain when the bot has no suggestion', () => { + const content = buildBotQuestionContent({ ...row, requestQuestion: pickQuestion }) + + const buttons = content?.blocks?.[1] as { elements: Array<{ style?: string }> } + expect(buttons.elements).toHaveLength(3) + expect(buttons.elements.every((button) => button.style === undefined)).toBe(true) + }) + it('replaces the question with the answer and what happens next, buttons gone', () => { const answered = buildBotAnsweredContent(row, question, question.options[0], 'U1') expect(answered.text).toContain( @@ -78,6 +110,23 @@ describe('answer values', () => { expect(decodeAnswerValue('abc|')).toBeNull() }) + it('describes a hand-off in a few words per question kind', () => { + const named = { projectName: 'Acme Widgets' } + expect(handOffReason(named, question)).toBe("the requester said Acme Widgets isn't Acme") + expect(handOffReason({ projectName: null }, question)).toBe( + "the requester didn't recognise the project I suggested", + ) + expect(handOffReason(named, pickQuestion)).toBe( + "the requester couldn't tell which Acme Widgets project this repo belongs to", + ) + expect(handOffReason({ projectName: null }, pickQuestion)).toBe( + "the requester couldn't tell which project this repo belongs to", + ) + expect(handOffReason(named, conflictQuestion)).toBe( + "the requester isn't sure whether Acme Widgets is the LF project Acme Labs", + ) + }) + it('finds options and describes an answered question', () => { expect(findQuestionOption(question, 'no')?.handOff).toBe(true) expect(findQuestionOption(question, 'maybe')).toBeNull() diff --git a/services/libs/project-onboarding/src/requestQuestion.ts b/services/libs/project-onboarding/src/requestQuestion.ts index 7c5081bbc1..597a76c695 100644 --- a/services/libs/project-onboarding/src/requestQuestion.ts +++ b/services/libs/project-onboarding/src/requestQuestion.ts @@ -45,7 +45,7 @@ function optionButtons(catalogId: string, options: IRequestQuestionOption[]): Kn action_id: answerActionId(start + offset), value: encodeAnswerValue(catalogId, option.id), text: { type: 'plain_text', text: buttonText(option.label) }, - ...(start + offset === 0 && !option.handOff ? { style: 'primary' as const } : {}), + ...(option.suggested ? { style: 'primary' as const } : {}), })), }) } @@ -85,6 +85,32 @@ export function buildBotAnsweredContent( return { text, blocks: mrkdwnSections(text) } } +function suggestedProjectName(question: IRequestQuestion): string | null { + return ( + question.options.find((option) => option.hints?.pccProject)?.hints?.pccProject?.name ?? null + ) +} + +export function handOffReason( + row: Pick, + question: IRequestQuestion, +): string { + const name = row.projectName + const suggested = suggestedProjectName(question) + switch (question.kind) { + case 'did_you_mean': + return name && suggested + ? `the requester said ${name} isn't ${suggested}` + : "the requester didn't recognise the project I suggested" + case 'pick_project': + return `the requester couldn't tell which ${name ? `${name} ` : ''}project this repo belongs to` + case 'lf_conflict': + return suggested + ? `the requester isn't sure whether ${name ?? 'this repo'} is the LF project ${suggested}` + : `the requester isn't sure whether ${name ?? 'this repo'} is an LF project` + } +} + export function describeAnsweredQuestion(question: IRequestQuestion): string | null { if (!question.answeredBy || !question.answer) { return null diff --git a/services/libs/project-onboarding/src/requestResolver.test.ts b/services/libs/project-onboarding/src/requestResolver.test.ts index c7245eb5ab..59f7fa8a06 100644 --- a/services/libs/project-onboarding/src/requestResolver.test.ts +++ b/services/libs/project-onboarding/src/requestResolver.test.ts @@ -131,6 +131,7 @@ describe('resolveOnboardingRequest', () => { isLeaf: true, }, }, + suggested: true, }, { id: 'no', label: 'No, ask the CDP team', handOff: true }, ], diff --git a/services/libs/project-onboarding/src/requestResolver.ts b/services/libs/project-onboarding/src/requestResolver.ts index 60a36b83de..269cf58e88 100644 --- a/services/libs/project-onboarding/src/requestResolver.ts +++ b/services/libs/project-onboarding/src/requestResolver.ts @@ -215,7 +215,12 @@ function didYouMean(projectName: string, best: IPccCandidate): OnboardingResolut kind: 'did_you_mean', text: `I couldn't find a project called "${projectName}". Did you mean ${best.name}?`, options: [ - { id: 'yes', label: `Yes, ${best.name}`, hints: { pccProject: toHintProject(best) } }, + { + id: 'yes', + label: `Yes, ${best.name}`, + hints: { pccProject: toHintProject(best) }, + suggested: true, + }, { id: 'no', label: 'No, ask the CDP team', handOff: true }, ], },