[#3306] Add save button #3677

acouch · 2025-01-29T16:19:27Z

Summary

One part of #3306

Time to review: 45 mins

This adds the save button to the opportunity page.

To Review

Run the API: cd api && make remake-backed

Start the site: cd frontend && npm run dev

Login and save an opportunity.

The button matches pretty well with PixelPerfect

frontend/src/app/[locale]/opportunity/[id]/page.tsx

acouch · 2025-02-06T17:26:10Z

frontend/src/app/[locale]/search/error.tsx

@@ -67,7 +67,7 @@ export default function SearchError({ error, reset }: ErrorProps) {
    ) {
      reset();
    }
-  }, [searchParams, reset]);
+  }, [searchParams, reset, previousSearchParams]);


This came up from ts:check locally.

acouch · 2025-02-06T17:27:22Z

frontend/src/app/api/auth/logout/route.ts

-      { message: `Error logging out: ${error.message}` },
-      { status: 500 },
+      { message: `Error logging out: ${(cause as string) ?? message}` },
+      { status },


This passes on the status from our own errors.

frontend/src/components/LoginButtonModal.tsx

acouch · 2025-02-06T17:32:03Z

frontend/src/components/SaveButton.tsx

+                "usa-alert__heading margin-bottom-0 margin-top-05 font-weight-100",
+                {
+                  "margin-left-2 font-sans-xs": !error,
+                  "margin-left-5 font-sans-sm": error,


The error and success designs are different, hence the margin changes:

vs

acouch · 2025-02-06T17:32:42Z

frontend/src/components/opportunity/OpportunityIntro.tsx

@@ -55,11 +53,7 @@ const OpportunityIntro = ({ opportunityData }: Props) => {
  };

  return (
-    <ContentLayout


Opp title is moved to the page as part of the update

acouch · 2025-02-06T17:33:28Z

frontend/src/components/user/OpportunitySaveUserControl.tsx

+      }
+    } catch (error) {
+      setSavedError(true);
+      console.error(error);


Would want to add new relic here in the future.

frontend/src/components/user/OpportunitySaveUserControl.tsx

acouch · 2025-02-06T17:36:08Z

frontend/src/errors.ts

-    });
-    super(serializedData);
+    };
+    super(message, { cause });


Adds the details to the cause property which serializes it.

doug-s-nava

do we want to make a follow up for e2e tests around this functionality?

one note on the error front - noticed a case where a request failed with a string message, and since parseErrorStatus is relying on the error message being a valid json string, it errors, and we end up with a 500 response. We should probably take a pass through the code to make sure we've got that right - in any case assuming that the error message will be a json string seems presumptuous.

frontend/src/app/[locale]/opportunity/[id]/page.tsx

doug-s-nava · 2025-02-10T14:23:03Z

frontend/src/app/api/user/saved-opportunities/[id]/route.ts

