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

Feat/sync newsletter settings #773

Merged
Merged
Show file tree
Hide file tree
Changes from 4 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
19 changes: 17 additions & 2 deletions app/(app)/settings/page.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { redirect } from "next/navigation";
import { notFound, redirect } from "next/navigation";
import Content from "./_client";
import { getServerAuthSession } from "@/server/auth";
import { customAlphabet } from "nanoid";
import prisma from "@/server/db/client";
import { isUserSubscribedToNewsletter } from "@/server/lib/newsletter";

export const metadata = {
title: "Settings - Update your profile",
Expand Down Expand Up @@ -55,5 +56,19 @@ export default async function Page() {
return <Content profile={newUser} />;
}

return <Content profile={user} />;
if (!session.user.email) {
return notFound();
}
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic to return a 404 not found page if the user's email is missing is a good security practice, ensuring that only users with a valid email can proceed. However, this might not directly relate to the newsletter subscription status. Consider adding a comment to clarify this choice or adjusting the logic to specifically check for newsletter subscription status.

+ // Ensure user has an email before proceeding. Consider checking for newsletter subscription explicitly if that's the intent.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if (!session.user.email) {
return notFound();
}
// Ensure user has an email before proceeding. Consider checking for newsletter subscription explicitly if that's the intent.
if (!session.user.email) {
return notFound();
}


try {
const newsletter = await isUserSubscribedToNewsletter(session.user.email);
const cleanedUser = {
...user,
newsletter,
};
return <Content profile={cleanedUser} />;
} catch (error) {
Sentry.captureException(error);
return <Content profile={user} />;
}
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"lint:fix": "next lint --fix",
"prettier": "prettier --check -c '**/*.{ts,tsx,js,jsx,json,json5,scss,css,html,mdx}'",
"prettier:fix": "prettier --write -c '**/*.{ts,tsx,js,jsx,json,json5,scss,css,html,mdx}'",
"test": "playwright test",
"test": "npx playwright install && playwright test",
"studio": "prisma studio",
"migrate": "npx prisma migrate",
"prepare": "husky install"
Expand Down
18 changes: 16 additions & 2 deletions server/api/router/profile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {
import { getPresignedUrl } from "../../common/getPresignedUrl";

import { createTRPCRouter, publicProcedure, protectedProcedure } from "../trpc";
import { manageNewsletterSubscription } from "@/server/lib/newsletter";
import {
isUserSubscribedToNewsletter,
manageNewsletterSubscription,
} from "@/server/lib/newsletter";
import { TRPCError } from "@trpc/server";
import { nanoid } from "nanoid";

Expand All @@ -22,7 +25,18 @@ export const profileRouter = createTRPCRouter({
},
});

if (existingProfile?.newsletter !== input.newsletter) {
const { email } = ctx.session.user;

if (!email) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Email not found",
});
}

const newsletter = await isUserSubscribedToNewsletter(email);

