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

Text message plugin #3024

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

asonnleitner
Copy link
Contributor

Description

Please include a summary of the changes and the related issue.

Breaking changes

Does this PR include any breaking changes we should be aware of?

Screenshots

You can add screenshots here if applicable.

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

… plugin

- Added configuration and dependencies for the new text message plugin.
- Implemented core functionalities including sending emails using nodemailer, handlebars, and mjml.
- Added default email handlers for order confirmation, password reset, email verification, and email address change.
- Included templates and partials for email generation.
- Added dev mailbox for testing email outputs in development mode.
- Added comprehensive test suite to ensure functionality.
…n and update related files

- Renamed EmailProcessor to TextMessageProcessor in plugin.spec.ts.
- Replaced EmailSender with TextMessageSender across the plugin.
- Updated dev-mailbox.html to dev-text-message-inbox.html.
- Changed all references to EmailEventHandler to TextMessageEventHandler.
- Modified template loader to reference text message templates.
- Updated constants and configuration to use TEXT_MESSAGE_PLUGIN.
- Adjusted generator and sender implementations for text messages.
…geEventListener

- Updated all instances of `EmailEventListener` to `TextMessageEventListener` to better reflect the functionality of the text message plugin.
- Adjusted related variable and function names to maintain consistency.
- Updated comments and documentation to reflect the changes in terminology from email to text message.

This refactor improves clarity and maintains consistency within the text message plugin codebase.
Copy link

vercel bot commented Aug 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Aug 20, 2024 10:48am

Copy link
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Comment on lines +22 to +24
server.get('/', (req, res) => {
res.sendFile(path.join(__dirname, '../../dev-text-message-inbox.html'));
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.
Comment on lines +25 to +29
server.get('/list', async (req, res) => {
const list = await fs.readdir(outputPath);
const contents = await this.getTextMessageList(outputPath);
res.send(contents);
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
, but is not rate-limited.
@michaelbromley
Copy link
Member

Hi!

Would it make sense to make text messaging a capability of the email plugin? I know the naming will then be wrong, but purely in terms of the technical implementation, will there be a large overlap there between the two?

@asonnleitner
Copy link
Contributor Author

@michaelbromley Will look into it, also looks like the way to go 😄

@dlhck
Copy link
Collaborator

dlhck commented Sep 14, 2024

@michaelbromley @asonnleitner if we put text messaging capabilities into the email plugin we should think about renaming it to notification plugin. Notifications are more generic, they could be transferred as emails, text messages or even SMS and WhatsApp. What do you think?

@michaelbromley
Copy link
Member

@dlhck yes it would definitely need renaming in that case. Which is fine long-term. We'd need to think about how to minimize the breaking change for existing users of the package.

@dlhck
Copy link
Collaborator

dlhck commented Sep 16, 2024

@michaelbromley we could think about renaming the current email plugin to LegacyEmailPlugin, so that APIs stay the same. We cloud start then with the NotificationsPlugin without thinking about backwards-compatibility.

@michaelbromley
Copy link
Member

@dlhck yes that's a possibility. We could take that opportunity to fix a bunch of other design issues with the current plugin - e.g. a better API for extending handlers, better i18n support in templates, etc. if we go with a clean break in a brand new package.

@monrostar
Copy link
Contributor

@dlhck yes that's a possibility. We could take that opportunity to fix a bunch of other design issues with the current plugin - e.g. a better API for extending handlers, better i18n support in templates, etc. if we go with a clean break in a brand new package.

That's a great idea. We've already had to write a few extensions to send messages to Telegram or do scheduled Emails and Sms

@dlhck
Copy link
Collaborator

dlhck commented Sep 16, 2024

@monrostar @michaelbromley I would also vote for an integration to Novu, an open-source notification system.

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.

4 participants