+  try {
+    const session = await getSession();
+    if (!session || !session.token) {
+      throw new Error("No active session to get saved opportunity");


should this be a 401?

frontend/src/app/api/user/saved-opportunities/[id]/route.ts

doug-s-nava · 2025-02-10T14:32:55Z

frontend/src/app/api/user/saved-opportunities/route.ts

+  try {
+    const session = await getSession();
+    if (!session || !session.token) {
+      throw new Error("No active session to save opportunity");


same note as above - should a situation with no session or token be a 401?

doug-s-nava · 2025-02-10T15:02:30Z

frontend/src/app/api/user/saved-opportunities/route.ts

+};
+
+export const DELETE = async (request: Request) => {
+  return await handleRequest(request, "DELETE");


shouldn't need a second argument here - request.method should give you the string you need, and then we can simplify this a little.

frontend/src/components/user/OpportunitySaveUserControl.tsx

doug-s-nava · 2025-02-10T16:30:09Z

frontend/src/services/fetch/fetchers/savedOpportunityFetcher.ts

+    (savedOpportunity: { opportunity_id: number }) =>
+      savedOpportunity.opportunity_id === opportunity_id,
+  );
+  return savedOpportunity ?? [];


if this is expected to return an array, maybe annotate that since getSavedOpportunity suggests to me that it would return a single opportunity rather than a list

doug-s-nava · 2025-02-10T17:02:27Z

frontend/src/styles/_uswds-theme-custom-styles.scss

@@ -238,6 +238,35 @@ i.e.
  padding: units(1) units(2);
 }

+.simpler-save-button {
+  padding: units(0.5) units(2) units(0.5) units(1);


can we do this with existing classes?

doug-s-nava · 2025-02-10T17:05:26Z

frontend/tests/api/user/saved-opportunities/[id]/route.test.ts

+
+describe("GET request", () => {
+  afterEach(() => jest.clearAllMocks());
+  it("gets a saved opportunity", async () => {


can you add a test for the unauthorized and no saved opps error cases?

doug-s-nava · 2025-02-10T17:29:44Z

frontend/tests/api/user/saved-opportunities/route.test.ts

+  } as unknown as NextRequest;
+};
+
+describe("POST and DELETE request", () => {


similar to the other test, can you add a couple of error tests?

acouch requested review from andycochran, btabaska and doug-s-nava as code owners January 29, 2025 16:19

acouch force-pushed the acouch/issue-3306-save-opps-buttons branch 3 times, most recently from 44bf30a to 8b68b3a Compare January 29, 2025 17:03

acouch marked this pull request as draft January 30, 2025 16:42

acouch commented Feb 6, 2025

View reviewed changes

frontend/src/app/[locale]/opportunity/[id]/page.tsx Outdated Show resolved Hide resolved

acouch commented Feb 6, 2025

View reviewed changes

frontend/src/components/LoginButtonModal.tsx Show resolved Hide resolved

acouch commented Feb 6, 2025

View reviewed changes

frontend/src/components/user/OpportunitySaveUserControl.tsx Show resolved Hide resolved

acouch commented Feb 6, 2025

View reviewed changes

acouch marked this pull request as ready for review February 6, 2025 19:48

acouch added 13 commits February 6, 2025 14:49

Add save button

232b62a

Update loading spinner

954f9c7

Mid-morning commit

13d4287

Put buttons in place

dabaac8

Add working save button

d71cd0a

Add delete route

76523a3

Update to page layout and data handling

11771af

Update to fetchers

03a5b0b

Update to modals

6e4d758

Update modal message

9f57f61

Clean up opp page

a9631f6

Add test for initial route

fd4037c

Update tests

7f2c5e8

acouch and others added 6 commits February 6, 2025 14:53

Add SaveButton test

9087c0a

Update tests

9882ebe

Remove moving error location

d59fe4b

Update frontend/src/app/[locale]/opportunity/[id]/page.tsx

e2383bf

Update OpportunityIntro.test.tsx

09bdf42

Cleanup error reporting

b46aecc

acouch force-pushed the acouch/issue-3306-save-opps-buttons branch from ca60c29 to 840bb78 Compare February 6, 2025 20:00

Update error.tsx to fix merge issue

fd15fcd

acouch force-pushed the acouch/issue-3306-save-opps-buttons branch from 840bb78 to fd15fcd Compare February 6, 2025 20:04

Update to add feature flag

09324d0

doug-s-nava reviewed Feb 10, 2025

View reviewed changes

doug-s-nava mentioned this pull request Feb 10, 2025

Shared components for inline messages #3827

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3306] Add save button #3677

[#3306] Add save button #3677

acouch commented Jan 29, 2025 •

edited

Loading

acouch Feb 6, 2025

acouch Feb 6, 2025

acouch Feb 6, 2025

acouch Feb 6, 2025

acouch Feb 6, 2025

acouch Feb 6, 2025

doug-s-nava left a comment

doug-s-nava Feb 10, 2025

doug-s-nava Feb 10, 2025

doug-s-nava Feb 10, 2025

doug-s-nava Feb 10, 2025

doug-s-nava Feb 10, 2025

doug-s-nava Feb 10, 2025

doug-s-nava Feb 10, 2025

[#3306] Add save button #3677

Are you sure you want to change the base?

[#3306] Add save button #3677

Conversation

acouch commented Jan 29, 2025 • edited Loading

Summary

Time to review: 45 mins

To Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-s-nava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acouch commented Jan 29, 2025 •

edited

Loading