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

Subscription feature #8

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

Loulou5702
Copy link

@Loulou5702 Loulou5702 commented Dec 13, 2024

This pull request allow a user tu subscribe with an email.
The user will be inscribed into the database on a local supabase.
A welcoming email will be send via the Postmark service with all the inscruction for futher communication.

Welocming Email with HTML + CSS format :
welcomingEmail_HTML_CSS

Welcoming Email in Raw Txt Markdown format :
welcomingEmail_RAW_TXT

Visual changes on the webpage with an input field :
subscribe_form

The supabase Subscribers table with some example raws :
supabase_subscriber_table

Aurelien-Gindre and others added 29 commits December 5, 2024 10:53
@benoitchazoule benoitchazoule added the RFR Ready For Review label Dec 16, 2024
@benoitchazoule
Copy link
Collaborator

My comment regarding translation applies to all messages returned by the services, whether they are error messages or success messages from processing

Copy link
Collaborator

@adguernier adguernier left a comment

Choose a reason for hiding this comment

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

Partial review. In addition, please be careful to your English in the description:

- This pull request allow a user tu subscribe with an email.
+ This pull request allows a user tu subscribe with an email.
- The user will be inscribed into the database on a local supabase.
+ The user will be saved in the database on a local supabase.
- A welcoming email will be send via the Postmark service with all the inscruction for futher communication.
+ A welcome email will be sent via the Postmark service with all instructions for further communication.

- Welocming Email with HTML + CSS format :
+ Welcoming Email with HTML + CSS format:

Copy link
Collaborator

@adguernier adguernier Dec 17, 2024

Choose a reason for hiding this comment

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

polish: Seeds are usually used to populate the database as described in the documentation.
To perform actions such as create, update, or delete your database schema, you should use migration instead.
This will simplify the deployment process

@@ -0,0 +1,11 @@
// src/utils/validateEmail.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

polish (niptick): this comment is useless, you could remove it


export function Features() {
const { t } = useTranslation();
const [email, setEmail] = useState('');
const [message, setMessage] = useState('');
const [isSubmitting, setIsSubmitting] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: What is this state for? isSbumitting appears to be unused

Copy link
Contributor

Choose a reason for hiding this comment

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

It was used to preserve the text field when reloading the page, but there are no other uses.

Comment on lines 20 to 61

// Necessery field to validate
if (!recipientEmail || !subject || !textBody || !htmlBody) {
return res.status(400).json({ error: 'Missing required fields' });
}

try {
const postmarkApiToken =
process.env.NEXT_PUBLIC_POSTMARK_API_SERVER_TOKEN;
const defaultSenderEmail = process.env.NEXT_PUBLIC_DEFAULT_SENDING_EMAIL;

if (!postmarkApiToken || !defaultSenderEmail) {
return res.status(500).json({
error: 'Missing Postmark API token or default sender email',
});
}

const client = new Client(postmarkApiToken);
const emailData = {
From: defaultSenderEmail,
To: recipientEmail,
Subject: subject,
TextBody: textBody,
HtmlBody: htmlBody,
};

// Sending Email via Postmark API
const response = await client.sendEmail(emailData);
if (response && response.ErrorCode) {
return res
.status(500)
.json({ error: `Postmark Error: ${response.Message}` });
}

return res.status(200).json({ message: 'Email sent successfully!' });
} catch (error) {
console.error('Error in sending email:', error);
return res.status(500).json({ error: 'Error sending email' });
}
} else {
return res.status(405).json({ error: 'Method Not Allowed' });
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (niptick): to avoid many nestedif:

Suggested change
// Necessery field to validate
if (!recipientEmail || !subject || !textBody || !htmlBody) {
return res.status(400).json({ error: 'Missing required fields' });
}
try {
const postmarkApiToken =
process.env.NEXT_PUBLIC_POSTMARK_API_SERVER_TOKEN;
const defaultSenderEmail = process.env.NEXT_PUBLIC_DEFAULT_SENDING_EMAIL;
if (!postmarkApiToken || !defaultSenderEmail) {
return res.status(500).json({
error: 'Missing Postmark API token or default sender email',
});
}
const client = new Client(postmarkApiToken);
const emailData = {
From: defaultSenderEmail,
To: recipientEmail,
Subject: subject,
TextBody: textBody,
HtmlBody: htmlBody,
};
// Sending Email via Postmark API
const response = await client.sendEmail(emailData);
if (response && response.ErrorCode) {
return res
.status(500)
.json({ error: `Postmark Error: ${response.Message}` });
}
return res.status(200).json({ message: 'Email sent successfully!' });
} catch (error) {
console.error('Error in sending email:', error);
return res.status(500).json({ error: 'Error sending email' });
}
} else {
return res.status(405).json({ error: 'Method Not Allowed' });
}
if (req.method !== 'POST') {
return res.status(405).json({ error: 'Method Not Allowed' });
}
const { recipientEmail, subject, textBody, htmlBody }: EmailRequestBody =
req.body;
// Necessery field to validate
if (!recipientEmail || !subject || !textBody || !htmlBody) {
return res.status(400).json({ error: 'Missing required fields' });
}
try {
const postmarkApiToken = process.env.NEXT_PUBLIC_POSTMARK_API_SERVER_TOKEN;
const defaultSenderEmail = process.env.NEXT_PUBLIC_DEFAULT_SENDING_EMAIL;
if (!postmarkApiToken || !defaultSenderEmail) {
return res.status(500).json({
error: 'Missing Postmark API token or default sender email',
});
}
const client = new Client(postmarkApiToken);
const emailData = {
From: defaultSenderEmail,
To: recipientEmail,
Subject: subject,
TextBody: textBody,
HtmlBody: htmlBody,
};
// Sending Email via Postmark API
const response = await client.sendEmail(emailData);
if (response && response.ErrorCode) {
return res
.status(500)
.json({ error: `Postmark Error: ${response.Message}` });
}
return res.status(200).json({ message: 'Email sent successfully!' });
} catch (error) {
console.error('Error in sending email:', error);
return res.status(500).json({ error: 'Error sending email' });
}