if (newsletter !== input.newsletter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic added to check the user's email existence and newsletter subscription status before managing the subscription is a good security and usability enhancement. However, there's a redundant email variable declaration inside the if block. Since email is already defined outside, you can remove the redundant declaration to clean up the code.

-        const email = existingProfile?.email;

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const { email } = ctx.session.user;
if (!email) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Email not found",
});
}
const newsletter = await isUserSubscribedToNewsletter(email);
if (newsletter !== input.newsletter) {
const { email } = ctx.session.user;
if (!email) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Email not found",
});
}
const newsletter = await isUserSubscribedToNewsletter(email);
if (newsletter !== input.newsletter) {

const email = existingProfile?.email;
const action = input.newsletter ? "subscribe" : "unsubscribe";
if (!email) {
Expand Down
52 changes: 41 additions & 11 deletions server/lib/newsletter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,51 @@ export async function manageNewsletterSubscription(
silent: "true", // Don't send a confirmation email (using this option for users signed up to platform, not newsletter only option)
}).toString();

try {
const response = await fetch(`${EMAIL_API_ENDPOINT}/${action}`, {
const response = await fetch(`${EMAIL_API_ENDPOINT}/${action}`, {
method: "POST",
headers: {
"Content-type": "application/x-www-form-urlencoded",
},
body: payload,
});

if (response.ok) {
return { message: `Successfully ${action}d to the newsletter.` };
} else {
throw new Error(`Failed to ${action} to the newsletter`);
}
Comment on lines +23 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

The manageNewsletterSubscription function's error handling has been improved by throwing an error when the response is not OK. This is a good practice as it ensures that any issues with the subscription process are caught and can be handled appropriately. However, consider enhancing the error message with more details from the response to aid in debugging.

-    throw new Error(`Failed to ${action} to the newsletter`);
+    const errorDetail = await response.text();
+    throw new Error(`Failed to ${action} to the newsletter: ${errorDetail}`);

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const response = await fetch(`${EMAIL_API_ENDPOINT}/${action}`, {
method: "POST",
headers: {
"Content-type": "application/x-www-form-urlencoded",
},
body: payload,
});
if (response.ok) {
return { message: `Successfully ${action}d to the newsletter.` };
} else {
throw new Error(`Failed to ${action} to the newsletter`);
}
const response = await fetch(`${EMAIL_API_ENDPOINT}/${action}`, {
method: "POST",
headers: {
"Content-type": "application/x-www-form-urlencoded",
},
body: payload,
});
if (response.ok) {
return { message: `Successfully ${action}d to the newsletter.` };
} else {
const errorDetail = await response.text();
throw new Error(`Failed to ${action} to the newsletter: ${errorDetail}`);
}

}

export async function isUserSubscribedToNewsletter(email: string) {
const EMAIL_API_ENDPOINT = process.env.EMAIL_API_ENDPOINT;
const EMAIL_API_KEY = process.env.EMAIL_API_KEY;
const EMAIL_NEWSLETTER_ID = process.env.EMAIL_NEWSLETTER_ID;

if (!EMAIL_API_ENDPOINT || !EMAIL_API_KEY || !EMAIL_NEWSLETTER_ID) {
throw new Error("Email API not configured");
}

const payload = new URLSearchParams({
email,
api_key: EMAIL_API_KEY,
list_id: EMAIL_NEWSLETTER_ID,
}).toString();

const response = await fetch(
`${EMAIL_API_ENDPOINT}/api/subscribers/subscription-status.php`,
{
method: "POST",
headers: {
"Content-type": "application/x-www-form-urlencoded",
},
body: payload,
});

if (response.ok) {
return { message: `Successfully ${action}d to the newsletter.` };
} else {
throw new Error(`Failed to ${action} to the newsletter`);
}
} catch (error) {
Sentry.captureException(error);
},
);

if (response.ok) {
const status = await response.text();
return status === "Subscribed";
} else {
throw new Error("Failed to check newsletter subscription");
Comment on lines +38 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

The new function isUserSubscribedToNewsletter correctly checks a user's newsletter subscription status. It's a good addition for enhancing the newsletter management capabilities. However, returning a boolean for subscription status is a bit limiting. Consider returning more detailed information about the subscription status, such as "Not Subscribed", "Subscribed", or "Pending Confirmation", to provide more context to the caller.

-    return status === "Subscribed";
+    return status; // Adjust the return type and handle the detailed status in the caller.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
export async function isUserSubscribedToNewsletter(email: string) {
const EMAIL_API_ENDPOINT = process.env.EMAIL_API_ENDPOINT;
const EMAIL_API_KEY = process.env.EMAIL_API_KEY;
const EMAIL_NEWSLETTER_ID = process.env.EMAIL_NEWSLETTER_ID;
if (!EMAIL_API_ENDPOINT || !EMAIL_API_KEY || !EMAIL_NEWSLETTER_ID) {
throw new Error("Email API not configured");
}
const payload = new URLSearchParams({
email,
api_key: EMAIL_API_KEY,
list_id: EMAIL_NEWSLETTER_ID,
}).toString();
const response = await fetch(
`${EMAIL_API_ENDPOINT}/api/subscribers/subscription-status.php`,
{
method: "POST",
headers: {
"Content-type": "application/x-www-form-urlencoded",
},
body: payload,
});
if (response.ok) {
return { message: `Successfully ${action}d to the newsletter.` };
} else {
throw new Error(`Failed to ${action} to the newsletter`);
}
} catch (error) {
Sentry.captureException(error);
},
);
if (response.ok) {
const status = await response.text();
return status === "Subscribed";
} else {
throw new Error("Failed to check newsletter subscription");
if (response.ok) {
const status = await response.text();
return status; // Adjust the return type and handle the detailed status in the caller.
} else {
throw new Error("Failed to check newsletter subscription");

}
}