[Issue #1362] Sendy Update #2530

acouch · 2024-10-21T15:39:07Z

Summary

Fixes #1362

Time to review: 15 mins

Changes proposed

This builds on #2428 by adding tests. See that PR for details.

emilycnava

Pair reviewed with Aaron, discussed changes between original draft PR and this one and it all LGTM!

doug-s-nava

Everything that's here looks good but curious why the e2e test was deleted. Will go through some manual testing now

doug-s-nava · 2024-10-25T19:46:05Z

frontend/src/app/[locale]/subscribe/actions.ts

+): Promise<sendyResponse> {
+  const schema = z.object({
+    name: z.string().min(1, {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call


does next-intl have any recommendations on how to pass around the t function in a typescript app? There has to be a better way, unless this typing is specifically designed to discourage passing it around. If that's the case then... really?

Looks like it is discouraged by the maintainer but there is a better way. Let's try that.

Very weird. It seems like they're saying they discourage passing the function around because the typing is cumbersome, rather than that they specifically left the typing cumbersome in order to discourage passing the function around for some more thought out reason.

I can see not wanting to pass this function around for one particular reason: if you pass t from a server component to a client component you end up potentially closing over the entire messages object, and shipping the entire text content of your site to the client unnecessarily. I'm not exactly sure how next-intl works or if this complexity is expected as the cost of doing business but it seems potentially, and this is a very scientific word here, slops.

I think there is a lot of thinking to be done around how next-intl can / should be used on a big, complex, scalable app that has to manage a large amount of translated content.

doug-s-nava · 2024-10-25T19:51:56Z

frontend/src/i18n/messages/en/index.ts

-        "Sorry, an unexpected error in our system occured when trying to save your subscription. If this continues to happen, you may email <email>[email protected]</email>. Error: <sendy_error/>",
+      server:
+        "Failed to subscribe, due to a server error. Please try again later.",
+      already_subscribed: "This email address has already been subscribed.",


[nit] curious why this was changed - was the existing message too long / complex?

Just curious where the impetus for changing "<email_address/> is already subscribed. If you’re not seeing our emails, check your spam folder and add [email protected] to your contacts, address book, or safe senders list. If you continue to not receive our emails, contact [email protected]." to "This email address has already been subscribed." is coming from exactly since I didn't see it in any requirements. Very possible I just missed that req somewhere

Sorry, I should have been more verbose. Not sure where that came from myself. @emilycnava ?

It wasn't a req, just a choice I made due to the positioning of the error message being different and long messages looking weird. Also the email var insertion code was broken with the old approach.

Old:

New:

I'm ok with this since it's fixing a bug with the formatting, but we should probably get sign off from design / content on the final text

frontend/tests/pages/subscribe/SubscriptionForm.test.tsx

frontend/src/app/[locale]/subscribe/actions.ts

doug-s-nava · 2024-10-25T19:58:23Z

frontend/tests/pages/subscribe/subscribeEmailAction.test.tsx

+    );
+    expect(testErrorAlreadySubscribed.validationErrors).toEqual({});
+
+    // Test response that indicates server error


[nit] might consider breaking this into 4 separate tests, so that if one use case breaks it's easier to track down

doug-s-nava

when running an npm install on the branch I get a small diff in the package-lock removing "@types/jest": "^29.5.13",. Seems like either something we should figure out or commit
~~I get a hydration error on the subscribe page when I load it up, see screenshot.~~ (Update: I think this was a blip, restarted my server and things seem fine)
I get a "failed to susbscrbe, due to a server error. Please try again later." message on the frontend when trying to subscribe. On the Next backend I see Error subscribing user: Failed to parse URL from /subscribe

acouch · 2024-10-28T15:56:00Z

@doug-s-nava

I get a "failed to susbscrbe, due to a server error. Please try again later." message on the frontend when trying to subscribe. On the Next backend I see Error subscribing user: Failed to parse URL from /subscribe

I think this might be because you don't have the sendy credentials? I updated the docs to reflect needing that: 94e62a8

Everything that's here looks good but curious why the e2e test was deleted. Will go through some manual testing now

They don't work currently with server actions. Created a ticket to fix that: #2537

doug-s-nava

Local test with the correct envs checks out. Holding on approval for clarity around the "already subscribed" messaging change but the rest all LGTM

emilycnava · 2024-10-28T19:13:04Z

@doug-s-nava Answered your Q r.e. already_subscribed msg above :)

doug-s-nava

a few qualms but nothing to block this from going out. Really good important stuff!

✅ ✅ ✅ ✅

doug-s-nava · 2024-10-29T14:41:10Z

frontend/src/i18n/messages/en/index.ts

-        "Sorry, an unexpected error in our system occured when trying to save your subscription. If this continues to happen, you may email <email>[email protected]</email>. Error: <sendy_error/>",
+      server:
+        "Failed to subscribe, due to a server error. Please try again later.",
+      already_subscribed: "This email address has already been subscribed.",


I'm ok with this since it's fixing a bug with the formatting, but we should probably get sign off from design / content on the final text

doug-s-nava · 2024-10-29T14:44:20Z

frontend/src/app/[locale]/subscribe/actions.ts

+}
+
+export async function subscribeEmailAction(
+  t: TFn,


I wonder if the real solution here is to just pass along the strings that the function needs rather than passing down the entire "t" function. That would likely mean passing a lot of strings, though, or basically partially recreating the messaging object in order to pass through just what we need. In any case I'm still worried about the entire message object getting sent down to the client, but not enough to continue blocking this PR, especially since I don't see an immediately ideal solution otherwise.

That was the solution I took as we moved to the app router: https://github.com/HHS/simpler-grants-gov/blob/2024.4.25-1/frontend/src/app/search/page.tsx#L51 . It is OK for small examples, but kind of got cumbersome for whole pages (can't find an example).

doug-s-nava · 2024-10-29T14:45:58Z

frontend/src/app/[locale]/subscribe/actions.ts

+  prevState: sendyResponse,
+  formData: FormData,
+) {
+  const t = await getTranslations();


if we do

Suggested change

const t = await getTranslations();

const t = await getTranslations("Subscribe");

do we only close over that part of the messaging object? if so that could be a nice middle ground here

await getTranslations("Subscribe"); caused a linting error.

github-actions bot added frontend typescript labels Oct 21, 2024

acouch force-pushed the acouch/issue-1362-testing-form branch from 0cb2ba0 to ac812dc Compare October 21, 2024 15:40

acouch marked this pull request as ready for review October 25, 2024 17:49

acouch requested review from andycochran and doug-s-nava as code owners October 25, 2024 17:50

emilycnava and others added 21 commits October 25, 2024 13:50

Form skeleton

dce1d76

Add validation

204f6b1

cleanup files

1912e75

fix pending state

17d50cc

undo unintended change

1c541e7

use i18n files

9bde5c3

Add test skeletons

6ae6ad7

test wip

826a1a3

button test

5a9bddf

add server error test

2ab57f7

more testing wip

ee180a5

tidy up pt 1

c070c16

tidy up cont

bafbc55

Add already subscribed handler and more robust logging

399d02f

Testing form tst

ef08370

Update for tests

cf11d7f

Update subscribe tests

a32ec54

Update tests for linting

40116a5

Update tests for linting

8a1cf6f

Update subscribe.spec.ts for linting

a4b72ab

Delete subscribe.spec.ts

3904d80

acouch force-pushed the acouch/issue-1362-testing-form branch from acda6a6 to 3904d80 Compare October 25, 2024 17:50

emilycnava mentioned this pull request Oct 25, 2024

Refactor api route and subscription form to use server actions/app router #2428

Closed

emilycnava approved these changes Oct 25, 2024

View reviewed changes

Update for proper mock function

3dabb52

acouch changed the title ~~DRAFT [Issue #1362] Sendy Update~~ [Issue #1362] Sendy Update Oct 25, 2024

doug-s-nava reviewed Oct 25, 2024

View reviewed changes

doug-s-nava reviewed Oct 28, 2024

View reviewed changes

Update README.md

94e62a8

github-actions bot added the documentation Improvements or additions to documentation label Oct 28, 2024

doug-s-nava reviewed Oct 28, 2024

View reviewed changes

Update to add TFn type

44d75b8

doug-s-nava approved these changes Oct 29, 2024

View reviewed changes

acouch merged commit fa3aabd into main Oct 29, 2024
13 checks passed

acouch deleted the acouch/issue-1362-testing-form branch October 29, 2024 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #1362] Sendy Update #2530

[Issue #1362] Sendy Update #2530

acouch commented Oct 21, 2024 •

edited

Loading

emilycnava left a comment

doug-s-nava left a comment

doug-s-nava Oct 25, 2024

acouch Oct 28, 2024

doug-s-nava Oct 28, 2024 •

edited

Loading

doug-s-nava Oct 25, 2024

acouch Oct 28, 2024

doug-s-nava Oct 28, 2024

acouch Oct 28, 2024

emilycnava Oct 28, 2024

doug-s-nava Oct 29, 2024

doug-s-nava Oct 25, 2024

doug-s-nava left a comment •

edited

Loading

acouch commented Oct 28, 2024

doug-s-nava left a comment

emilycnava commented Oct 28, 2024

doug-s-nava left a comment

doug-s-nava Oct 29, 2024

doug-s-nava Oct 29, 2024

acouch Oct 29, 2024

doug-s-nava Oct 29, 2024

acouch Oct 29, 2024

	const t = await getTranslations();
	const t = await getTranslations("Subscribe");

[Issue #1362] Sendy Update #2530

[Issue #1362] Sendy Update #2530

Conversation

acouch commented Oct 21, 2024 • edited Loading

Summary

Time to review: 15 mins

Changes proposed

emilycnava left a 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

doug-s-nava Oct 28, 2024 • edited Loading

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

doug-s-nava left a comment • edited Loading

Choose a reason for hiding this comment

acouch commented Oct 28, 2024

doug-s-nava left a comment

Choose a reason for hiding this comment

emilycnava commented Oct 28, 2024

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

acouch commented Oct 21, 2024 •

edited

Loading

doug-s-nava Oct 28, 2024 •

edited

Loading

doug-s-nava left a comment •

edited

Loading