Repository navigation
newsletter: hide footer band on token pages; assert subscribe payload attribution - #270
Merged
devin-ai-integration[bot] merged 2 commits intoOct 29, 2026
Merged
Conversation
… attribution - Hide the footer signup band on /newsletter/confirm and /newsletter/unsubscribe (published locales only): a signup prompt under 'You're subscribed!' is redundant, and under an opt-out confirmation contradictory. Both pages already link to /newsletter. - Extend the footer integration tests for the new routes and for unpublished localized variants (nl keeps the band). - New e2e spec intercepting the browser's own subscribe POST to assert the submitted payload carries campaign/source/placement/audience: group- and individual-flyer DEMA links, both-audience selection, attribution preserved across audience changes, footer source=website/placement=footer, and no email in request or page URLs. Closes the test gap in #269. Endpoints are fully mocked. Co-Authored-By: Chad <chadnuttall1@gmail.com>
Contributor
Author
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Reviewer's GuideThe PR hides redundant or contradictory footer signup prompts on published newsletter and token pages while retaining them for unpublished localized routes, and adds mocked Playwright coverage that verifies DEMA attribution and audience data reach the actual subscribe POST payload without exposing email addresses in URLs. Sequence diagram for newsletter attribution submissionsequenceDiagram
actor Visitor
participant Page as NewsletterPage
participant Browser
participant API as SubscribeAPI
Visitor->>Page: Select audience and submit email
Page->>Browser: POST /api/newsletter/subscribe
Browser->>API: JSON email subscription.audiences attribution
API-->>Browser: Mocked subscribe response
Browser-->>Page: Submission result
Flow diagram for localized newsletter footer visibilityflowchart TD
A[Current pathname] --> B[parsePathname]
B --> C{Newsletter route?}
C -->|No| D[Render footer signup band]
C -->|Yes| E{Route published for locale?}
E -->|No| D
E -->|Yes| F[Hide footer signup band]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/e2e/newsletter-attribution.spec.ts" line_range="31" />
<code_context>
+function mockSubscribe(page: import("@playwright/test").Page) {
+ const seen: SubscribePayload[] = [];
+ const requestUrls: string[] = [];
+ void page.route("**/api/newsletter/subscribe", async (route) => {
+ requestUrls.push(route.request().url());
+ seen.push(route.request().postDataJSON() as SubscribePayload);
</code_context>
<issue_to_address>
**Real newsletter signups escape mocks**
When route registration has not completed before form submission, `mockSubscribe` discards the `page.route` promise, so the POST reaches the app instead of the mock and `seen` stays empty; tests can create real subscriptions or send emails.
Make `mockSubscribe` await route registration and wait for it to complete before allowing form submission.
</issue_to_address>
### Comment 2
<location path="tests/integration/newsletter-footer-cta.test.tsx" line_range="75" />
<code_context>
+ // No Dutch newsletter pages are published yet — the band remains the
+ // only signup entry point on those routes.
+ mockPathname = "/nl/newsletter/confirm";
+ render(<NewsletterFooterCta />);
+ expect(
+ screen.getByRole("textbox", { name: /newsletter email/i })
</code_context>
<issue_to_address>
**Integration test finds duplicate forms**
When the localized-token test renders the component a second time without cleanup, `screen.getByRole` matches both forms after the second render and throws a multiple-elements error, so the integration test fails.
Clean up the first render before rendering again, or use `rerender` to update the existing render.
Also at `tests/integration/newsletter-footer-cta.test.tsx:78`.
</issue_to_address>
### Comment 3
<location path="components/newsletter-footer-cta.tsx" line_range="30" />
<code_context>
+ const HIDDEN_ON = ["/newsletter", "/newsletter/confirm", "/newsletter/unsubscribe"];
const { locale, path } = parsePathname(pathname ?? "/");
- if (path === "/newsletter" && isRoutePublished(locale, "/newsletter")) {
+ if (HIDDEN_ON.includes(path) && isRoutePublished(locale, path)) {
return null;
}
</code_context>
<issue_to_address>
**Resubscribe link disappears after opt-out**
When a visitor wants to subscribe again immediately after successfully unsubscribing, `NewsletterFooterCta` returns `null` while `NewsletterTokenHandler` shows only the success message and detail, and the footer navigation has no newsletter link, so the visitor has no visible route back to signup.
Keep the signup CTA visible on the successful unsubscribe page or provide another visible link to signup.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: tests/e2e/newsletter-attribution.spec.ts:31, tests/integration/newsletter-footer-cta.test.tsx:75, components/newsletter-footer-cta.tsx:30
- Await page.route() registration in mockSubscribe before navigation so no subscribe request can escape the mock (Sourcery finding 1). - Split the duplicated localized-token render into it.each cases so each route gets an isolated DOM (finding 2). - Give the unsubscribe success state a text link back to /newsletter for visitors who want to resubscribe, without restoring the signup band (finding 3). Asserted in e2e. Co-Authored-By: Chad <chadnuttall1@gmail.com>
Contributor
Author
|
End-to-end verified on a production test build ( Band hidden on token/signup pages, footer nav intact:
Controls: band still renders where expected
Footer handoff + DEMA attribution still work
|
devin-ai-integration
Bot
deleted the
devin/newsletter-footer-attribution
branch
October 29, 2026 23:06
This branch was successfully deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Part of #269 (newsletter polish for DEMA 2026). Two independent changes:
1. Hide the footer signup band on newsletter token pages
NewsletterFooterCtanow returnsnullon/newsletter/confirmand/newsletter/unsubscribein addition to/newsletter. A signup prompt under "You're subscribed!" is redundant, and under "You've been unsubscribed." it contradicts the action just taken. All three pages already carry links to/newsletterin their outcome states, so no signup entry point is lost. The hide is still gated byisRoutePublished(locale, path)— unpublished localized routes (e.g./nl/newsletter/confirm, no Dutch newsletter pages exist yet) keep the band as the visitor's only signup entry point. Footer nav, contact info, and links are untouched; ordinary pages are untouched.2. Assert the actual subscribe payload carries DEMA attribution
Issue #269 noted the existing tests only prove attribution params stay in the URL, not that the submitted request contains them. New
tests/e2e/newsletter-attribution.spec.tsintercepts the browser's ownPOST /api/newsletter/subscribe(fully mocked — no real subscribers, no real email) and asserts on the submitted JSON:campaign=dema-2026,source=qr,placement=group-flyer/individual-flyerwith matchingaudiencesubscription.audiencescontains both memberships when both boxes are checkedsource=website,placement=footer,campaign/audience=nullTests fail if attribution is silently dropped from the payload (exact-match assertions on
attribution), which is the regression #269 asked to catch.Verification
npx vitest runon touched suites: 13/13 pass (footer band hidden on/newsletter/confirm+/newsletter/unsubscribe, visible on/nl/newsletter*variants)npx eslint+tsc --noEmit: cleannpm run check(docs/env/hygiene): passnpm run build:test: production build succeedsnpx playwright test newsletter-attribution newsletter-footer: 36/36 pass across desktop-chromium, mobile-chromium, mobile-webkitNo production endpoints touched; all e2e traffic is intercepted and mocked.
Link to Devin session: https://app.devin.ai/sessions/64a46d6928c84834a009ce1898be907f
Open in Devin Desktop: https://app.devin.ai/desktop/session/64a46d6928c84834a009ce1898be907f?variant=devin
Requested by: @spizeck
Summary by Sourcery
Prevent redundant newsletter signup prompts on token outcome pages and verify that attribution reaches the subscription request payload.
Bug Fixes:
Tests: