Skip to content
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

[Issue #2962] Login modal #3433

Merged
merged 8 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions frontend/next.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ const nextConfig = {
},
],
},
// don't cache the api
{
source: "/api/:path*",
headers: [
{
key: "Cache-Control",
value: "no-store, must-revalidate",
},
],
},
];
},
basePath,
Expand Down
16 changes: 16 additions & 0 deletions frontend/src/app/api/auth/login/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { environment } from "src/constants/environments";

import { NextResponse } from "next/server";

export const dynamic = "force-dynamic";

export function GET() {
try {
if (!environment.AUTH_LOGIN_URL) {
throw new Error("AUTH_LOGIN_URL not defined");
}
return NextResponse.redirect(environment.AUTH_LOGIN_URL);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning NextResponse.redirect instead of redirect doesn't throw an error for the try/catch block.

} catch (error) {
return new NextResponse(error as string, { status: 500 });
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should have a more graceful error message, but this is temporary anyway.

}
}
9 changes: 0 additions & 9 deletions frontend/src/app/api/env/route.ts

This file was deleted.

77 changes: 77 additions & 0 deletions frontend/src/components/LoginModal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { useTranslations } from "next-intl";
import { useRef } from "react";
import {
ButtonGroup,
Modal,
ModalFooter,
ModalHeading,
ModalRef,
ModalToggleButton,
} from "@trussworks/react-uswds";

import { USWDSIcon } from "src/components/USWDSIcon";

const LOGIN_URL = "/api/auth/login";

export const LoginModal = ({
navLoginLinkText,
}: {
navLoginLinkText: string;
}) => {
const t = useTranslations("LoginModal");
const modalRef = useRef<ModalRef>(null);

return (
<>
<div className="usa-nav__primary margin-top-0 padding-top-2px text-no-wrap desktop:order-last margin-left-auto">
<div className="usa-nav__primary-item border-0">
<ModalToggleButton
modalRef={modalRef}
opener
className="usa-nav__link text-ysaprimary font-sans-2xs display-flex text-normal border-0"
>
<USWDSIcon
className="usa-icon margin-right-05 margin-left-neg-05"
name="login"
key="login-link-icon"
/>
{navLoginLinkText}
</ModalToggleButton>
</div>
</div>
<Modal
ref={modalRef}
forceAction
aria-labelledby="login-modal-heading"
aria-describedby="login-modal-description"
id="login-modal"
>
<ModalHeading id="login-modal-heading">{t("title")}</ModalHeading>
<div className="usa-prose">
<p>{t("help")}</p>
<p className="font-sans-2xs margin-y-4">{t("description")}</p>
</div>
<ModalFooter>
<ButtonGroup>
<a href={LOGIN_URL} key="login-link" className="usa-button">
{t("button")}
<USWDSIcon
className="usa-icon margin-right-05 margin-left-neg-05"
name="launch"
key="login-gov-link-icon"
/>
</a>
<ModalToggleButton
modalRef={modalRef}
closer
unstyled
className="padding-105 text-center"
>
{t("close")}
</ModalToggleButton>
</ButtonGroup>
</ModalFooter>
</Modal>
</>
);
};
48 changes: 3 additions & 45 deletions frontend/src/components/user/UserControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,16 @@ import { UserProfile } from "src/services/auth/types";
import { useUser } from "src/services/auth/useUser";

import { useTranslations } from "next-intl";
import { useCallback, useEffect, useState } from "react";
import { useCallback, useState } from "react";
import {
IconListContent,
Menu,
NavDropDownButton,
} from "@trussworks/react-uswds";

import { LoginModal } from "src/components/LoginModal";
import { USWDSIcon } from "src/components/USWDSIcon";

const LoginLink = ({
navLoginLinkText,
loginUrl,
}: {
navLoginLinkText: string;
loginUrl: string;
}) => {
return (
<div className="usa-nav__primary-item border-0 desktop:margin-top-0 margin-top-1">
<a
{...(loginUrl ? { href: loginUrl } : "")}
key="login-link"
className="usa-nav__link font-sans-2xs display-flex"
>
<USWDSIcon
className="margin-right-05 margin-left-neg-05"
name="login"
key="login-link-icon"
/>
{navLoginLinkText}
</a>
</div>
);
};

// used in three different places
// 1. on desktop - nav item drop down button content
// 2. on mobile - nav item drop down button content, without email text
Expand Down Expand Up @@ -138,27 +114,9 @@ export const UserControl = () => {
await refreshUser();
}, [refreshUser]);

const [authLoginUrl, setAuthLoginUrl] = useState<string | null>(null);

useEffect(() => {
async function fetchEnv() {
const res = await fetch("/api/env");
const data = (await res.json()) as { auth_login_url: string };
data.auth_login_url
? setAuthLoginUrl(data.auth_login_url)
: console.error("could not access auth_login_url");
}
fetchEnv().catch((error) => console.warn("error fetching api/env", error));
}, []);

return (
<>
{!user?.token && (
<LoginLink
navLoginLinkText={t("nav_link_login")}
loginUrl={authLoginUrl || ""}
/>
)}
{!user?.token && <LoginModal navLoginLinkText={t("nav_link_login")} />}
Copy link
Collaborator

@doug-s-nava doug-s-nava Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works because the LoginModal component includes the modal control button but if you're expecting to see something like <LoginButton> here, it's a little confusing to see <LoginModal> instead. Something for later, but there's probably a refactor that would make this a little easier to understand

{!!user?.token && (
<UserDropdown
user={user}
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/i18n/messages/en/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,14 @@ export const messages = {
nav_link_logout: "Sign out",
title: "Simpler.Grants.gov",
},
LoginModal: {
title: "Sign in to Simpler.Grants.gov",
help: "Simpler.Grants.gov uses Login.gov to verify your identity and manage your account securely. You don't need a separate username or password for this site.",
description:
"You’ll be redirected to Login.gov to sign in or create an account. Then, you’ll return to Simpler.Grants.gov as a signed-in user.",
button: "Sign in with Login.gov",
close: "Cancel",
},
Hero: {
title: "We're building a simpler Grants.gov!",
content:
Expand Down
3 changes: 0 additions & 3 deletions frontend/src/styles/_uswds-theme-custom-styles.scss
Original file line number Diff line number Diff line change
Expand Up @@ -380,9 +380,6 @@ button.usa-pagination__button.usa-button {
}

@include at-media("desktop") {
.desktop\:margin-bottom-5px {
margin-bottom: 5px !important;
}
.usa-nav__submenu {
right: 0;
}
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/utils/testing/commonTestUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class NoErrorThrownError extends Error {}
Jest doesn't like it when you put expect calls in catch blocks
This is unavoidable, though, when testing route handlers that use Next's redirect functionality,
as that implementation throws errors on redirect by design.
WHen testing those sorts of functions, wrap the call to the route handler in an anonymous function and
When testing those sorts of functions, wrap the call to the route handler in an anonymous function and
pass it into this wrapper, which will spit out the error as a return value
see https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/no-conditional-expect.md
*/
Expand Down
23 changes: 23 additions & 0 deletions frontend/tests/__mocks__/focus-trap-react.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import type * as FocusTrapType from "focus-trap-react";

import { ComponentType } from "react";

const FocusTrap =
jest.requireActual<ComponentType<FocusTrapType.Props>>("focus-trap-react");

/**
* Override displayCheck for testing. See: https://github.com/focus-trap/tabbable#testing-in-jsdom
*/
const FixedComponent = ({
focusTrapOptions,
...props
}: FocusTrapType.Props) => {
const fixedOptions = { ...focusTrapOptions };
fixedOptions.tabbableOptions = {
...fixedOptions.tabbableOptions,
displayCheck: "none",
};
return <FocusTrap {...props} focusTrapOptions={fixedOptions} />;
};

module.exports = FixedComponent;
29 changes: 29 additions & 0 deletions frontend/tests/api/auth/login/route.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* @jest-environment node
*/
import { GET } from "src/app/api/auth/login/route";
import { environment } from "src/constants/environments";

jest.mock("src/constants/environments", () => ({
environment: { AUTH_LOGIN_URL: "http://simpler.grants.gov/login" },
}));

describe("/api/auth/login GET handler", () => {
afterEach(() => jest.clearAllMocks());
it("redirects correctly", () => {
const response = GET();

expect(response.headers.get("location")).toBe(
"http://simpler.grants.gov/login",
);
expect(response.status).toBe(307);
});
it("errors correctly", () => {
jest.replaceProperty(environment, "AUTH_LOGIN_URL", "");

const response = GET();

expect(response.headers.get("location")).toBe(null);
expect(response.status).toBe(500);
});
});
20 changes: 0 additions & 20 deletions frontend/tests/api/env/route.test.ts

This file was deleted.

5 changes: 0 additions & 5 deletions frontend/tests/components/Header.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,6 @@ describe("Header", () => {
const closeButton = screen.getByRole("button", { name: /close/i });

expect(closeButton).toBeInTheDocument();

expect(screen.getByRole("link", { name: /Sign in/i })).toHaveAttribute(
"href",
"/login-url",
);
});

it("displays expandable government banner", async () => {
Expand Down
46 changes: 46 additions & 0 deletions frontend/tests/components/LoginModal.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import userEvent from "@testing-library/user-event";
import { render, screen } from "tests/react-utils";

import Header from "src/components/Header";
import { LoginModal } from "src/components/LoginModal";

const usePathnameMock = jest.fn().mockReturnValue("/fakepath");

jest.mock("next/navigation", () => ({
usePathname: () => usePathnameMock() as string,
}));

jest.mock("src/hooks/useFeatureFlags", () => ({
useFeatureFlags: () => ({
checkFeatureFlag: () => true,
}),
}));

describe("LoginModal", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test that the login button directs to the correct location?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constant now, so it would be just checking the link. Can add in a follow up

it("renders", () => {
render(<LoginModal navLoginLinkText="Sign in" />);
const loginGovLink = screen.getByRole("link", {
name: /Sign in with Login.gov/i,
});
expect(loginGovLink).toBeInTheDocument();
const modalTitle = screen.getByRole("heading", { level: 2 });
expect(modalTitle).toHaveTextContent("Sign in to Simpler.Grants.gov");
});

it("displays modal when clicked", async () => {
render(<Header />);

const loginButton = screen.getByRole("button", { name: /Sign in/i });
expect(loginButton).toBeInTheDocument();

const modal = screen.getByRole("dialog");
expect(modal).toHaveClass("is-hidden");

await userEvent.click(loginButton);
expect(modal).toHaveClass("is-visible");

const cancelButton = screen.getByRole("button", { name: /Cancel/i });
await userEvent.click(cancelButton);
expect(modal).toHaveClass("is-hidden");
});
});