"next": "^14.0.4",
"postmark": "^4.0.5",
"postmark": "^4.0.5",

Choose a reason for hiding this comment

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

issue: postmark is duplicated here

@@ -11,6 +11,8 @@
"browserslist": "defaults, not ie <= 11",
"dependencies": {
"@headlessui/react": "^2.1.0",
"@supabase/supabase-js": "^2.46.2",
"@supabase/supabase-js": "^2.46.2",

Choose a reason for hiding this comment

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

issue: @supabase/supabase-js is duplicated here

@@ -31,6 +57,38 @@ export function Features() {
</li>
))}
</ul>
{/* email form */}
<form className="mt-10 px-20" onSubmit={handleEmailSubmission}>
<h3 className="text-lg font-medium tracking-tight text-black">

Choose a reason for hiding this comment

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

question: Is there an h2 prior to this h3 in the page once rendered ? If not, can you make this an h2 for semantics ?

{t('feature.try')}
</h3>
<div className="mt-4 sm:relative sm:flex sm:items-center sm:py-0.5 sm:pr-2.5">
<div className="relative sm:static sm:flex-auto">

Choose a reason for hiding this comment

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

nit-pick: A label tag would be probably better for accessibility here with corresponding aria attributes here

<h3 className="text-lg font-medium tracking-tight text-black">
{t('feature.try')}
</h3>
<div className="mt-4 sm:relative sm:flex sm:items-center sm:py-0.5 sm:pr-2.5">

Choose a reason for hiding this comment

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

suggestion: using relative positioning with a relative background is probably not the best way to design this button.

You should look at flex positioning with the parent element having the border

