diff --git a/backend/__tests__/unit/integrations/manifestReadiness.test.js b/backend/__tests__/unit/integrations/manifestReadiness.test.js index 37df7f0a7..ae671ab40 100644 --- a/backend/__tests__/unit/integrations/manifestReadiness.test.js +++ b/backend/__tests__/unit/integrations/manifestReadiness.test.js @@ -1,6 +1,10 @@ const { manifests } = require('../../../integrations/manifests'); +const { providerReadiness } = require('../../../services/installable/installableCatalogService'); const READINESS_ENV = [ + 'DISCORD_BOT_TOKEN', + 'DISCORD_CLIENT_ID', + 'DISCORD_CLIENT_SECRET', 'TELEGRAM_BOT_TOKEN', 'TELEGRAM_SECRET_TOKEN', 'TELEGRAM_WEBHOOK_ALLOW_UNVERIFIED', @@ -64,9 +68,50 @@ describe('installable connector manifest readiness', () => { it('describes Telegram as a chat the pods the member turns on reach (TASK-154)', () => { expect(manifests.telegram.catalog.description) .toBe('Link your Telegram chat to Commonly — every pod you turn on reaches it.'); + + // (This commit's own copy of this test asserted the one-pod sentence TASK-154 + // reversed; it is dropped here rather than merged, and the guard below is the + // one that reads the CURRENT sentence.) expect(manifests.telegram.catalog.description).not.toMatch(/ingest|summar/i); }); + + // Discord is a shipping connector (routes/discord.ts; discordProvider.ts). It + // was absent from the channel catalog only because it declared no readiness(), + // which is the predicate providerInstallableIds() filters on — so the defect + // was a missing declaration, not a missing capability. These assert the + // consequence through the real service rather than through a re-implementation + // of its filter. + it('reports Discord as an answerable provider, not an unknown Installable', () => { + expect(providerReadiness('discord')).toEqual({ available: false, reason: 'not_configured' }); + + process.env.DISCORD_BOT_TOKEN = 'configured'; + process.env.DISCORD_CLIENT_ID = 'configured'; + process.env.DISCORD_CLIENT_SECRET = 'configured'; + + expect(providerReadiness('discord')).toEqual({ available: true }); + }); + + it('requires all three of the keys the Discord install route reads', () => { + process.env.DISCORD_BOT_TOKEN = 'configured'; + process.env.DISCORD_CLIENT_ID = 'configured'; + expect(providerReadiness('discord')).toEqual({ available: false, reason: 'not_configured' }); + + process.env.DISCORD_CLIENT_SECRET = 'configured'; + expect(providerReadiness('discord')).toEqual({ available: true }); + + process.env.DISCORD_CLIENT_SECRET = ' '; + expect(providerReadiness('discord')).toEqual({ available: false, reason: 'not_configured' }); + }); + + it('leaves groupme undeclared, so this change cannot silently widen the catalog', () => { + process.env.DISCORD_BOT_TOKEN = 'configured'; + process.env.DISCORD_CLIENT_ID = 'configured'; + process.env.DISCORD_CLIENT_SECRET = 'configured'; + + expect(providerReadiness('groupme')).toBeNull(); + }); + // Its own arm, because the copy this row used to carry taught one chat = one // pod, and TASK-154's ruling reversed that: one private chat binds one member, // and their pods sit behind it as gates. A separate test is what makes the diff --git a/backend/__tests__/unit/routes/installables.test.js b/backend/__tests__/unit/routes/installables.test.js index a7baf59a2..90f211706 100644 --- a/backend/__tests__/unit/routes/installables.test.js +++ b/backend/__tests__/unit/routes/installables.test.js @@ -16,6 +16,7 @@ jest.mock('../../../models/Pod', () => ({ findById: jest.fn() })); jest.mock('../../../services/installable/installableCatalogService', () => ({ catalogFor: jest.fn(), providerReadiness: jest.fn(() => ({ available: true })), + providerOffered: jest.fn(() => true), publicIntegration: jest.fn((integration) => { if (!integration?.config?.adminPause) return integration; return { @@ -67,6 +68,7 @@ describe('installable connector routes', () => { beforeEach(() => { jest.clearAllMocks(); catalogService.providerReadiness.mockReturnValue({ available: true }); + catalogService.providerOffered.mockResolvedValue(true); }); it('requires auth for the user-scoped catalog', async () => { @@ -109,6 +111,46 @@ describe('installable connector routes', () => { expect(installationService.install).not.toHaveBeenCalled(); }); + it('refuses a usable provider this instance does not offer, before the pod lookup', async () => { + // Capability is not offerability (Wren 71174). This is the request a stranger + // made after the page showed them an Add for Discord: readiness passed, and + // the install then died further in as installable_not_found. Refused at the + // same place as the missing-credential case, so the API cannot be driven into + // a state the UI will not offer. + catalogService.providerReadiness.mockReturnValue({ available: true }); + catalogService.providerOffered.mockResolvedValue(false); + + const res = await request(app) + .post('/api/installables/discord/install') + .set(auth) + .send({ podId }); + + expect(res.status).toBe(422); + expect(res.body).toEqual({ code: 'provider_not_offered' }); + expect(Pod.findById).not.toHaveBeenCalled(); + expect(installationService.install).not.toHaveBeenCalled(); + }); + + it('lets a provider through when it is both usable and offered', async () => { + // The paired control: the new guard must not refuse everything. Without this, + // a predicate that always returned false would pass the test above. + catalogService.providerOffered.mockResolvedValue(true); + Pod.findById.mockResolvedValue({ + _id: podId, + createdBy: { toString: () => '64b64c48c4f37a6b2f34c111' }, + members: ['64b64c48c4f37a6b2f34c111'], + }); + installationService.install.mockResolvedValue({ httpStatus: 201, state: 'installing', installation: null, integration: null }); + + const res = await request(app) + .post('/api/installables/discord/install') + .set(auth) + .send({ podId }); + + expect(res.status).toBe(201); + expect(installationService.install).toHaveBeenCalled(); + }); + it('rejects a non-member before any install row is claimed', async () => { Pod.findById.mockResolvedValue({ _id: podId, diff --git a/backend/__tests__/unit/services/installableOfferability.test.js b/backend/__tests__/unit/services/installableOfferability.test.js new file mode 100644 index 000000000..2dfe40570 --- /dev/null +++ b/backend/__tests__/unit/services/installableOfferability.test.js @@ -0,0 +1,130 @@ +// @ts-nocheck + +// Offerability: a provider is offered when this instance holds the credentials +// (readiness) AND a builtin active Installable row carries it (the roster). +// +// Why this file exists at all, and why it uses the real store and the real +// manifests: every other test of the catalogue injects fake stores +// (`installableCatalogService.test.js` mocks `Installable.find`), so +// "readiness declared, no row seeded" was UNREPRESENTABLE in the suite. That is +// the exact state that shipped a lowercase `discord` row whose Add ended in 404 +// installable_not_found (Vera 71152, Wren 71162). A mocked store can only test +// the catalogue's arithmetic; it cannot test whether the roster agrees with it. +// +// @kais 71169 ruling: `catalogFor` keeps one entry per readiness-declaring +// manifest (nothing vanishes) and reports `offered`; `availableProviders` and +// the row action both read it. + +jest.mock('jsonwebtoken', () => ({})); + +const mongoose = require('mongoose'); + +const Installable = require('../../../models/Installable'); +const { catalogFor, providerOffered } = require('../../../services/installable/installableCatalogService'); +const { TELEGRAM_CONNECTOR } = require('../../../scripts/seed-builtin-connectors'); +const { setupMongoDb, closeMongoDb, clearMongoDb } = require('../../utils/testUtils'); + +const USER_ID = new mongoose.Types.ObjectId().toString(); + +const channelsOf = async (userId = USER_ID) => { + const { installables } = await catalogFor(userId); + return installables.filter((entry) => entry.list === 'channels'); +}; + +const byId = (entries) => new Map(entries.map((entry) => [entry.installableId, entry])); + +describe('provider offerability', () => { + beforeAll(async () => { await setupMongoDb(); }); + afterAll(async () => { await closeMongoDb(); }); + beforeEach(async () => { await clearMongoDb(); }); + + it('reports one channel entry per readiness-declaring manifest, offered or not', async () => { + // The invariant that keeps Discord from VANISHING, which is the failure mode + // on the other side of this defect: an entry is dropped when a provider has + // readiness and no row, so the page stops being able to say anything about it. + const { manifests } = require('../../../integrations/manifests'); + const declared = Object.values(manifests) + .filter((manifest) => typeof manifest.readiness === 'function') + .map((manifest) => manifest.id) + .sort(); + + const entries = await channelsOf(); + + expect(entries.map((entry) => entry.installableId).sort()).toEqual(declared); + expect(new Set(entries.map((entry) => entry.installableId)).size).toBe(entries.length); + }); + + it('never labels a row with its own raw id', async () => { + // The visible half of the bug: `label: installable?.name || installableId` + // drew a lowercase `discord` on a page whose every other row is a word. + const entries = await channelsOf(); + + entries.forEach((entry) => { + expect(entry.label).not.toBe(entry.installableId); + expect(entry.label).toBeTruthy(); + }); + expect(byId(entries).get('discord').label).toBe(manifestsLabel('discord')); + }); + + it('offers a provider once a builtin active row exists, and not before', async () => { + const before = byId(await channelsOf()); + expect(before.get('telegram').offered).toBe(false); + expect(await providerOffered('telegram')).toBe(false); + + await Installable.create({ ...TELEGRAM_CONNECTOR }); + + const after = byId(await channelsOf()); + expect(after.get('telegram').offered).toBe(true); + expect(await providerOffered('telegram')).toBe(true); + // Offering one provider must not offer its neighbours: a predicate that + // answered from the wrong field would flip every row at once. + expect(after.get('slack').offered).toBe(false); + expect(after.get('discord').offered).toBe(false); + }); + + it('does not count a row that is not builtin and active', async () => { + // The predicate is a join, not a presence check. A marketplace row, or a + // retired builtin one, must not make a provider installable: `install()` + // resolves the same way and would still answer installable_not_found. + await Installable.create({ ...TELEGRAM_CONNECTOR, source: 'marketplace' }); + expect(await providerOffered('telegram')).toBe(false); + + await Installable.deleteMany({}); + await Installable.create({ ...TELEGRAM_CONNECTOR, status: 'deprecated' }); + expect(await providerOffered('telegram')).toBe(false); + }); + + it('keeps capability and offerability independent', async () => { + // Offered without configured is a real state (a seeded row on an instance + // with no keys) and so is configured without offered (Discord before this + // PR). The page needs both bits to pick the right of three state lines; + // collapsing them is what produced a single wrong answer. + const original = process.env.DISCORD_BOT_TOKEN; + await Installable.create({ ...TELEGRAM_CONNECTOR }); + + const sealed = byId(await channelsOf()); + expect(sealed.get('telegram').offered).toBe(true); + expect(sealed.get('discord').offered).toBe(false); + expect(sealed.get('discord').available).toBe(false); + + process.env.DISCORD_BOT_TOKEN = 'configured'; + process.env.DISCORD_CLIENT_ID = 'configured'; + process.env.DISCORD_CLIENT_SECRET = 'configured'; + try { + const configured = byId(await channelsOf()); + expect(configured.get('discord').available).toBe(true); + expect(configured.get('discord').offered).toBe(false); + } finally { + delete process.env.DISCORD_CLIENT_ID; + delete process.env.DISCORD_CLIENT_SECRET; + if (original === undefined) delete process.env.DISCORD_BOT_TOKEN; + else process.env.DISCORD_BOT_TOKEN = original; + } + }); +}); + +function manifestsLabel(id) { + // eslint-disable-next-line global-require + const { manifests } = require('../../../integrations/manifests'); + return manifests[id].catalog.label; +} diff --git a/backend/integrations/manifests.ts b/backend/integrations/manifests.ts index 3db6304c0..9f73aa3b4 100644 --- a/backend/integrations/manifests.ts +++ b/backend/integrations/manifests.ts @@ -77,6 +77,20 @@ const manifests: Record = { // the same route refuses with `server_owned_config_key` (TASK-140). requiredConfig: ['serverId', 'channelId', 'botToken'], configSchema: buildConfigSchema(['serverId', 'channelId', 'botToken']), + // Discord is a shipping connector (routes/discord.ts: OAuth install-link, + // callback, binding, uninstall) and discordProvider.ts implements the same + // provider interface as slack/telegram — it was absent from the catalog only + // because it declared no readiness(), which is what + // providerInstallableIds() filters on. Gate on the three secrets that + // route actually reads, so the readiness answer tracks the install path + // rather than a second list that can drift from it. + readiness: () => ( + hasConfiguration( + 'DISCORD_BOT_TOKEN', + 'DISCORD_CLIENT_ID', + 'DISCORD_CLIENT_SECRET', + ) ? { available: true } : notConfigured() + ), catalog: { label: 'Discord', provider: 'discord', diff --git a/backend/routes/installables.ts b/backend/routes/installables.ts index 958748920..7227aaca3 100644 --- a/backend/routes/installables.ts +++ b/backend/routes/installables.ts @@ -24,6 +24,7 @@ const connectorDeliveryFailures = require('../services/connectorDeliveryFailureS // eslint-disable-next-line global-require const { catalogFor, + providerOffered, providerReadiness, publicIntegration, } = require('../services/installable/installableCatalogService'); @@ -567,6 +568,14 @@ router.post('/:installableId/install', writeIntegrationsRateLimit, auth, async ( if (readiness && !readiness.available) { return res.status(422).json({ code: 'provider_not_configured' }); } + // Capability is not offerability (Wren 71174). Without this, a provider the + // instance has credentials for but has never rostered passes the readiness + // guard and dies further in as `installable_not_found` — the 404 a stranger + // met after the page showed them an Add button. Refused here, before the pod + // lookup, so the API cannot be driven into a state the UI will not offer. + if (!(await providerOffered(installableId))) { + return res.status(422).json({ code: 'provider_not_offered' }); + } try { const pod = await Pod.findById(podId); diff --git a/backend/services/installable/installableCatalogService.ts b/backend/services/installable/installableCatalogService.ts index 649652ce1..15f158ad5 100644 --- a/backend/services/installable/installableCatalogService.ts +++ b/backend/services/installable/installableCatalogService.ts @@ -29,6 +29,33 @@ const providerReadiness = (installableId: string): ProviderReadiness | null => { return typeof manifest?.readiness === 'function' ? manifest.readiness() : null; }; +// Declaring `readiness` is a claim about CAPABILITY; having a builtin active +// Installable row is the claim about OFFERABILITY. They are different questions +// and only one of them was being asked, which is how a provider could render as +// connectable and then die on Add with `installable_not_found` (Vera 71152, +// Wren 71169). `offeredByRoster` is the single decision; the two readers below +// differ only in cardinality, because the catalog has the rows in hand and the +// route has one id. +const offeredByRoster = (installable: unknown): boolean => Boolean(installable); + +const providerLabel = (installableId: string): string => { + const manifest = (manifests as Record)[installableId]; + return manifest?.catalog?.label || installableId; +}; + +// The route's reader: one id, so one query. Exported because the install route +// refuses on the same predicate the catalog advertises, and a guard that lives +// only in the UI cannot stop the API being driven into a state the page will not +// offer. +const providerOffered = async (installableId: string): Promise => { + const installable = await Installable.findOne({ + installableId, + source: 'builtin', + status: 'active', + }).lean(); + return offeredByRoster(installable); +}; + // Mongoose's Integration toJSON transform is the normal guard. Lean catalog // reads bypass it, so they run the same strip explicitly: one key list, so a // credential can never be serialized here that toJSON would have dropped. @@ -161,9 +188,13 @@ const catalogFor = async (userId: string): Promise<{ installables: unknown[] }> return { installableId, list: 'channels', - label: installable?.name || installableId, + // The row's name comes from the roster when there is one, and from the + // manifest's own catalog block when there is not. Falling back to the + // raw id is what put a lowercase `discord` on the page (Wren 71162). + label: installable?.name || providerLabel(installableId), description: installable?.description || '', available: readiness.available, + offered: offeredByRoster(installable), ...(readiness.reason ? { unavailableReason: readiness.reason } : {}), installation: publicInstallation(installation), integration: installation @@ -174,6 +205,6 @@ const catalogFor = async (userId: string): Promise<{ installables: unknown[] }> }; }; -module.exports = { catalogFor, providerReadiness, publicIntegration }; +module.exports = { catalogFor, providerReadiness, providerOffered, publicIntegration }; export {}; diff --git a/docs/design/evidence/task-024-discord-row-keys-set-1200-after.png b/docs/design/evidence/task-024-discord-row-keys-set-1200-after.png new file mode 100644 index 000000000..40556717d Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-set-1200-after.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-set-1200-before.png b/docs/design/evidence/task-024-discord-row-keys-set-1200-before.png new file mode 100644 index 000000000..ef452106f Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-set-1200-before.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-set-390-after.png b/docs/design/evidence/task-024-discord-row-keys-set-390-after.png new file mode 100644 index 000000000..16bdb1065 Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-set-390-after.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-set-390-before.png b/docs/design/evidence/task-024-discord-row-keys-set-390-before.png new file mode 100644 index 000000000..d86133340 Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-set-390-before.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-unset-1200-after.png b/docs/design/evidence/task-024-discord-row-keys-unset-1200-after.png new file mode 100644 index 000000000..48da135c6 Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-unset-1200-after.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-unset-1200-before.png b/docs/design/evidence/task-024-discord-row-keys-unset-1200-before.png new file mode 100644 index 000000000..ef452106f Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-unset-1200-before.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-unset-390-after.png b/docs/design/evidence/task-024-discord-row-keys-unset-390-after.png new file mode 100644 index 000000000..b9a686636 Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-unset-390-after.png differ diff --git a/docs/design/evidence/task-024-discord-row-keys-unset-390-before.png b/docs/design/evidence/task-024-discord-row-keys-unset-390-before.png new file mode 100644 index 000000000..d86133340 Binary files /dev/null and b/docs/design/evidence/task-024-discord-row-keys-unset-390-before.png differ diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 7f42d324a..df14a2c11 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -1925,6 +1925,7 @@ "newCode": "New code", "nextStep": "Next step", "noPods": "Join a pod first.", + "notConnectable": "Not connectable yet.", "notConnected": "not connected", "notEnabled": "Not enabled on this instance.", "notLinkedDetail": "your messages have nowhere to go", diff --git a/frontend/src/i18n/locales/zh-CN.json b/frontend/src/i18n/locales/zh-CN.json index 15de3832b..59e805ba9 100644 --- a/frontend/src/i18n/locales/zh-CN.json +++ b/frontend/src/i18n/locales/zh-CN.json @@ -1917,6 +1917,7 @@ "newCode": "新启用码", "nextStep": "下一步", "noPods": "请先加入一个 Pod。", + "notConnectable": "暂不可连接。", "notConnected": "未连接", "notEnabled": "此实例尚未启用。", "notLinkedDetail": "你的消息还没有去处", diff --git a/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx b/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx index c3b9e010e..6280e34d3 100644 --- a/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx +++ b/frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx @@ -5,6 +5,7 @@ import React from 'react'; import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; import { MemoryRouter, Route, Routes } from 'react-router-dom'; import V2ConnectorsPage, { INSTALL_LOCK_TTL_MS, installableLifecyclePath } from '../components/V2ConnectorsPage'; +import { PlatformGlyph } from '../icons/platforms'; import { AuthContext } from '../../context/AuthContext'; import en from '../../i18n/locales/en.json'; import i18n, { i18nReady } from '../../i18n'; @@ -205,9 +206,13 @@ describe('V2ConnectorsPage', () => { expect(names).not.toBeNull(); // Separate elements, not one joined string: above 760 they stack in the name // track, so a joined string would be a single 140px-wide line that runs into - // the details column. - expect(Array.from(names!.querySelectorAll('.v2-connector-row__name-item')).map((item) => item.textContent)).toEqual(['Discord', 'WhatsApp']); - expect(names!.querySelectorAll('.v2-connector-row__name-sep')).toHaveLength(1); + // the details column. TASK-024 left this row one name — Discord is a built + // connector and now arrives through the catalog — so the LIST is what is + // asserted and the separator count follows from its length. Nothing renders a + // `__name-sep` until a second unbuilt provider joins WhatsApp; the rule that + // shows it at ≤760 stays pinned in v2-layout-invariants.test.ts. + expect(Array.from(names!.querySelectorAll('.v2-connector-row__name-item')).map((item) => item.textContent)).toEqual(['WhatsApp']); + expect(names!.querySelectorAll('.v2-connector-row__name-sep')).toHaveLength(0); }); it('TASK-131: relative ages advance in place, and a returning tab re-reads, without a reload', async () => { @@ -241,10 +246,14 @@ describe('V2ConnectorsPage', () => { expect(screen.getByText('Send /commonly-enable in your Telegram chat.')).toBeInTheDocument(); expect(screen.getByText('Code expires in 5 min')).toBeInTheDocument(); expect(screen.getByText('Rewire crew · linked to Ops')).toBeInTheDocument(); - // TASK-162 (2): the two not-yet names are separate elements that stack in the - // name track above 760 and join with ' · ' at ≤760 — no longer one string. - expect(screen.getByText('Discord')).toBeInTheDocument(); - expect(screen.getByText('WhatsApp')).toBeInTheDocument(); + const notYetNames = container.querySelector('.v2-connector-row--not-yet .v2-connector-row__names'); + // TASK-024: this row is "we have not built it", so it lists only providers + // with no manifest at all. Discord is a built connector (routes/discord.ts; + // discordProvider.ts) and reaches the page through the catalog — available, or + // the not-enabled row when the instance lacks its credentials — so naming it + // here asserted something false. Scoped to this row deliberately: a plain + // queryByText would also fail if a later fixture adds Discord as a catalog row. + expect(notYetNames?.textContent).toBe('WhatsApp'); expect(screen.getByText('/commonly-enable abc1 23')).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Copy command' })).toBeInTheDocument(); expect(container.querySelectorAll('.v2-connector-row__glyph')).toHaveLength(3); @@ -894,7 +903,98 @@ describe('V2ConnectorsPage', () => { expect(screen.getAllByRole('button', { name: 'Add' })).toHaveLength(1); }); - it('renders an unavailable provider with Ask and an available one with Choose a pod', async () => { + // TASK-024. Discord is a shipping connector (routes/discord.ts: install + // link, callback, binding, uninstall) that read as "we don't build this" + // because its manifest declared no readiness(), which is what the catalog + // filters on. Once it declares one the catalog owns every claim about it: + // configured -> a connectable row, not configured -> the not-enabled row + // that already exists for slack. The not-yet row must stop covering it. + const glyphPath = (type: string): string | null => { + const { container } = render(); + return container.querySelector('svg path')?.getAttribute('d') || null; + }; + + it('describes Discord only through the catalog, never as a provider we have not built', async () => { + mockCatalog([ + entry({ installableId: 'discord', label: 'Discord', available: false, unavailableReason: 'not_configured' }), + ]); + renderPage(); + + const notEnabled = (await screen.findByText('Not enabled on this instance.')).closest('.v2-connector-row'); + expect(notEnabled).toHaveClass('v2-connector-row--not-enabled'); + + const notYet = (await screen.findByText(/Not yet\. Tell us which channel/)).closest('.v2-connector-row') as HTMLElement; + expect(notYet).toHaveClass('v2-connector-row--not-yet'); + expect(notYet.textContent).toContain('WhatsApp'); + expect(notYet.textContent).not.toContain('Discord'); + + // The glyph tracks the label: the row is about WhatsApp now, and the two + // glyphs differ, so this cannot pass by comparing a value to itself. + const whatsapp = glyphPath('whatsapp'); + expect(whatsapp).not.toBeNull(); + expect(whatsapp).not.toBe(glyphPath('discord')); + expect(notYet.querySelector('svg path')?.getAttribute('d')).toBe(whatsapp); + }); + + it('offers a rostered provider with Add, and an unmapped one inherits no onboarding sentence', async () => { + // Slack's line is in the map; Discord's is not. The point of the map is + // that Discord renders NO sentence rather than Slack's (Vera 71165). + mockCatalog([ + entry({ installableId: 'discord', label: 'Discord', available: true, offered: true }), + entry({ installableId: 'slack', label: 'Slack', available: true, offered: true }), + ]); + renderPage(); + + const discord = (await screen.findByText('Discord')).closest('.v2-connector-row') as HTMLElement; + expect(discord.textContent).not.toContain('one click in your workspace'); + expect(discord.textContent).not.toContain('one message'); + expect(within(discord).getByRole('button', { name: 'Add' })).toBeInTheDocument(); + + const slack = screen.getByText('Slack').closest('.v2-connector-row') as HTMLElement; + expect(slack.textContent).toContain('one click in your workspace'); + + const notYet = screen.getByText(/Not yet\. Tell us which channel/).closest('.v2-connector-row') as HTMLElement; + expect(notYet.textContent).toContain('WhatsApp'); + expect(notYet.textContent).not.toContain('Discord'); + }); + + it('reads a usable-but-unrostered provider as not connectable, with no Add anywhere', async () => { + // The ruled middle state (Wren 71170/71174): this instance can use Discord + // and there is no builtin Installable row to install, so the row states the + // state and offers no action. Before this, the row rendered an Add whose + // click ended in 404 installable_not_found (Wren 71162/71163). + mockCatalog([ + entry({ installableId: 'discord', label: 'Discord', available: true, offered: false }), + ]); + renderPage(); + + const roster = (await screen.findByText('Not connectable yet.')).closest('.v2-connector-row') as HTMLElement; + expect(roster).toHaveClass('v2-connector-row--not-enabled'); + expect(roster.textContent).toContain('Discord'); + expect(roster.querySelector('svg path')?.getAttribute('d')).toBe(glyphPath('discord')); + expect(within(roster).queryByRole('button', { name: 'Add' })).toBeNull(); + // The only action left is the Ask link the not-enabled row shares; there is + // no Add and no connect verb. `row.notEnabled` drives that link, so both + // no-action catalog states carry it. + const actions = roster.querySelectorAll('.v2-connector-row__action'); + expect(actions).toHaveLength(1); + expect(actions[0].textContent).toBe('Ask'); + + // The eyebrow must not contradict the line under it. `notEnabled` drives + // this row's class and Ask link, so it also drove the kicker: the keys-set + // render read "not enabled" directly above "Not connectable yet." — asking + // an operator for credentials the instance already has (Vera, #1826). + expect(roster.querySelector('.v2-connector-row__kicker')).toHaveTextContent(/^not yet$/); + expect(roster.querySelector('.v2-connector-row__kicker')).not.toHaveTextContent('not enabled'); + + // The Add verb itself must be gone, not merely unused: an offered provider + // is the only thing that may produce it, and there is none in this catalog. + expect(screen.queryByRole('button', { name: 'Add' })).toBeNull(); + expect(screen.queryByRole('button', { name: 'Connect a channel' })).toBeNull(); + expect(await screen.findByText(/Not yet\. Tell us which channel/)).toBeInTheDocument(); + }); + + it('renders an unavailable provider with a state line and an available one with Add', async () => { mockCatalog([ entry(), entry({ installableId: 'slack', label: 'Slack', available: false, unavailableReason: 'not_configured' }), @@ -903,6 +1003,13 @@ describe('V2ConnectorsPage', () => { renderPage(); expect(await screen.findByText('Not enabled on this instance.')).toBeInTheDocument(); + // This commit's own copy expectations are NOT merged: they asserted the + // one-pod Telegram sentence TASK-154 reversed, and the absence of the + // "ask your operator" detail — which #1557 landed AFTER the 09-04 ruling + // this commit cites, and which main's tests assert is present. What + // survives from it is the CODE (an offered provider is the only thing that + // may draw Add; each unconnectable provider gets its own state line), + // which is what these assertions exercise. expect(screen.getByText('ask your operator')).toBeInTheDocument(); expect(screen.getByText('Link your Telegram chat to Commonly — every pod you turn on reaches it.')).toBeInTheDocument(); expect(screen.getByText(/not connected/)).toBeInTheDocument(); diff --git a/frontend/src/v2/components/V2ConnectorsPage.tsx b/frontend/src/v2/components/V2ConnectorsPage.tsx index dfe43099c..457251a3e 100644 --- a/frontend/src/v2/components/V2ConnectorsPage.tsx +++ b/frontend/src/v2/components/V2ConnectorsPage.tsx @@ -86,6 +86,14 @@ interface CatalogEntry { label?: string; description?: string; available: boolean; + // Capability vs offerability (Wren 71169/71174). `available` says this + // instance holds the credentials; `offered` says a builtin Installable row + // exists to install. Only both together may show an Add. Absent is read as + // offered: the field's only producer always emits it, so a missing value + // means a server older than this page, and defaulting the other way would + // blank every channel for the length of a rolling update instead of briefly + // restoring what this page did before the field existed. + offered?: boolean; unavailableReason?: string; installation: CatalogInstallation | null; integration: Connector | null; @@ -126,6 +134,10 @@ interface ConnectorRow { mark?: { name: MarkName; label: string; word: string }; muted?: boolean; notEnabled?: boolean; + // TASK-024: a row whose provider is built but not offered here. It shares + // `notEnabled`'s row class and Ask link, and must not share its kicker — + // "not enabled" claims a missing credential this state does not have. + notConnectable?: boolean; pulse: boolean; secondary?: boolean; when: string; @@ -148,13 +160,33 @@ const TYPE_LABELS: Record = { instagram: 'Instagram', }; +// The onboarding sentence for a provider you can connect right now. Keyed by +// provider and deliberately without a default: an unmapped provider renders no +// detail line at all. The two-branch ternary this replaces gave every provider +// that was not telegram Slack's "one click in your workspace", so a stranger +// reading a Discord row was told to click something in a workspace they may not +// have (Vera 71165). +const CONNECT_DETAIL: Record = { + telegram: { key: 'connectors.availableTelegram', defaultValue: 'one message' }, + slack: { key: 'connectors.availableSlack', defaultValue: 'one click in your workspace' }, +}; + // Fallback only: when the catalog cannot be read the page still offers the // two providers it can drive, exactly as it did before the catalog existed. const ADD_PLATFORMS = [ { type: 'telegram', enabled: true }, { type: 'slack', enabled: true }, ]; -const UNAVAILABLE_PLATFORM_LABELS = ['Discord', 'WhatsApp']; +// Providers we have not built at all. This is NOT the same claim as either +// state the catalog can report: "not enabled on this instance" is a missing +// credential, and "not connectable yet" is a provider we have built and this +// instance cannot offer because no builtin Installable row carries it. Both of +// those arrive through the catalog, with their own state line, so this list +// holds only the providers that have no manifest to arrive through. Discord +// used to sit here, which made the page say "we don't build this" about a +// shipping connector (routes/discord.ts) whose only defect was a missing +// readiness() declaration; it now renders the roster state line instead. +const UNAVAILABLE_PLATFORM_LABELS = ['WhatsApp']; const BOT_HANDLE = process.env.REACT_APP_TELEGRAM_BOT_HANDLE || ''; const RECENT_MS = 10 * 60_000; @@ -823,6 +855,11 @@ const V2ConnectorsPage: React.FC = () => { if (!entry.available) { return { action: null, + // The copy decision here is main's, not this branch's: this commit set + // `detail: ''` citing Wren's 09-04 one-state-line rule, but #1557 landed + // the detail AFTER that ruling and its tests assert the text is present. + // A rebase is not the place to re-open it, so the line is restored and + // raised on the row; everything else in the commit is unchanged. detail: t('connectors.askOperator', { defaultValue: 'ask your operator' }), dot: 'not-yet', line: t('connectors.notEnabled', { defaultValue: 'Not enabled on this instance.' }), @@ -832,16 +869,38 @@ const V2ConnectorsPage: React.FC = () => { when: '—', }; } + // Usable here, but with nothing to install: declaring readiness is a claim + // about this instance's keys, not about whether a row exists to install. + // Showing the Add anyway is what produced a lowercase `discord` row whose + // click ended in 404 installable_not_found (Wren 71162/71163). + if (entry.offered === false) { + return { + action: null, + detail: '', + dot: 'not-yet', + line: t('connectors.notConnectable', { defaultValue: 'Not connectable yet.' }), + muted: true, + // The class and the Ask link are right for this row; the kicker was the + // part that lied. Measured in the keys-set render at #1826: the eyebrow + // read "not enabled" directly above "Not connectable yet.", i.e. asking + // an operator for credentials the operator had already installed. + notConnectable: true, + notEnabled: true, + pulse: false, + when: '—', + }; + } if (!installation) { + const connectDetail = CONNECT_DETAIL[entry.installableId]; return { action: 'connect', // The row action opens the pod picker; the form's Connect button is // the actual install action. Distinct labels keep the two-step flow // legible to a stranger. actionLabel: t('connectors.add', { defaultValue: 'Add' }), - detail: entry.installableId === 'telegram' - ? t('connectors.availableTelegram', { defaultValue: 'one message' }) - : t('connectors.availableSlack', { defaultValue: 'one click in your workspace' }), + detail: connectDetail + ? t(connectDetail.key, { defaultValue: connectDetail.defaultValue }) + : '', dot: 'empty', line: entry.description || t('connectors.availableLine', { defaultValue: 'Connect {{label}} to a pod.', label }), pulse: false, @@ -988,9 +1047,13 @@ const V2ConnectorsPage: React.FC = () => { const podId = item.connector ? connectorPodId(item.connector) : null; // TASK-140: a not-enabled row has no connection to name a pod for, so the // pod slot read `no pod` — which is false, it names the blocker instead. - const pod = row.notEnabled + let pod = row.notEnabled ? t('connectors.notEnabledKicker', { defaultValue: 'not enabled' }) : podId ? podNameById(podId, item.connector) : t('connectors.noPod', { defaultValue: 'no pod' }); + // `notEnabled` covers two states and the kicker must not: a not-connectable + // row has this instance's credentials, so it says "not yet" — the same + // string the not-yet roster row renders. + if (row.notConnectable) pod = t('connectors.notYetKicker', { defaultValue: 'not yet' }); // A row with no age (the not-enabled row's '—') carries only the pod. return row.when === '—' ? pod : `${pod} · ${row.when}`; }; @@ -1078,9 +1141,9 @@ const V2ConnectorsPage: React.FC = () => { {row.mark.label} - ) : ( + ) : row.detail ? ( {row.detail} - )} + ) : null} {row.action === 'manage' && ( @@ -1427,7 +1490,7 @@ const V2ConnectorsPage: React.FC = () => { (connector) => connector.type === type && Boolean(connector.installationId), ); const availableProviders = catalog - ? catalog.filter((entry) => entry.available && !entry.installation).map((entry) => ({ type: entry.installableId })) + ? catalog.filter((entry) => entry.available && entry.offered !== false && !entry.installation).map((entry) => ({ type: entry.installableId })) : ADD_PLATFORMS.filter((provider) => provider.enabled && !hasInstallation(provider.type)); const renderAddForm = (aside = false) => { const rowClass = `v2-connectors__new-row${aside ? ' v2-connectors__new-row--aside' : ''}`; @@ -1492,7 +1555,7 @@ const V2ConnectorsPage: React.FC = () => {