Feat/sync newsletter settings #773
Conversation
|
@NiallJoeMaher is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate Limit Exceeded@NiallJoeMaher has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 46 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update integrates a newsletter subscription check into user profile access and management, enhancing user experience and security. Users not subscribed to the newsletter are redirected to a 404 page when accessing profile settings. The backend now verifies newsletter subscription status and improves error handling and readability in managing subscriptions. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
package.jsonis excluded by:!**/*.json
Files selected for processing (3)
- app/(app)/settings/page.tsx (2 hunks)
- server/api/router/profile.ts (2 hunks)
- server/lib/newsletter.ts (1 hunks)
Additional comments: 2
app/(app)/settings/page.tsx (1)
- 1-1: The import statement correctly adds
notFoundfromnext/navigationandisUserSubscribedToNewsletterfrom the server library, aligning with the PR's objectives to enhance the user settings page by checking newsletter subscription status.server/api/router/profile.ts (1)
- 11-14: The import statements for
isUserSubscribedToNewsletterandmanageNewsletterSubscriptionare correctly added, enabling the profile management API to utilize these functions for newsletter subscription management.
| if (!session.user.email) { | ||
| return notFound(); | ||
| } |
There was a problem hiding this comment.
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.
| 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(); | |
| } |
| 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`); | ||
| } |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
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.
| 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"); |
server/api/router/profile.ts
Outdated
| 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) { |
There was a problem hiding this comment.
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.
| 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) { |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✨ Codu Pull Request 💻
Pull Request details
Any Breaking changes
Summary by CodeRabbit