req: NextApiRequest,
res: NextApiResponse,
) {
if (req.method === 'POST') {

Choose a reason for hiding this comment

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

issue: Avoid nested if here, it makes code less readable. Use early return instead.

* Handles the logic for sending a welcome email.
* @param email - The recipient's email address.
*/
export async function handleSendWelcomeEmail(email: string): Promise<void> {
Copy link

@arnault-dev arnault-dev Dec 18, 2024

Choose a reason for hiding this comment

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

question: Why not using a server action here ? This would be make the code way easier to maintain over time instead of splitting service / api

* Handles the logic for subscribing an email, including validation.
* @param email - The email address to subscribe.
*/
export async function handleSubscription(email: string): Promise<void> {
Copy link

@arnault-dev arnault-dev Dec 18, 2024

Choose a reason for hiding this comment

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

question: Why not using a server action here ? This would be make the code way easier to maintain over time instead of splitting service / api

@@ -0,0 +1,120 @@
export const welcomeEmailText = `

Choose a reason for hiding this comment

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

nit-pick: Maybe add i18n also for welcome email ? If the website is in french, I expect the welcome mail to be in french as well.

@@ -19,7 +19,7 @@ export default function RootLayout({
}) {
useEffect(() => {
document.documentElement.lang = i18next.language;
}, [i18next.language]);
});

Choose a reason for hiding this comment

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

Why remove the dependency array here ?

If you do not want the effect to run more that once, add an empty array as dependencies:

Suggested change
});
}, []);


const handleEmailSubmission = async (e: React.FormEvent) => {
const HandleEmailSubmission = async (e: React.FormEvent) => {

Choose a reason for hiding this comment

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

Please use lower camel case for function naming

Suggested change
const HandleEmailSubmission = async (e: React.FormEvent) => {
const handleEmailSubmission = async (e: React.FormEvent) => {

},
"mail": {
"txt": "Welcome to Curator AI! Curator AI is here to simplify your information tracking. With our AI, you'll receive a newsletter containing summarized and curated articles based on your preferred topics and sources. Send back an email with one of these instructions to interact with Curator AI:\n### 📚 Add topics: *I want to follow the topics AI and health.*\n### 🌐 Add sites: *Add https://www.lemonde.fr/ to my sources.*\n### ⏰ Set delivery schedule: *Send me the newsletter every Monday at 8 AM.*\nBy default, you will receive your first newsletter within 5 minutes, and then once a week on the same day and time as your registration.",
"html": "<!DOCTYPE html><html><head>STYLE TOKEN</head><body><div class=\"email-container\"><h1 class=\"header\">Welcome to Curator AI!</h1><p class=\"intro\">We're thrilled to have you on board 🎉.</p><p class=\"content\">With our AI, you'll receive a personalized newsletter containing the most relevant articles, summarized and tailored to your preferences.</p><h2 class=\"subheader\">How to interact with Curator AI? Answer by email with one of the followings:</h2><ul class=\"instructions\"><li> 📚 Add topics: <strong>\"I want to follow the topics AI and health.\"</strong></li><li> 🌐 Add a site: <strong>\"Add https://www.lemonde.fr/ to my sources.\"</strong></li><li> ⏰ Set delivery schedule: <strong>\"Send me the newsletter every Monday at 8 AM.\"</strong></li></ul><p class=\"reminder\">📩 Your first newsletter will be sent within 5 minutes of signing up. By default, future newsletters will arrive weekly at the same time as your registration.</p><p class=\"closing\">For any questions or changes, just reply to this email. Happy exploring!</p><p class=\"signature\">The Curator AI Team 🚀</p></div></body></html>"

Choose a reason for hiding this comment

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

nit-pick: not a big fan of having the whole HTML email in translations. This makes maintenance harder over time. Can you use a template with mutiple tanslation keys ?

setMessage('');

try {
await handleSubscription(email);

await handleSendWelcomeEmail(email);
const response = await fetch('/api/sendWelcomeEmail', {

Choose a reason for hiding this comment

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

Why not using a server side action for this instead of an API call ?

const recipientEmail = email;
const subject = 'Welcome to CURATOR AI! 🚀';
htmlBody = htmlBody.replace(
'STYLE TOKEN',

Choose a reason for hiding this comment

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

This implementation might lead to bugs over time, please add the styles to the HTML templates directly

import { NextApiRequest, NextApiResponse } from 'next';
import { handleSendWelcomeEmail } from '@/services/emailService';

export default function sendWelcomeEmail(

Choose a reason for hiding this comment

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

Please use a server side action instead

@@ -74,13 +81,13 @@ export function Features() {
onChange={(e) => setEmail(e.target.value)} // Mettez à jour l'email dans l'état
className="peer relative z-10 w-full appearance-none bg-transparent px-4 py-2 text-base text-black placeholder:text-slate-400/70 focus:outline-none sm:py-3"
/>
<div className="absolute inset-0 rounded-md border border-black/40 peer-focus:border-black peer-focus:ring-1 peer-focus:ring-blue-300 sm:rounded-xl" />
{/* <div className="absolute " /> */}

Choose a reason for hiding this comment

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

issue: commented code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants