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: emails #10

Merged
merged 23 commits into from
Aug 1, 2024
Merged

feat: emails #10

merged 23 commits into from
Aug 1, 2024

Conversation

typeWolffo
Copy link
Member

No description provided.

@typeWolffo typeWolffo requested a review from mikoscz July 23, 2024 00:17
@typeWolffo typeWolffo self-assigned this Jul 23, 2024
@typeWolffo typeWolffo requested a review from k1eu July 23, 2024 00:17
@@ -11,6 +11,12 @@ import { omit } from "lodash";
import hashPassword from "src/common/helpers/hashPassword";
import { truncateAllTables } from "test/helpers/test-helpers";

jest.mock("../../common/emails/emails.service", () => ({
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to create EmailTestingAdapter which would get all messages and save them to array/object and provides methods like getAllEmail/lastEmail etc. this way we can easily check in tests what emails were sent and assert them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

If we have EmailTestingAdapter, you don't need to mock it, inject it in the test application as an adapter, then use DI to work with it.

import { ConfigService } from "@nestjs/config";

@Injectable()
export class EmailConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Personally I would create another adapter like LocalAdapter which has the mailhog values hardcoded and inject config directly to the nodemailer, and then use .env with EMAIL_ADAPTER=mailhog, in prod environments smtp auth is not always the case, sometimes it is just service account. This way we can add/remove adapters without changing configuration or if we want to test something from the local we could easily use real one.

SMTP_PORT: Type.Number(),
SMTP_USER: Type.String(),
SMTP_PASSWORD: Type.String(),
USE_MAILHOG: Type.Boolean(),
Copy link
Member

Choose a reason for hiding this comment

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

Do we still use this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, in smtp adapter.

Copy link
Member Author

Choose a reason for hiding this comment

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

referring to previous comment

in prod environments smtp auth is not always the case, sometimes it is just service account

then we just need to create a new adapter?

Copy link
Member

Choose a reason for hiding this comment

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

We should require environments based on what we want to use, eg I am going with local one I don't need to setup aws.

Copy link
Member Author

@typeWolffo typeWolffo Aug 1, 2024

Choose a reason for hiding this comment

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

so it works like that - it checks what adapter we have specified in env EMAIL_ADAPTER and dynamically instantiate the adapter using moduleRef.create() in emails-adapter.factory. If the value in env for the specified adapter is missing - it returns an error

Copy link
Member Author

Choose a reason for hiding this comment

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

but I may not understand something 😅

@@ -0,0 +1,7 @@
export interface Email {
Copy link
Member

Choose a reason for hiding this comment

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

With this interface it is possible to pas an Email without both text and html. Can we make it that we need at least one?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@mikoscz mikoscz Aug 1, 2024

Choose a reason for hiding this comment

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

When I specify email with only text, the html property has string | undefined should be undefined same other way around. Could we go with something simpler?

type Email = {
  to: string;
  from: string;
  subject: string;
} & (
  | { html: string; text?: never }
  | { text: string; html?: never }
  | { text: string; html: string }
);

createAdapter(): EmailAdapter {
const adapterType = this.configService.get<AdapterType>("EMAIL_ADAPTER");

return match(adapterType)
Copy link
Member

Choose a reason for hiding this comment

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

What advantage do we get with ts-pattern?

Copy link
Member Author

Choose a reason for hiding this comment

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

It compares config from .env and returns the corresponding adapter. In addition, it handles errors on bad config

this.sentEmails.push(finalEmail);
}

setEmailOverride(override: Partial<Email>): void {
Copy link
Member

Choose a reason for hiding this comment

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

What is the practical use of this override?

Copy link
Member Author

Choose a reason for hiding this comment

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

For tests only - to overwrite react emails with custom content. When unused it sends email from template.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good idea. Adapter is only responsible for sending part so ok. there is an email I need to send it somehow. But in this case it is changing the Email that is not the part of the job. So whenever I set something I am basically testing the mock not the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikoscz ok so I removed that and simplified test to only check the "inbox" length

@@ -7,6 +7,7 @@
"lint": "turbo lint",
"format": "prettier --write \"**/*.{ts,tsx,md}\"",
"generate:client": "pnpm run --filter=web generate:client",
"generate:emails": "pnpm run --filter=email-templates build",
Copy link
Member

Choose a reason for hiding this comment

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

Do we need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@@ -0,0 +1,98 @@
import path from "path";
Copy link
Member

Choose a reason for hiding this comment

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

If this one works probably we don't need the exports in index

Copy link
Member Author

Choose a reason for hiding this comment

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

index is generated by this plugin

Copy link
Member

@mikoscz mikoscz left a comment

Choose a reason for hiding this comment

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

I didn't check at runtime, but do I need to specify all the environments for AWS if I want to use just the local adapter?

@typeWolffo
Copy link
Member Author

I didn't check at runtime, but do I need to specify all the environments for AWS if I want to use just the local adapter?

previously yes, but this has now been fixed

@typeWolffo typeWolffo requested a review from mikoscz July 31, 2024 08:49
@typeWolffo typeWolffo merged commit 2636bc1 into main Aug 1, 2024
@typeWolffo typeWolffo deleted the jw/emails branch August 1, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants