Update the branch from Release 3.5.1 to Release 3.4.1

[vanitha1822]

📋 Description

JIRA ID:

Please provide a summary of the change and the motivation behind it. Include relevant context and details.

---

✅ Type of Change

• 🐞 Bug fix (non-breaking change which resolves an issue)
• ✨ New feature (non-breaking change which adds functionality)
• 🔥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 🛠 Refactor (change that is neither a fix nor a new feature)
• ⚙️ Config change (configuration file or build script updates)
• 📚 Documentation (updates to docs or readme)
• 🧪 Tests (adding new or updating existing tests)
• 🎨 UI/UX (changes that affect the user interface)
• 🚀 Performance (improves performance)
• 🧹 Chore (miscellaneous changes that don't modify src or test files)

---

ℹ️ 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

  • Per-record and per-table sync progress with detailed success/failure reasons and consolidated sync results.
  • Automatic initialization/propagation of vanSerialNo after saving visits, complaints, prescriptions, lab results and related records.
  • Doctor signature flag captured, persisted, and propagated through doctor save/update flows, affecting beneficiary flow updates.
  • Retrieval of recent archived test visits.

• Refactor

  • Reworked sync pipeline for batched group/table processing, improved HTTP/JSON handling, centralized SyncResult tracking.
  • Internal dependency wiring refactored for clearer injection.

• Chores

  • Increased application and REST timeouts.
  • Project version bumped to 3.4.1.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Bumped project version to 3.4.1. Added persistent doctorSignatureFlag to ben-flow and propagated it through services/repos. Added many repository updateVanSerialNo methods and post-save vanSerialNo assignments. Expanded NCDCareDiagnosis constructor/projection. Large DataSync refactor: SyncResult, batching, status/reason, timeouts, and logging.

Changes

Cohort / File(s)	Summary
Version bump pom.xml	Project version 3.4.0 → 3.4.1.
Import cleanup src/main/java/com/iemr/mmu/data/doctor/CancerGynecologicalExamination.java	Removed duplicate Lombok import.
NCDCareDiagnosis constructor & projection src/main/java/com/iemr/mmu/data/ncdcare/NCDCareDiagnosis.java, src/main/java/com/iemr/mmu/repo/nurse/ncdcare/NCDCareDiagnosisRepo.java	Constructor now accepts createdBy, createdDate; repo projection expanded to include these fields.
New/updated updateVanSerialNo repo methods src/main/java/com/iemr/mmu/repo/nurse/BenVisitDetailRepo.java, src/main/java/com/iemr/mmu/repo/quickConsultation/BenChiefComplaintRepo.java, src/main/java/com/iemr/mmu/repo/quickConsultation/PrescriptionDetailRepo.java, src/main/java/com/iemr/mmu/repo/labModule/LabResultEntryRepo.java, src/main/java/com/iemr/mmu/repo/quickConsultation/LabTestOrderDetailRepo.java	Added @Transactional/@Modifying JPQL update methods updateVanSerialNo(...) to set vanSerialNo by entity id.
BeneficiaryFlowStatus field & repo changes src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java, src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java	Added persistent doctorSignatureFlag (default false); repo SELECTs include it and multiple doctor-related update queries now set it; nurse update also sets vanSerialNo = :benFlowID.
Post-save vanSerialNo assignments & DI refactors src/main/java/.../ANCNurseServiceImpl.java, src/main/java/.../CommonNurseServiceImpl.java, src/main/java/.../CSDoctorServiceImpl.java, src/main/java/.../LabTechnicianServiceImpl.java, src/main/java/.../QuickConsultationServiceImpl.java	After persist, services set vanSerialNo on entities or call repo.updateVanSerialNo; several services refactored to setter-based DI and added/adjusted logging.
Doctor signature propagation src/main/java/.../*ServiceImpl.java (many)	Extract doctorSignatureFlag from requests (default false) and propagate to ben‑flow update calls; updated method signatures for updateBenFlowAfterDocData / updateBenFlowtableAfterDocDataSave/Update to accept the flag.
DataSyncRepository API change src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java	updateProcessedFlagInVan now accepts List<String> vanSerialNos, plus status and reason; SQL and JDBC parameterization updated.
UploadDataToServerImpl refactor src/main/java/com/iemr/mmu/service/dataSyncActivity/UploadDataToServerImpl.java	Reworked upload flow: group/table batching, per-table/group progress tracking, timeout-enabled RestTemplate, structured responses, detailed status/reason handling and improved error handling.
Download/import adjustments src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java	Extract vanID from original JSON, build local import payload {vanID, benIDList}, and add logging.
Transactional schema change for central updates src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerTransactionalImpl.java	Central schema for several processed‑flag updates changed from db_iemr_sync → db_iemr.
Central sync: SyncResult & flow changes src/main/java/com/iemr/mmu/service/dataSyncActivity/SyncResult.java, src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/DataSyncRepositoryCentral.java, src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImpl.java	Added SyncResult DTO; consolidated per-record SyncResult tracking, added logging, adjusted whitelist table name, and improved per-record status/reason handling and batch result propagation.
DataSync client/server robustness & timeouts src/main/java/com/iemr/mmu/service/dataSyncActivity/UploadDataToServerImpl.java, src/main/resources/application.properties	Added JSON parsing resilience, ResourceAccessException handling, configurable RestTemplate timeouts; application properties updated with multiple timeout settings and DB pool tweaks.
Lab technician additions src/main/java/com/iemr/mmu/service/labtechnician/LabTechnicianServiceImpl.java	Added setter DI for repos, refactored lab/radiology JSON assembly, and new method getLast_3_ArchivedTestVisitList(Long, Long).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant Service as Service Layer
  participant Repo as Repository
  participant DB as Database

  Client->>Service: saveEntity(payload)
  Service->>Repo: save(entity)
  Repo->>DB: INSERT ... (returns id)
  DB-->>Repo: persisted entity with id
  Repo-->>Service: saved entity
  alt set vanSerialNo on entity
    Service->>Service: entity.setVanSerialNo(id)
    Service->>Repo: (optional) save(entity)
  else update via repo
    Service->>Repo: updateVanSerialNo(id)
    Repo->>DB: UPDATE ... SET vanSerialNo = id WHERE id = ?
  end
  Service-->>Client: return id/status

Loading

sequenceDiagram
  autonumber
  actor Operator
  participant Upload as UploadDataToServerImpl
  participant Repo as DataSyncRepository
  participant Central as Central API

  Operator->>Upload: syncIntercepter(vanID,user,token)
  Upload->>Upload: load DataSyncGroups
  loop per group
    Upload->>Upload: load tables for group
    loop per table (batches)
      Upload->>Repo: fetchBatch(table,batchSize)
      Upload->>Central: POST batch (timeout-configured)
      alt success
        Central-->>Upload: success counts
        Upload->>Repo: updateProcessedFlagInVan(..., status="S", reason=null)
      else partial/failure
        Central-->>Upload: failure/details
        Upload->>Repo: updateProcessedFlagInVan(..., status="F"/"P", reason="message")
      end
      Upload->>Upload: append per-record SyncResult entries
    end
    Upload->>Upload: aggregate group status
  end
  Upload-->>Operator: groupsProgress JSON

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Areas needing extra attention:

• DataSync refactor: batching correctness, SQL parameterization, status/reason propagation, JSON parsing, and HTTP timeout/error handling.
• Threading of doctorSignatureFlag across many services/repos and ensuring all method signatures and call sites are updated.
• New updateVanSerialNo repo methods: transactional boundaries and concurrent update behavior.
• NCDCareDiagnosis constructor change vs. repo projection alignment.

Possibly related PRs

• Move code from Release 3.4.0 to Main  #119 — overlaps on vanSerialNo propagation, BeneficiaryFlowStatus and related ben-flow changes.

Poem

I saved a row and found its id,
I hopped and stamped vanSerial beside.
Batches hum and retries sing,
Flags and reasons each record bring.
Rabbit logs a tiny ping. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title describes a version update (3.5.1 to 3.4.1) but conflicts with the actual changeset, which implements extensive feature additions including vanSerialNo propagation, doctorSignatureFlag tracking, repository methods, service refactoring, and configuration updates—not a simple version downgrade.	Revise the title to accurately reflect the actual changes, such as 'Add vanSerialNo propagation and doctorSignatureFlag tracking across services' or similar language reflecting the substantive feature work in the changeset.
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch release-3.4.1

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 8

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/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (1)

74-99: Van serial update never reaches the database

beneficiaryFlowStatusRepo.save(obj) commits before we call obj.setVanSerialNo(...). Because this service method is not transactional, the entity is detached once save returns, so the assignment never flushes. The same pattern repeats in the else branch. Result: vanSerialNo stays null in the persisted row, breaking the downstream sync logic that relies on it. Move the assignment before save, or update the returned objRS and persist it (e.g., set objRS.setVanSerialNo(...) and invoke a follow-up save/update) so the value is stored.

src/main/java/com/iemr/mmu/service/anc/ANCNurseServiceImpl.java (1)

152-170: Post-save vanSerialNo assignment is dropped

After ancCareRepo.save(...) / ancWomenVaccineRepo.saveAll(...) return, the repository transaction is already closed (this service isn’t annotated with @Transactional). Setting vanSerialNo on the returned entity objects therefore never persists, so the column remains null. Assign the value before calling save, or persist the update explicitly (e.g., set it on the managed entity inside the repository layer or issue an update query) to ensure the van serial is stored.

🧹 Nitpick comments (2)

src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/DataSyncRepositoryCentral.java (1)

117-118: Consider using debug level for detailed diagnostic logging.

These logging statements provide useful diagnostic information but might be too verbose for production at the info level. Consider using logger.debug() instead to avoid log clutter in production environments.

Apply this diff:

-              logger.info("Checking record presence: schema={}, table={}, vanSerialNo={}, vanID={}, column={}, syncFacilityID={}",
+              logger.debug("Checking record presence: schema={}, table={}, vanSerialNo={}, vanID={}, column={}, syncFacilityID={}",
                 schemaName, tableName, vanSerialNo, vanID, vanAutoIncColumnName, syncFacilityID);
         jdbcTemplate = getJdbcTemplate();
         List<Object> params = new ArrayList<>();
 
         if (!isValidSchemaName(schemaName) || !isValidTableName(tableName) ||
                 !isValidDatabaseIdentifierCharacter(vanAutoIncColumnName)) {
             logger.error("Invalid identifiers: schema={}, table={}, column={}", schemaName, tableName,
                     vanAutoIncColumnName);
             throw new IllegalArgumentException("Invalid identifiers provided.");
         }
 
         StringBuilder queryBuilder = new StringBuilder("SELECT ")
                 .append(vanAutoIncColumnName).append(" FROM ")
                 .append(schemaName).append(".").append(tableName).append(" WHERE VanSerialNo = ?");
-        logger.info("Constructed query: {}", queryBuilder.toString());
+        logger.debug("Constructed query: {}", queryBuilder.toString());

Also applies to: 132-132

src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java (1)

361-361: Consider validating vanID before including in payload.

The vanID can be null if it's not present in the original request. While this might be intentional for certain scenarios, explicitly validating and documenting this behavior would improve code clarity and prevent potential downstream issues.

Consider adding validation and documentation:

+        // Validate vanID presence for new beneficiary registrations
+        if (vanID == null) {
+            logger.warn("vanID not provided in request - proceeding with null value");
+        }
+        
         JSONObject localImportPayload = new JSONObject();
         localImportPayload.put("vanID", vanID);
         localImportPayload.put("benIDList", obj.get("data"));

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94e4ddb and e8f40d7.

📒 Files selected for processing (22)

• pom.xml (1 hunks)
• src/main/java/com/iemr/mmu/data/doctor/CancerGynecologicalExamination.java (0 hunks)
• src/main/java/com/iemr/mmu/data/ncdcare/NCDCareDiagnosis.java (4 hunks)
• src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java (1 hunks)
• src/main/java/com/iemr/mmu/repo/nurse/BenVisitDetailRepo.java (1 hunks)
• src/main/java/com/iemr/mmu/repo/nurse/ncdcare/NCDCareDiagnosisRepo.java (1 hunks)
• src/main/java/com/iemr/mmu/repo/quickConsultation/BenChiefComplaintRepo.java (1 hunks)
• src/main/java/com/iemr/mmu/repo/quickConsultation/PrescriptionDetailRepo.java (1 hunks)
• src/main/java/com/iemr/mmu/service/anc/ANCNurseServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/cancerScreening/CSDoctorServiceImpl.java (1 hunks)
• src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (1 hunks)
• src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java (1 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerTransactionalImpl.java (5 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncActivity/SyncResult.java (1 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncActivity/UploadDataToServerImpl.java (3 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/DataSyncRepositoryCentral.java (3 hunks)
• src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImpl.java (12 hunks)
• src/main/java/com/iemr/mmu/service/quickConsultation/QuickConsultationServiceImpl.java (3 hunks)
• src/main/resources/application.properties (2 hunks)

💤 Files with no reviewable changes (1)

• src/main/java/com/iemr/mmu/data/doctor/CancerGynecologicalExamination.java

🧰 Additional context used

🧬 Code graph analysis (2)

src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java (1)

src/main/java/com/iemr/mmu/utils/RestTemplateUtil.java (1)

• RestTemplateUtil (35-70)

src/main/java/com/iemr/mmu/service/dataSyncActivity/UploadDataToServerImpl.java (2)

src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java (1)

• Service (46-140)

src/main/java/com/iemr/mmu/utils/RestTemplateUtil.java (1)

• RestTemplateUtil (35-70)

🔇 Additional comments (3)

src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/DataSyncRepositoryCentral.java (1)

65-65: Table name correction verified: All instances updated to t_benchiefcomplaint with no residual references to t_benchefcomplaint.

src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java (2)

24-24: LGTM!

The new imports for BigInteger and HashMap are appropriate for the added functionality.

Also applies to: 27-27

---

351-354: Verify local import API contract update
I couldn’t locate the controller or service handling benImportUrlLocal; ensure the endpoint is updated to consume the new JSON payload (vanID and benIDList) and still supports the previous format for backward compatibility.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Parameterize the IN clause to avoid SQL injection & invalid SQL.

String.join(",", vanSerialNos) drops the values straight into the SQL text. Any non-numeric serial, quote, or crafted payload will either break the statement or allow SQL injection. An empty list also yields IN (), which the DB rejects. Build a placeholder list and pass the serials as bind variables instead.

-       if (tableName != null && tableName.toLowerCase().equals("i_ben_flow_outreach")) {
-           query = "UPDATE " + schemaName + "." + tableName
-                   + " SET created_date = ? , processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
-                   + "WHERE " + autoIncreamentColumn + " IN (" + String.join(",", vanSerialNos) + ")";
-       } else {
-           query = "UPDATE " + schemaName + "." + tableName
-                   + " SET CreatedDate = ? , processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
-                   + "WHERE " + autoIncreamentColumn + " IN (" + String.join(",", vanSerialNos) + ")";
-       }
-
-       Timestamp syncedDate = new Timestamp(System.currentTimeMillis());
-       int updatedRows = jdbcTemplate.update(query, syncedDate, status, syncedDate, user, reason);
+       if (vanSerialNos == null || vanSerialNos.isEmpty()) {
+           logger.warn("No vanSerialNos supplied for table {}", tableName);
+           return 0;
+       }
+       String placeholders = String.join(",", java.util.Collections.nCopies(vanSerialNos.size(), "?"));
+       if (tableName != null && tableName.equalsIgnoreCase("i_ben_flow_outreach")) {
+           query = "UPDATE " + schemaName + "." + tableName
+                   + " SET created_date = ? , processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
+                   + "WHERE " + autoIncreamentColumn + " IN (" + placeholders + ")";
+       } else {
+           query = "UPDATE " + schemaName + "." + tableName
+                   + " SET CreatedDate = ? , processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
+                   + "WHERE " + autoIncreamentColumn + " IN (" + placeholders + ")";
+       }
+
+       Timestamp syncedDate = new Timestamp(System.currentTimeMillis());
+       List<Object> params = new ArrayList<>(5 + vanSerialNos.size());
+       params.add(syncedDate);
+       params.add(status);
+       params.add(syncedDate);
+       params.add(user);
+       params.add(reason);
+       params.addAll(vanSerialNos);
+       int updatedRows = jdbcTemplate.update(query, params.toArray());

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java
around lines 108 to 120, the code builds the IN clause using String.join which
allows SQL injection, breaks on non-numeric values and fails for empty lists;
replace that by dynamically creating a comma-separated list of ? placeholders
matching vanSerialNos.size(), return early (0) or skip update if vanSerialNos is
empty to avoid IN (), and construct an Object[] (or List<Object>) of bind values
containing the vanSerialNos (converted to the expected type) appended to the
existing parameters (syncedDate, status, syncedDate, user, reason) in the
correct order, then call jdbcTemplate.update(queryWithPlaceholders, bindArgs) so
the serials are passed as bind variables instead of inlined.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add error handling for vanID extraction.

The getBigInteger method can throw JSONException or NumberFormatException if the vanID value is not a valid BigInteger format. This could cause the entire beneficiary registration flow to fail unexpectedly.

Apply this diff to add proper error handling:

-        JSONObject originalRequest = new JSONObject(requestOBJ);
-        BigInteger vanID = null;
-        if (originalRequest.has("vanID")) {
-            vanID = originalRequest.getBigInteger("vanID");
-        }
-        logger.info("Extracted vanID from original request: " + vanID);
+        JSONObject originalRequest = new JSONObject(requestOBJ);
+        BigInteger vanID = null;
+        try {
+            if (originalRequest.has("vanID")) {
+                vanID = originalRequest.getBigInteger("vanID");
+            }
+            logger.info("Extracted vanID from original request: " + vanID);
+        } catch (NumberFormatException | JSONException e) {
+            logger.error("Invalid vanID format in request: " + e.getMessage());
+            throw new Exception("Invalid vanID format: " + e.getMessage());
+        }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	JSONObject originalRequest = new JSONObject(requestOBJ);
	BigInteger vanID = null;
	if (originalRequest.has("vanID")) {
	vanID = originalRequest.getBigInteger("vanID");
	}
	logger.info("Extracted vanID from original request: " + vanID);
	JSONObject originalRequest = new JSONObject(requestOBJ);
	BigInteger vanID = null;
	try {
	if (originalRequest.has("vanID")) {
	vanID = originalRequest.getBigInteger("vanID");
	}
	logger.info("Extracted vanID from original request: " + vanID);
	} catch (NumberFormatException | JSONException e) {
	logger.error("Invalid vanID format in request: " + e.getMessage());
	throw new Exception("Invalid vanID format: " + e.getMessage());
	}

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java
around lines 330 to 335, the code calls originalRequest.getBigInteger("vanID")
without guarding against malformed input; wrap the extraction in a try-catch
that catches JSONException and NumberFormatException, validate the vanID value
(check originalRequest.has("vanID") and that the value is a numeric/string that
can be parsed), and on error log a clear message including the raw vanID value
and exception message, then set vanID to null and either short-circuit the flow
with an appropriate error response/return or handle the null case downstream to
avoid crashing the registration flow.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Remove or redact sensitive data from log statements.

Logging the entire request payload (line 340), response body (line 346), and local import payload (line 358) could expose Personally Identifiable Information (PII) such as beneficiary names, contact details, addresses, and health information. This violates privacy best practices and potentially GDPR/data protection regulations.

Apply this diff to log only non-sensitive metadata:

-        logger.info("request obj check="+requestOBJ);
+        logger.info("Calling central API to generate BenID for vanID: " + (originalRequest.has("vanID") ? originalRequest.get("vanID") : "N/A"));

-        logger.info("Response from benGenUrlCentral: " + response.getBody());
+        logger.info("Response received from benGenUrlCentral with status code: " + (obj != null && obj.has("statusCode") ? obj.getInt("statusCode") : "unknown"));

-        logger.info("Payload to local import: " + localImportPayload.toString());
+        logger.info("Calling local import API for vanID: " + vanID);

Also applies to: 346-346, 358-358

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Partial failures reported as full success

When any table ends up in a "partial" status the enclosing group inherits "partial", yet the final branch still emits "Data sync completed successfully" because we only check for "failed". That masks partial failures from callers. Please return a message that reflects partial outcomes (e.g., “completed with partial failures”) or treat "partial" groups as failures before building the final response.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Return the collected failure reasons

startDataSync expects syncResult.get("failureReasons"), but syncDataToServer never publishes that list in the returned map. As a result the caller always sees null and we lose per-table failure details. Please include the accumulated failureReasons (and any other metadata you rely on upstream) when building result.

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/dataSyncActivity/UploadDataToServerImpl.java
around lines 594 to 606, the returned result map currently only contains status,
successCount and failCount but does not include the accumulated failureReasons
(or other metadata expected by startDataSync). Add the failureReasons list to
the result map (e.g., result.put("failureReasons", failureReasons)) and ensure
you put an empty list when there are no failures so upstream never receives
null; also include any other metadata keys startDataSync reads (add them to
result similarly).

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Remove redundant logging.

The table name is already logged at line 117-118 as part of the method entry log. This additional log at line 134 is redundant and adds unnecessary log clutter.

Apply this diff:

         logger.debug("Constructed query: {}", queryBuilder.toString());
         params.add(vanSerialNo);
-        logger.info("Table name="+tableName.toLowerCase());
         if (List.of("t_patientissue", "t_physicalstockentry", "t_stockadjustment", "t_saitemmapping",

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	logger.info("Table name="+tableName.toLowerCase());
	logger.debug("Constructed query: {}", queryBuilder.toString());
	params.add(vanSerialNo);
	if (List.of("t_patientissue", "t_physicalstockentry", "t_stockadjustment", "t_saitemmapping",
	// … rest of the condition …

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/DataSyncRepositoryCentral.java
around line 134, remove the redundant logger.info("Table
name="+tableName.toLowerCase()); statement because the table name is already
logged at lines 117–118; delete this extra log line to reduce log clutter and
keep the method entry logging only.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Replace generic log messages with descriptive ones.

The log messages "If block" and "Else part" provide no meaningful context. Replace them with descriptive messages that explain what condition triggered the branch and what action is being taken.

Apply this diff:

         if (List.of("t_patientissue", "t_physicalstockentry", "t_stockadjustment", "t_saitemmapping",
                 "t_stocktransfer", "t_patientreturn", "t_facilityconsumption", "t_indent",
                 "t_indentorder", "t_indentissue", "t_itemstockentry", "t_itemstockexit")
                 .contains(tableName.toLowerCase()) && syncFacilityID > 0) {
-                    logger.info("If block");
+            logger.debug("Using SyncFacilityID filter for inventory table: {}", tableName);
             queryBuilder.append(" AND SyncFacilityID = ?");
             params.add(syncFacilityID);
         } else {
-            logger.info("Else part");
+            logger.debug("Using VanID filter for table: {}", tableName);
             queryBuilder.append(" AND VanID = ?");
             params.add(vanID);
         }

Also applies to: 143-143

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/DataSyncRepositoryCentral.java
around lines 139 and 143, replace the generic logger.info("If block") and
logger.info("Else part") calls with descriptive messages that state which
condition was true and what action will be taken; include relevant contextual
data (e.g., variable names/values used in the condition) and use a clear phrase
like "Condition X matched: performing Y" or "Condition X not met: performing Z"
so logs are actionable and informative.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Always returning success hides sync failures

We build syncSuccess / errorMessage, but the response body is hard-coded to statusCode = 200 and “Data sync completed” regardless of failure. That means callers get a success response even when the sync blew up, masking operational issues. Please drive the status/message off syncSuccess (and include errorMessage when false) so failures surface correctly.

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/dataSyncLayerCentral/GetDataFromVanAndSyncToDBImpl.java
around lines 166 to 173, the response is always hard-coded to statusCode 200 and
"Data sync completed" which hides failures; change the response to derive
statusCode and message from the syncSuccess boolean (use 200 and a success
message when true, and an appropriate error status like 500 and include the
errorMessage when false), ensure the responseMap includes both syncSuccess and
errorMessage fields (or sets errorMessage null/empty on success), log the full
response including errorMessage, and return the serialized map so callers
receive an accurate status and message reflecting failures.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (7)

src/main/java/com/iemr/mmu/service/labtechnician/LabTechnicianServiceImpl.java (7)

45-63: Consider constructor injection for required dependencies.

The shift to setter-based injection is functional, but constructor injection is generally preferred for required dependencies as it:

• Makes dependencies explicit and immutable
• Enables compile-time verification
• Simplifies testing

Apply this diff to use constructor injection:

-    private V_benLabTestOrderedDetailsRepo v_benLabTestOrderedDetailsRepo;
-    private LabResultEntryRepo labResultEntryRepo;
-    private CommonBenStatusFlowServiceImpl commonBenStatusFlowServiceImpl;
-
-    @Autowired
-    public void setCommonBenStatusFlowServiceImpl(CommonBenStatusFlowServiceImpl commonBenStatusFlowServiceImpl) {
-        this.commonBenStatusFlowServiceImpl = commonBenStatusFlowServiceImpl;
-    }
-
-    @Autowired
-    public void setLabResultEntryRepo(LabResultEntryRepo labResultEntryRepo) {
-        this.labResultEntryRepo = labResultEntryRepo;
-    }
-
-    @Autowired
-    public void setV_benLabTestOrderedDetailsRepo(V_benLabTestOrderedDetailsRepo v_benLabTestOrderedDetailsRepo) {
-        this.v_benLabTestOrderedDetailsRepo = v_benLabTestOrderedDetailsRepo;
-    }
+    private final V_benLabTestOrderedDetailsRepo v_benLabTestOrderedDetailsRepo;
+    private final LabResultEntryRepo labResultEntryRepo;
+    private final CommonBenStatusFlowServiceImpl commonBenStatusFlowServiceImpl;
+
+    @Autowired
+    public LabTechnicianServiceImpl(
+            V_benLabTestOrderedDetailsRepo v_benLabTestOrderedDetailsRepo,
+            LabResultEntryRepo labResultEntryRepo,
+            CommonBenStatusFlowServiceImpl commonBenStatusFlowServiceImpl) {
+        this.v_benLabTestOrderedDetailsRepo = v_benLabTestOrderedDetailsRepo;
+        this.labResultEntryRepo = labResultEntryRepo;
+        this.commonBenStatusFlowServiceImpl = commonBenStatusFlowServiceImpl;
+    }

---

136-139: Remove commented-out code.

Commented-out code adds clutter and should be removed. Version control preserves the history if needed.

Apply this diff:

-                    // System.out.println(procDetails.get("procedureID"));
-//					if (procDetails.get("procedureID") instanceof Integer
-//							&& obj.getProcedureID() == procDetails.get("procedureID")) {
-//						//System.out.println("hello");
-//					}

---

100-244: Simplify by extracting helper methods.

This method is 145 lines with deeply nested conditionals, making it difficult to test and maintain. Consider extracting helper methods for:

• Building procedure details
• Building component details
• Building component options

Example structure:

private ArrayList<Object> getPrescribedLabTestInJsonFormatlaboratory(
        ArrayList<V_benLabTestOrderedDetails> orderedLabTestList) {
    ArrayList<Object> returnOBJ = new ArrayList<>();
    
    if (orderedLabTestList == null || orderedLabTestList.isEmpty()) {
        return returnOBJ;
    }
    
    Map<String, Object> procDetails = null;
    ArrayList<Object> compList = null;
    
    for (V_benLabTestOrderedDetails obj : orderedLabTestList) {
        if (isNewProcedure(procDetails, obj)) {
            procDetails = buildProcedureDetails(obj);
            compList = new ArrayList<>();
            procDetails.put("compListDetails", compList);
            returnOBJ.add(procDetails);
        }
        
        addComponentToList(compList, obj);
    }
    
    return returnOBJ;
}

private boolean isNewProcedure(Map<String, Object> procDetails, V_benLabTestOrderedDetails obj) {
    return procDetails == null || 
           (procDetails.containsKey("procedureID") && 
            !obj.getProcedureID().equals(procDetails.get("procedureID")));
}

private Map<String, Object> buildProcedureDetails(V_benLabTestOrderedDetails obj) {
    // Extract procedure detail building logic
}

private void addComponentToList(ArrayList<Object> compList, V_benLabTestOrderedDetails obj) {
    // Extract component building logic
}

---

501-517: Align closing braces for the outer if-else block.

Lines 512–515 are indented too deeply, which obscures the matching opening brace at line 501. Realign these closing braces underneath their corresponding if to improve readability.

---

100-244: Consider refactoring for reduced complexity.

The getPrescribedLabTestInJsonFormatlaboratory method has deep nesting with multiple conditional branches and variable reassignments. This increases cognitive complexity and makes maintenance more difficult.

Consider extracting helper methods for:

• Building procedure details (lines 115-135)
• Building component details (lines 142-169, 171-198, 207-232)
• Handling component options (lines 159-166, 188-194, 224-230)

Example refactor structure:

private Map<String, Object> buildProcedureDetails(V_benLabTestOrderedDetails obj) {
    Map<String, Object> procDetails = new HashMap<>();
    procDetails.put("procedureID", obj.getProcedureID());
    procDetails.put("procedureName", obj.getProcedureName());
    // ... other fields
    return procDetails;
}

private Map<String, Object> buildComponentDetails(V_benLabTestOrderedDetails obj) {
    Map<String, Object> compDetails = new HashMap<>();
    compDetails.put("testComponentID", obj.getTestComponentID());
    // ... other fields
    return compDetails;
}

---

136-139: Remove commented code.

The commented code block should be removed to improve code clarity. If this logic might be needed in the future, rely on version control history instead.

Apply this diff:

-//					if (procDetails.get("procedureID") instanceof Integer
-//							&& obj.getProcedureID() == procDetails.get("procedureID")) {
-//						//System.out.println("hello");
-//					}

---

287-334: Remove large commented code block.

This 47-line commented block should be removed. If this represents an older implementation that might be referenced, commit it separately before removal or document the changes in commit messages.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8f40d7 and b1e6a4a.

📒 Files selected for processing (4)

• src/main/java/com/iemr/mmu/repo/labModule/LabResultEntryRepo.java (2 hunks)
• src/main/java/com/iemr/mmu/repo/quickConsultation/LabTestOrderDetailRepo.java (1 hunks)
• src/main/java/com/iemr/mmu/service/anc/ANCNurseServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/labtechnician/LabTechnicianServiceImpl.java (3 hunks)

🔇 Additional comments (7)

src/main/java/com/iemr/mmu/repo/quickConsultation/LabTestOrderDetailRepo.java (1)

50-53: Accept vanSerialNo update as intended. updateVanSerialNo mirrors labTestOrderID per the established data-sync convention across entities; no changes required.

src/main/java/com/iemr/mmu/service/labtechnician/LabTechnicianServiceImpl.java (5)

505-509: Verify updateVanSerialNo success and consider logging failures.

The code calls updateVanSerialNo for each saved result but doesn't check the return value. If an update fails (returns 0), it continues silently.

Consider adding verification:

                 if (labResultEntryRes != null && labResultsListNew.size() == labResultEntryRes.size()) {
                     for (LabResultEntry result : labResultEntryRes) {
                         if (result.getID() != null) {
-                            labResultEntryRepo.updateVanSerialNo(result.getID());
+                            int updated = labResultEntryRepo.updateVanSerialNo(result.getID());
+                            if (updated == 0) {
+                                // Log warning or throw exception
+                                logger.warn("Failed to update vanSerialNo for LabResultEntry ID: {}", result.getID());
+                            }
                         }
                     }

Additionally, for large result sets, consider a batch update approach to improve performance.

---

521-530: LGTM!

The new utility methods getLast_3_ArchivedTestVisitList and getLabResultForVisitcode are clean, focused, and follow the established pattern of fetching data from the repository and converting to JSON.

---

46-63: LGTM! Setter-based dependency injection.

The migration from field injection to setter-based injection is appropriate and follows Spring best practices. All dependencies are properly wired.

---

505-509: LGTM! Post-persistence vanSerialNo update.

The loop correctly calls updateVanSerialNo for each saved entity, following the established pattern for populating vanSerialNo after persistence.

---

521-525: LGTM! New archived visits method.

The getLast_3_ArchivedTestVisitList method correctly fetches and formats the last 3 archived visits for a beneficiary.

src/main/java/com/iemr/mmu/repo/labModule/LabResultEntryRepo.java (1)

27-27: LGTM! Post-persistence vanSerialNo update pattern.

The new updateVanSerialNo method follows the established pattern for initializing vanSerialNo after entity persistence. The imports, annotations, and query structure are correct.

Also applies to: 35-35, 52-55

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

LGTM! Verify return value handling in service layer.

The updateVanSerialNo method correctly implements the post-persistence pattern for populating vanSerialNo with the entity ID. The JPQL query syntax and annotations are appropriate.

However, ensure that calling code in the service layer checks the return value to confirm successful updates, especially since this operation occurs after the initial save.

Run the following script to verify that callers handle the return value:

---

🏁 Script executed:

#!/bin/bash
# Description: Check if callers of updateVanSerialNo verify the return value

# Search for calls to updateVanSerialNo
rg -nP -A 2 'updateVanSerialNo\s*\(' --type=java

Length of output: 4394

---

Handle updateVanSerialNo return value All service-layer calls to updateVanSerialNo ignore the returned row count; capture the int result and verify it’s >0, handling failures (e.g., throw or log) as appropriate.

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/repo/labModule/LabResultEntryRepo.java around
lines 52 to 55, service-layer callers currently ignore the int return value from
updateVanSerialNo; update those service methods to capture the returned row
count, verify it is > 0, and handle failures by either logging an error and/or
throwing a meaningful exception (e.g., EntityNotUpdatedException) when the
result is 0 or negative; ensure the service method signatures and callers
propagate or handle the exception appropriately and add unit tests to cover the
failure path.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Van serial never persisted to the database

ancCareRepo.save(...) runs in its own transactional boundary. Once it returns, the entity is detached; simply calling setVanSerialNo(...) on the returned object will not flush the update. As written, vanSerialNo stays NULL in the table even though callers receive the generated ID. Persist the serial explicitly—e.g. issue a second save after assigning it (or delegate to a repository update method) before returning.

Apply this diff to ensure the update is flushed:

-        if (ancCareDetailsRS != null) {
-                ancCareDetailsRS.setVanSerialNo((ancCareDetailsRS.getID()));
-                ancCareSuccessFlag = ancCareDetailsRS.getID();
-        }
+        if (ancCareDetailsRS != null) {
+                Long generatedId = ancCareDetailsRS.getID();
+                if (generatedId != null) {
+                        ancCareDetailsRS.setVanSerialNo(generatedId);
+                        ancCareRepo.save(ancCareDetailsRS);
+                        ancCareSuccessFlag = generatedId;
+                }
+        }

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/service/anc/ANCNurseServiceImpl.java around lines
165 to 169, the code sets vanSerialNo on the returned entity after
ancCareRepo.save(...) but the entity is detached so the update is never flushed;
modify the method to persist the vanSerialNo explicitly by setting
ancCareDetailsRS.setVanSerialNo(ancCareDetailsRS.getID()) and then calling
ancCareRepo.save(ancCareDetailsRS) (or call a repository update method/execute
an update query) so the vanSerialNo is written to the database before returning.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Same persistence gap for immunization records

After saveAll, the returned entities are no longer managed once the repository call completes. Updating vanSerialNo on the first element therefore never hits the database, leaving the column null. Persist the assignment explicitly (e.g. save the mutated entity again or call a repo-level update) before you return the ID.

Apply this diff to flush the change:

-        if (!ancWomenVaccineDetailRSList.isEmpty()) {
-                ancWomenVaccineDetailRSList.get(0).setVanSerialNo(ancWomenVaccineDetailRSList.get(0).getID());
-                successFlag = ancWomenVaccineDetailRSList.get(0).getID();
-        }
+        if (!ancWomenVaccineDetailRSList.isEmpty()) {
+                ANCWomenVaccineDetail persistedDetail = ancWomenVaccineDetailRSList.get(0);
+                Long generatedId = persistedDetail.getID();
+                if (generatedId != null) {
+                        persistedDetail.setVanSerialNo(generatedId);
+                        ancWomenVaccineRepo.save(persistedDetail);
+                        successFlag = generatedId;
+                }
+        }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	List<ANCWomenVaccineDetail> ancWomenVaccineDetailRSList = (List<ANCWomenVaccineDetail>) ancWomenVaccineRepo
	.saveAll(ancWomenVaccineDetailList);
	if (!ancWomenVaccineDetailRSList.isEmpty()) {
	ancWomenVaccineDetailRSList.get(0).setVanSerialNo(ancWomenVaccineDetailRSList.get(0).getID());
	successFlag = ancWomenVaccineDetailRSList.get(0).getID();
	}
	List<ANCWomenVaccineDetail> ancWomenVaccineDetailRSList = (List<ANCWomenVaccineDetail>) ancWomenVaccineRepo
	.saveAll(ancWomenVaccineDetailList);
	if (!ancWomenVaccineDetailRSList.isEmpty()) {
	ANCWomenVaccineDetail persistedDetail = ancWomenVaccineDetailRSList.get(0);
	Long generatedId = persistedDetail.getID();
	if (generatedId != null) {
	persistedDetail.setVanSerialNo(generatedId);
	ancWomenVaccineRepo.save(persistedDetail);
	successFlag = generatedId;
	}
	}

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/service/anc/ANCNurseServiceImpl.java around lines
178 to 183, after calling saveAll the code mutates
ancWomenVaccineDetailRSList.get(0).setVanSerialNo(...), but that change is not
persisted; update the code to explicitly persist the mutated entity before
returning the ID (for example: retrieve the saved entity reference,
setVanSerialNo on it, then call ancWomenVaccineRepo.save(theEntity) or invoke a
repo-level update method that sets vanSerialNo by ID), and then set successFlag
to the returned/saved entity ID.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify return value consistency.

The method returns null when the condition on line 403 is false (i.e., when both labResultsList and radiologyTestResults are empty or null). However, the public saveLabTestResult(JsonObject) method at line 336 returns 1 in similar scenarios (line 355).

Ensure this behavior is intentional and consistent with the expected contract.

Run the following script to check how callers handle null returns:

---

🏁 Script executed:

#!/bin/bash
# Description: Find callers of saveLabTestResult and check null handling

# Search for method calls
rg -n -A 5 -B 2 'saveLabTestResult\s*\(' --type java -g '!**/LabTechnicianServiceImpl.java'

Length of output: 148

---

🏁 Script executed:

#!/bin/bash
# Inspect both overloads of saveLabTestResult to compare return behavior
sed -n '330,380p' src/main/java/com/iemr/mmu/service/labtechnician/LabTechnicianServiceImpl.java

Length of output: 2215

---

🏁 Script executed:

#!/bin/bash
# Find all references to saveLabTestResult across codebase
rg -n '\.saveLabTestResult\s*\(' -g '*.java'

Length of output: 166

---

Standardize return value on no-op in wrapper method
saveLabTestResult(WrapperLabResultEntry) currently returns null when both result lists are empty, while the JsonObject overload returns 1; change the wrapper overload to return 1 in its no-op path to avoid nulls and align behavior.

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/labtechnician/LabTechnicianServiceImpl.java
around lines 397 to 517, the wrapper overload
saveLabTestResult(WrapperLabResultEntry) can return null when both lab and
radiology lists are empty; change the no-op path to return 1 to match the
JsonObject overload by initializing or assigning labResultSaveFlag = 1 before
the final return (or explicitly return 1) in the branch where no entries are
processed so the method never returns null.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (1)

2087-2095: Apply the same batch update optimization here.

This code has the same N+1 query problem as the saveBenChiefComplaints method (lines 439-446). Both methods should use batch updates for consistency and performance.

Apply the same batch update pattern suggested for lines 439-446:

 
 if (!benChiefComplaintResultList.isEmpty()) {
-    for (BenChiefComplaint complaint : benChiefComplaintResultList) {
-        if (complaint.getBenChiefComplaintID() != null) {
-            benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
-        }
-    }
+    List<Long> ids = benChiefComplaintResultList.stream()
+        .map(BenChiefComplaint::getBenChiefComplaintID)
+        .filter(Objects::nonNull)
+        .collect(Collectors.toList());
+    if (!ids.isEmpty()) {
+        int updated = benChiefComplaintRepo.updateVanSerialNoBatch(ids);
+        if (updated != ids.size()) {
+            throw new IEMRException("Failed to update vanSerialNo for some complaints");
+        }
+    }
     r = benChiefComplaintResultList.size();
 }

🧹 Nitpick comments (2)

src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (2)

439-446: Consider batch update to avoid N+1 query problem.

The code iterates through saved complaints and calls updateVanSerialNo for each one individually, resulting in N database calls. This is inefficient and lacks error handling.

Consider implementing a batch update method in the repository:

 if (benChiefComplaintListNew.size() == benChiefComplaintResultList.size())
 {
-    for (BenChiefComplaint complaint : benChiefComplaintResultList) {
-        if (complaint.getBenChiefComplaintID() != null) {
-            benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
-        }
-    }
+    List<Long> ids = benChiefComplaintResultList.stream()
+        .map(BenChiefComplaint::getBenChiefComplaintID)
+        .filter(Objects::nonNull)
+        .collect(Collectors.toList());
+    if (!ids.isEmpty()) {
+        int updated = benChiefComplaintRepo.updateVanSerialNoBatch(ids);
+        if (updated != ids.size()) {
+            throw new IEMRException("Failed to update vanSerialNo for some complaints");
+        }
+    }
     r = 1;
 }

Then add a batch update method to the repository:

@Modifying
@Query("UPDATE BenChiefComplaint b SET b.vanSerialNo = b.benChiefComplaintID WHERE b.benChiefComplaintID IN :ids")
int updateVanSerialNoBatch(@Param("ids") List<Long> ids);

---

2831-3016: Consider refactoring deeply nested frequency logic for maintainability.

The expanded frequency handling adds many string variants (e.g., "Once Daily(OD) Before Food", "Once Daily(OD) After Food"), but the deeply nested if-else structure (6+ levels) and duplicated dose calculation logic make this method difficult to maintain and extend.

Consider extracting the dose calculation logic and using a more maintainable approach:

private double getQtyForOneDay(String form, String dose, String frequency) {
    if (form == null || dose == null || frequency == null) {
        return 0;
    }
    
    // Normalize frequency by removing food-timing suffixes
    String normalizedFreq = normalizeFrequency(frequency);
    int frequencyMultiplier = getFrequencyMultiplier(normalizedFreq);
    
    if (frequencyMultiplier == 0) {
        return 0;
    }
    
    return calculateDosage(form, dose) * frequencyMultiplier;
}

private String normalizeFrequency(String frequency) {
    // Remove food timing and other suffixes
    return frequency.replaceAll("(?i)\\s*(Before Food|After Food|At Bedtime|BF|AF)$", "").trim();
}

private int getFrequencyMultiplier(String frequency) {
    if (frequency.equalsIgnoreCase("Once Daily(OD)")) return 1;
    if (frequency.equalsIgnoreCase("Twice Daily(BD)")) return 2;
    if (frequency.equalsIgnoreCase("Thrice Daily (TID)")) return 3;
    if (frequency.equalsIgnoreCase("Four Times in a Day (QID)")) return 4;
    if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")) return 1;
    if (frequency.equalsIgnoreCase("Once in a Week")) return 1; // Special handling for weekly
    if (frequency.equalsIgnoreCase("SOS")) return 1;
    return 0;
}

private double calculateDosage(String form, String dose) {
    if (form.equalsIgnoreCase("Capsule")) {
        return 1.0;
    }
    
    if (form.equalsIgnoreCase("Tablet")) {
        if (dose.equalsIgnoreCase("Half Tab")) return 0.5;
        if (dose.equalsIgnoreCase("One Tab")) return 1.0;
        if (dose.equalsIgnoreCase("One & Half Tab")) return 1.5;
        if (dose.equalsIgnoreCase("Two Tabs")) return 2.0;
    }
    
    return 0;
}

This refactoring:

• Reduces nesting from 6+ levels to 1-2 levels
• Eliminates code duplication
• Makes it easier to add new frequencies or doses
• Improves testability by separating concerns

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1e6a4a and 12e1ddd.

📒 Files selected for processing (1)

• src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (10 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build_and_check_coverage

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Ensure updateVanSerialNo is in the same transaction and add error handling.

The updateVanSerialNo call occurs after the initial save, but there's no error handling. If this call fails, the method will still return a successful benVisitID, leaving the vanSerialNo field in an inconsistent state.

Consider wrapping both operations in a transaction and adding error handling:

 response = benVisitDetailRepo.save(beneficiaryVisitDetail);
-benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
+if (response != null && response.getBenVisitID() != null) {
+    int updated = benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
+    if (updated == 0) {
+        throw new IEMRException("Failed to update vanSerialNo for visit: " + response.getBenVisitID());
+    }
+}

Additionally, verify that this method is annotated with @Transactional to ensure atomicity:

---

🏁 Script executed:

#!/bin/bash
# Check if saveBeneficiaryVisitDetails method has @Transactional annotation
rg -n -A 3 -B 3 "saveBeneficiaryVisitDetails.*BeneficiaryVisitDetail" --type java

Length of output: 1648

---

Annotate saveBeneficiaryVisitDetails with @transactional and handle updateVanSerialNo failures

Add @Transactional at the class or method level in CommonNurseServiceImpl.java to ensure atomicity, and verify the result of updateVanSerialNo, throwing IEMRException if it returns 0:

+ @Transactional
  @Override
  public Long saveBeneficiaryVisitDetails(BeneficiaryVisitDetail beneficiaryVisitDetail) {
      BeneficiaryVisitDetail response = benVisitDetailRepo.save(beneficiaryVisitDetail);
-     benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
+     if (response != null && response.getBenVisitID() != null) {
+         int updated = benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
+         if (updated == 0) {
+             throw new IEMRException("Failed to update vanSerialNo for visit: " + response.getBenVisitID());
+         }
+     }
      return response.getBenVisitID();
  }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	response = benVisitDetailRepo.save(beneficiaryVisitDetail);
	benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
	@Transactional
	@Override
	public Long saveBeneficiaryVisitDetails(BeneficiaryVisitDetail beneficiaryVisitDetail) {
	BeneficiaryVisitDetail response = benVisitDetailRepo.save(beneficiaryVisitDetail);
	if (response != null && response.getBenVisitID() != null) {
	int updated = benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
	if (updated == 0) {
	throw new IEMRException("Failed to update vanSerialNo for visit: " + response.getBenVisitID());
	}
	}
	return response.getBenVisitID();
	}

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java
around lines 261 to 262, the code saves beneficiaryVisitDetail and then calls
benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID()) without
transactional protection or result checking; annotate the method (or the class)
with @Transactional and after calling updateVanSerialNo check its return value,
and if it returns 0 throw an IEMRException so the transaction rolls back and
failure is propagated.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add error handling for updateVanSerialNo and remove blank line.

Similar to line 262, this updateVanSerialNo call lacks error handling. If it fails, the method continues as if everything succeeded.

Apply this diff:

 PrescriptionDetail prescriptionRS = prescriptionDetailRepo.save(prescription);
-prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID());
-
 if (prescriptionRS != null && prescriptionRS.getPrescriptionID() > 0) {
+    int updated = prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID());
+    if (updated == 0) {
+        throw new IEMRException("Failed to update vanSerialNo for prescription: " + prescriptionRS.getPrescriptionID());
+    }
     r = prescriptionRS.getPrescriptionID();
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	PrescriptionDetail prescriptionRS = prescriptionDetailRepo.save(prescription);
	prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID());
	PrescriptionDetail prescriptionRS = prescriptionDetailRepo.save(prescription);
	if (prescriptionRS != null && prescriptionRS.getPrescriptionID() > 0) {
	int updated = prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID());
	if (updated == 0) {
	throw new IEMRException("Failed to update vanSerialNo for prescription: " + prescriptionRS.getPrescriptionID());
	}
	r = prescriptionRS.getPrescriptionID();
	}

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java
around lines 2701 to 2703, the call to
prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID())
lacks error handling and there is an extra blank line; wrap the
updateVanSerialNo invocation in appropriate error handling (e.g., check its
return status or wrap in try-catch), log the failure with context and the
prescription ID using the existing logger, and rethrow or return an
error/rollback consistent with the surrounding method's error-handling pattern;
also remove the blank line following the call so the code matches style.

[coderabbitai]

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 (5)

src/main/java/com/iemr/mmu/service/generalOPD/GeneralOPDServiceImpl.java (2)

471-474: Logic bug: unconditional reset overrides actual save result

childFeedingSuccessFlag and perinatalHistorySuccessFlag are set to 1 in standalone blocks, overwriting prior success/failure. These should be else branches of the preceding ifs.

Apply:

-        }
-        {
-            childFeedingSuccessFlag = new Long(1);
-        }
+        } else {
+            childFeedingSuccessFlag = Long.valueOf(1L);
+        }
...
-        }
-        {
-            perinatalHistorySuccessFlag = new Long(1);
-        }
+        } else {
+            perinatalHistorySuccessFlag = Long.valueOf(1L);
+        }

Also applies to: 488-491

---

886-891: Null-safety around investigation wrapper

wrapperBenInvestigationANC may be null when "investigation" is missing; calling getExternalInvestigations() will NPE.

Use a guard:

- WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson()
-     .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class);
- // Save Prescription
- prescriptionDetail.setExternalInvestigation(wrapperBenInvestigationANC.getExternalInvestigations());
+ WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson()
+     .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class);
+ // Save Prescription
+ if (wrapperBenInvestigationANC != null) {
+     prescriptionDetail.setExternalInvestigation(wrapperBenInvestigationANC.getExternalInvestigations());
+ }

