Skip to content

Commit b0f56b6

Browse files
Merge pull request #124 from CodeForPhilly/feat/global-connect-github-banner
feat(web): persistent connect-GitHub banner under the navbar
2 parents 0d0f747 + 6fcfb70 commit b0f56b6

5 files changed

Lines changed: 202 additions & 94 deletions

File tree

apps/web/src/components/AppShell.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Outlet } from 'react-router';
22
import { AppHeader } from '@/components/AppHeader';
33
import { AppFooter } from '@/components/AppFooter';
4+
import { ConnectGitHubBanner } from '@/components/ConnectGitHubBanner';
45
import { OfflineBanner } from '@/components/OfflineBanner';
56
import { TopProgressBar } from '@/components/TopProgressBar';
67

@@ -18,6 +19,7 @@ export function AppShell() {
1819
<TopProgressBar />
1920
<OfflineBanner />
2021
<AppHeader />
22+
<ConnectGitHubBanner />
2123

2224
<main id="main-content" className="flex-1" tabIndex={-1}>
2325
<Outlet />
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { useState } from 'react';
2+
import { Button } from '@/components/ui/button';
3+
import { useAuth } from '@/hooks/useAuth';
4+
5+
/**
6+
* Persistent nag banner shown directly under the navbar to legacy
7+
* users (signed in via password) who haven't linked GitHub yet.
8+
*
9+
* Visibility rule per specs/screens/account.md +
10+
* specs/behaviors/account-migration.md:
11+
*
12+
* person is signed in
13+
* AND hasGitHubLink === false
14+
* AND lastLoginMethod ∈ {legacy_password, password_reset}
15+
*
16+
* Dismissible per-session via in-memory state; reload brings it back
17+
* (the nag is intentionally persistent across navigations to keep the
18+
* "link your account" prompt visible until the user either links or
19+
* dismisses).
20+
*/
21+
export function ConnectGitHubBanner() {
22+
const { person, loading, hasGitHubLink, lastLoginMethod } = useAuth();
23+
const [dismissed, setDismissed] = useState(false);
24+
25+
if (loading || !person) return null;
26+
if (hasGitHubLink) return null;
27+
if (lastLoginMethod !== 'legacy_password' && lastLoginMethod !== 'password_reset') {
28+
return null;
29+
}
30+
if (dismissed) return null;
31+
32+
return (
33+
<div
34+
role="region"
35+
aria-label="Connect GitHub"
36+
className="border-b border-primary/40 bg-primary/5 print:hidden"
37+
>
38+
<div className="container mx-auto px-4 py-2 flex flex-col sm:flex-row sm:items-center gap-3">
39+
<p className="flex-1 text-sm">
40+
<span className="font-medium">Connect your GitHub account</span>
41+
<span className="text-muted-foreground">
42+
{' '}— faster sign-in next time, and one less password to remember.
43+
Code for Philly plans to retire password sign-in eventually.
44+
</span>
45+
</p>
46+
<form method="POST" action="/api/auth/link-github" className="shrink-0">
47+
<Button type="submit" size="sm">
48+
Connect GitHub
49+
</Button>
50+
</form>
51+
<Button
52+
type="button"
53+
variant="ghost"
54+
size="sm"
55+
onClick={() => setDismissed(true)}
56+
aria-label="Dismiss"
57+
>
58+
Dismiss
59+
</Button>
60+
</div>
61+
</div>
62+
);
63+
}

apps/web/src/screens/Account.tsx

Lines changed: 1 addition & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,10 @@ const LINK_GITHUB_ERROR_MESSAGES: Record<string, string> = {
3535
};
3636

3737
export function Account() {
38-
const { person, loading, signOut, reload, hasGitHubLink, lastLoginMethod } = useAuth();
38+
const { person, loading, signOut, reload, hasGitHubLink } = useAuth();
3939
const navigate = useNavigate();
4040
const queryClient = useQueryClient();
4141
const [searchParams, setSearchParams] = useSearchParams();
42-
const [bannerDismissed, setBannerDismissed] = useState(false);
4342

4443
// Toast on /account?linked=github or ?error=<code> from the link-flow
4544
// callback, then strip the param so reloading doesn't re-toast.
@@ -115,44 +114,10 @@ export function Account() {
115114

116115
const sessions = sessionsQ.data?.data ?? [];
117116

118-
const showConnectGitHubBanner =
119-
!bannerDismissed &&
120-
!hasGitHubLink &&
121-
(lastLoginMethod === 'legacy_password' || lastLoginMethod === 'password_reset');
122-
123117
return (
124118
<div className="container mx-auto px-4 py-8 max-w-3xl space-y-6">
125119
<h1 className="text-2xl font-bold">Account Settings</h1>
126120

127-
{showConnectGitHubBanner && (
128-
<div
129-
role="region"
130-
aria-label="Connect GitHub"
131-
className="rounded-md border border-primary/40 bg-primary/5 p-4 flex flex-col sm:flex-row sm:items-center gap-3"
132-
>
133-
<div className="flex-1 text-sm">
134-
<p className="font-medium">Connect your GitHub account</p>
135-
<p className="text-muted-foreground mt-1">
136-
Faster sign-in next time, and one less password to remember. Code
137-
for Philly plans to retire password sign-in eventually — link
138-
GitHub now to stay ahead.
139-
</p>
140-
</div>
141-
<form method="POST" action="/api/auth/link-github" className="shrink-0">
142-
<Button type="submit">Connect GitHub</Button>
143-
</form>
144-
<Button
145-
type="button"
146-
variant="ghost"
147-
size="sm"
148-
onClick={() => setBannerDismissed(true)}
149-
aria-label="Dismiss"
150-
>
151-
Dismiss
152-
</Button>
153-
</div>
154-
)}
155-
156121
{/* Identity */}
157122
<Card>
158123
<CardHeader>

apps/web/tests/Account.test.tsx

Lines changed: 8 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,11 @@
11
/**
2-
* Account screen tests focused on the phase-D additions:
3-
* - Connect-GitHub banner renders only for legacy-credential users
4-
* who haven't linked yet.
5-
* - Identity card swaps "Manage on GitHub" for "Connect GitHub" when
6-
* hasGitHubLink is false.
7-
* - Banner dismiss button hides it for the rest of the session.
8-
*
9-
* The existing Account page predates this test file — these tests focus
10-
* narrowly on the banner + identity-card branches and leave the
11-
* newsletter / sessions / claim-legacy regions alone.
2+
* Account screen tests focused on the GitHub-link affordances inside
3+
* the Identity card. The persistent "Connect GitHub" nag banner has
4+
* been hoisted to a top-level ConnectGitHubBanner component (rendered
5+
* by AppShell on every page), and is covered by its own test file.
126
*/
137
import { describe, expect, it, vi, afterEach } from 'vitest';
14-
import { fireEvent, screen, waitFor } from '@testing-library/react';
8+
import { screen, waitFor } from '@testing-library/react';
159
import { renderScreen, mockOk } from './test-utils.js';
1610
import { Account } from '../src/screens/Account.js';
1711
import { AuthProvider } from '../src/hooks/useAuth.js';
@@ -80,50 +74,6 @@ function render() {
8074
);
8175
}
8276

83-
describe('Account — Connect-GitHub banner', () => {
84-
afterEach(() => {
85-
vi.restoreAllMocks();
86-
});
87-
88-
it('renders for a legacy-password user with no GitHub link', async () => {
89-
mockApi(legacyPerson);
90-
render();
91-
await waitFor(() => {
92-
expect(
93-
screen.getByRole('region', { name: /connect github/i }),
94-
).toBeInTheDocument();
95-
});
96-
// Banner has a Connect button + a Dismiss button
97-
const region = screen.getByRole('region', { name: /connect github/i });
98-
expect(region.querySelector('form[action="/api/auth/link-github"]')).not.toBeNull();
99-
expect(screen.getByRole('button', { name: /dismiss/i })).toBeInTheDocument();
100-
});
101-
102-
it('does not render for a github-signed-in user', async () => {
103-
mockApi(githubPerson);
104-
render();
105-
// Wait for the Identity card to render — that proves the page is past loading.
106-
await waitFor(() => {
107-
expect(screen.getByText(/connected primary identity/i)).toBeInTheDocument();
108-
});
109-
expect(
110-
screen.queryByRole('region', { name: /connect github/i }),
111-
).not.toBeInTheDocument();
112-
});
113-
114-
it('dismiss button hides the banner for the rest of the session', async () => {
115-
mockApi(legacyPerson);
116-
render();
117-
const dismissBtn = await screen.findByRole('button', { name: /dismiss/i });
118-
fireEvent.click(dismissBtn);
119-
await waitFor(() => {
120-
expect(
121-
screen.queryByRole('region', { name: /connect github/i }),
122-
).not.toBeInTheDocument();
123-
});
124-
});
125-
});
126-
12777
describe('Account — Identity card', () => {
12878
afterEach(() => {
12979
vi.restoreAllMocks();
@@ -135,9 +85,9 @@ describe('Account — Identity card', () => {
13585
await waitFor(() => {
13686
expect(screen.getByText(/not connected/i)).toBeInTheDocument();
13787
});
138-
// Two forms post to the link endpoint: banner + identity card
88+
// Identity card has a form posting to the link endpoint.
13989
const forms = document.querySelectorAll('form[action="/api/auth/link-github"]');
140-
expect(forms.length).toBeGreaterThanOrEqual(2);
90+
expect(forms.length).toBeGreaterThanOrEqual(1);
14191
});
14292

14393
it('shows the "Manage on GitHub" link when hasGitHubLink is true', async () => {
@@ -149,7 +99,7 @@ describe('Account — Identity card', () => {
14999
expect(
150100
screen.getByRole('link', { name: /manage on github/i }),
151101
).toHaveAttribute('href', 'https://github.com/settings');
152-
// No link-github form when already connected
102+
// No link-github form when already connected.
153103
expect(
154104
document.querySelectorAll('form[action="/api/auth/link-github"]').length,
155105
).toBe(0);
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/**
2+
* Tests for ConnectGitHubBanner — the persistent "Connect your GitHub
3+
* account" nag rendered directly under the navbar on every page.
4+
*
5+
* Visibility rule: signed-in + hasGitHubLink === false + lastLoginMethod
6+
* ∈ {legacy_password, password_reset} + not dismissed.
7+
*/
8+
import { describe, expect, it, vi, afterEach } from 'vitest';
9+
import { fireEvent, screen, waitFor } from '@testing-library/react';
10+
import { renderScreen, mockOk } from './test-utils.js';
11+
import { ConnectGitHubBanner } from '../src/components/ConnectGitHubBanner.js';
12+
import { AuthProvider } from '../src/hooks/useAuth.js';
13+
14+
interface MeShape {
15+
person: { id: string; slug: string; fullName: string; accountLevel: string; avatarUrl: string | null } | null;
16+
accountLevel: string;
17+
hasGitHubLink: boolean;
18+
lastLoginMethod: 'github' | 'legacy_password' | 'password_reset' | null;
19+
}
20+
21+
function mockMe(me: MeShape): void {
22+
vi.spyOn(globalThis, 'fetch').mockImplementation(((input: string) => {
23+
if (input.startsWith('/api/auth/me')) {
24+
return Promise.resolve(
25+
new Response(JSON.stringify(mockOk(me)), {
26+
status: 200,
27+
headers: { 'content-type': 'application/json' },
28+
}),
29+
);
30+
}
31+
return Promise.resolve(new Response(null, { status: 404 }));
32+
}) as typeof fetch);
33+
}
34+
35+
const baseLegacyPerson: MeShape = {
36+
person: {
37+
id: '01951a3c-0000-7000-8000-0000ffffff01',
38+
slug: 'legacy-user',
39+
fullName: 'Legacy User',
40+
accountLevel: 'user',
41+
avatarUrl: null,
42+
},
43+
accountLevel: 'user',
44+
hasGitHubLink: false,
45+
lastLoginMethod: 'legacy_password',
46+
};
47+
48+
const anonMe: MeShape = {
49+
person: null,
50+
accountLevel: 'anonymous',
51+
hasGitHubLink: false,
52+
lastLoginMethod: null,
53+
};
54+
55+
function render() {
56+
return renderScreen(
57+
<AuthProvider>
58+
<ConnectGitHubBanner />
59+
</AuthProvider>,
60+
);
61+
}
62+
63+
describe('ConnectGitHubBanner', () => {
64+
afterEach(() => {
65+
vi.restoreAllMocks();
66+
});
67+
68+
it('renders for legacy-password user with no GitHub link', async () => {
69+
mockMe(baseLegacyPerson);
70+
render();
71+
await waitFor(() => {
72+
expect(
73+
screen.getByRole('region', { name: /connect github/i }),
74+
).toBeInTheDocument();
75+
});
76+
// CTA form posts to the link endpoint.
77+
const region = screen.getByRole('region', { name: /connect github/i });
78+
expect(region.querySelector('form[action="/api/auth/link-github"]')).not.toBeNull();
79+
expect(screen.getByRole('button', { name: /dismiss/i })).toBeInTheDocument();
80+
});
81+
82+
it('renders for a user whose session was minted via password reset', async () => {
83+
mockMe({ ...baseLegacyPerson, lastLoginMethod: 'password_reset' });
84+
render();
85+
await waitFor(() => {
86+
expect(
87+
screen.getByRole('region', { name: /connect github/i }),
88+
).toBeInTheDocument();
89+
});
90+
});
91+
92+
it('does not render for a github-signed-in user', async () => {
93+
mockMe({
94+
...baseLegacyPerson,
95+
hasGitHubLink: true,
96+
lastLoginMethod: 'github',
97+
});
98+
render();
99+
// Wait for /api/auth/me to settle (loading → resolved). The
100+
// simplest signal is a tick: a known-true assertion plus a short
101+
// microtask gap.
102+
await new Promise((r) => setTimeout(r, 0));
103+
expect(
104+
screen.queryByRole('region', { name: /connect github/i }),
105+
).not.toBeInTheDocument();
106+
});
107+
108+
it('does not render for anonymous viewers', async () => {
109+
mockMe(anonMe);
110+
render();
111+
await new Promise((r) => setTimeout(r, 0));
112+
expect(
113+
screen.queryByRole('region', { name: /connect github/i }),
114+
).not.toBeInTheDocument();
115+
});
116+
117+
it('hides after the user clicks dismiss', async () => {
118+
mockMe(baseLegacyPerson);
119+
render();
120+
const dismissBtn = await screen.findByRole('button', { name: /dismiss/i });
121+
fireEvent.click(dismissBtn);
122+
await waitFor(() => {
123+
expect(
124+
screen.queryByRole('region', { name: /connect github/i }),
125+
).not.toBeInTheDocument();
126+
});
127+
});
128+
});

0 commit comments

Comments
 (0)