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

[PHEE-296A] Email api #79

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ ext {
dependencies {
implementation("org.springframework.boot:spring-boot-starter-web:2.5.5")
implementation("org.springframework.boot:spring-boot-starter-actuator:$springBootVersion")
implementation ("org.springframework.boot:spring-boot-starter-validation:$springBootVersion")
Copy link
Contributor

Choose a reason for hiding this comment

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

these validations may not produce error messages in our custom format. Please be careful with that

implementation 'org.springframework.boot:spring-boot-starter-mail'
testImplementation 'org.springframework.boot:spring-boot-starter-test'
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this dependency, I could not find any tests below

implementation("org.springframework.boot:spring-boot-starter-data-jpa:2.5.2")
implementation("org.flywaydb:flyway-core:6.4.0")
implementation('org.apache.velocity:velocity:1.7')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package org.fineract.messagegateway.sms.api;

import org.fineract.messagegateway.sms.data.EmailRequestDTO;
import org.fineract.messagegateway.sms.service.EmailService;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

import javax.validation.Valid;

@RestController
@RequestMapping("/emails")
public class EmailApiResource {

@Autowired
EmailService emailService;

@PostMapping
public ResponseEntity<String> sendEmail(
Copy link
Contributor

Choose a reason for hiding this comment

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

API documentation missing

@RequestHeader("Platform-Tenant-Id") String platformTenantId,
@RequestHeader(value = "X-CallbackUrl", required = false) String callbackUrl,
@RequestHeader(value = "X-Correlation-Id", required = false) String correlationId,
@Valid @RequestBody EmailRequestDTO emailRequest
) {
if (platformTenantId == null || platformTenantId.isEmpty()) {
return ResponseEntity.status(HttpStatus.NOT_FOUND).body("Platform-Tenant-Id header is missing");
Copy link
Contributor

Choose a reason for hiding this comment

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

Supported param validations(etc: email regex) and basic field validations are missing.
Supported header validations are missing.
Error message not in the right format.

}

emailService.sendEmail(emailRequest,callbackUrl);

return ResponseEntity.status(HttpStatus.ACCEPTED).body("Email accepted to be sent");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

send response in JSON format

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package org.fineract.messagegateway.sms.data;

import java.util.List;
import javax.validation.Valid;
import javax.validation.constraints.Email;
import javax.validation.constraints.NotEmpty;
public class EmailRequestDTO {
@NotEmpty
private List< String> to;

@NotEmpty
private String subject;

@NotEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

These annotation validations to be handled and sent in phee/fineract specific format.

private String body;

// Getters and setters

public List<String> getTo() {
Copy link
Contributor

Choose a reason for hiding this comment

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

use Lombok for getters and setters

return to;
}

public void setTo(List<String> to) {
this.to = to;
}

public String getSubject() {
return subject;
}

public void setSubject(String subject) {
this.subject = subject;
}

public String getBody() {
return body;
}

public void setBody(String body) {
this.body = body;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package org.fineract.messagegateway.sms.service;

import org.fineract.messagegateway.MessageGateway;
import org.fineract.messagegateway.sms.data.EmailRequestDTO;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.client.ClientHttpRequestFactory;
import org.springframework.http.client.SimpleClientHttpRequestFactory;
import org.springframework.mail.SimpleMailMessage;
import org.springframework.mail.javamail.JavaMailSender;
import org.springframework.mail.javamail.JavaMailSenderImpl;
import org.springframework.stereotype.Service;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.web.client.RestTemplate;

import java.util.Properties;


@Service
public class EmailService {

private static final Logger logger = LoggerFactory.getLogger(EmailService.class);
@Autowired
private JavaMailSender mailSender;

@Value("${spring.mail.host}")
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have to specify 'spring' here? looks like an overhead. we are not using this anywhere else.

private String smtpHost;

@Value("${spring.mail.port}")
private int smtpPort;

@Value("${spring.mail.username}")
private String smtpUsername;

@Value("${spring.mail.password}")
private String smtpPassword;




public void sendEmail(EmailRequestDTO emailRequest, String callbackUrl) {
boolean flag = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

name this flag properly to meet its usecase

String error = null;
try{
((JavaMailSenderImpl) mailSender).setHost(smtpHost);
Copy link
Contributor

Choose a reason for hiding this comment

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

Severe issue:
The values and the mail sender are global( available at startup). Setting those everytime when the API is called is a bad design. This should be created once and reused. My suggestion is to use mailSender bean with these properties use here.

((JavaMailSenderImpl) mailSender).setPort(smtpPort);
((JavaMailSenderImpl) mailSender).setUsername(smtpUsername);
((JavaMailSenderImpl) mailSender).setPassword(smtpPassword);
Properties mailProperties = ((JavaMailSenderImpl) mailSender).getJavaMailProperties();
mailProperties.put("mail.smtp.connectiontimeout", 5000);
Copy link
Contributor

Choose a reason for hiding this comment

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

these timeouts should be configurable.

mailProperties.put("mail.smtp.timeout", 5000);
mailProperties.put("mail.smtp.writetimeout", 5000);
((JavaMailSenderImpl) mailSender).setJavaMailProperties(mailProperties);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above ( severe issue)


SimpleMailMessage message = new SimpleMailMessage();
message.setTo(emailRequest.getTo().toArray(new String[0]));
message.setSubject(emailRequest.getSubject());
message.setText(emailRequest.getBody());
mailSender.send(message);
flag = true;

} catch (Exception e) {
error = e.getMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. If possible handle specific exception
  2. add error level logger for exception

}
RestTemplate restTemplate = new RestTemplate();
Copy link
Contributor

Choose a reason for hiding this comment

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

consider restTemplate as a costly resource and to be created with specific settings. create it as bean and Autowire. My suggestion is to re-use the call back mechanism specified in connector common.


if(flag)
{
String requestBody = "Email sent successfully to {to}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this has to be written here? can this be handled inside the try block?

Copy link
Contributor

Choose a reason for hiding this comment

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

the request body doesn't seem to be right.
you should be sending a Json body rather than a string.
Send in a proper format with status.

restTemplate.postForObject(callbackUrl, requestBody.replace("{to}",
Copy link
Contributor

Choose a reason for hiding this comment

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

use string formatting to replace placeholders.

emailRequest.getTo().toString()), String.class);
logger.info("Email sent to: " + emailRequest.getTo());
Copy link
Contributor

Choose a reason for hiding this comment

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

donot use + to concatenate log statements use {}

}
else {
String requestBody = "Email could not be sent to {to} because of {error} ";
requestBody = requestBody.replace("{error}",error);
Copy link
Contributor

Choose a reason for hiding this comment

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

use string formatter to replace place holders

restTemplate.postForObject(callbackUrl, requestBody.replace("{to}",
Copy link
Contributor

Choose a reason for hiding this comment

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

please create the response in valid JSON format rather than String.

emailRequest.getTo().toString()), String.class);
logger.info("Email failed to be sent because {}", error);

}



}
private ClientHttpRequestFactory createTimeoutRequestFactory() {
Copy link
Contributor

Choose a reason for hiding this comment

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

could not find a usage for this method.
Again, the rest template should be create as a bean with these settings so that we don't need t create it on the fly every time.

As rest template is going to be deprecated, better stick with webclient / retroft. There is already one implementation added to connector common

SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory();
factory.setConnectTimeout(5000);
factory.setReadTimeout(5000);
return factory;
}


}
15 changes: 14 additions & 1 deletion src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ spring:
password: ${MYSQL_PASSWORD:password}
driver-class-name: org.drizzle.jdbc.DrizzleDriver

mail:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you attach a demo video with Gmail settings to show that the email is sending successfully with Gmail?

host: ${EMAIL_URL:localhost}
port: 3025
username: greenmail
password: greenmail
properties:
mail:
smtp:
auth: false
starttls:
enable: false

# Status Callback configuration for Twilio. Port will be taken from server configuration
hostconfig:
host: localhost
Expand Down Expand Up @@ -91,4 +103,5 @@ zeebe:
contactpoint: "zeebe-zeebe-gateway:26500"
worker:
timer: "PT15S"
retries: 3
retries: 3