src/main/java/com/iemr/mmu/service/covid19/Covid19ServiceImpl.java (2)

507-510: Logic bug: unconditional reset overrides actual save result

childFeedingSuccessFlag and perinatalHistorySuccessFlag are always set to 1 due to free-standing blocks. Make them else branches.

Apply:

-        }
-        {
-            childFeedingSuccessFlag = new Long(1);
-        }
+        } else {
+            childFeedingSuccessFlag = Long.valueOf(1L);
+        }
...
-        }
-        {
-            perinatalHistorySuccessFlag = new Long(1);
-        }
+        } else {
+            perinatalHistorySuccessFlag = Long.valueOf(1L);
+        }

Also applies to: 525-527

---

1209-1217: Null-safety: investigation wrapper may be null

Guard before dereference to avoid NPE when “investigation” is missing.

- WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson()
-     .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class);
- prescriptionID = commonNurseServiceImpl.savePrescriptionDetailsCovid19(
-     wrapperBenInvestigationANC.getBeneficiaryRegID(), ...
+ WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson()
+     .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class);
+ if (wrapperBenInvestigationANC == null) {
+     throw new RuntimeException("Missing investigation payload required for prescription generation");
+ }
+ prescriptionID = commonNurseServiceImpl.savePrescriptionDetailsCovid19(
+     wrapperBenInvestigationANC.getBeneficiaryRegID(), ...

src/main/java/com/iemr/mmu/service/pnc/PNCServiceImpl.java (1)

328-336: Null-safety: guard investigation wrapper before use

If “investigation” is absent/null, wrapperBenInvestigationANC will be null; subsequent use leads to NPE.

- WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson()
-     .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class);
- prescriptionID = commonNurseServiceImpl.savePrescriptionDetailsAndGetPrescriptionID(
-     wrapperBenInvestigationANC.getBeneficiaryRegID(), ...
+ WrapperBenInvestigationANC wrapperBenInvestigationANC = InputMapper.gson()
+     .fromJson(requestOBJ.get("investigation"), WrapperBenInvestigationANC.class);
+ if (wrapperBenInvestigationANC == null) {
+     throw new RuntimeException("Missing investigation payload required for prescription generation");
+ }
+ prescriptionID = commonNurseServiceImpl.savePrescriptionDetailsAndGetPrescriptionID(
+     wrapperBenInvestigationANC.getBeneficiaryRegID(), ...

🧹 Nitpick comments (5)

src/main/java/com/iemr/mmu/service/cancerScreening/CSServiceImpl.java (1)

729-733: Reading doctorSignatureFlag: consistent defaulting, document API

Defaulting to false is good. Ensure clients send doctorSignatureFlag at the root consistently across flows and update the API contract.

Consider a small helper to extract this flag to avoid repetition across services.

src/main/java/com/iemr/mmu/service/ncdCare/NCDCareServiceImpl.java (1)

874-876: Pass-through to CommonDoctorServiceImpl

Call updated correctly. Consider centralizing flag extraction to reduce duplication across services.

src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (1)

75-76: Non-persisted vanSerialNo assignment

obj.setVanSerialNo(objRS.getBenFlowID()) is set on the transient obj and never saved. It has no effect and confuses readers.

Remove or persist explicitly with an update if needed.

Also applies to: 95-96

src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (1)

217-221: Post-persist vanSerialNo updates LGTM; consider batching

Per-row updates are fine for small lists; if large, consider a repo batch method to reduce round trips.

src/main/java/com/iemr/mmu/service/quickConsultation/QuickConsultationServiceImpl.java (1)

178-185: vanSerialNo updates after save LGTM; optional bulk optimization

Correctness is fine. If lists can be big, consider a repo method to update many IDs in one statement.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12e1ddd and f83cd43.

📒 Files selected for processing (12)

• src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java (3 hunks)
• src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java (3 hunks)
• src/main/java/com/iemr/mmu/service/anc/ANCServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/cancerScreening/CSServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (3 hunks)
• src/main/java/com/iemr/mmu/service/covid19/Covid19ServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/generalOPD/GeneralOPDServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/ncdCare/NCDCareServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/ncdscreening/NCDScreeningServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/pnc/PNCServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/quickConsultation/QuickConsultationServiceImpl.java (5 hunks)

🧰 Additional context used

🪛 GitHub Actions: TestCase Coverage Check

src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java

[error] 406-406: method updateBenFlowStatusAfterDoctorActivity in interface com.iemr.mmu.repo.benFlowStatus.BeneficiaryFlowStatusRepo cannot be applied to given types; required: java.lang.Long,java.lang.Long,java.lang.Long,java.lang.Short,java.lang.Short,java.lang.Short,java.lang.Short,int,java.sql.Timestamp,java.lang.Boolean; found: java.lang.Long,java.lang.Long,java.lang.Long,short,short,short,short,int,java.sql.Timestamp reason: actual and formal argument lists differ in length

---

[error] 411-411: method updateBenFlowAfterDoctorActivity in interface com.iemr.mmu.repo.benFlowStatus.BeneficiaryFlowStatusRepo cannot be applied to given types; required: java.lang.Long,java.lang.Long,java.lang.Long,java.lang.Short,java.lang.Short,java.lang.Short,java.lang.Short,int,java.sql.Timestamp,java.lang.Boolean; found: java.lang.Long,java.lang.Long,java.lang.Long,short,short,short,short,int,java.sql.Timestamp reason: actual and formal argument lists differ in length

---

[error] 438-438: method updateBenFlowStatusAfterDoctorActivity in interface com.iemr.mmu.repo.benFlowStatus.BeneficiaryFlowStatusRepo cannot be applied to given types; required: java.lang.Long,java.lang.Long,java.lang.Long,java.lang.Short,java.lang.Short,java.lang.Short,java.lang.Short,int,java.sql.Timestamp,java.lang.Boolean; found: java.lang.Long,java.lang.Long,java.lang.Long,short,short,short,short,int,java.sql.Timestamp reason: actual and formal argument lists differ in length

🔇 Additional comments (19)

src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java (1)

982-990: Accessor methods LGTM

Getters/setters look correct and match the field type.

src/main/java/com/iemr/mmu/service/anc/ANCServiceImpl.java (2)

267-273: doctorSignatureFlag ingestion LGTM

Root-level extraction with safe default. Keep consistent across services.

---

421-423: The review comment is incorrect.

The codebase intentionally maintains two separate methods with different signatures: updateBenFlowtableAfterDocDataSave() (accepts doctorSignatureFlag) and updateBenFlowtableAfterDocDataUpdate() (does not). The code at ANCServiceImpl lines 421-423 correctly calls the Save variant, which already receives doctorSignatureFlag. The Update variant across all services consistently omits this parameter, indicating the distinction is intentional—initial saves support doctor signatures, while subsequent updates do not.

Likely an incorrect or invalid review comment.

src/main/java/com/iemr/mmu/service/ncdscreening/NCDScreeningServiceImpl.java (2)

1172-1175: doctorSignatureFlag: consistent parsing

Default false + optional override is fine.

---

1353-1355: Method overload verified and consistently used across codebase

The 5-parameter updateBenFlowtableAfterDocDataSave method exists in CommonDoctorServiceImpl and is called uniformly across all seven service implementations (GeneralOPDServiceImpl, QuickConsultationServiceImpl, PNCServiceImpl, NCDScreeningServiceImpl, NCDCareServiceImpl, ANCServiceImpl, Covid19ServiceImpl). No old variants are in use.

src/main/java/com/iemr/mmu/service/ncdCare/NCDCareServiceImpl.java (1)

688-691: doctorSignatureFlag extraction LGTM

Same pattern as others; OK.

src/main/java/com/iemr/mmu/service/cancerScreening/CSServiceImpl.java (1)

819-821: Signature propagation verified across all call sites

All invocations of updateBenFlowAfterDocData (2 call sites) and updateBenFlowtableAfterDocDataSave (7 call sites) pass the doctorSignatureFlag parameter correctly as the final argument, matching their method signatures. The code at lines 819–821 in CSServiceImpl.java is consistent with this pattern.

src/main/java/com/iemr/mmu/service/generalOPD/GeneralOPDServiceImpl.java (2)

787-790: doctorSignatureFlag extraction LGTM

Defaulting to false and honoring provided value is correct.

---

946-947: Propagating doctorSignatureFlag to flow update

Good pass-through to the updated CommonDoctorServiceImpl API. Ensure all other save flows do the same (you already did for Covid/PNC/QC).

src/main/java/com/iemr/mmu/service/covid19/Covid19ServiceImpl.java (1)

1152-1155: doctorSignatureFlag: extraction and propagation LGTM

Flag handled consistently and passed to repository via service layers.

Also applies to: 1264-1266

src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (2)

264-279: Extended API and flow update LGTM

New signature with signatureFlag and adjusted pharma flag derivation look correct; exception rethrow is good.

---

294-296: Update path supplies default signatureFlag=false

Good compatibility choice for “update” flows; aligns with repo’s new arity.

src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (1)

767-771: New doctorSignatureFlag parameter wired through correctly

Signature change and downstream call include the flag; matches repo API.

Also applies to: 811-814

src/main/java/com/iemr/mmu/service/quickConsultation/QuickConsultationServiceImpl.java (2)

34-36: Logger addition LGTM

SLF4J logger wired; fine.

Also applies to: 89-90

---

360-364: doctorSignatureFlag wired through in QC save flow

Extraction and propagation look correct.

Also applies to: 495-497

src/main/java/com/iemr/mmu/service/pnc/PNCServiceImpl.java (1)

240-243: doctorSignatureFlag extraction and propagation LGTM

Consistent with other services.

Also applies to: 389-391

src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java (3)

59-61: Setting vanSerialNo during nurse update LGTM

Uses existing :benFlowID param; no extra args required.

---

76-82: Left-panel projection extended with doctorSignatureFlag

Select list updated consistently.

---

156-169: All call sites updated correctly; no stale invocations found

Both call sites in CommonBenStatusFlowServiceImpl.java (lines 272–273 and 294–295) have been updated to pass the new signatureFlag parameter. No stale calls remain.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Persisting doctorSignatureFlag: ensure schema + nullability are aligned

Looks fine. If nulls aren’t expected, mark the column non-nullable and confirm the migration adds it with a safe default.

Please verify:

• A DB migration adds i_ben_flow_outreach.doctor_signature_flag with a default false and without breaking existing rows.
• Any native/JPQL projections include this column consistently.

---

🏁 Script executed:

#!/bin/bash
# Find migrations and projections for doctor_signature_flag
rg -n "doctor_signature_flag" -g "!**/target/**"
rg -nP "getBenDetailsForLeftSidePanel|BeneficiaryFlowStatusRepo" -C3

Length of output: 35304

---

#!/bin/bash
# Get the full @Query definition for getBenDetailsForLeftSidePanel
sed -n '75,84p' src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java

# Find the static mapper method that converts Object[] to BeneficiaryFlowStatus
rg -nA15 "getBeneficiaryFlowStatusForLeftPanel" src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java

# Search for migration files
fd -e sql -e java | xargs rg -l "doctor_signature_flag" | grep -iE "(migration|flyway|liquibase|schema)" || echo "No migration files found"

---

</parameter>
</invoke>

---

🏁 Script executed:

```shell
#!/bin/bash
# 1. Get full @Query definition for getBenDetailsForLeftSidePanel
echo "=== Full query definition ==="
sed -n '75,84p' src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java

# 2. Find the static mapper method getBeneficiaryFlowStatusForLeftPanel
echo -e "\n=== Mapper method ==="
rg -nA20 "getBeneficiaryFlowStatusForLeftPanel" src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java

# 3. Search for migration/schema files containing doctor_signature_flag
echo -e "\n=== Migration files ==="
find . -type f \( -name "*.sql" -o -name "*.java" -o -name "*.yml" -o -name "*.yaml" \) -path "*/migration*" -o -path "*/db/*" | head -20

# 4. Search across all files for doctor_signature_flag in migrations
echo -e "\n=== Files referencing doctor_signature_flag ==="
rg -l "doctor_signature_flag" --type-list | head -20

Length of output: 749

---

🏁 Script executed:

#!/bin/bash
# 1. Get full @Query definition for getBenDetailsForLeftSidePanel
echo "=== Full query definition ==="
sed -n '75,84p' src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java

# 2. Find the static mapper method getBeneficiaryFlowStatusForLeftPanel with context
echo -e "\n=== Mapper method ==="
rg -nA25 "getBeneficiaryFlowStatusForLeftPanel" src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java

# 3. Search for migration/schema files
echo -e "\n=== Migration and schema files ==="
find . -type f \( -name "*.sql" -o -name "*V*.java" \) | grep -iE "(migration|flyway|db)" | head -20

# 4. Search for doctor_signature_flag in all files
echo -e "\n=== All references to doctor_signature_flag ==="
rg "doctor_signature_flag" -n

Length of output: 2554

---

Fix unsafe Boolean cast and add missing database migration

The mapper at line 378 uses an unsafe direct cast: (Boolean) objArr[21]. JDBC drivers return Integer or Short (0/1) for boolean columns, not Boolean type. This will throw a ClassCastException at runtime.

Additionally, no database migration file was found in the repository. The schema migration for doctor_signature_flag must be included in the PR.

Required fixes:

1. Mapper (line 378): Replace direct Boolean cast with safe conversion:

   if (objArr.length > 21 && objArr[21] != null) {
       Number numValue = (Number) objArr[21];
       obj.setDoctorSignatureFlag(numValue.intValue() != 0);
   }

2. Database migration: Add a Flyway/Liquibase migration file that adds the doctor_signature_flag column with TINYINT(1) type, default 0, and NOT NULL constraint to the i_ben_flow_outreach table. Ensure it handles existing rows safely (e.g., via UPDATE statement).

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/data/benFlowStatus/BeneficiaryFlowStatus.java
around lines 278-281 (and the mapper usage around line 378), fix the unsafe
direct cast of objArr[21] to Boolean by treating the value as a Number (e.g.,
check null, cast to Number, then set doctorSignatureFlag = num.intValue() != 0)
to avoid ClassCastException from JDBC returning Integer/Short; additionally add
a database migration (Flyway or Liquibase) that alters table i_ben_flow_outreach
to add column doctor_signature_flag TINYINT(1) NOT NULL DEFAULT 0 and include a
safe UPDATE for existing rows if needed so the schema and code remain in sync.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Robust Boolean extraction from native result to avoid ClassCastException

Direct cast to Boolean can blow up when JDBC returns 0/1 (Number). Parse defensively and avoid the magic index.

Apply:

-                if (objArr.length > 21) {
-                    obj.setDoctorSignatureFlag((Boolean) objArr[21]);
-                }
+                if (objArr.length > 21) {
+                    Object v = objArr[21];
+                    Boolean flag = (v instanceof Boolean) ? (Boolean) v
+                            : (v instanceof Number) ? (((Number) v).intValue() != 0)
+                            : (v != null ? Boolean.parseBoolean(v.toString()) : null);
+                    obj.setDoctorSignatureFlag(flag);
+                }

Optionally, replace 21 with a named constant tied to the SELECT list to prevent drift.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (1)

217-221: Consider batch updating vanSerialNo to avoid N+1 query problem.

The current implementation issues individual updateVanSerialNo calls inside a loop after batch-saving chief complaints. This creates N separate UPDATE queries, which can degrade performance for larger complaint lists.

Consider adding a batch update method to the repository, or update vanSerialNo before the saveAll operation:

  if (tmpBenCHiefComplaintsTMP.size() > 0) {
+     // Set vanSerialNo before saving
+     for (BenChiefComplaint complaint : tmpBenCHiefComplaintsTMP) {
+         // Populate vanSerialNo from the appropriate source
+     }
      ArrayList<BenChiefComplaint> benChiefComplaintListRS = (ArrayList<BenChiefComplaint>) benChiefComplaintRepo
              .saveAll(tmpBenCHiefComplaintsTMP);
      if (tmpBenCHiefComplaintsTMP.size() == benChiefComplaintListRS.size()) {
-         for (BenChiefComplaint complaint : benChiefComplaintListRS) {
-             if (complaint.getBenChiefComplaintID() != null) {
-                 benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
-             }
-         }
          chiefComFlag = 1;
      }

Alternatively, verify if updateVanSerialNo can be replaced with a single batch UPDATE query.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f83cd43 and 606c45d.

📒 Files selected for processing (10)

• src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java (3 hunks)
• src/main/java/com/iemr/mmu/service/anc/ANCServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (8 hunks)
• src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (6 hunks)
• src/main/java/com/iemr/mmu/service/covid19/Covid19ServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/generalOPD/GeneralOPDServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/ncdCare/NCDCareServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/ncdscreening/NCDSCreeningDoctorServiceImpl.java (2 hunks)
• src/main/java/com/iemr/mmu/service/pnc/PNCServiceImpl.java (4 hunks)
• src/main/java/com/iemr/mmu/service/quickConsultation/QuickConsultationServiceImpl.java (7 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/java/com/iemr/mmu/service/covid19/Covid19ServiceImpl.java
• src/main/java/com/iemr/mmu/service/ncdCare/NCDCareServiceImpl.java

🔇 Additional comments (11)

src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (3)

828-831: LGTM: Consistent doctorSignatureFlag propagation across all update paths.

The doctorSignatureFlag is correctly threaded through all beneficiary flow update variants:

• TC Specialist path (line 857)
• TM Referred path (line 885)
• WDF path (line 891)

This ensures consistent signature flag handling regardless of the flow type.

Also applies to: 855-857, 883-886, 889-892

---

811-814: No changes required — doctorSignatureFlag is properly initialized at all call sites.

The doctorSignatureFlag parameter is initialized to false in all seven calling services (GeneralOPDServiceImpl, ANCServiceImpl, PNCServiceImpl, Covid19ServiceImpl, NCDCareServiceImpl, NCDScreeningServiceImpl, QuickConsultationServiceImpl) before being passed to updateBenFlowtableAfterDocDataSave. The flag is never null; it's either false by default or updated from the JSON request. The parameter threading to updateBenFlowAfterDocData is correct.

---

767-770: No issues found - all call sites properly handle the new throws Exception.

The method updateBenFlowtableAfterDocDataSave now declares throws Exception, and verification confirms all 7 call sites within service implementations (QuickConsultationServiceImpl, PNCServiceImpl, NCDScreeningServiceImpl, NCDCareServiceImpl, GeneralOPDServiceImpl, Covid19ServiceImpl, and ANCServiceImpl) already declare throws Exception in their enclosing method signatures. The exception properly propagates through the call chain with no unhandled exceptions.

src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java (4)

156-169: LGTM: doctorSignatureFlag correctly added to doctor activity update.

The UPDATE query properly sets t.doctorSignatureFlag = :signatureFlag and the method signature includes the corresponding @Param("signatureFlag") Boolean signatureFlag parameter. The implementation is consistent with the existing pattern.

---

173-184: LGTM: Consistent signatureFlag handling across all doctor activity update variants.

Both updateBenFlowStatusAfterDoctorActivityWDF and updateBenFlowStatusAfterDoctorActivityTCSpecialist correctly include the signatureFlag parameter and update t.doctorSignatureFlag in their queries. This ensures uniform signature flag handling across all workflow paths.

Also applies to: 186-197

---

59-59: The vanSerialNo assignment to benFlowID is correct and intentional.

The codebase demonstrates a consistent pattern across multiple entities where vanSerialNo is set equal to the entity's primary identifier. This is evident in quick consultation repositories (BenChiefComplaintRepo, PrescriptionDetailRepo, LabTestOrderDetailRepo, BenVisitDetailRepo) where the same pattern is applied: vanSerialNo = [EntityID]. The BeneficiaryFlowStatus update follows this established convention, setting t.vanSerialNo=:benFlowID. The field tracks the original record identity within the Van (mobile) system, making this assignment appropriate.

---

76-82: Code is correctly implemented.

The verification confirms all components are properly integrated:

• The BeneficiaryFlowStatus entity has the doctorSignatureFlag field (line 280) with a corresponding setter (lines 988-989)
• The getBeneficiaryFlowStatusForLeftPanel() population logic correctly handles the new field: it checks if the result array has 22+ elements before accessing and mapping index 21 (lines 377-379)
• All three service methods calling this repository method will automatically benefit from the new field mapping

src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java (4)

264-279: LGTM: Improved error message and proper exception propagation.

The method signature correctly adds Boolean signatureFlag and throws Exception. The error message was updated from "ben flow creation" to "ben flow update", which more accurately reflects the method's purpose. The signatureFlag is properly passed to the repository layer.

---

281-301: LGTM: Consistent signatureFlag propagation.

The updateBenFlowAfterDocDataUpdate method correctly includes the signatureFlag parameter and passes it through to the repository layer, maintaining consistency with the other update methods.

---

303-323: LGTM: WDF variant correctly handles signatureFlag.

The WDF (Without Doctor Flag) update path properly includes and propagates the signatureFlag parameter, maintaining consistency with the main update flow.

---

325-345: LGTM: TC Specialist variant correctly handles signatureFlag.

The Teleconsultation Specialist update path properly includes and propagates the signatureFlag parameter, completing the consistent implementation across all beneficiary flow update variants.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Reinstate the null guard before reading doctorSignatureFlag.

Both the save and update doctor flows dereference requestOBJ prior to the existing null check, turning a previously handled null into a crash. Please restore the guard in each block.

-		Boolean doctorSignatureFlag = false;
-		if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
+		Boolean doctorSignatureFlag = false;
+		if (requestOBJ != null && requestOBJ.has("doctorSignatureFlag")
+				&& !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
 			doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean();
 		}

Also applies to: 1479-1481

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/service/anc/ANCServiceImpl.java around lines
269-272 (and also apply the same change at lines 1479-1481), the code
dereferences requestOBJ to read doctorSignatureFlag without first guarding that
requestOBJ is non-null; restore the original null guard by first checking
requestOBJ != null (or requestOBJ.isJsonNull() as appropriate) before calling
requestOBJ.has(...) and requestOBJ.get(...), and only then set
doctorSignatureFlag from getAsBoolean(); apply this exact null-check pattern to
both the save and update doctor flows.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify if vanSerialNo needs to be persisted to the database.

The code sets vanSerialNo on the in-memory object after saving, but this value is not persisted back to the database. If vanSerialNo must be stored, you need an additional save() call or an UPDATE query. If it's only for in-memory tracking or returned to the caller, the current implementation is acceptable.

Run the following script to check if vanSerialNo is read from the database elsewhere:

Also applies to: 95-95

---

🏁 Script executed:

#!/bin/bash
# Check if vanSerialNo is queried from database in BeneficiaryFlowStatus
ast-grep --pattern $'SELECT $$$vanSerialNo$$$'

# Also check repository methods that might read vanSerialNo
rg -nP 'vanSerialNo' --type=java -g '*Repo.java'

Length of output: 2698

---

---

🏁 Script executed:

```shell
#!/bin/bash
# View the context around line 75 in CommonBenStatusFlowServiceImpl to see if vanSerialNo is updated
cat src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java | sed -n '60,110p'

Length of output: 1762

---

🏁 Script executed:

#!/bin/bash
# View BeneficiaryFlowStatusRepo to see the UPDATE method
cat src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java | sed -n '55,75p'

Length of output: 1560

---

🏁 Script executed:

#!/bin/bash
# Check if there are any methods that update vanSerialNo in CommonBenStatusFlowServiceImpl
rg -nP 'update|Update' src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java -A2 -B2

Length of output: 4717

---

🏁 Script executed:

#!/bin/bash
# Search for updateBenFlowStatus or similar method calls after the save in CommonBenStatusFlowServiceImpl
rg -nP '(update|Update).*benFlowStatus|vanSerialNo' src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java -C 5

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check the full BeneficiaryFlowStatusRepo class for update methods
rg -nP 'public.*update|@Query.*UPDATE' src/main/java/com/iemr/mmu/repo/benFlowStatus/BeneficiaryFlowStatusRepo.java -A 3

Length of output: 7711

---

Add UPDATE query or refresh to persist vanSerialNo after save.

Lines 75 and 95 set vanSerialNo on the in-memory object after save(), but this change is not persisted to the database. Other update methods in BeneficiaryFlowStatusRepo (e.g., updateBenFlowStatusAfterNurseActivity) explicitly persist vanSerialNo via UPDATE queries. Since vanSerialNo is queried from the database in other repositories throughout the codebase, it must be persisted. Add an UPDATE call or refresh operation after save() to persist the vanSerialNo value.

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/benFlowStatus/CommonBenStatusFlowServiceImpl.java
around lines 75 and 95, the code sets obj.setVanSerialNo(...) on the in-memory
object after save() but does not persist that field to the DB; run an explicit
persistence step after saving (either call the repository update method to run
an UPDATE that sets vanSerialNo for the saved entity or use the EntityManager to
refresh/merge the entity and then flush) so the vanSerialNo is stored in the
database and visible to other repositories that query it.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Null-check requestOBJ before accessing doctorSignatureFlag.

The new flag extraction now runs before the existing null guard in both doctor save/update flows, so a null request triggers an NPE instead of the prior fallback. Please reintroduce the guard.

-		Boolean doctorSignatureFlag = false;
-		if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
+		Boolean doctorSignatureFlag = false;
+		if (requestOBJ != null && requestOBJ.has("doctorSignatureFlag")
+				&& !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
 			doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean();
 		}

Also applies to: 1430-1432

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/service/generalOPD/GeneralOPDServiceImpl.java
around lines 787-790 and 1430-1432, the code reads doctorSignatureFlag from
requestOBJ without first checking requestOBJ for null, which can cause an NPE;
add a null-check guard similar to the existing flows (e.g., if (requestOBJ !=
null && requestOBJ.has("doctorSignatureFlag") &&
!requestOBJ.get("doctorSignatureFlag").isJsonNull()) { ... }) so you only call
has/get on a non-null requestOBJ and preserve the prior fallback behavior.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Null-guard requestOBJ before accessing doctorSignatureFlag.

requestOBJ is dereferenced before the existing null check, so a null payload now throws instead of following the prior graceful branch. Please keep the method null-safe.

-		Boolean doctorSignatureFlag = false;
-		if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
+		Boolean doctorSignatureFlag = false;
+		if (requestOBJ != null && requestOBJ.has("doctorSignatureFlag")
+				&& !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
 			doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean();
 		}

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/ncdscreening/NCDSCreeningDoctorServiceImpl.java
around lines 79 to 82, the code dereferences requestOBJ to read
doctorSignatureFlag without first checking if requestOBJ itself is null; add a
null-guard so you only call requestOBJ.has(...) and requestOBJ.get(...) when
requestOBJ != null (and optionally not JsonNull), otherwise leave
doctorSignatureFlag as the default false to preserve the prior graceful branch.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Protect requestOBJ before dereferencing it for doctorSignatureFlag.

Both doctor save/update paths now touch requestOBJ before checking for null, breaking the earlier null-safe behavior and introducing a potential NullPointerException.

-		Boolean doctorSignatureFlag = false;
-		if (requestOBJ.has("doctorSignatureFlag") && !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
+		Boolean doctorSignatureFlag = false;
+		if (requestOBJ != null && requestOBJ.has("doctorSignatureFlag")
+				&& !requestOBJ.get("doctorSignatureFlag").isJsonNull()) {
 			doctorSignatureFlag = requestOBJ.get("doctorSignatureFlag").getAsBoolean();
 		}

Also applies to: 1479-1483

🤖 Prompt for AI Agents

In src/main/java/com/iemr/mmu/service/pnc/PNCServiceImpl.java around lines
240-243 (and likewise at 1479-1483), the code dereferences requestOBJ to read
doctorSignatureFlag without first checking if requestOBJ is null; guard against
NullPointerException by first checking if requestOBJ != null (or
returning/handling the null case) before calling requestOBJ.has(...) and
requestOBJ.get(...). If requestOBJ is null, keep doctorSignatureFlag as false
(or apply the intended default behavior); apply the same null-check pattern to
the other location (1479-1483) so both save/update paths remain null-safe.

[coderabbitai]

Actionable comments posted: 1

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/mmu/service/common/transaction/CommonDoctorServiceImpl.java (1)

100-100: Critical: TMReferred instance variable creates race condition.

The TMReferred instance variable (line 100) is modified in multiple methods (saveBenReferDetails, updateBenReferDetails) and then read in updateBenFlowtableAfterDocDataSave and updateBenFlowtableAfterDocDataUpdate. Since this service is a singleton (Spring @Service), concurrent requests will share this variable, causing race conditions where one request's value overwrites another's.

Solution: Pass TMReferred as a return value or method parameter instead of using instance state:

-private int TMReferred = 0;

-public Long saveBenReferDetails(JsonObject obj) throws IEMRException {
+public SaveReferDetailsResult saveBenReferDetails(JsonObject obj) throws IEMRException {
     Long ID = null;
+    int tmReferred = 0;
     // ... existing code ...
     
     if (sm.getServiceName().equalsIgnoreCase(tmReferCheckValue)) {
-        TMReferred = 1;
+        tmReferred = 1;
     } else {
-        TMReferred = 0;
+        tmReferred = 0;
     }
     // ... existing code ...
-    return ID;
+    return new SaveReferDetailsResult(ID, tmReferred);
 }

Then update the callers to pass this value explicitly to updateBenFlowtableAfterDocDataSave and updateBenFlowtableAfterDocDataUpdate.

🧹 Nitpick comments (3)

src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (3)

217-221: Consider batching the vanSerialNo updates.

Calling updateVanSerialNo in a loop creates N individual database calls. If benChiefComplaintListRS typically contains multiple entries, this could impact performance.

Consider refactoring the repository method to accept a collection of IDs and perform a batch update:

-  for (BenChiefComplaint complaint : benChiefComplaintListRS) {
-    if (complaint.getBenChiefComplaintID() != null) {
-        benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
-    }
-}
+List<Long> ids = benChiefComplaintListRS.stream()
+    .map(BenChiefComplaint::getBenChiefComplaintID)
+    .filter(Objects::nonNull)
+    .collect(Collectors.toList());
+if (!ids.isEmpty()) {
+    benChiefComplaintRepo.updateVanSerialNoInBatch(ids);
+}

Then update the repository to support batch updates:

@Modifying
@Query("UPDATE BenChiefComplaint b SET b.vanSerialNo = ... WHERE b.benChiefComplaintID IN :ids")
void updateVanSerialNoInBatch(@Param("ids") List<Long> ids);

---

766-768: Avoid throwing broad Exception type.

The method signature declares throws Exception, which is too broad and makes it difficult for callers to handle specific error conditions appropriately.

Consider throwing more specific exception types (e.g., IEMRException which is already used elsewhere in this codebase, or a custom exception):

-public int updateBenFlowtableAfterDocDataSave(CommonUtilityClass commonUtilityClass,
-        Boolean isTestPrescribed, Boolean isMedicinePrescribed,
-        TeleconsultationRequestOBJ tcRequestOBJ, Boolean doctorSignatureFlag) throws Exception {
+public int updateBenFlowtableAfterDocDataSave(CommonUtilityClass commonUtilityClass,
+        Boolean isTestPrescribed, Boolean isMedicinePrescribed,
+        TeleconsultationRequestOBJ tcRequestOBJ, Boolean doctorSignatureFlag) throws IEMRException {

---

827-829: Avoid throwing broad Exception type.

Same issue as updateBenFlowtableAfterDocDataSave - the method declares throws Exception which is too broad.

Apply the same fix:

-public int updateBenFlowtableAfterDocDataUpdate(CommonUtilityClass commonUtilityClass,
-        Boolean isTestPrescribed, Boolean isMedicinePrescribed,
-        TeleconsultationRequestOBJ tcRequestOBJ, Boolean doctorSignatureFlag) throws Exception {
+public int updateBenFlowtableAfterDocDataUpdate(CommonUtilityClass commonUtilityClass,
+        Boolean isTestPrescribed, Boolean isMedicinePrescribed,
+        TeleconsultationRequestOBJ tcRequestOBJ, Boolean doctorSignatureFlag) throws IEMRException {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 606c45d and d81128f.

📒 Files selected for processing (1)

• src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java (7 hunks)

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify null handling for doctorSignatureFlag parameter.

The doctorSignatureFlag parameter uses the Boolean wrapper type, which can be null. Ensure that the downstream service method updateBenFlowAfterDocData handles null values correctly, or add validation here.

---

🏁 Script executed:

#!/bin/bash
# Description: Check how updateBenFlowAfterDocData handles the doctorSignatureFlag parameter

# Find the implementation of updateBenFlowAfterDocData
ast-grep --pattern $'updateBenFlowAfterDocData($$$) {
  $$$
}'

# Also search for any null checks on this parameter
rg -nP -C5 'doctorSignatureFlag\s*[=!]=' --type=java

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Search for updateBenFlowAfterDocData method definition (broader search)
rg -nP --type=java 'updateBenFlowAfterDocData\s*\(' -A15 | head -50

# Search for CommonBenStatusFlowServiceImpl class
fd -type f -name "CommonBenStatusFlowServiceImpl.java" -exec cat {} \;

Length of output: 6018

---

🏁 Script executed:

#!/bin/bash
# Find the repository interface/implementation that handles updateBenFlowStatusAfterDoctorActivity
rg -n 'updateBenFlowStatusAfterDoctorActivity' --type-add 'javafile:*.java' -A 20

# Also search for BeneficiaryFlowStatusRepo class/interface definition
fd -type f -name "*BeneficiaryFlowStatusRepo*" -o -name "*BenFlowStatusRepo*"

Length of output: 19764

---

🏁 Script executed:

#!/bin/bash
# Find BeneficiaryFlowStatus entity to check column nullability
fd -type f -name "BeneficiaryFlowStatus.java" -exec cat {} \;

Length of output: 170

---

🏁 Script executed:

#!/bin/bash
# Search for BeneficiaryFlowStatus class definition with column annotations
rg -n 'class BeneficiaryFlowStatus' -A 50 | head -100

# Also search for doctorSignatureFlag field in entity files
rg -n 'doctorSignatureFlag' -B 2 -A 2 | grep -A 5 'BeneficiaryFlowStatus\|@Column\|@NotNull\|nullable'

Length of output: 9951

---

Add null handling for doctorSignatureFlag parameter.

The doctorSignatureFlag parameter uses the Boolean wrapper type which can be null. The service method updateBenFlowAfterDocData and the downstream repository method updateBenFlowStatusAfterDoctorActivity do not validate or handle null values before passing to the database. While the entity field initializes to false, this creates inconsistency when null is explicitly passed as a parameter. Add a null check in the service method (e.g., doctorSignatureFlag == null ? false : doctorSignatureFlag) or ensure the caller always provides a non-null value.

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/common/transaction/CommonDoctorServiceImpl.java
around lines 810 to 812, the nullable Boolean doctorSignatureFlag is passed
directly to updateBenFlowAfterDocData which and downstream repository expects a
non-null boolean; ensure a non-null value is passed by converting null to false
before the call (e.g., compute a local primitive boolean or use
doctorSignatureFlag == null ? false : doctorSignatureFlag) so
updateBenFlowAfterDocData and updateBenFlowStatusAfterDoctorActivity always
receive a concrete false/true value.

[sonarqubecloud]

Quality Gate failed

Failed conditions
10.0% Duplication on New Code (required ≤ 3%)
B Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java (1)

108-116: Critical SQL injection vulnerability remains unaddressed.

The IN clause construction using String.join(",", vanSerialNos) directly concatenates untrusted input into the SQL query, allowing SQL injection. Additionally, an empty vanSerialNos list will produce IN (), which is invalid SQL. This issue was already raised in a previous review and must be fixed before release.

Please apply the fix suggested in the previous review comment (lines 108-120) that uses parameterized placeholders and bind variables instead of string concatenation.

🧹 Nitpick comments (1)

src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java (1)

108-116: Remove redundant if/else branches.

Both the i_ben_flow_outreach branch (lines 109-111) and the else branch (lines 113-115) now construct identical SQL queries. The conditional check on line 108 serves no purpose.

Apply this diff to eliminate the redundant branching:

-	if (tableName != null && tableName.toLowerCase().equals("i_ben_flow_outreach")) {
-		query = "UPDATE " + schemaName + "." + tableName
-				+ " SET processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
-				+ "WHERE " + autoIncreamentColumn + " IN (" + String.join(",", vanSerialNos) + ")";
-	} else {
-		query = "UPDATE " + schemaName + "." + tableName
-				+ " SET processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
-				+ "WHERE " + autoIncreamentColumn + " IN (" + String.join(",", vanSerialNos) + ")";
-	}
+	query = "UPDATE " + schemaName + "." + tableName
+			+ " SET processed = ?, SyncedDate = ?, Syncedby = ? , SyncFailureReason = ? "
+			+ "WHERE " + autoIncreamentColumn + " IN (" + String.join(",", vanSerialNos) + ")";

Note: This diff assumes the removal of created_date/CreatedDate is intentional (see previous comment). Also, the SQL injection vulnerability must still be fixed regardless of this refactor.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d81128f and 95475bd.

📒 Files selected for processing (1)

• src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java (1 hunks)

🔇 Additional comments (2)

src/main/java/com/iemr/mmu/service/dataSyncActivity/DataSyncRepository.java (2)

101-102: API change verified—all callers have been updated correctly.

The two invocation sites in UploadDataToServerImpl.java (lines 585 and 590) have both been updated to match the new method signature, passing List<String> for vanSerialNos and the new status and reason parameters.

---

108-116: The removal of created_date from the UPDATE statement is intentional and correct.

The created_date column in the BeneficiaryFlowStatus entity (which maps to i_ben_flow_outreach) is explicitly marked as updatable = false. This database schema constraint prevents updates to this column, so removing it from the UPDATE statement properly reflects the immutability of this audit field. No regression is introduced.

Separately, the if/else branches (lines 108–120) contain identical SQL queries and represent unnecessary code duplication that could be simplified by removing the conditional.

Likely an incorrect or invalid review comment.

[sonarqubecloud]

Quality Gate failed

Failed conditions
10.0% Duplication on New Code (required ≤ 3%)
B Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[coderabbitai]

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 (1)

src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (1)

435-446: Missing error handling for updateVanSerialNo calls in loop.

The updateVanSerialNo return value is ignored. If any call fails, the method still returns success (r = 1), leaving data in an inconsistent state. Consider:

1. Checking return values and rolling back on failure
2. Using a batch update instead of N individual calls for better performance

 if (benChiefComplaintListNew.size() == benChiefComplaintResultList.size())
 {
-     for (BenChiefComplaint complaint : benChiefComplaintResultList) {
-        if (complaint.getBenChiefComplaintID() != null) {
-            benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
-        }
-    }
+    for (BenChiefComplaint complaint : benChiefComplaintResultList) {
+        if (complaint.getBenChiefComplaintID() != null) {
+            int updated = benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
+            if (updated == 0) {
+                logger.error("Failed to update vanSerialNo for complaint: {}", complaint.getBenChiefComplaintID());
+                return 0; // or throw exception
+            }
+        }
+    }
     r = 1;
 }

♻️ Duplicate comments (2)

src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (2)

261-268: Potential NPE and missing error handling for updateVanSerialNo.

The updateVanSerialNo call on line 262 occurs before the null check on line 264. If response is null, this will throw a NullPointerException. Additionally, the return value is ignored, so failures go undetected.

This issue was previously flagged. Apply the suggested fix from the earlier review: add @Transactional, move the call inside the null check, and verify the return value.

 response = benVisitDetailRepo.save(beneficiaryVisitDetail);
-benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
-
-if (response != null) {
+if (response != null && response.getBenVisitID() != null) {
+    int updated = benVisitDetailRepo.updateVanSerialNo(response.getBenVisitID());
+    if (updated == 0) {
+        throw new IEMRException("Failed to update vanSerialNo for visit: " + response.getBenVisitID());
+    }
     // Long visitCode = updateVisitCode(response, 10);
     return response.getBenVisitID();
-} else
+} else {
     return null;
+}

---

2701-2707: Potential NPE and missing error handling for prescription updateVanSerialNo.

Similar to saveBeneficiaryVisitDetails, the updateVanSerialNo call occurs before the null check. If prescriptionRS is null, this throws NPE. The return value is also ignored.

This was previously flagged. Move the call inside the null check and handle failures:

 PrescriptionDetail prescriptionRS = prescriptionDetailRepo.save(prescription);
-prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID());
-
 if (prescriptionRS != null && prescriptionRS.getPrescriptionID() > 0) {
+    int updated = prescriptionDetailRepo.updateVanSerialNo(prescriptionRS.getPrescriptionID());
+    if (updated == 0) {
+        throw new IEMRException("Failed to update vanSerialNo for prescription: " + prescriptionRS.getPrescriptionID());
+    }
     r = prescriptionRS.getPrescriptionID();
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95475bd and 6f2922e.

📒 Files selected for processing (1)

• src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (10 hunks)

🔇 Additional comments (1)

src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java (1)

2859-2960: Frequency handling expansions look correct.

The additions for BD, TID, QID, and "Once in a Week" variants follow the existing pattern correctly and handle the Before Food/After Food variations appropriately.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Same error handling issue as saveBenChiefComplaints.

The updateVanSerialNo calls ignore return values. Failed updates go undetected, and the method reports success regardless. Apply the same fix pattern as suggested for saveBenChiefComplaints.

-
 if (!benChiefComplaintResultList.isEmpty()) {
-      for (BenChiefComplaint complaint : benChiefComplaintResultList) {
-        if (complaint.getBenChiefComplaintID() != null) {
-            benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
-        }
-    }
+    for (BenChiefComplaint complaint : benChiefComplaintResultList) {
+        if (complaint.getBenChiefComplaintID() != null) {
+            int updated = benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
+            if (updated == 0) {
+                logger.error("Failed to update vanSerialNo for complaint: {}", complaint.getBenChiefComplaintID());
+                return 0;
+            }
+        }
+    }
     r = benChiefComplaintResultList.size();
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (!benChiefComplaintResultList.isEmpty()) {
	for (BenChiefComplaint complaint : benChiefComplaintResultList) {
	if (complaint.getBenChiefComplaintID() != null) {
	benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
	}
	}
	r = benChiefComplaintResultList.size();
	if (!benChiefComplaintResultList.isEmpty()) {
	for (BenChiefComplaint complaint : benChiefComplaintResultList) {
	if (complaint.getBenChiefComplaintID() != null) {
	int updated = benChiefComplaintRepo.updateVanSerialNo(complaint.getBenChiefComplaintID());
	if (updated == 0) {
	logger.error("Failed to update vanSerialNo for complaint: {}", complaint.getBenChiefComplaintID());
	return 0;
	}
	}
	}
	r = benChiefComplaintResultList.size();
	}

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java
around lines 2087 to 2094, the loop calling
benChiefComplaintRepo.updateVanSerialNo(...) ignores the method's return value
so failed updates are not detected; change the loop to capture the update result
for each complaint, increment a success counter only when the update indicates
success (or rowsAffected > 0), log or collect failures, and set r to the count
of successful updates (or throw/return an error if any update fails) mirroring
the fix pattern used in saveBenChiefComplaints so failures are surfaced instead
of always reporting full success.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Typo: Double spaces in frequency strings will cause matching failures.

The strings "Single Dose Before Food" and "Single Dose After Food" contain two spaces before "Food", while all other similar strings use single spaces. This inconsistency will likely cause frequency matching to fail for these variants.

-if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")|| 
-		frequency.equalsIgnoreCase("Single Dose Before  Food") || frequency.equalsIgnoreCase("Single Dose After  Food")) {
+if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")|| 
+		frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")||
	frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) {
	if (frequency.equalsIgnoreCase("Single Dose") || frequency.equalsIgnoreCase("Stat Dose")||
	frequency.equalsIgnoreCase("Single Dose Before Food") || frequency.equalsIgnoreCase("Single Dose After Food")) {

🤖 Prompt for AI Agents

In
src/main/java/com/iemr/mmu/service/common/transaction/CommonNurseServiceImpl.java
around lines 2934-2935, the frequency literals "Single Dose Before  Food" and
"Single Dose After  Food" contain double spaces which will break
equalsIgnoreCase matching; fix by replacing those literals with single-space
versions "Single Dose Before Food" and "Single Dose After Food" or, better,
normalize the frequency string first (trim and collapse multiple spaces, e.g.,
frequency = frequency.trim().replaceAll("\\s+"," ")) then perform the
equalsIgnoreCase checks.

[vanitha1822]

Not required