Release 3.8.2 to main - #438
Open
snehar-nd wants to merge 45 commits into
Open
Conversation
add bengali language translation on dynamic forms
* fix: aam-2313 phone number leading with zero - removed zero * fix: allow concurrent sessions for admin, superadmin, and supervisor roles Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: added admin and superadmin for the condition --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ession logout redisTemplate has Jackson2JsonRedisSerializer<User> as value serializer, so reading the plain-string jti: value caused a deserialization failure (statusCode 5000). jti: keys are written via stringRedisTemplate at login, so reads and deletes must also use stringRedisTemplate — restoring the behaviour from commit 80fa0e5 that was accidentally reverted in #423. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix role name from 'admin' to 'provideradmin' to match actual DB value - Add concurrent session exemption to superUserAuthenticate so SuperAdmin can log in from multiple tabs without being blocked Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix: use stringRedisTemplate for jti: key read/delete in concurrent session
…ssword Feature/handel multiple attempt password
Redis keys were stored on MMU login with 30-day TTL but never deleted on logout. Added cleanup in userLogout() so stale camp config does not persist after the MMU session ends. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Lets a device on the same network discover the API's LAN IP and port without needing it typed in manually, and bumps version to 3.8.2.
Sn/token expiry
fix issue of feature multiple login attempt
Fix login issue multiple login
….8.2 # Conflicts: # src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java # src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
Merge release-3.8.1, main ti release-3.8.2
Saurav's PR #431 added a "Remaining attempts: N" message on failed login to warn users before account lockout. Port this into the refactored handleFailedLoginAttempt helper so both userAuthenticate and superUserAuthenticate keep the behavior after merging release-3.8.1's account-lock refactor into release-3.8.2.
The merge of release-3.8.1's account-lock refactor into release-3.8.2 replaced Saurav Mishra's inline multiple-login-attempt logic (PR #426/#431/#432) with the refactored handlePasswordValidationAndLocking helper. The refactor kept the "Remaining attempts" warning but dropped the final lockout message text, falling back to the unrelated generateLockoutErrorMessage used for already-locked accounts. Restore the exact lockout message from PR #432 so the refactored helper preserves both branches' intended behavior.
Merging #431 commit
Contributor
📝 WalkthroughWalkthroughThe changes add GPS demographic propagation, Bengali dynamic-form translations, role-based authentication session handling, Firebase notification and token updates, a LAN connectivity endpoint, revised health and JWT behavior, and related configuration changes. ChangesIdentity data propagation
Bengali dynamic form translation
Authentication and session handling
Firebase notification delivery
Runtime and API configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| public static String getLanIPAddress() { | ||
| try (DatagramSocket socket = new DatagramSocket()) { | ||
| socket.connect(InetAddress.getByName("8.8.8.8"), 10002); |
vanitha1822
approved these changes
Jun 30, 2026
vishwab1
approved these changes
Jul 1, 2026
* fix(health): stop reporting MySQL as DOWN when SELECT 1 succeeds checkDatabaseConnectivity() was deriving the status field from the background diagnostic severity (pool usage, long transactions, deadlocks) even after the live SELECT 1 connectivity check succeeded - so a CRITICAL finding from the background scan (e.g. connection pool >95% full) caused /common-api/health to report mysql.status=DOWN despite the database being fully reachable and serving real queries (confirmed: login worked fine while health reported DOWN). status now reflects connectivity only (UP if SELECT 1 succeeds, DOWN only on an actual connection failure); severity continues to surface the background diagnostic findings independently, so capacity/performance warnings are still visible without being misreported as an outage. Removed resolveDatabaseStatus() and the now-unused STATUS_DEGRADED constant, both made dead by this change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(beneficiary): enhance GPS data handling in beneficiary demographics and address flows * Update `gpsTimestamp` in `BeneficiaryDemographicsModel` and `Address` from `Timestamp` to `Long` to support epoch-millisecond values received from clients. * Add missing GPS-related fields (`latitude`, `longitude`, `digipin`, `isGpsUnavailable`, and `gpsUnavailableReason`) to the `Address` DTO for complete GPS data transfer. * Extend mapper decorators (`BenCompleteDetailMapperDecorator`, `CommonIdentityMapperDecorator`, and `IdentityBenEditMapperDecorator`) to propagate GPS information across beneficiary and identity workflows. * fix: add missing $ to resolve GEN_BENEFICIARY_IDS_API_URL in docker properties Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Sehjot Singh Pannu <sehjot.singh@unthinkable.co>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java (1)
311-327: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftKeep credential failures externally indistinguishable.
These messages reveal that a username exists, its remaining attempts, and lock status; unknown usernames still receive only the generic error. Since
IEMRException.getMessage()is propagated, this enables account enumeration. Return one generic authentication error externally and surface attempt guidance only through a non-enumerable flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java` around lines 311 - 327, The failed-login branches in the relevant authentication method currently expose account existence, remaining attempts, and lock status through IEMRException messages. Replace all externally propagated credential-failure messages with one generic authentication error for both known and unknown usernames, while moving any attempt guidance to a non-enumerable internal flow such as logging without username-specific disclosure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/common/controller/users/IEMRAdminController.java`:
- Around line 182-186: Remove the redundant failed-attempt reset and save after
successful authentication in IEMRAdminController.java:182-186, leaving
account-state persistence to IEMRAdminUserServiceImpl. Remove the raw save(User)
contract from IEMRAdminUserService.java:136 and its repository delegation from
IEMRAdminUserServiceImpl.java:1388-1391; update any resulting references as
needed.
- Around line 195-210: The concurrent-session exemption currently inspects only
the first role mapping and may use an unpopulated collection. Update the checks
around IEMRAdminController’s login flow and the path near userExitsCheck() to
reload and inspect complete role mappings, defining and consistently applying
whether any or all roles must be exempt; add a multi-role test covering the
chosen behavior. Both affected sites require the same complete-mapping logic.
In `@src/main/java/com/iemr/common/dto/identity/Address.java`:
- Around line 55-60: The isGpsUnavailable default is being overwritten with null
during Address mapping. Update Address to default isGpsUnavailable to false or
adjust BenCompleteDetailMapperDecorator to avoid setting the target when the
source value is null; apply the corresponding change at
src/main/java/com/iemr/common/dto/identity/Address.java#L55-L60 and
src/main/java/com/iemr/common/mapper/BenCompleteDetailMapperDecorator.java#L161-L166,
preserving false as the default when no value is supplied.
In
`@src/main/java/com/iemr/common/service/dynamicForm/FormMasterServiceImpl.java`:
- Around line 212-213: Update the option-language checks in
FormMasterServiceImpl to use case-insensitive matching for both English and
Bengali, replacing the exact comparisons in the branches that populate opt
labels. Preserve the existing label selection behavior while ensuring values
such as “BN” and “EN” select the corresponding translations.
- Around line 163-165: Update the Bengali branches in the relevant label,
placeholder, and option translation logic to use the Bengali translation only
when it is nonblank; otherwise preserve the existing field/placeholder default
or English option label. Apply this consistently to the branches assigning
translatedLabel and the corresponding symbols near the referenced locations.
In `@src/main/java/com/iemr/common/utils/CookieUtil.java`:
- Around line 38-39: Update CookieUtil’s cookie Max-Age configuration to match
the 8-hour jwt.access.expiration value in application.properties, replacing the
current 10-hour lifetime while preserving the existing seconds-based setting and
comment.
In `@src/main/java/com/iemr/common/utils/NetworkUtil.java`:
- Around line 51-53: Update the NetworkUtil LAN address resolution failure path
to stop returning FALLBACK_IP when resolution fails. Propagate the failure or
obtain a configured non-loopback address, and ensure ConnectController can
produce its explicit unavailable response instead of advertising loopback with
HTTP 200.
---
Outside diff comments:
In `@src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java`:
- Around line 311-327: The failed-login branches in the relevant authentication
method currently expose account existence, remaining attempts, and lock status
through IEMRException messages. Replace all externally propagated
credential-failure messages with one generic authentication error for both known
and unknown usernames, while moving any attempt guidance to a non-enumerable
internal flow such as logging without username-specific disclosure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 280d9ccc-54cb-4338-a9db-5bd05be6a89b
📒 Files selected for processing (19)
pom.xmlsrc/main/environment/common_docker.propertiessrc/main/java/com/iemr/common/controller/connect/ConnectController.javasrc/main/java/com/iemr/common/controller/users/IEMRAdminController.javasrc/main/java/com/iemr/common/data/dynamic_from/FormFieldOption.javasrc/main/java/com/iemr/common/data/translation/Translation.javasrc/main/java/com/iemr/common/dto/identity/Address.javasrc/main/java/com/iemr/common/mapper/BenCompleteDetailMapperDecorator.javasrc/main/java/com/iemr/common/mapper/CommonIdentityMapperDecorator.javasrc/main/java/com/iemr/common/mapper/IdentityBenEditMapperDecorator.javasrc/main/java/com/iemr/common/model/beneficiary/BeneficiaryDemographicsModel.javasrc/main/java/com/iemr/common/service/dynamicForm/FormMasterServiceImpl.javasrc/main/java/com/iemr/common/service/health/HealthService.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserService.javasrc/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javasrc/main/java/com/iemr/common/utils/CookieUtil.javasrc/main/java/com/iemr/common/utils/JwtUserIdValidationFilter.javasrc/main/java/com/iemr/common/utils/NetworkUtil.javasrc/main/resources/application.properties
Comment on lines
+182
to
+186
| User loggedInUser = mUser.get(0); | ||
|
|
||
| loggedInUser.setFailedAttempt(0); | ||
|
|
||
| iemrAdminUserServiceImpl.save(loggedInUser); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the duplicate stale account-state write. Successful authentication already clears and persists failed-attempt state in IEMRAdminUserServiceImpl; this later save can overwrite a concurrent failed-login/lock update.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java#L182-L186: do not reset and save the returned user again.src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java#L136-L136: remove the rawsave(User)contract.src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java#L1388-L1391: remove the corresponding repository delegation.
📍 Affects 3 files
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java#L182-L186(this comment)src/main/java/com/iemr/common/service/users/IEMRAdminUserService.java#L136-L136src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java#L1388-L1391
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/controller/users/IEMRAdminController.java`
around lines 182 - 186, Remove the redundant failed-attempt reset and save after
successful authentication in IEMRAdminController.java:182-186, leaving
account-state persistence to IEMRAdminUserServiceImpl. Remove the raw save(User)
contract from IEMRAdminUserService.java:136 and its repository delegation from
IEMRAdminUserServiceImpl.java:1388-1391; update any resulting references as
needed.
Comment on lines
+195
to
+210
| String userRole = ""; | ||
| if (mUser.size() == 1 && mUser.get(0).getM_UserServiceRoleMapping() != null) { | ||
| for (UserServiceRoleMapping usrm : mUser.get(0).getM_UserServiceRoleMapping()) { | ||
| if (usrm.getM_Role() != null && usrm.getM_Role().getRoleName() != null) { | ||
| userRole = usrm.getM_Role().getRoleName(); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (!CONCURRENT_SESSION_EXEMPT_ROLES.contains(userRole.trim().toLowerCase())) { | ||
| String tokenFromRedis = getConcurrentCheckSessionObjectAgainstUser( | ||
| m_User.getUserName().trim().toLowerCase()); | ||
| if (tokenFromRedis != null) { | ||
| throw new IEMRException( | ||
| "You are already logged in,please confirm to logout from other device and login again"); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'getUserServiceRoleMapping\s*\(|userExitsCheck\s*\(|CONCURRENT_SESSION_EXEMPT_ROLES' \
src/main/java/com/iemr/common
rg -n -C 5 'IEMRAdminController|concurrent.*session|provideradmin|superadmin' \
src/test/javaRepository: PSMRI/Common-API
Length of output: 38446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '175,220p' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
printf '\n----\n'
sed -n '410,470p' src/main/java/com/iemr/common/controller/users/IEMRAdminController.java
printf '\n----\n'
sed -n '700,760p' src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.java
printf '\n----\n'
sed -n '1328,1395p' src/main/java/com/iemr/common/service/users/IEMRAdminUserServiceImpl.javaRepository: PSMRI/Common-API
Length of output: 10532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'findByUserName\s*\(|userExitsCheck\s*\(' src/main/java/com/iemr/common
printf '\n----\n'
rg -n -C 6 'interface .*RepositoryCustom|class .*RepositoryCustom|findByUserName' src/main/java/com/iemr/common
printf '\n----\n'
sed -n '1,220p' src/main/java/com/iemr/common/repository/users/IEMRUserRepositoryCustom.javaRepository: PSMRI/Common-API
Length of output: 39428
Use the complete role mappings for concurrent-session checks.
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java#L195-L210: this branch only inspects the first mapping, butgetUserServiceRoleMapping()iterates aSet<Object[]>, so the chosen role can vary for multi-role users.src/main/java/com/iemr/common/controller/users/IEMRAdminController.java#L437-L458: reload the mappings before applying the same check;userExitsCheck()returns users only, so this path should not depend on an already populated role collection.
Define whether an exempt role should match any mapping or require all mappings to be exempt, then add a multi-role test.
📍 Affects 1 file
src/main/java/com/iemr/common/controller/users/IEMRAdminController.java#L195-L210(this comment)src/main/java/com/iemr/common/controller/users/IEMRAdminController.java#L437-L458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/controller/users/IEMRAdminController.java`
around lines 195 - 210, The concurrent-session exemption currently inspects only
the first role mapping and may use an unpopulated collection. Update the checks
around IEMRAdminController’s login flow and the path near userExitsCheck() to
reload and inspect complete role mappings, defining and consistently applying
whether any or all roles must be exempt; add a multi-role test covering the
chosen behavior. Both affected sites require the same complete-mapping logic.
Comment on lines
+55
to
+60
| private Double gpsLatitude; | ||
| private Double gpsLongitude; | ||
| private String digipin; | ||
| private Long gpsTimestamp; | ||
| private Boolean isGpsUnavailable; | ||
| private String gpsUnavailableReason; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Address.java =="
sed -n '1,220p' src/main/java/com/iemr/common/dto/identity/Address.java
echo
echo "== BenCompleteDetailMapperDecorator.java =="
sed -n '1,260p' src/main/java/com/iemr/common/mapper/BenCompleteDetailMapperDecorator.java
echo
echo "== Search for isGpsUnavailable usages =="
rg -n "isGpsUnavailable|getIsGpsUnavailable|setIsGpsUnavailable|GpsUnavailableReason|gpsUnavailable" src/main/javaRepository: PSMRI/Common-API
Length of output: 16013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BeneficiaryDemographicsModel excerpt =="
sed -n '150,220p' src/main/java/com/iemr/common/model/beneficiary/BeneficiaryDemographicsModel.java
echo
echo "== All direct reads of getIsGpsUnavailable =="
rg -n "getIsGpsUnavailable\\(" src/main/java
echo
echo "== All direct Boolean checks on isGpsUnavailable =="
rg -n "isGpsUnavailable|GpsUnavailable" src/main/java/com/iemr/common | sed -n '1,200p'Repository: PSMRI/Common-API
Length of output: 4570
Preserve the isGpsUnavailable default when mapping from Address.
BeneficiaryDemographicsModel defaults this flag to false, but Address leaves it nullable and BenCompleteDetailMapperDecorator copies that null straight over, clearing the default. If null is not a meaningful third state, default the DTO to false or skip the setter when the source is null.
src/main/java/com/iemr/common/dto/identity/Address.java#L55-L60src/main/java/com/iemr/common/mapper/BenCompleteDetailMapperDecorator.java#L161-L166
📍 Affects 2 files
src/main/java/com/iemr/common/dto/identity/Address.java#L55-L60(this comment)src/main/java/com/iemr/common/mapper/BenCompleteDetailMapperDecorator.java#L161-L166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/dto/identity/Address.java` around lines 55 -
60, The isGpsUnavailable default is being overwritten with null during Address
mapping. Update Address to default isGpsUnavailable to false or adjust
BenCompleteDetailMapperDecorator to avoid setting the target when the source
value is null; apply the corresponding change at
src/main/java/com/iemr/common/dto/identity/Address.java#L55-L60 and
src/main/java/com/iemr/common/mapper/BenCompleteDetailMapperDecorator.java#L161-L166,
preserving false as the default when no value is supplied.
Comment on lines
+163
to
+165
| }else if ("bn".equalsIgnoreCase(lang)) { | ||
| translatedLabel = label.getBengaliTranslation(); | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve a fallback when Bengali text is unavailable.
These branches assign nullable Bengali values directly, so existing or partially translated records can produce null field labels, placeholders, and option labels. Use Bengali only when it is nonblank; otherwise retain the field/placeholder default and the English option label.
Also applies to: 177-179, 212-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/service/dynamicForm/FormMasterServiceImpl.java`
around lines 163 - 165, Update the Bengali branches in the relevant label,
placeholder, and option translation logic to use the Bengali translation only
when it is nonblank; otherwise preserve the existing field/placeholder default
or English option label. Apply this consistently to the branches assigning
translatedLabel and the corresponding symbols near the referenced locations.
Comment on lines
+212
to
+213
| else if("en".equals(lang)) map.put("label", opt.getLabelEn()); | ||
| else if("bn".equals(lang)) map.put("label", opt.getLabelBn()); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use case-insensitive matching for option languages.
The field and placeholder paths use equalsIgnoreCase, but option selection requires exact en/bn casing. A request with lang=BN can therefore translate field text while omitting the option label; use equalsIgnoreCase consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/service/dynamicForm/FormMasterServiceImpl.java`
around lines 212 - 213, Update the option-language checks in
FormMasterServiceImpl to use case-insensitive matching for both English and
Bengali, replacing the exact comparisons in the branches that populate opt
labels. Preserve the existing label selection behavior while ensuring values
such as “BN” and “EN” select the corresponding translations.
Comment on lines
+38
to
+39
| // Set the Max-Age (expiry time) in seconds (10 hours) | ||
| cookie.setMaxAge(60 * 60 * 10); // 10 hours expiration |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the cookie lifetime aligned with the access-token lifetime.
The cookie now lasts 10 hours, but jwt.access.expiration in src/main/resources/application.properties is still 8 hours. Browsers can therefore send the stale Jwttoken for two hours after JWT expiry, causing repeated 401 responses. Align the two values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/utils/CookieUtil.java` around lines 38 - 39,
Update CookieUtil’s cookie Max-Age configuration to match the 8-hour
jwt.access.expiration value in application.properties, replacing the current
10-hour lifetime while preserving the existing seconds-based setting and
comment.
Comment on lines
+51
to
+53
| } catch (Exception e) { | ||
| logger.error("Failed to resolve LAN IP address", e); | ||
| return FALLBACK_IP; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not return loopback as a successful LAN address.
When resolution fails, this returns 127.0.0.1; ConnectController then advertises it as the server IP with HTTP 200. A phone will connect to itself and the connectivity flow fails. Propagate the failure or use a configured address so the controller can return an explicit unavailable response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/utils/NetworkUtil.java` around lines 51 - 53,
Update the NetworkUtil LAN address resolution failure path to stop returning
FALLBACK_IP when resolution fails. Propagate the failure or obtain a configured
non-loopback address, and ensure ConnectController can produce its explicit
unavailable response instead of advertising loopback with HTTP 200.
Feature/save user fcm token
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java (2)
147-153: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
getUserToken()looks up by the wrong key after the entity's PK change.
userTokenRepo.findById(userId)now queries byUserFcmTokenData's auto-generatedid, not byuser_id— those are no longer the same column. This method should usefindByUserId, exactly asupdateToken()(line 130) already does. As written, this either returns no token for a valid user, or, if another row's auto-generatedidhappens to equal this user's id, returns a different user's FCM token.🐛 Use findByUserId instead of findById
- return userTokenRepo.findById(Integer.parseInt(jwtUtil.getUserIdFromToken(jwtTokenFromCookie))) // because your userId is Long in DB + return userTokenRepo.findByUserId(Integer.parseInt(jwtUtil.getUserIdFromToken(jwtTokenFromCookie))) .map(UserFcmTokenData::getToken) .orElse(null); //🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java` around lines 147 - 153, Update getUserToken() to query userTokenRepo with findByUserId using the user ID extracted from the JWT, matching the lookup used by updateToken(). Preserve the existing token mapping and null fallback, and remove the findById-based lookup that targets the entity’s generated primary key.
129-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck-then-act race in
updateToken, plusupdatedAtis no longer set.
findByUserId→ decide insert/update →saveis not atomic; concurrentupdateTokencalls for the same user (e.g., multiple devices/tabs) can race and create two rows for the sameuser_id, since there's no unique constraint on that column (seeUserFcmTokenData.java). If duplicates ever exist,findByUserId's derived query will throwIncorrectResultSizeDataAccessExceptionon subsequent calls. Separately,updatedAtis no longer populated, so the column staysnullfor every row.🔒 Set updatedAt and tighten the upsert
public String updateToken(UserToken userToken) { - Optional<UserFcmTokenData> existingTokenData = userTokenRepo.findByUserId(userToken.getUserId()); - - UserFcmTokenData userTokenData; - - if (existingTokenData.isPresent()) { - userTokenData = existingTokenData.get(); - userTokenData.setToken(userToken.getToken()); - } else { - userTokenData = new UserFcmTokenData(); - userTokenData.setUserId(userToken.getUserId()); - userTokenData.setToken(userToken.getToken()); - } - + UserFcmTokenData userTokenData = userTokenRepo.findByUserId(userToken.getUserId()) + .orElseGet(() -> { + UserFcmTokenData newData = new UserFcmTokenData(); + newData.setUserId(userToken.getUserId()); + return newData; + }); + userTokenData.setToken(userToken.getToken()); + userTokenData.setUpdatedAt(new Timestamp(System.currentTimeMillis())); userTokenRepo.save(userTokenData); return "Save Successfully"; }A DB-level
uniqueconstraint onuser_id(plus catching the resulting constraint violation) is still needed to fully close the race.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java` around lines 129 - 145, Update updateToken to populate updatedAt on every insert or token update. Tighten the upsert by enforcing a database-level unique constraint on user_id in UserFcmTokenData and handling the resulting constraint violation so concurrent calls do not create duplicate rows; preserve the existing token update behavior.
🧹 Nitpick comments (2)
src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java (2)
94-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal
messageshadows the unused class field declared at line 68.SonarCloud flags this. The field
private Message message;appears unused elsewhere in the file; renaming the local variable (or removing the dead field) avoids the shadowing and clarifies whichmessageis in play.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java` around lines 94 - 103, Resolve the shadowing in the notification-building code by removing the unused class-level Message field or renaming the local variable in the relevant method; ensure all subsequent references use the intended locally built Message instance.Source: Linters/SAST tools
70-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated mandatory Firebase properties; also a misleading log line.
This class re-declares
firebaseEnabled/firebaseCredentialFile(no defaults) purely for logging, duplicatingFirebaseMessagingConfig. Same startup-failure risk applies here as in that class if either property is missing from a profile.firebaseMessagingis already@Autowired(required = false), so itsnullcheck (line 87) is sufficient to detect a disabled/misconfigured Firebase — these fields and the "Initializing Firebase" log (which fires on every notification request, not at actual init) can be dropped.♻️ Drop duplicated config fields; rely on firebaseMessaging null-check
- `@Value`("${firebase.enabled}") - private boolean firebaseEnabled; - - `@Value`("${firebase.credential-file}") - private String firebaseCredentialFile; - - public String sendNotification(NotificationMessage notificationMessage) { - logger.info("===== Initializing Firebase ====="); - logger.info("firebaseEnabled={}", firebaseEnabled); - logger.info("firebaseCredentialFile={}", firebaseCredentialFile); - logger.info("========== FCM Notification Request ==========");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java` around lines 70 - 82, Remove the duplicated firebaseEnabled and firebaseCredentialFile fields and their `@Value` injections from FirebaseNotificationService, along with the per-request “Initializing Firebase” and related property logs in sendNotification. Rely on the existing optional firebaseMessaging dependency and its null-check to handle disabled or misconfigured Firebase while preserving notification behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.java`:
- Around line 24-36: The Firebase configuration properties must have safe
defaults so missing environment values do not prevent startup: update the fields
in FirebaseMessagingConfig to use false for firebase.enabled and an empty value
for firebase.credential-file. Remove the duplicated property fields and related
bindings from FirebaseNotificationService, and remove its per-request
“Initializing Firebase” log; FirebaseMessagingConfig should be the sole owner of
these settings. Apply these changes in
src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.java lines
24-36 and
src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java
lines 70-82.
In `@src/main/java/com/iemr/common/data/userToken/UserFcmTokenData.java`:
- Around line 34-46: Add a database migration for the schema represented by
UserFcmTokenData, creating db_iemr.user_fcm_tokens with an auto-generated id,
user_id, token, and updated_at columns, and migrate existing rows from
user_tokens. Add a unique constraint on user_id to enforce one token row per
user and ensure the migration supports already-deployed environments before
FirebaseNotificationService.updateToken() uses the entity.
---
Outside diff comments:
In
`@src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java`:
- Around line 147-153: Update getUserToken() to query userTokenRepo with
findByUserId using the user ID extracted from the JWT, matching the lookup used
by updateToken(). Preserve the existing token mapping and null fallback, and
remove the findById-based lookup that targets the entity’s generated primary
key.
- Around line 129-145: Update updateToken to populate updatedAt on every insert
or token update. Tighten the upsert by enforcing a database-level unique
constraint on user_id in UserFcmTokenData and handling the resulting constraint
violation so concurrent calls do not create duplicate rows; preserve the
existing token update behavior.
---
Nitpick comments:
In
`@src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java`:
- Around line 94-103: Resolve the shadowing in the notification-building code by
removing the unused class-level Message field or renaming the local variable in
the relevant method; ensure all subsequent references use the intended locally
built Message instance.
- Around line 70-82: Remove the duplicated firebaseEnabled and
firebaseCredentialFile fields and their `@Value` injections from
FirebaseNotificationService, along with the per-request “Initializing Firebase”
and related property logs in sendNotification. Rely on the existing optional
firebaseMessaging dependency and its null-check to handle disabled or
misconfigured Firebase while preserving notification behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6df9a34-811b-4e4d-bb68-94c756c3642d
📒 Files selected for processing (6)
src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.javasrc/main/java/com/iemr/common/controller/firebaseNotification/FirebaseNotificationController.javasrc/main/java/com/iemr/common/data/userToken/UserFcmTokenData.javasrc/main/java/com/iemr/common/model/notification/NotificationMessage.javasrc/main/java/com/iemr/common/repo/userToken/UserFcmTokenRepo.javasrc/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java
Comment on lines
+24
to
+36
| @Value("${firebase.enabled}") | ||
| private boolean firebaseEnabled; | ||
|
|
||
| @Value("${firebase.credential-file:}") | ||
| @Value("${firebase.credential-file}") | ||
| private String firebaseCredentialFile; | ||
|
|
||
|
|
||
| @Bean | ||
| @ConditionalOnProperty(name = "firebase.enabled", havingValue = "true") | ||
| public FirebaseMessaging firebaseMessaging() throws IOException { | ||
| logger.info("===== Initializing Firebase ====="); | ||
| logger.info("firebaseEnabled={}", firebaseEnabled); | ||
| logger.info("firebaseCredentialFile={}", firebaseCredentialFile); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'FirebaseMessagingConfig.java|FirebaseNotificationService.java' . || true
echo "== locate property sources / firebase properties =="
rg -n "firebase\.(enabled|credential-file)|firebase" -S . --glob '!**/build/**' --glob '!**/target/**' --glob '!**/node_modules/**' | head -200 || true
echo "== relevant source =="
for f in $(fd 'FirebaseMessagingConfig.java|FirebaseNotificationService.java' .); do
echo "--- $f ($( wc -l < "$f" ) lines) ---"
ast-grep outline "$f" || true
cat -n "$f"
doneRepository: PSMRI/Common-API
Length of output: 20758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== application property files =="
git ls-files | rg '(^|/)application(\..*)?\.properties$|application(\..*)?\.yml$|.*firebase.*\.(properties|yml|yaml)$' | sort
echo "== property placeholders =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.properties'):
s = p.read_text(errors='replace').splitlines()
for i, line in enumerate(s, 1):
if 'firebase' in line.lower() and ('enabled' in line.lower() or 'credential' in line.lower()):
print(f"{p}:{i}:{line}")
PY
echo "== read-only `@Value` missing-property probe =="
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('src/main/java').rglob('*.java'):
text = p.read_text(errors='replace')
for token in ('firebase.enabled', 'firebase.credential-file'):
for m in re.finditer(r'`@Value`\([^#]*?'+ re.escape(f'${{{token}}}') + r'[^)]*\)', text, re.S):
expr = m.group(0).replace('\n',' ')
has_default = ':}' in expr[expr.find(token):expr.find(')')]
owner = str(p)
# identify containing method/class declaration roughly
before = text[:m.start()].rsplit('\n', 6)[-1]
enclosing = before.rsplit('.',1)[-1].rsplit('{',1)[-1].strip() or p
print(f"{owner}: {token}: expr={expr}; has_default={has_default}; enclosing={enclosing}")
PYRepository: PSMRI/Common-API
Length of output: 1847
Make Firebase config optional at startup and remove the duplicated @Values. Both FirebaseMessagingConfig and FirebaseNotificationService bind firebase.enabled/firebase.credential-file without defaults; if these env-substituted properties are absent, Spring fails before Firebase can be used or disabled. Add explicit defaults (@Value("${firebase.enabled:false}") / @Value("${firebase.credential-file:}")), let only the config bean own the config, and remove the duplicated fields and misleading per-request “Initializing Firebase” log from the service.
📍 Affects 2 files
src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.java#L24-L36(this comment)src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java#L70-L82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.java`
around lines 24 - 36, The Firebase configuration properties must have safe
defaults so missing environment values do not prevent startup: update the fields
in FirebaseMessagingConfig to use false for firebase.enabled and an empty value
for firebase.credential-file. Remove the duplicated property fields and related
bindings from FirebaseNotificationService, and remove its per-request
“Initializing Firebase” log; FirebaseMessagingConfig should be the sole owner of
these settings. Apply these changes in
src/main/java/com/iemr/common/config/firebase/FirebaseMessagingConfig.java lines
24-36 and
src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java
lines 70-82.
Comment on lines
34
to
+46
| @Entity | ||
| @Table(name = "user_tokens", schema = "db_iemr") | ||
| @Table(name = "user_fcm_tokens", schema = "db_iemr") | ||
| @Data | ||
| public class UserTokenData { | ||
| public class UserFcmTokenData { | ||
| @Id | ||
| @GeneratedValue(strategy = IDENTITY) | ||
| private int id; | ||
| @Column(name = "user_id") | ||
| Integer userId; | ||
| private Integer userId; | ||
| @Column(name = "token") | ||
| String token; | ||
| private String token; | ||
| @Column(name = "updated_at") | ||
| Timestamp updatedAt; | ||
| private Timestamp updatedAt; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for migration scripts referencing the renamed table
rg -n "user_fcm_tokens|user_tokens" -g '*.sql' -g '*.xml' -g '*.yml'
fd -i changelogRepository: PSMRI/Common-API
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -u
echo "== files matching migration-related names =="
git ls-files | rg -n -i 'liquibase|flyway|migration|changelog|db|schema|sql$|yaml|yml' | head -200
echo
echo "== tracking all references to token tables across tracked files =="
rg -n -i "user_fcm_tokens|user_tokens|firebase|fcm" . \
-g '!**/target/**' -g '!**/build/**' -g '!**/.git/**' -g '!**/node_modules/**' \
| head -200
echo
echo "== candidate data files =="
fd -i 'UserFcmTokenData.java|FirebaseNotificationService.java' .Repository: PSMRI/Common-API
Length of output: 17601
🏁 Script executed:
#!/bin/bash
set -u
echo "== UserFcmTokenData =="
cat -n src/main/java/com/iemr/common/data/userToken/UserFcmTokenData.java
echo
echo "== UserFcmTokenRepo =="
cat -n src/main/java/com/iemr/common/repo/userToken/UserFcmTokenRepo.java
echo
echo "== FirebaseNotificationService relevant method =="
sed -n '120,165p' src/main/java/com/iemr/common/service/firebaseNotification/FirebaseNotificationService.java | cat -n
echo
echo "== targeted table references in all tracked files =="
python3 - <<'PY'
import subprocess
result=subprocess.run(["git","ls-files","*"],capture_output=True,text=True,check=True)
terms=("user_fcm_tokens","user_tokens","firebase","fcm")
for line in result.stdout.splitlines():
if not line.startswith(".") or line.startswith(".github"):
try:
data=open(line,errors="ignore").read()
except Exception:
continue
if any(t in data for t in terms):
if "user_fcm_tokens" in data or "user_tokens" in data:
print(f"\n--- {line} ---")
for i,l in enumerate(data.splitlines(),1):
if "user_fcm_tokens" in l or "user_tokens" in l:
print(f"{i}: {l}")
PYRepository: PSMRI/Common-API
Length of output: 5484
Add a migration for the new FCM token schema.
UserFcmTokenData now maps to db_iemr.user_fcm_tokens with an auto-generated id and no user_id uniqueness, but there are no repo SQL migrations creating this table/columns or moving existing user_tokens rows. Add a migration before relying on this entity in already-deployed environments.
Also add a DB-unique constraint on user_id because findByUserId(...) plus the check-then-save workflow in FirebaseNotificationService.updateToken() can otherwise allow duplicate token rows per user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/iemr/common/data/userToken/UserFcmTokenData.java` around
lines 34 - 46, Add a database migration for the schema represented by
UserFcmTokenData, creating db_iemr.user_fcm_tokens with an auto-generated id,
user_id, token, and updated_at columns, and migrate existing rows from
user_tokens. Add a unique constraint on user_id to enforce one token row per
user and ensure the migration supports already-deployed environments before
FirebaseNotificationService.updateToken() uses the entity.
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
5 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.
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes