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: Add email template for sending OTP to the user #582

Merged

Conversation

Allan2000-Git
Copy link
Contributor

@Allan2000-Git Allan2000-Git commented Dec 7, 2024

User description

Description

This PR adds an email template for sending OTP to the user.

Fixes #51

Dependencies

No new dependencies introduced.

Future Improvements

N/A

Mentions

@rajdip-b

Screenshots of relevant screens

otp-email-template

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Introduced a new React component OTPEmailTemplate for sending OTP emails with a vibrant design.
  • Updated styles in common-styles.ts to support the new OTP email template.
  • Integrated the new OTP email template into the mail service, replacing the old HTML string with a component-based approach.

Changes walkthrough 📝

Relevant files
Enhancement
otp-email-template.tsx
Add OTP Email Template Component                                                 

apps/api/src/mail/emails/otp-email-template.tsx

  • Added a new React component for OTP email template.
  • Utilized BaseEmailTemplate for consistent styling.
  • Included OTP display and expiration message.
  • +34/-0   
    common-styles.ts
    Update Styles for OTP Email Template                                         

    apps/api/src/mail/emails/styles/common-styles.ts

  • Added otpStyle for styling OTP text.
  • Modified workspaceDetails style for better appearance.
  • +10/-2   
    mail.service.ts
    Integrate OTP Email Template in Mail Service                         

    apps/api/src/mail/services/mail.service.ts

  • Integrated new OTP email template in mail service.
  • Replaced old HTML string with React component rendering.
  • +9/-19   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    51 - Partially compliant

    Compliant requirements:

    • Created and implemented a new vibrant OTP email template with improved styling

    Non-compliant requirements:

    • No explicit confirmation that the implemented template matches the Figma design
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Email Content Exposure:
    The OTP is being sent in plaintext in the email body. While this is common practice for OTP delivery, consider adding a warning message about not sharing the OTP with others and implementing rate limiting for OTP requests to prevent brute force attacks.

    ⚡ Recommended focus areas for review

    Code Duplication
    The OTP sending logic is duplicated between sendOtp() and sendEmailChangedOtp(). Consider refactoring to reuse the OTPEmailTemplate component.

    Missing Validation
    The OTP prop is directly rendered without any validation or sanitization. Consider adding input validation.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate input parameters to prevent rendering invalid data to users

    Add input validation to ensure the OTP prop is not empty or invalid before
    rendering, as displaying an invalid OTP could cause user confusion.

    apps/api/src/mail/emails/otp-email-template.tsx [10-12]

     export const OTPEmailTemplate = ({ otp }: OTPEmailTemplateProps) => {
    +  if (!otp?.trim()) {
    +    throw new Error('OTP cannot be empty');
    +  }
       return (
         <BaseEmailTemplate
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Input validation for the OTP is important for preventing runtime errors and ensuring a better user experience, as an empty or invalid OTP could cause confusion or application errors.

    7
    General
    Extract string literals into constants to improve maintainability and reduce duplication

    Move the subject string to a constant at the class/module level to maintain
    consistency and avoid duplication, since it's used in both the email template and
    service.

    apps/api/src/mail/services/mail.service.ts [54]

    -const subject = 'Your One Time Password (OTP) for Keyshade'
    +private static readonly OTP_EMAIL_SUBJECT = 'Your One Time Password (OTP) for Keyshade';
    +// ...
    +const subject = MailService.OTP_EMAIL_SUBJECT;
    Suggestion importance[1-10]: 5

    Why: Moving the duplicated subject string to a constant would improve maintainability and reduce the risk of inconsistency, though the impact is moderate since it's only used in two places.

    5

    💡 Need additional feedback ? start a PR chat

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    Can you drop a preview of how this looks?

    @Allan2000-Git
    Copy link
    Contributor Author

    Can you drop a preview of how this looks?

    Have already attached in the PR description
    otp-email-template

    @Allan2000-Git
    Copy link
    Contributor Author

    @rajdip-b Can you review the changes made?

    @rajdip-b rajdip-b merged commit cb6bbcb into keyshade-xyz:develop Dec 8, 2024
    4 checks passed
    @Allan2000-Git Allan2000-Git deleted the feat/send-otp-email-template branch December 8, 2024 13:30
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Create a mail template for OTP
    2 participants