MX-236: Implement i18N for the messages sent by SMS or Email - #148
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR introduces a new configurable email service implementation and migrates the self-service registration and password-reset flows from Gmail-backed delivery to this new service. Changes include adding Spring Boot mail support, implementing a primary email service with SMTP configuration, updating service wiring and dependency injection, and modifying the authorization email template. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.java (2)
315-316:⚠️ Potential issue | 🟠 MajorHardcoded Spanish text not internationalized.
The SMS message (lines 315-316) and email body (lines 329-331) contain hardcoded Spanish text ("Hola", "Código de Autorización"). Given the PR objective is to implement i18N for messages, these should use message resources similar to
SelfServiceRegistrationWritePlatformServiceImpl.Also applies to: 329-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.java` around lines 315 - 316, Replace the hardcoded Spanish strings in SelfServiceForgotPasswordWritePlatformServiceImpl (the SMS `message` and the email body around the variables used at lines 329-331) with localized message lookups using the same i18n resource pattern used in SelfServiceRegistrationWritePlatformServiceImpl (e.g., obtain the MessageSource/translation utility used there and fetch messages by key, inserting parameters like firstName and externalAuthorizationToken); update the keys (e.g., "selfservice.forgotpassword.greeting" and "selfservice.forgotpassword.authorizationCode") in the resource bundle and use those keys when constructing the SMS `message` and the email body string.
328-336:⚠️ Potential issue | 🟠 MajorPlain text body sent as HTML email.
The
sendAuthorizationMailmethod constructs a plain text body (lines 330-331) but callssendFormattedEmail, which renders the body as HTML (setText(body, true)). This will cause newline characters (\n) to be ignored in email clients, resulting in poorly formatted output.Either:
- Use
sendDefinedEmailfor plain text, or- Convert the body to HTML format with
<br/>tags, or- Use an HTML template as done in
SelfServiceRegistrationWritePlatformServiceImpl.🐛 Proposed fix using sendDefinedEmail for plain text
private void sendAuthorizationMail(SelfServiceRegistration selfServiceRegistration) { final String subject = "Código de Autorización "; final String body = "Hola " + selfServiceRegistration.getFirstName() + "," + "\nCódigo de Autorización : " + selfServiceRegistration.getExternalAuthorizationToken(); final EmailDetail emailDetail = new EmailDetail(subject, body, selfServiceRegistration.getEmail(), selfServiceRegistration.getFirstName()); - this.selfServicePluginEmailService.sendFormattedEmail(emailDetail); + this.selfServicePluginEmailService.sendDefinedEmail(emailDetail); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.java` around lines 328 - 336, sendAuthorizationMail currently builds a plain-text body but calls selfServicePluginEmailService.sendFormattedEmail (which treats it as HTML), causing newlines to be lost; update sendAuthorizationMail to send a plain-text email by calling selfServicePluginEmailService.sendDefinedEmail (or otherwise convert the body to HTML with <br/> if you prefer HTML output), keeping the same subject, recipient and firstName fields and using selfServiceRegistration.getExternalAuthorizationToken() for the content so formatting matches intent.
🧹 Nitpick comments (2)
src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java (2)
61-101: Extract common SMTP configuration to reduce duplication.Both
sendFormattedEmailandsendDefinedEmailcontain identical JavaMailSender configuration logic (lines 62-87 and 105-130). Extract this into a private helper method.♻️ Proposed refactor
+ private JavaMailSenderImpl createMailSender() { + final SMTPCredentialsData smtpCredentialsData = this.externalServicesReadPlatformService.getSMTPCredentials(); + final JavaMailSenderImpl mailSender = new JavaMailSenderImpl(); + mailSender.setHost(smtpCredentialsData.getHost()); + mailSender.setPort(Integer.parseInt(smtpCredentialsData.getPort())); + mailSender.setUsername(smtpCredentialsData.getUsername()); + mailSender.setPassword(smtpCredentialsData.getPassword()); + + Properties props = mailSender.getJavaMailProperties(); + props.put("mail.transport.protocol", "smtp"); + props.put("mail.smtp.auth", "true"); + props.put("mail.smtp.starttls.enable", "true"); + props.put("mail.smtp.socketFactory.port", Integer.parseInt(smtpCredentialsData.getPort())); + props.put("mail.smtp.socketFactory.class", "javax.net.ssl.SSLSocketFactory"); + props.put("mail.smtp.socketFactory.fallback", "true"); + return mailSender; + } + + private String getFromEmail() { + return this.externalServicesReadPlatformService.getSMTPCredentials().getFromEmail(); + }Also applies to: 103-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java` around lines 61 - 101, Both sendFormattedEmail and sendDefinedEmail duplicate the JavaMailSenderImpl configuration; extract that logic into a private helper such as private JavaMailSenderImpl createMailSender(SMTPCredentialsData smtpCredentialsData) that sets host, port, username, password and the JavaMail properties (mail.transport.protocol, mail.smtp.auth, mail.debug, mail.smtp.starttls.enable, socketFactory.* etc.) and returns the configured JavaMailSenderImpl; replace the duplicated blocks in sendFormattedEmail and sendDefinedEmail with calls to createMailSender(this.externalServicesReadPlatformService.getSMTPCredentials()) and use the returned mailSender as before (keep exception handling and message setup unchanged).
46-59: Missing Javadoc on public methods.Public methods
sendToUserAccount,sendFormattedEmail, andsendDefinedEmaillack Javadoc documentation. As per coding guidelines, public methods and classes must have Javadoc.Also applies to: 61-101, 103-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java` around lines 46 - 59, Add Javadoc comments for the public methods sendToUserAccount, sendFormattedEmail, and sendDefinedEmail and for the class itself: document each method’s purpose, list all parameters with `@param` (organisationName, contactName, address, username, unencodedPassword for sendToUserAccount, and the EmailDetail parameter for sendDefinedEmail), describe return behavior (void) and any exceptions thrown with `@throws` if applicable, and include a short class-level Javadoc describing the service responsibility; ensure the Javadoc is concise and follows project style conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`:
- Line 79: The mail.debug property is hardcoded to "true" in
SelfServicePluginEmailService (props.put("mail.debug", "true")) which exposes
verbose SMTP logs; change this to be configurable or disabled by default by
reading a configuration flag (e.g., from environment, application properties, or
an injected Config/Properties bean) and only set props.put("mail.debug", value)
when the configured value is true, otherwise omit the property or set it to
"false"; update all occurrences (including the second instance) to use the same
config-backed approach so production does not emit debug SMTP logs.
- Line 69: The code calls
mailSender.setPort(Integer.parseInt(smtpCredentialsData.getPort())) without
validating the port string; wrap parsing in validation/handling (e.g., check
smtpCredentialsData.getPort() for null/empty, then parse inside a try/catch for
NumberFormatException) and either use a sensible default (587) or rethrow a
clear IllegalArgumentException if invalid, then pass the parsed int to
mailSender.setPort; apply the same fix wherever smtpCredentialsData.getPort() is
parsed (the other occurrences around the setPort calls).
In
`@src/main/java/org/apache/fineract/selfservice/useradministration/domain/SelfServiceUserDomainServiceImpl.java`:
- Around line 37-39: The SelfServiceUserDomainServiceImpl constructor currently
requires an unconditional `@Qualifier`("selfServicePluginEmailService")
PlatformEmailService which will fail when
mifos.self.service.plugin.email.enabled is false; modify the constructor to
accept Optional<PlatformEmailService> (with the same qualifier) or mark the
whole class with
`@ConditionalOnProperty`(name="mifos.self.service.plugin.email.enabled",
havingValue="true") so the bean is only loaded when the plugin email bean
exists; update internal field(s) that reference the email service in
SelfServiceUserDomainServiceImpl to handle Optional.empty() (or assume presence
when using the conditional) and keep existing constructor parameters
AppSelfServiceUserRepository and PlatformPasswordEncoder unchanged.
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.java`:
- Around line 315-316: Replace the hardcoded Spanish strings in
SelfServiceForgotPasswordWritePlatformServiceImpl (the SMS `message` and the
email body around the variables used at lines 329-331) with localized message
lookups using the same i18n resource pattern used in
SelfServiceRegistrationWritePlatformServiceImpl (e.g., obtain the
MessageSource/translation utility used there and fetch messages by key,
inserting parameters like firstName and externalAuthorizationToken); update the
keys (e.g., "selfservice.forgotpassword.greeting" and
"selfservice.forgotpassword.authorizationCode") in the resource bundle and use
those keys when constructing the SMS `message` and the email body string.
- Around line 328-336: sendAuthorizationMail currently builds a plain-text body
but calls selfServicePluginEmailService.sendFormattedEmail (which treats it as
HTML), causing newlines to be lost; update sendAuthorizationMail to send a
plain-text email by calling selfServicePluginEmailService.sendDefinedEmail (or
otherwise convert the body to HTML with <br/> if you prefer HTML output),
keeping the same subject, recipient and firstName fields and using
selfServiceRegistration.getExternalAuthorizationToken() for the content so
formatting matches intent.
---
Nitpick comments:
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`:
- Around line 61-101: Both sendFormattedEmail and sendDefinedEmail duplicate the
JavaMailSenderImpl configuration; extract that logic into a private helper such
as private JavaMailSenderImpl createMailSender(SMTPCredentialsData
smtpCredentialsData) that sets host, port, username, password and the JavaMail
properties (mail.transport.protocol, mail.smtp.auth, mail.debug,
mail.smtp.starttls.enable, socketFactory.* etc.) and returns the configured
JavaMailSenderImpl; replace the duplicated blocks in sendFormattedEmail and
sendDefinedEmail with calls to
createMailSender(this.externalServicesReadPlatformService.getSMTPCredentials())
and use the returned mailSender as before (keep exception handling and message
setup unchanged).
- Around line 46-59: Add Javadoc comments for the public methods
sendToUserAccount, sendFormattedEmail, and sendDefinedEmail and for the class
itself: document each method’s purpose, list all parameters with `@param`
(organisationName, contactName, address, username, unencodedPassword for
sendToUserAccount, and the EmailDetail parameter for sendDefinedEmail), describe
return behavior (void) and any exceptions thrown with `@throws` if applicable, and
include a short class-level Javadoc describing the service responsibility;
ensure the Javadoc is concise and follows project style conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be8d6d07-cc41-4cbc-bb34-e0eae2e36fd4
📒 Files selected for processing (9)
pom.xmlsrc/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.javasrc/main/java/org/apache/fineract/selfservice/useradministration/domain/SelfServiceUserDomainServiceImpl.javasrc/main/resources/mail-templates/authorization-email.htmlsrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceForgotPasswordWritePlatformServiceImplTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java
| Properties props = mailSender.getJavaMailProperties(); | ||
| props.put("mail.transport.protocol", "smtp"); | ||
| props.put("mail.smtp.auth", "true"); | ||
| props.put("mail.debug", "true"); |
There was a problem hiding this comment.
Remove or externalize mail.debug=true setting.
Hardcoded mail.debug=true will produce verbose SMTP protocol logs in production, potentially exposing sensitive information and impacting performance. This should be configurable or disabled by default.
🔧 Proposed fix
- props.put("mail.debug", "true");
+ // Enable only for troubleshooting; consider externalizing this settingAlso applies to: 122-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/apache/fineract/infrastructure/core/service/SelfServicePluginEmailService.java`
at line 79, The mail.debug property is hardcoded to "true" in
SelfServicePluginEmailService (props.put("mail.debug", "true")) which exposes
verbose SMTP logs; change this to be configurable or disabled by default by
reading a configuration flag (e.g., from environment, application properties, or
an injected Config/Properties bean) and only set props.put("mail.debug", value)
when the configured value is true, otherwise omit the property or set it to
"false"; update all occurrences (including the second instance) to use the same
config-backed approach so production does not emit debug SMTP logs.
Comment on lines
+37
to
+39
| public SelfServiceUserDomainServiceImpl(final AppSelfServiceUserRepository userRepository, | ||
| final PlatformPasswordEncoder applicationPasswordEncoder, | ||
| @Qualifier("selfServicePluginEmailService")final PlatformEmailService emailService) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other PlatformEmailService implementations that might serve as fallback
ast-grep --pattern 'class $_ implements PlatformEmailService'Repository: openMF/selfservice-plugin
Length of output: 51
🏁 Script executed:
# Find SelfServicePluginEmailService and check its `@ConditionalOnProperty` annotation
fd -t f "SelfServicePluginEmailService.java" -exec cat -n {} \;Repository: openMF/selfservice-plugin
Length of output: 299
🏁 Script executed:
# Search for `@ConditionalOnProperty` patterns in the codebase
rg "@ConditionalOnProperty" -A 3Repository: openMF/selfservice-plugin
Length of output: 1946
🏁 Script executed:
# Search for all PlatformEmailService references
rg "PlatformEmailService" -B 2 -A 2Repository: openMF/selfservice-plugin
Length of output: 3300
🏁 Script executed:
# Check if SelfServiceUserDomainServiceImpl has any conditional configuration
fd -t f "SelfServiceUserDomainServiceImpl.java" -exec cat -n {} \;Repository: openMF/selfservice-plugin
Length of output: 299
🏁 Script executed:
# Search for other PlatformEmailService implementations (not using ast-grep which failed)
rg "implements PlatformEmailService" -B 5Repository: openMF/selfservice-plugin
Length of output: 888
🏁 Script executed:
# Check if SelfServiceUserDomainServiceImpl is conditionally loaded
rg "class SelfServiceUserDomainServiceImpl" -B 10 | head -30Repository: openMF/selfservice-plugin
Length of output: 1777
🏁 Script executed:
# Check if Optional<PlatformEmailService> pattern is used anywhere
rg "Optional.*PlatformEmailService"Repository: openMF/selfservice-plugin
Length of output: 51
🏁 Script executed:
# Verify if there's a default PlatformEmailService bean defined in config
rg "PlatformEmailService" --type java | grep -E "(Bean|@Component|@Service)" | head -20Repository: openMF/selfservice-plugin
Length of output: 51
SelfServiceUserDomainServiceImpl requires the conditional SelfServicePluginEmailService bean, which will cause startup failure if mifos.self.service.plugin.email.enabled is false or not set.
The class is unconditionally loaded as a @Service, but its constructor dependency on @Qualifier("selfServicePluginEmailService") PlatformEmailService requires a bean that only exists when the property is set to true. When the property is false or absent, Spring will fail to resolve the dependency and application startup will fail.
Either:
- Make
SelfServiceUserDomainServiceImplconditionally loaded to match its dependency, or - Use
Optional<PlatformEmailService>to handle the missing bean gracefully, or - Ensure the property is always set to
truein all active profiles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/apache/fineract/selfservice/useradministration/domain/SelfServiceUserDomainServiceImpl.java`
around lines 37 - 39, The SelfServiceUserDomainServiceImpl constructor currently
requires an unconditional `@Qualifier`("selfServicePluginEmailService")
PlatformEmailService which will fail when
mifos.self.service.plugin.email.enabled is false; modify the constructor to
accept Optional<PlatformEmailService> (with the same qualifier) or mark the
whole class with
`@ConditionalOnProperty`(name="mifos.self.service.plugin.email.enabled",
havingValue="true") so the bean is only loaded when the plugin email bean
exists; update internal field(s) that reference the email service in
SelfServiceUserDomainServiceImpl to handle Optional.empty() (or assume presence
when using the conditional) and keep existing constructor parameters
AppSelfServiceUserRepository and PlatformPasswordEncoder unchanged.
This was referenced Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores