-
Notifications
You must be signed in to change notification settings - Fork 163
feat(webhooks): show workflow-data callout on MRF webhook settings #9760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
yin-boop
wants to merge
4
commits into
opengovsg:develop
Choose a base branch
from
yin-boop:webhooks-workflow-infobox
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
35a7fc2
feat(webhooks): add workflow-data callout component
yin-boop d40139f
feat(webhooks): show workflow callout on MRF webhook settings
yin-boop fe0f7ef
refactor(webhooks): dedupe GrowthBook story decorators into helper
yin-boop 11d5141
docs(webhooks): add PR body and review findings for workflow infobox
yin-boop File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24 changes: 24 additions & 0 deletions
24
apps/frontend/src/features/admin-form/settings/SettingsWebhooksPage.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { composeStories } from '@storybook/react' | ||
| import { render, screen } from '@testing-library/react' | ||
|
|
||
| import * as stories from './SettingsWebhooksPage.stories' | ||
|
|
||
| const { MrfMode, StorageModeEmpty } = composeStories(stories) | ||
|
|
||
| const WORKFLOW_CALLOUT_TEXT = /can use data from all workflow steps/i | ||
|
|
||
| describe('SettingsWebhooksPage workflow callout visibility', () => { | ||
| it('shows the callout on an MRF form', async () => { | ||
| render(<MrfMode />) | ||
| await screen.findByText('Endpoint URL') | ||
|
|
||
| expect(screen.getByText(WORKFLOW_CALLOUT_TEXT)).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('hides the callout on a Storage form', async () => { | ||
| render(<StorageModeEmpty />) | ||
| await screen.findByText('Endpoint URL') | ||
|
|
||
| expect(screen.queryByText(WORKFLOW_CALLOUT_TEXT)).not.toBeInTheDocument() | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 changes: 12 additions & 0 deletions
12
...eatures/admin-form/settings/components/WebhooksSection/WebhookWorkflowInfobox.stories.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { Meta, StoryFn } from '@storybook/react' | ||
|
|
||
| import { WebhookWorkflowInfobox } from './WebhookWorkflowInfobox' | ||
|
|
||
| export default { | ||
| title: 'Features/AdminForm/Settings/WebhookWorkflowInfobox', | ||
| component: WebhookWorkflowInfobox, | ||
| } as Meta | ||
|
|
||
| const Template: StoryFn = () => <WebhookWorkflowInfobox /> | ||
|
|
||
| export const Default = Template.bind({}) |
41 changes: 41 additions & 0 deletions
41
...c/features/admin-form/settings/components/WebhooksSection/WebhookWorkflowInfobox.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { composeStories } from '@storybook/react' | ||
| import { render, screen } from '@testing-library/react' | ||
|
|
||
| import { MRF_CUTOVER_FAQ_LINK } from 'formsg-shared/constants/links' | ||
|
|
||
| import { OGP_PLUMBER } from '~constants/links' | ||
|
|
||
| import * as stories from './WebhookWorkflowInfobox.stories' | ||
|
|
||
| const { Default } = composeStories(stories) | ||
|
|
||
| describe('WebhookWorkflowInfobox', () => { | ||
| it('renders the workflow-data heads-up message', () => { | ||
| render(<Default />) | ||
|
|
||
| expect( | ||
| screen.getByText(/can use data from all workflow steps/i), | ||
| ).toBeInTheDocument() | ||
| expect( | ||
| screen.getByText(/other tools can only use data from the first step/i), | ||
| ).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('links "Plumber" to plumber.gov.sg', () => { | ||
| render(<Default />) | ||
|
|
||
| expect(screen.getByRole('link', { name: 'Plumber' })).toHaveAttribute( | ||
| 'href', | ||
| OGP_PLUMBER, | ||
| ) | ||
| }) | ||
|
|
||
| it('links "Learn more" to the simplified-modes FAQ', () => { | ||
| render(<Default />) | ||
|
|
||
| expect(screen.getByRole('link', { name: /learn more/i })).toHaveAttribute( | ||
| 'href', | ||
| MRF_CUTOVER_FAQ_LINK, | ||
| ) | ||
| }) | ||
| }) |
19 changes: 19 additions & 0 deletions
19
...nd/src/features/admin-form/settings/components/WebhooksSection/WebhookWorkflowInfobox.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { useTranslation } from 'react-i18next' | ||
|
|
||
| import { MRF_CUTOVER_FAQ_LINK } from 'formsg-shared/constants/links' | ||
|
|
||
| import { OGP_PLUMBER } from '~constants/links' | ||
| import InlineMessage from '~components/InlineMessage' | ||
|
|
||
| export const WebhookWorkflowInfobox = (): JSX.Element => { | ||
| const { t } = useTranslation() | ||
|
|
||
| return ( | ||
| <InlineMessage variant="info" useMarkdown> | ||
| {t('features.adminForm.settings.webhooks.workflowInfobox', { | ||
| plumberUrl: OGP_PLUMBER, | ||
| learnMoreUrl: MRF_CUTOVER_FAQ_LINK, | ||
| })} | ||
| </InlineMessage> | ||
| ) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,4 +11,5 @@ export interface Webhooks extends HasTitle { | |
| label: string | ||
| description: string | ||
| } | ||
| workflowInfobox: string | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # PR body — feat(webhooks): show workflow-data callout on MRF webhook settings | ||
|
|
||
| ## Problem | ||
|
|
||
| Admins can connect a webhook to an MRF form to send each submission to another tool in real time, but not every destination can do the same thing with it: only Plumber can use the multi-step workflow data across all steps — any other tool can only use the first step's data. Admins have no way to know this at the moment they configure a webhook, so they may wire up the wrong integration and be surprised when their tool can't act on later steps. | ||
|
|
||
| ## Solution | ||
|
|
||
| Adds an always-on informational callout at the top of the webhook settings, above the endpoint URL field, telling admins that if workflows are enabled, only [Plumber](https://plumber.gov.sg/) can use data from all workflow steps while other tools can only use data from the first step, with a "Learn more" link to the simplified-modes FAQ. The callout is a neutral heads-up, not an error, and shows regardless of whether a workflow is configured yet or whether the entered URL is a Plumber link. | ||
|
|
||
| Visibility is gated on response mode alone: `SettingsWebhooksPage` computes `showWorkflowInfobox = responseMode === Multirespondent` and passes a single boolean into `WebhooksSection`. No feature flag is checked at this line — `enable-mrf-webhooks` is already guaranteed true on this code path by the page's existing `enableWebhooks` gate, and `mrf-cutover` governs form-creation defaults, a separate concern from webhook messaging. | ||
|
|
||
| **Alternatives considered** | ||
|
|
||
| - We considered having `WebhooksSection` (or the callout itself) read `useAdminFormSettings()` directly, like its siblings `WebhookUrlInput` and `RetryToggle` do. We kept the visibility decision at the page instead: the page already owns the settings query and the response-mode branching, so the section stays a dumb layout component with no data lookups of its own, and the callout stays a prop-less, non-fetching component that tests in isolation. | ||
| - An earlier draft (the `webhooks-workflow-callout` branch) gated the callout on `mrf-cutover` && MRF. We dropped the cutover flag: it couples the callout to the cutover rollout, while the capability distinction the copy describes is true regardless of cutover state. | ||
|
|
||
| **Screenshots** | ||
|
|
||
| Captured from the page stories in Storybook. The MRF "before" is unavailable via Storybook — the MRF story is introduced by this PR; on develop the same MRF form rendered the section without the callout, visually identical to the Storage row. | ||
|
|
||
| | Page | Before | After | | ||
| | --- | --- | --- | | ||
| | Settings → Webhooks (MRF form, `enable-mrf-webhooks` on) | unavailable — see note above |  | | ||
| | Settings → Webhooks (Storage form — unchanged) |  |  | | ||
|
|
||
| **Breaking Changes** | ||
|
|
||
| No - backwards compatible. | ||
|
|
||
| ## Tests | ||
|
|
||
| **TC1: Callout shows on an MRF form** | ||
|
|
||
| - [ ] Enable the `enable-mrf-webhooks` GrowthBook flag for your admin | ||
| - [ ] Open Settings → Webhooks on a Multirespondent form | ||
| - [ ] An info (not error) callout appears above the Endpoint URL field | ||
| - [ ] "Plumber" opens https://plumber.gov.sg/ and "Learn more" opens https://go.gov.sg/formsg-guide-faq-simplified-modes | ||
|
|
||
| **TC2: Callout hidden on a Storage-mode form** | ||
|
|
||
| - [ ] Open Settings → Webhooks on a Storage-mode form — webhook settings render with no callout | ||
|
|
||
| **TC3: Callout is unconditional on workflow/URL** | ||
|
|
||
| - [ ] On the MRF form, confirm the callout shows before any workflow steps exist and does not change when a Plumber URL is entered |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Review findings — webhooks-workflow-infobox vs develop | ||
|
|
||
| Multi-axis review (`/review`) of the 3-commit diff adding the webhook workflow-data callout, its MRF-only gate, and the two-quadrant visibility tests. Each axis ran as an independent reviewer; Standards/Spec findings were verified against false positives by independent per-finding verifiers (cutoff 70). | ||
|
|
||
| ## Standards | ||
|
|
||
| **Clean.** No documented-standard violations — i18n key placement and interpolation match `retry.description` exactly, tests follow the repo's `composeStories` pattern, and the story title prefix matches convention. The most borderline drop (scored 60 on independent verification, below cutoff): | ||
|
|
||
| - The callout takes a new, untagged dependency on `MRF_CUTOVER_FAQ_LINK`, a constant `WorkspacePage.tsx:53` explicitly plans to remove ("Remove this infobox (and MRF_CUTOVER_FAQ_LINK) once the cutover is complete"). The verifier confirmed the maintenance risk is real but the issue spec explicitly directs this link and acknowledges the constant's name is a misnomer while the linked content stays accurate. Cheap defusal if desired: rename the constant (e.g. `SIMPLIFIED_MODES_FAQ_LINK`) or amend the WorkspacePage TODO so cutover cleanup doesn't delete a constant this permanent callout still uses. | ||
|
|
||
| ## Spec | ||
|
|
||
| **Clean.** All eight acceptance criteria verified met against the diff: placement above the endpoint URL field, gate is `responseMode === Multirespondent` only (no `mrf-cutover`, no `enableMrfWebhooks` re-check), character-exact copy sourced from the `workflowInfobox` i18n key, correct Plumber/FAQ hrefs, info variant, two-quadrant page tests, component copy/link tests, and an MRF page story. Two spec items confirmed moot due to spec staleness rather than diff defects: | ||
|
|
||
| - `MRF_CUTOVER_FAQ_LINK` already exists in `packages/shared/constants/links.ts` on develop, so no shared-package change was needed. | ||
| - `WebhookV1SchemaInfobox` no longer exists anywhere in the repo (removed on develop in `62ea89447`), so the "leave its gate untouched" criterion is satisfied vacuously. | ||
|
|
||
| One informational note dropped at 50 (below cutoff): converting the pre-existing `StorageModeMrfCutoverOn` story to the new `withGrowthBookFeatures` helper is a behaviour-preserving refactor the spec didn't request; the spec's out-of-scope list doesn't forbid it. | ||
|
|
||
| ## Architecture / Divergent (merged — both axes flagged the same two lines) | ||
|
|
||
| 1. **`showWorkflowInfobox` prop-drilling diverges from the section's own pattern** (`WebhooksSection.tsx:7`; also raised by Standards below threshold). Both siblings (`WebhookUrlInput`, `RetryToggle`) read `useAdminFormSettings()` themselves and React Query dedupes the fetch, so the section could derive the boolean locally, deleting the prop and the page-level computation; the story-based tests would pass unchanged. **Orchestrator note:** the issue spec and PRD explicitly mandate the current shape ("passes a single boolean into `WebhooksSection`, which keeps it a dumb layout component free of its own data lookups"), and a breadcrumb records it — so this finding re-litigates a spec decision. Acting on it is a spec change, not a fix. | ||
| 2. **`withGrowthBookFeatures` helper deduped into the wrong home** (`SettingsWebhooksPage.stories.tsx:56`; flagged by Architecture, Divergent, and Standards). The identical GrowthBook decorator boilerplate exists in at least three other story files (`CreateFormModal`, `DuplicateFormModal`, `UseTemplateModal` stories), and shared story helpers already live in `src/utils/storybook.tsx`. Hoisting the new helper there costs the same as defining it locally and prevents a fifth copy; migrating the other three files can stay a follow-up. | ||
|
|
||
| All other choices were examined by the Divergent axis and confirmed optimal: always-on callout rather than workflow-presence detection, single i18n key with markdown links and URL interpolation, reuse of `InlineMessage variant="info" useMarkdown`, and `composeStories`-based tests. | ||
|
|
||
| ## Summary | ||
|
|
||
| 2 surviving findings (both Architecture+Divergent, cross-axis merged); Standards and Spec clean. Worst issue: the helper's placement (the one clearly actionable in-PR change), with the prop-drilling finding gated on whether the spec's structure decision should be revisited. 3 findings dropped below threshold (scores 60, 50, and two sub-threshold Standards duplicates of the judgement findings). |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this gate checks only response mode: it deliberately does not re-check
enableMrfWebhooks— this code path is unreachable unless that flag already passed the page-levelenableWebhookscheck above, so re-checking would be dead logic. It also does not gate onmrf-cutover(an earlier draft did): that flag controls form-creation defaults app-wide, a separate concern from whether an MRF form's webhook settings should carry this heads-up. The capability distinction the copy describes is true regardless of cutover state.🤖 Generated with Claude Code