MX-236: Implement i18N for the messages sent by SMS or Email#134
Open
ansh-varshney wants to merge 3 commits intoopenMF:developfrom
Open
MX-236: Implement i18N for the messages sent by SMS or Email#134ansh-varshney wants to merge 3 commits intoopenMF:developfrom
ansh-varshney wants to merge 3 commits intoopenMF:developfrom
Conversation
…ssages Replace hardcoded Spanish strings in sendAuthorizationMail() and sendAuthorizationMessage() with Thymeleaf HTML templates and ResourceBundleMessageSource.
📝 WalkthroughWalkthroughAdds Thymeleaf and i18n support for registration notifications: new Thymeleaf Maven dependency, MessageSource and SpringTemplateEngine beans, service updated to use localized SMS/email content, English/Spanish resource bundles, and a Thymeleaf HTML email template. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RegistrationService as RegistrationService
participant MessageSource as MessageSource
participant TemplateEngine as TemplateEngine
participant SmsService as SmsService
participant EmailService as EmailService
Client->>RegistrationService: submit registration request
RegistrationService->>MessageSource: resolve SMS template (key, args, locale)
MessageSource-->>RegistrationService: localized SMS text
RegistrationService->>SmsService: send SMS(phone, text)
RegistrationService->>MessageSource: resolve email subject (key, locale)
MessageSource-->>RegistrationService: localized subject
RegistrationService->>TemplateEngine: render "authorization-email" (firstName, requestId, authToken, locale)
TemplateEngine-->>RegistrationService: rendered HTML
RegistrationService->>EmailService: send email(to, subject, html)
EmailService-->>Client: delivery ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
ansh-varshney
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/selfservice/registration/starter/SelfRegistrationConfiguration.java`:
- Around line 52-73: Add Javadoc comments for the two new public bean factory
methods: registrationMessageSource() and registrationTemplateEngine(). For
registrationMessageSource(), add a brief description that it creates and
configures a ResourceBundleMessageSource for i18n (basename "i18n/messages",
UTF-8, no system locale fallback) and document the return type (MessageSource).
For registrationTemplateEngine(), add a brief description that it creates and
configures a SpringTemplateEngine with a ClassLoaderTemplateResolver for mail
HTML templates (prefix "mail-templates/", suffix ".html", HTML mode, UTF-8), and
document the return type (SpringTemplateEngine). Keep descriptions short and
follow existing Javadoc style used in the codebase.
🪄 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: c6d1a5b4-b884-4c53-b59f-02025a33e102
📒 Files selected for processing (6)
pom.xmlsrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.javasrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_es.propertiessrc/main/resources/mail-templates/authorization-email.html
...java/org/apache/fineract/selfservice/registration/starter/SelfRegistrationConfiguration.java
Show resolved
Hide resolved
…istrationTemplateEngine beans
Contributor
|
@AnshVarshney there is an issue, check the CI errors |
Contributor
|
@AnshVarshney https://github.com/openMF/selfservice-plugin/blob/develop/TESTING.md take a look, this will help you out to run tests locally so that you can confidently raise the prs! |
Author
|
Thanks @DeathGun44, will surely look at it! |
…and messageSource mocks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java (1)
67-89: Add behavioral tests for i18n/template usage, not only constructor wiring.The new mocks are injected, but no test verifies
registrationMessageSource.getMessage(...)orregistrationTemplateEngine.process(...). This leaves the MX-236 behavior unguarded.Suggested test extension
import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ +import org.thymeleaf.context.Context; @@ `@Test` + void createRegistrationRequest_emailMode_usesLocalizedSubjectAndTemplate() { + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.accountNumberParamName), any())).thenReturn("12345"); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.firstNameParamName), any())).thenReturn("John"); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.middleNameParamName), any())).thenReturn(null); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.lastNameParamName), any())).thenReturn("Doe"); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.usernameParamName), any())).thenReturn("jdoe"); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.passwordParamName), any())).thenReturn("Password123!"); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.authenticationModeParamName), any())).thenReturn("email"); + when(fromApiJsonHelper.extractStringNamed(eq(SelfServiceApiConstants.emailParamName), any())).thenReturn("john@test.com"); + + PasswordValidationPolicy policy = mock(PasswordValidationPolicy.class); + when(policy.getRegex()).thenReturn(".*"); + when(passwordValidationPolicyRepository.findActivePasswordValidationPolicy()).thenReturn(policy); + when(appUserReadPlatformService.isUsernameExist("jdoe")).thenReturn(false); + when(selfServiceRegistrationReadPlatformService.isClientExist(anyString(), anyString(), any(), anyString(), any(), anyBoolean())) + .thenReturn(true); + when(clientRepository.getClientByAccountNumber("12345")).thenReturn(mock(Client.class)); + + when(registrationMessageSource.getMessage(eq("email.subject"), eq(null), any())).thenReturn("Authorization"); + when(registrationTemplateEngine.process(eq("authorization-email"), any(Context.class))).thenReturn("<html/>"); + + service.createRegistrationRequest("{}"); + + verify(registrationMessageSource).getMessage(eq("email.subject"), eq(null), any()); + verify(registrationTemplateEngine).process(eq("authorization-email"), any(Context.class)); + } + + `@Test` void createRegistrationRequest_persistsRegistration() {As per coding guidelines, "Verify both happy path and edge cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java` around lines 67 - 89, Add behavioral tests in SelfServiceRegistrationWritePlatformServiceImplTest that stub and verify internationalization and template usage: mock registrationMessageSource.getMessage(...) to return a known localized string and assert the service (use the service instance created in setUp) consumes it, and mock registrationTemplateEngine.process(...) to return rendered HTML and verify the service calls it with expected template name/variables. Include both a happy-path test (valid inputs leading to message/template invocation and expected result) and an edge-case test (missing message/template or null return values) asserting graceful handling or exceptions. Use Mockito when(...).thenReturn(...) plus verify(registrationMessageSource).getMessage(...) and verify(registrationTemplateEngine).process(...) to ensure coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java`:
- Around line 67-89: Add behavioral tests in
SelfServiceRegistrationWritePlatformServiceImplTest that stub and verify
internationalization and template usage: mock
registrationMessageSource.getMessage(...) to return a known localized string and
assert the service (use the service instance created in setUp) consumes it, and
mock registrationTemplateEngine.process(...) to return rendered HTML and verify
the service calls it with expected template name/variables. Include both a
happy-path test (valid inputs leading to message/template invocation and
expected result) and an edge-case test (missing message/template or null return
values) asserting graceful handling or exceptions. Use Mockito
when(...).thenReturn(...) plus verify(registrationMessageSource).getMessage(...)
and verify(registrationTemplateEngine).process(...) to ensure coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea5d5a0f-2bb2-4b75-8bc0-c52b95c8afe0
📒 Files selected for processing (1)
src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImplTest.java
Author
|
It appears that the local compilation failures are pre-existing on develop and caused by missing fineract-*:1.15.0-SNAPSHOT JARs in the local Maven cache. I have checked all 3 test files covering the changed classes in this pr and it appeared that only CI failure introduced by this pr was constructor mismatch in SelfServiceRegistrationWritePlatformServiceImplTest. I have fixed this now. |
Contributor
|
@ansh-varshney help me to resolve the conflicts too |
Author
Yes, I am on it. I'll provide a brief explanation about what should de done acc to me for conflict resoltuion. |
Author
|
@IOhacker, |
Author
|
@IOhacker, |
Author
|
@IOhacker, same for the third file also - same pattern only. lets keep both. |
Contributor
|
Hi Ansh, could you please rebase? |
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
3 participants
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.
Fixes: MX-236
Replace hardcoded Spanish strings in sendAuthorizationMail() and sendAuthorizationMessage() with Thymeleaf HTML templates and ResourceBundleMessageSource.
Summary by CodeRabbit