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 #3308] View Saved Opportunities on Saved grants page #3992

Merged
merged 16 commits into from
Feb 26, 2025
19 changes: 19 additions & 0 deletions documentation/frontend/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,25 @@ It's recommended that developers configure their code editor to auto run these t
- `npm run format-check`: Check files for prettier formatting violations without fixing them
- `npm run all-checks`: Runs linting, typescript check, unit testing, and creates a build - simulating locally tests that are run on PRs in Github Actions, other than e2e tests

#### Frontend coding conventions

Some common uses and conventions are not covered in formatting and linting tools or widely agreed upon across the industry.

##### Naming resolved and unresolved promises

Constants that represent unresolved promises should be named `varNamePromise(s)`.

Constants that represent resolved promises should be named `resolvedVarName(s)`

For example:

```javascript

const bunnyPromises = getBunnyPromises();
const resolvedBunnies = Promise.all(bunnyPromies);
```


### 🖼️ Storybook

Storybook is a [frontend workshop](https://bradfrost.com/blog/post/a-frontend-workshop-environment/) for developing and documenting pages and components in isolation. It allows you to render the same React components and files in the `src/` directory in a browser, without the need for a server or database. This allows you to develop and manually test components without having to run the entire Next.js application.
Expand Down
84 changes: 71 additions & 13 deletions frontend/src/app/[locale]/saved-grants/page.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import clsx from "clsx";
import { Metadata } from "next";
import { getOpportunityDetails } from "src/services/fetch/fetchers/opportunityFetcher";
import { fetchSavedOpportunities } from "src/services/fetch/fetchers/savedOpportunityFetcher";
import { LocalizedPageProps } from "src/types/intl";
import { Opportunity } from "src/types/opportunity/opportunityResponseTypes";

import { getTranslations } from "next-intl/server";
import Link from "next/link";
import { Button, GridContainer } from "@trussworks/react-uswds";

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

export async function generateMetadata({ params }: LocalizedPageProps) {
Expand All @@ -17,9 +22,62 @@ export async function generateMetadata({ params }: LocalizedPageProps) {
return meta;
}

const SavedOpportunitiesList = ({
opportunities,
}: {
opportunities: Opportunity[];
}) => {
return (
<ul className="usa-prose usa-list--unstyled">
{opportunities.map((opportunity) => (
<>
{opportunity && (
<li key={opportunity.opportunity_id}>
<SearchResultsListItem opportunity={opportunity} saved={true} />
</li>
)}
</>
))}
</ul>
);
};

const NoSavedOpportunities = ({
noSavedCTA,
searchButtonText,
}: {
noSavedCTA: React.ReactNode;
searchButtonText: string;
}) => {
return (
<>
<USWDSIcon
name="star_outline"
className="grid-col-1 usa-icon usa-icon--size-6 margin-top-4"
/>
<div className="margin-top-2 grid-col-11">
<p className="usa-intro ">{noSavedCTA}</p>{" "}
<Link href="/search">
<Button type="button">{searchButtonText}</Button>
</Link>
</div>
</>
);
};

export default async function SavedGrants({ params }: LocalizedPageProps) {
const { locale } = await params;
const t = await getTranslations({ locale });
const savedOpportunities = await fetchSavedOpportunities();
Copy link
Collaborator

Choose a reason for hiding this comment

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

[a note for later] is there a way we could move this complexity to the API side so that we don't have to make so many round trips here, or will caching make that more or less a moot point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be ideal if the saved opportunity returned enough detail to create the results summary. Since that is changing with the new design we shouldn't make a change there right now.

const opportunityPromises = savedOpportunities.map(
async (savedOpportunity) => {
const { data: opportunityData } = await getOpportunityDetails(
String(savedOpportunity.opportunity_id),
);
return opportunityData;
},
);
const resolvedOpportunities = await Promise.all(opportunityPromises);

return (
<>
Expand All @@ -28,27 +86,27 @@ export default async function SavedGrants({ params }: LocalizedPageProps) {
{t("SavedGrants.heading")}
</h1>
</GridContainer>
<div className="bg-base-lightest">
<div
className={clsx({
"bg-base-lightest": resolvedOpportunities.length === 0,
})}
>
<div className="grid-container padding-y-5 display-flex">
<USWDSIcon
name="star_outline"
className="grid-col-1 usa-icon usa-icon--size-6 margin-top-4"
/>
<div className="margin-top-2 grid-col-11">
<p className="usa-intro ">
{t.rich("SavedGrants.noSavedCTA", {
{resolvedOpportunities.length > 0 ? (
<SavedOpportunitiesList opportunities={resolvedOpportunities} />
) : (
<NoSavedOpportunities
noSavedCTA={t.rich("SavedGrants.noSavedCTA", {
br: () => (
<>
<br />
<br />
</>
),
})}
</p>
<Link href="/search">
<Button type="button">{t("SavedGrants.searchButton")}</Button>
</Link>
</div>
searchButtonText={t("SavedGrants.searchButton")}
/>
)}
</div>
</div>
</>
Expand Down
16 changes: 1 addition & 15 deletions frontend/src/components/search/SearchResultsList.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
"use server";

import { getSession } from "src/services/auth/session";
import { getSavedOpportunities } from "src/services/fetch/fetchers/savedOpportunityFetcher";
import { SavedOpportunity } from "src/types/saved-opportunity/savedOpportunityResponseTypes";
import { fetchSavedOpportunities } from "src/services/fetch/fetchers/savedOpportunityFetcher";
import { SearchAPIResponse } from "src/types/search/searchResponseTypes";

import { getTranslations } from "next-intl/server";
Expand All @@ -14,18 +12,6 @@ interface ServerPageProps {
searchResults: SearchAPIResponse;
}

const fetchSavedOpportunities = async (): Promise<SavedOpportunity[]> => {
const session = await getSession();
if (!session || !session.token) {
return [];
}
const savedOpportunities = await getSavedOpportunities(
session.token,
session.user_id as string,
);
return savedOpportunities;
};

export default async function SearchResultsList({
searchResults,
}: ServerPageProps) {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/search/SearchResultsListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export default function SearchResultsListItem({
${opportunity?.summary?.award_ceiling?.toLocaleString() || "--"}
</span>
</span>
<span className="margin-left-3 desktop:display-block text-right desktop:margin-right-0 desktop:padding-right-0">
<span className="border-left-1px border-base-lighter margin-left-1 padding-left-1 text-right desktop:border-0 desktop:display-block desktop:margin-left-3 desktop:margin-right-0 desktop:padding-right-0">
<strong>{t("resultsListItem.floor")}</strong>
{opportunity?.summary?.award_floor?.toLocaleString() || "--"}
</span>
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/components/user/OpportunitySaveUserControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ export const OpportunitySaveUserControl = () => {
<ModalToggleButton
modalRef={modalRef}
opener
className="simpler-save-button usa-button usa-button--outline"
className="usa-button usa-button--outline"
>
<USWDSIcon name="star_outline" />
<USWDSIcon name="star_outline" className="button-icon-large" />
{t("save_button.save")}
</ModalToggleButton>
<LoginModal
Expand Down
22 changes: 22 additions & 0 deletions frontend/src/services/fetch/fetchers/savedOpportunityFetcher.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
"server-only";

import { getSession } from "src/services/auth/session";
import { userSavedOpportunity } from "src/services/fetch/fetchers/fetchers";
import { SavedOpportunity } from "src/types/saved-opportunity/savedOpportunityResponseTypes";

Expand Down Expand Up @@ -69,3 +72,22 @@ export const getSavedOpportunity = async (
);
return savedOpportunity ?? null;
};

export const fetchSavedOpportunities = async (): Promise<
SavedOpportunity[]
> => {
try {
const session = await getSession();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be overkill, but since the session will only be available on the server side (until now it's only been used in route handlers) maybe we should put a "server only" on this file?

Copy link
Collaborator Author

@acouch acouch Feb 25, 2025

Choose a reason for hiding this comment

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

The client component would immediately break with a notice if you tried to import the file, so doesn't seem like a risk that someone would use this incorrectly. server-only designates it is a server function, but also that it should be able to be used directly by the client, which isn't true in this case. Do we want to add server-only to every file that is server side?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree there's no big risk here since this would break if we tried to use it in the client, but potentially would add some dev time if someone didn't realize that. If we pop "server-only" in here, since it's a utilities file rather than anything react specific, I think we'd gain the ability to break faster and provide an annotation for devs so that the use case for the file is immediately clear. https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#keeping-server-only-code-out-of-the-client-environment

Not entirely necessary, but worth maybe discussing further and thinking about documenting as we make a decision one way or another about how we want to use these designations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was conflating server-only and use server, the former poisoning clients from importing while the latter endorsing. I added server-only.

if (!session || !session.token) {
return [];
}
const savedOpportunities = await getSavedOpportunities(
session.token,
session.user_id as string,
);
return savedOpportunities;
} catch (e) {
console.error("Error fetching saved opportunities:", e);
return [];
}
};
16 changes: 16 additions & 0 deletions frontend/src/utils/testing/opportunityMock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { Opportunity } from "src/types/search/searchResponseTypes";

export const mockOpportunity: Opportunity = {
opportunity_id: 12345,
opportunity_title: "Test Opportunity",
opportunity_status: "posted",
summary: {
archive_date: "2023-01-01",
close_date: "2023-02-01",
post_date: "2023-01-15",
agency_name: "Test Agency",
award_ceiling: 50000,
award_floor: 10000,
},
opportunity_number: "OPP-12345",
} as Opportunity;
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jest.mock("src/services/auth/session", () => ({
}));

jest.mock("src/services/fetch/fetchers/savedOpportunityFetcher", () => ({
getSavedOpportunities: () => [{ opportunity_id: 1 }],
fetchSavedOpportunities: () => [{ opportunity_id: 1 }],
}));

const makeSearchResults = (overrides = {}) => ({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,26 +1,11 @@
import { axe } from "jest-axe";
import { Opportunity } from "src/types/search/searchResponseTypes";
import { mockOpportunity } from "src/utils/testing/opportunityMock";
import { render, screen, waitFor } from "tests/react-utils";

import React from "react";

import SearchResultsListItem from "src/components/search/SearchResultsListItem";

const mockOpportunity: Opportunity = {
opportunity_id: 12345,
opportunity_title: "Test Opportunity",
opportunity_status: "posted",
summary: {
archive_date: "2023-01-01",
close_date: "2023-02-01",
post_date: "2023-01-15",
agency_name: "Test Agency",
award_ceiling: 50000,
award_floor: 10000,
},
opportunity_number: "OPP-12345",
} as Opportunity;

describe("SearchResultsListItem", () => {
it("should not have basic accessibility issues", async () => {
const { container } = render(
Expand Down
29 changes: 28 additions & 1 deletion frontend/tests/pages/saved-grants/page.test.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
import { render, screen, waitFor } from "@testing-library/react";
import { axe } from "jest-axe";
import SavedGrants from "src/app/[locale]/saved-grants/page";
import { OpportunityApiResponse } from "src/types/opportunity/opportunityResponseTypes";
import { SavedOpportunity } from "src/types/saved-opportunity/savedOpportunityResponseTypes";
import { localeParams, mockUseTranslations } from "src/utils/testing/intlMocks";
import { mockOpportunity } from "src/utils/testing/opportunityMock";
import { render, screen, waitFor } from "tests/react-utils";

jest.mock("next-intl/server", () => ({
getTranslations: () => Promise.resolve(mockUseTranslations),
}));

const savedOpportunities = jest.fn().mockReturnValue([]);
const opportunity = jest.fn().mockReturnValue({ data: [] });

jest.mock("src/services/fetch/fetchers/opportunityFetcher", () => ({
getOpportunityDetails: () => opportunity() as Promise<OpportunityApiResponse>,
}));

jest.mock("src/services/fetch/fetchers/savedOpportunityFetcher", () => ({
fetchSavedOpportunities: () =>
savedOpportunities() as Promise<SavedOpportunity[]>,
}));

describe("Saved Grants page", () => {
it("renders intro text for user with no saved grants", async () => {
const component = await SavedGrants({ params: localeParams });
Expand All @@ -17,6 +32,18 @@ describe("Saved Grants page", () => {
expect(content).toBeInTheDocument();
});

it("renders a list of saved grants", async () => {
savedOpportunities.mockReturnValue([{ opportunity_id: 12345 }]);
opportunity.mockReturnValue({ data: mockOpportunity });
const component = await SavedGrants({ params: localeParams });
render(component);

expect(screen.getByText("Test Opportunity")).toBeInTheDocument();
expect(screen.getByText("OPP-12345")).toBeInTheDocument();
const listItems = screen.getAllByRole("listitem");
expect(listItems).toHaveLength(1);
});

it("passes accessibility scan", async () => {
const component = await SavedGrants({ params: localeParams });
const { container } = render(component);
Expand Down
Loading