Thematic content fixes for whatsapp and conatct us details updated #138
Conversation
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
WalkthroughImplements preloading of signature images and updates certificate generation to use deep-copied buffers and FLW-derived directory/filenames. Adds logging and error handling tweaks. Adjusts report DAO to exclude “opt” week rows. Updates Contact Us page text content. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Admin/Scheduler
participant CS as CertificateServiceImpl
participant FLW as FLW Repository
participant PDF as PDF Renderer
participant FS as File System
Note over CS: Static init at startup<br/>Preload certificate_sign1/2/3.png
Admin->>CS: createAllCertificateUptoCurrentMonthInBulk()
loop For each completion
CS->>FLW: resolveFrontLineWorker(flwId)
FLW-->>CS: FrontLineWorker(state,district,block, name, mobile)
CS->>PDF: createCertificatePdf(name, mobile, preloaded signatures)
PDF-->>CS: PDF bytes
CS->>FS: write /state/district/block/mobile.pdf
end
CS-->>Admin: Done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🔭 Outside diff range comments (2)
NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/business/impl/CertificateServiceImpl.java (2)
516-538: Null-safety for FLW lookups, resilient directory building, and filename arithmetic fix.
- frontLineWorker or its state/district/block can be null; current code risks NPE.
- blockMap.get(district) can be null; guard before .get(block).
- fileName uses string concatenation which turns “… + 1” into string append, not arithmetic.
Apply this diff:
- FrontLineWorkers frontLineWorker = frontLineWorkersDao.getFlwById(flw.getFlwId()); - // creating directories - String dir = frontLineWorker.getState() + "_" + stateName + "/"; - if (frontLineWorker.getDistrict() != null) { - dir = dir + "/" + frontLineWorker.getDistrict() + "_" + districtMap.get(frontLineWorker.getDistrict()); - if (frontLineWorker.getBlock() != null) { - dir = dir + "/" + frontLineWorker.getBlock() + "_" + blockMap.get(frontLineWorker.getDistrict()).get(frontLineWorker.getBlock()); - } else { - dir = dir + "/" + "other"; - } - } else { - dir = dir + "/" + "other"; - } + FrontLineWorkers frontLineWorker = frontLineWorkersDao.getFlwById(flw.getFlwId()); + // creating directories (null-safe) + StringBuilder dirBuilder = new StringBuilder(); + Integer stateIdForDir = (frontLineWorker != null && frontLineWorker.getState() != null) + ? frontLineWorker.getState() + : state.getStateId(); + dirBuilder.append(stateIdForDir).append("_").append(stateName).append("/"); + if (frontLineWorker != null && frontLineWorker.getDistrict() != null) { + Integer distId = frontLineWorker.getDistrict(); + String distName = districtMap.getOrDefault(distId, "other"); + dirBuilder.append(distId).append("_").append(distName).append("/"); + if (frontLineWorker.getBlock() != null) { + Integer blkId = frontLineWorker.getBlock(); + String blkName = Optional.ofNullable(blockMap.get(distId)) + .map(m -> m.get(blkId)) + .orElse("other"); + dirBuilder.append(blkId).append("_").append(blkName); + } else { + dirBuilder.append("other"); + } + } else { + dirBuilder.append("other"); + } + String dir = dirBuilder.toString(); @@ - String fileName = frontLineWorker.getMobileNumber() + "_" + flw.getId() * 2 + 1 + ".pdf"; + String msisdnStr = (frontLineWorker != null && frontLineWorker.getMobileNumber() != null) + ? frontLineWorker.getMobileNumber().toString() + : flw.getMsisdn().toString(); + String fileName = msisdnStr + "_" + (flw.getId() * 2 + 1) + ".pdf";Optional hardening (outside this hunk): sanitize directory name segments to remove unsafe characters before concatenation.
261-263: Fix filename arithmetic precedence (2n+1 currently concatenates “1” as a string).Parentheses ensure integer arithmetic precedes string concatenation.
- String fileName = flw.getMsisdn()+"_"+flw.getId()*2+1+".pdf"; + String fileName = flw.getMsisdn() + "_" + (flw.getId() * 2 + 1) + ".pdf";
🧹 Nitpick comments (5)
NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/dao/impl/KilkariThematicContentReportDaoImpl.java (1)
41-41: Avoid deprecated Double constructor.Use primitives or valueOf for clarity and to avoid unnecessary boxing.
- Double d = new Double(0.00); + double d = 0.0;NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/htmlpages/ContactUs.java (1)
50-55: Escape ampersand and make the email clickable.Minor HTML correctness and UX improvement.
- " <p>Ministry of Health & Family Welfare (MoHFW),</p>\n" + + " <p>Ministry of Health & Family Welfare (MoHFW),</p>\n" + @@ - " <p>Email Id - mmpc-mohfw@gov.in</p>" + + " <p>Email ID - <a href=\"mailto:mmpc-mohfw@gov.in\">mmpc-mohfw@gov.in</a></p>" +NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/business/impl/CertificateServiceImpl.java (3)
721-731: Avoid double-scaling images; scale once to preserve aspect ratio.Calling scaleByWidth then scaleByHeight overwrites the prior scaling and can distort images. Scale once.
- Image image1 = new Image(deepCopy(certificateSign1Image)); - image1 = image1.scaleByWidth(110).scaleByHeight(110); + Image image1 = new Image(deepCopy(certificateSign1Image)); + image1 = image1.scaleByWidth(110); @@ - Image image2 = new Image(deepCopy(certificateSign2Image)); - image2 = image2.scaleByWidth(65).scaleByHeight(55); + Image image2 = new Image(deepCopy(certificateSign2Image)); + image2 = image2.scaleByWidth(65); @@ - Image image3 = new Image(deepCopy(certificateSign3Image)); - image3 = image3.scaleByWidth(125).scaleByHeight(105); + Image image3 = new Image(deepCopy(certificateSign3Image)); + image3 = image3.scaleByWidth(125);
650-654: Log errors with stack trace; avoid System.out.Promote to error-level logging with context and stack trace for triaging. Remove println.
- } catch (IOException ignore) { - response = "failed to load sample certificate"; - System.out.println("---Error---=>" + response); - LOGGER.info(ignore.getMessage()); + } catch (IOException e) { + response = "failed to load sample certificate"; + LOGGER.error("Error generating certificate PDF at path={} for name={}, msisdn={}: {}", + pdfFile, name, msisdn, response, e);
588-589: Avoid logging PII (full names) at INFO.Consider reducing the detail or redacting personally identifiable information.
- LOGGER.info("Successfully generated Telugu certificate for FLW: {}", name); + LOGGER.info("Successfully generated Telugu certificate");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/business/impl/CertificateServiceImpl.java(8 hunks)NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/dao/impl/KilkariThematicContentReportDaoImpl.java(1 hunks)NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/htmlpages/ContactUs.java(1 hunks)
🔇 Additional comments (2)
NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/business/impl/CertificateServiceImpl.java (2)
83-85: LGTM: Make sample certificate files final.Marking these as static final is a good improvement (immutable configuration).
813-819: LGTM: deepCopy utility is correct.Proper BufferedImage duplication avoiding shared raster/CM side-effects.
Comment on lines
+95
to
+103
| static { | ||
| try { | ||
| certificateSign1Image = ImageIO.read(Files.newInputStream(Paths.get(retrieveDocuments() + "Certificate/certificate_sign1.png"))); | ||
| certificateSign2Image = ImageIO.read(Files.newInputStream(Paths.get(retrieveDocuments() + "Certificate/certificate_sign2.png"))); | ||
| certificateSign3Image = ImageIO.read(Files.newInputStream(Paths.get(retrieveDocuments() + "Certificate/certificate_sign3.png"))); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to preload signature images", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Close streams and avoid crashing class initialization on missing images.
- Files.newInputStream streams are never closed here, causing FD leaks.
- Throwing at class init will fail app start if images are missing. Prefer logging and a graceful fallback.
- Also reuse the already-initialized
documentsinstead of re-calling retrieveDocuments().
Apply this diff:
static {
try {
- certificateSign1Image = ImageIO.read(Files.newInputStream(Paths.get(retrieveDocuments() + "Certificate/certificate_sign1.png")));
- certificateSign2Image = ImageIO.read(Files.newInputStream(Paths.get(retrieveDocuments() + "Certificate/certificate_sign2.png")));
- certificateSign3Image = ImageIO.read(Files.newInputStream(Paths.get(retrieveDocuments() + "Certificate/certificate_sign3.png")));
- } catch (IOException e) {
- throw new RuntimeException("Failed to preload signature images", e);
- }
+ certificateSign1Image = ImageIO.read(new File(documents + "Certificate/certificate_sign1.png"));
+ certificateSign2Image = ImageIO.read(new File(documents + "Certificate/certificate_sign2.png"));
+ certificateSign3Image = ImageIO.read(new File(documents + "Certificate/certificate_sign3.png"));
+ } catch (IOException e) {
+ LOGGER.warn("Failed to preload signature images; will fall back to on-demand loading.", e);
+ certificateSign1Image = null;
+ certificateSign2Image = null;
+ certificateSign3Image = null;
+ }
}Optionally add a tiny helper and fallback usage (outside this hunk) to on-demand load if cached is null:
// helper (place near deepCopy)
private static BufferedImage requireSignImage(BufferedImage cached, String relativePath) {
if (cached != null) return cached;
try { return ImageIO.read(new File(documents + relativePath)); }
catch (IOException e) {
LOGGER.error("Unable to load signature image: {}", relativePath, e);
return null;
}
}And replace uses with requireSignImage(certificateSign1Image, "Certificate/certificate_sign1.png") etc.
🤖 Prompt for AI Agents
In
NMSReportingSuite/src/main/java/com/beehyv/nmsreporting/business/impl/CertificateServiceImpl.java
around lines 95-103, the static initializer opens Files.newInputStream without
closing streams, re-calls retrieveDocuments(), and throws a RuntimeException
which will break class initialization if images are missing; change to reuse the
already-initialized documents variable, read images with try-with-resources or
use ImageIO.read(File) so streams are closed, stop throwing from the static
block and instead catch exceptions, log the error via LOGGER.error(...) and
leave the static BufferedImage fields null as a graceful fallback, and
optionally add the suggested requireSignImage helper to lazily load and log
errors when images are requested.
Comment on lines
+36
to
38
| Restrictions.eq("date", date), | ||
| Restrictions.ne("messageWeekNumber", "opt") | ||
| )); |
There was a problem hiding this comment.
Fix Criteria construction; add null-safety for messageWeekNumber.
Good call excluding "opt". However, Restrictions.and only accepts two Criterion; passing 5 will not compile and/or won’t work as intended. Also consider excluding null messageWeekNumber values explicitly.
Apply this diff to build the predicates correctly and add isNotNull:
- criteria.add(Restrictions.and(
- Restrictions.eq("locationId",locationId.longValue()),
- Restrictions.eq("locationType",locationType),
- Restrictions.eq("periodType",periodType),
- Restrictions.eq("date", date),
- Restrictions.ne("messageWeekNumber", "opt")
- ));
+ criteria.add(Restrictions.eq("locationId", locationId.longValue()));
+ criteria.add(Restrictions.eq("locationType", locationType));
+ criteria.add(Restrictions.eq("periodType", periodType));
+ criteria.add(Restrictions.eq("date", date));
+ criteria.add(Restrictions.isNotNull("messageWeekNumber"));
+ criteria.add(Restrictions.ne("messageWeekNumber", "opt"));Also applies to: 32-38
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR contains 3 fixes:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation