Update Beneficiary #143
Update Beneficiary #143
Conversation
WalkthroughThe pull request introduces several modifications across three service implementation classes: Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/main/java/com/iemr/common/service/beneficiary/RegisterBenificiaryServiceImpl.java (2)
117-120: Consider removing redundant community field assignmentThe code sets both
communityandcommunityNameto the same value. This appears redundant and might indicate confusion about which field should be used.if(null != benificiaryDetails.getI_bendemographics()) { - identityEditDTO.setCommunity(benificiaryDetails.getI_bendemographics().getCommunityName()); identityEditDTO.setCommunityName(benificiaryDetails.getI_bendemographics().getCommunityName()); }
Line range hint
12-24: Review discount and fee application logicThe current implementation can result in customers paying more after a discount due to the flat fee. For example:
- Original amount: $100
- With 10% discount: $90
- After $20 fee: $110 (higher than original)
This negates the benefit of the loyalty discount for smaller purchases.
Consider one of these alternatives:
- Apply fee before discount
- Scale fee based on purchase amount
- Remove fee for discounted purchases
Would you like me to propose a revised implementation?src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (1)
Line range hint
1-700: Consider centralizing error handling logicMultiple methods contain duplicate error handling code. Consider extracting the common pattern into a utility method:
+ private void handleIdentityResponse(OutputResponse response) throws IEMRException { + if (response != null && response.getStatusCode() == OutputResponse.USERID_FAILURE) { + throw new IEMRException(response.getErrorMessage()); + } + }This would reduce code duplication and make error handling more maintainable.
src/main/java/com/iemr/common/service/sms/SMSServiceImpl.java (2)
Line range hint
804-883: Consider using proper synchronization mechanism instead of static boolean flag.The current implementation uses a static boolean flag
publishingSMSto prevent concurrent executions, which might not be thread-safe in a highly concurrent environment.Consider using one of these alternatives:
- Use
java.util.concurrent.locks.Lock:-private static Boolean publishingSMS = false; +private static final Lock publishingSMSLock = new ReentrantLock(); @Async @Override public void publishSMS() { - try { - if (!SMSServiceImpl.publishingSMS) { - SMSServiceImpl.publishingSMS = true; + if (publishingSMSLock.tryLock()) { + try { // SMS publishing logic - } - } catch (Exception e) { - logger.error("publishSMS failed with error " + e.getMessage()); - } finally { - SMSServiceImpl.publishingSMS = false; + } catch (Exception e) { + logger.error("publishSMS failed with error " + e.getMessage()); + } finally { + publishingSMSLock.unlock(); + } } }
- Or use
java.util.concurrent.Semaphore:-private static Boolean publishingSMS = false; +private static final Semaphore publishingSMSSemaphore = new Semaphore(1); @Async @Override public void publishSMS() { - try { - if (!SMSServiceImpl.publishingSMS) { - SMSServiceImpl.publishingSMS = true; + if (publishingSMSSemaphore.tryAcquire()) { + try { // SMS publishing logic - } - } catch (Exception e) { - logger.error("publishSMS failed with error " + e.getMessage()); - } finally { - SMSServiceImpl.publishingSMS = false; + } catch (Exception e) { + logger.error("publishSMS failed with error " + e.getMessage()); + } finally { + publishingSMSSemaphore.release(); + } } }
Line range hint
804-883: Enhance error handling with more specific exception handling.The current implementation catches all exceptions generically. Consider adding more specific exception handling for better error management and debugging.
@Async @Override public void publishSMS() { try { if (!SMSServiceImpl.publishingSMS) { SMSServiceImpl.publishingSMS = true; // Configuration and date setup try { // SMS sending logic - } catch (Exception e) { + } catch (HttpClientErrorException e) { + logger.error("SMS API client error for phone no/benRegID: " + sms.getPhoneNo() + "/" + + sms.getBeneficiaryRegID() + " with error " + e.getMessage(), e); + handleSMSError(sms, e); + } catch (HttpServerErrorException e) { + logger.error("SMS API server error for phone no/benRegID: " + sms.getPhoneNo() + "/" + + sms.getBeneficiaryRegID() + " with error " + e.getMessage(), e); + handleSMSError(sms, e); + } catch (Exception e) { logger.error("Failed to send sms on phone no/benRegID: " + sms.getPhoneNo() + "/" + sms.getBeneficiaryRegID() + " with error " + e.getMessage(), e); - sms.setTransactionError(e.getMessage()); - sms.setIsTransactionError(true); - sms.setTransactionID(null); - sms.setSmsStatus(SMSNotification.NOT_SENT); - sms = smsNotification.save(sms); + handleSMSError(sms, e); } } } catch (Exception e) { logger.error("publishSMS failed with error " + e.getMessage()); } finally { SMSServiceImpl.publishingSMS = false; } } +private void handleSMSError(SMSNotification sms, Exception e) { + sms.setTransactionError(e.getMessage()); + sms.setIsTransactionError(true); + sms.setTransactionID(null); + sms.setSmsStatus(SMSNotification.NOT_SENT); + sms = smsNotification.save(sms); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java(1 hunks)src/main/java/com/iemr/common/service/beneficiary/RegisterBenificiaryServiceImpl.java(1 hunks)src/main/java/com/iemr/common/service/sms/SMSServiceImpl.java(2 hunks)
🔇 Additional comments (1)
src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java (1)
33-33: LGTM: Required import for JSON processing
The Gson import is necessary for JSON processing operations used throughout the file.
📋 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
Bug Fixes
Refactor
Style