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

Implement Backend Functionality for Email Sending #89

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

shreya5653
Copy link
Contributor

#87

  • Modified existing codes for newsletter subscription and unsubscription:

    • Enhanced the subscription logic to check for existing subscribers and manage reactivation.
    • Implemented the unsubscription logic to handle user requests using a unique token.
  • Integrated JavaMailSender for sending subscription confirmation emails:

    • Set up email configuration to enable sending emails via Gmail.
    • Added functionality to send an email notification to users upon successful subscription.

…ion:

  - Enhanced the subscription logic to check for existing subscribers and manage reactivation.
  - Implemented the unsubscription logic to handle user requests using a unique token.
- Integrated JavaMailSender for sending subscription confirmation emails:
  - Set up email configuration to enable sending emails via Gmail.
  - Added functionality to send an email notification to users upon successful subscription.
@shreya5653
Copy link
Contributor Author

shreya5653 commented Oct 27, 2024

@ajaynegi45 @Guhapriya01 Despite the application not running on my local machine right now, I made the necessary changes related to the newsletter subscription and unsubscription functionality when my application was running properly. While testing, the application failed to start, but I believe the changes I implemented are correct. Therefore, I have raised the PR for review. Please let me know if any modifications are needed. I've tried to keep the code as less redundant as possible, and as you suggested, I configured the email functionality using Gmail. In the unsubscribe link, I used 'localhost:8080', which we will need to update to our actual domain name once it is finalized. If the changes are okay for you then please assign the PR to me add labels and merge it. 😊

@shreya5653
Copy link
Contributor Author

shreya5653 commented Oct 28, 2024

@ajaynegi45 @Guhapriya01 ,

I tested the subscribe and unsubscribe endpoints, and everything is functioning as expected. Upon a successful subscription, the subscriber receives a confirmation email with a link to unsubscribe. Clicking the link correctly redirects the user to the unsubscribe endpoint, displaying a confirmation message.

Additionally, if a user resubscribes to the newsletter, they receive a resubscription confirmation email, ensuring clear communication throughout the process. I’ve implemented the backend functionality to handle the sending of subscription and unsubscription emails effectively. To maintain clarity and avoid complications, I chose to keep the logic for subscription emails and newsletters separate.

For your reference, I’ve sent a sample subscription email to you. Please note that the unsubscribe link in the email won’t work since I am currently testing in a local environment.

Subscribed an email
Screenshot (125)

Storing it in the database
Screenshot (126)

Receiving a subscription confirmation email
Screenshot (127)

Redirected to Unsubscription endpoint
Screenshot (128)

Changes into database
Screenshot (129)

Unsubscription email
Screenshot (130)

Resubscribing the user
Screenshot (131)

Hope it will work for you!

Copy link
Collaborator

@Guhapriya01 Guhapriya01 left a comment

Choose a reason for hiding this comment

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

Hi @shreya5653,

Thanks for your work on the newsletter features! I have a few suggestions:

  1. HTTP Status Codes: The API should return appropriate status codes (e.g., 400 for bad requests) to indicate the outcome of operations.

  2. API URL Convention: For consistency with other endpoints, please change the URL from /newsletter to /api/newsletter.

  3. Redundant Email Service: The EmailService1 class overlaps with the existing EmailService. Consider using the existing one and making changes to the structure if needed.

  4. Unnecessary Configuration: The MailConfig class is redundant since email settings are already defined in application.properties.

Let me know if you have any questions regarding these.

@shreya5653
Copy link
Contributor Author

shreya5653 commented Oct 29, 2024

@Guhapriya01
Thank you for reviewing my code and for your valuable suggestions for changes. I apologize for any silly mistakes I made earlier. I have implemented the required changes and tested them. I would appreciate it if you could review them and let me know if everything looks okay.

@Guhapriya01
Copy link
Collaborator

@shreya5653,

Thanks for making the changes! They look great. I wanted to discuss the current structure of the EmailSender interface and EmailService. Since the interface currently only includes the send method for notification emails, it might not fully address our broader email-sending needs, like for newsletters.

To enhance the design, I suggest we keep the EmailSender interface as it is. Instead of a single EmailService, we could create two classes: NotificationEmailSender and NewsletterEmailSender, both implementing the EmailSender interface. This would help maintain clear separation of concerns and make the codebase more extensible.

Additionally, as @rishabhrawat05 suggested in issue #87, we could add two new notification types: SUBSCRIBED and UNSUBSCRIBED for the newsletter emails.

Looking forward to hearing your thoughts on this approach!

@shreya5653
Copy link
Contributor Author

shreya5653 commented Oct 29, 2024

@shreya5653,

Thanks for making the changes! They look great. I wanted to discuss the current structure of the EmailSender interface and EmailService. Since the interface currently only includes the send method for notification emails, it might not fully address our broader email-sending needs, like for newsletters.

To enhance the design, I suggest we keep the EmailSender interface as it is. Instead of a single EmailService, we could create two classes: NotificationEmailSender and NewsletterEmailSender, both implementing the EmailSender interface. This would help maintain clear separation of concerns and make the codebase more extensible.

Additionally, as @rishabhrawat05 suggested in issue #87, we could add two new notification types: SUBSCRIBED and UNSUBSCRIBED for the newsletter emails.

Looking forward to hearing your thoughts on this approach!

@Guhapriya01

Thanks for sharing your thoughts! I was planning to keep the logic for notification emails, newsletter emails, and subscription emails separate to avoid complexity and enhance understanding. Since I’m not entirely sure how we’ll send notifications yet, I found it more convenient to focus on implementing the core functionalities first before merging any logic.

I appreciate your suggestion about creating separate services, but I’m concerned that it might increase complexity and take additional time to implement all the necessary functionality for sending notifications and emails. So far, I’ve successfully added the backend functionality for sending subscription and unsubscription emails and handling the corresponding errors.

I think we can revisit this design later once we have a clearer picture. Let me know!

@Guhapriya01
Copy link
Collaborator

Hey @shreya5653,

Since your approach for the core functionality is solid, would you be open to making another PR later for those design changes we discussed and for adding authorization with @PreAuthorize to ensure users can only subscribe to newsletters for themselves? This way, I can go ahead and merge this one to keep things moving, and we can revisit the structure improvements separately when the timing is right. Let me know what you think!

@shreya5653
Copy link
Contributor Author

Hey @shreya5653,

Since your approach for the core functionality is solid, would you be open to making another PR later for those design changes we discussed and for adding authorization with @PreAuthorize to ensure users can only subscribe to newsletters for themselves? This way, I can go ahead and merge this one to keep things moving, and we can revisit the structure improvements separately when the timing is right. Let me know what you think!

Hello @Guhapriya01
I’d be happy to make another PR later for the design changes and adding authorization with @PreAuthorize to ensure users can only subscribe for themselves. That sounds like a good plan to keep things organized and manageable.

I really appreciate you moving ahead with merging this one. Looking forward to revisiting the improvements when the time is right!

Thanks again!

@Guhapriya01 Guhapriya01 merged commit 664b333 into ajaynegi45:main Oct 30, 2024
1 of 2 checks passed
@Guhapriya01 Guhapriya01 added enhancement New feature or request hacktoberfest-accepted hacktoberfest gssoc GirlScript Summer Of Code gssoc-ext level3 GirlScript Summer Of Code - 35 points labels Oct 30, 2024
@Guhapriya01
Copy link
Collaborator

Hello @shreya5653,

Thank you for your contribution! 😊 I’ve merged your PR, and I appreciate you being open to updating the design and adding the authorization later.

Great work!

@shreya5653
Copy link
Contributor Author

Hello @shreya5653,

Thank you for your contribution! 😊 I’ve merged your PR, and I appreciate you being open to updating the design and adding the authorization later.

Great work!

Thank you so much! @Guhapriya01 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gssoc GirlScript Summer Of Code gssoc-ext hacktoberfest hacktoberfest-accepted level3 GirlScript Summer Of Code - 35 points
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants