Added teleconsultation extra field #86
Conversation
WalkthroughThe changes replace the Boolean field and logic for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant DataModel
Client->>Controller: Request user role mapping
Controller->>Service: getMappedRole()
Service->>DataModel: Retrieve role mapping (includes teleConsultation)
DataModel-->>Service: Return mapping with teleConsultation field
Service-->>Controller: Return mapped roles with teleConsultation
Controller-->>Client: Respond with mapping (teleConsultation info)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/main/java/com/iemr/admin/controller/employeemaster/EmployeeMasterController.java (1)
1842-1862: **teleConsultationis set only once per outer loop – the inner loop result is overwritten **
resDataMap1is instantiated inside the innerfor (Priveleges1097_2 previl1 …)loop, yet it is added toresList1after that loop finishes (line 1861).
As a consequence, only the lastprevil1encountered is retained; every previous iteration is lost (including itsteleConsultation, inbound / outbound flags, etc.).for (Priveleges1097_2 previl1 : predata2) { resDataMap1 = new M_UserServiceRoleMapping2(); resDataMap1.setRoleID(previl1.getRoleID()); … -} -resDataMap1.setUserID(employeeMaster.get(x).getUserID()); -… -resList1.add(resDataMap1); + resDataMap1.setUserID(employeeMaster.get(x).getUserID()); + resDataMap1.setProviderServiceMapID(previl.getProviderServiceMapID()); + … + resList1.add(resDataMap1); // add *inside* the loop +}Putting the remaining setters +
add()inside the inner loop preserves every mapping record.
🧹 Nitpick comments (6)
src/main/java/com/iemr/admin/controller/employeemaster/EmployeeMasterController.java (2)
1848-1850: Guard-clause is redundant
teleConsultationis already nullable in the entity. If a null arrives, doing nothing or explicitly setting null is equivalent. You can drop theif (…)check and set the value unconditionally to keep the code compact.
1908-1911: Setter duplicated – consider early-return insteadExactly the same null-check & setter for
teleConsultationwas added here. If you refactor the earlier loop as suggested, this block can stay, but watch out for code duplication across controller methods – a private helper (applyTeleConsultationIfPresent) would avoid divergence.src/main/java/com/iemr/admin/to/employeemaster/Priveleges1097_2.java (1)
29-55: Clarify semantics ofteleConsultationYou replaced a Boolean
isSanjeevaniwith aString teleConsultation, but the valid values / semantics are not documented.
If the field is meant to act as an on/off flag (e.g."Y"/"N"), sticking toBooleanor anenum { YES, NO }is safer than a free-form string and simplifies validation throughout the stack.src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (2)
995-1000: Redundant self-assignments – remove to reduce noise and avoid misleading readers
setBlockID,setBlockName,setVillageID, andsetVillageNameare invoked with the exact value already held by the same instance.
These statements do nothing and only add cognitive overhead.- if (mapping.getServiceID() != null) { - mapping.setBlockID(mapping.getBlockID()); - mapping.setBlockName(mapping.getBlockName()); - mapping.setVillageID(mapping.getVillageID()); - mapping.setVillageName(mapping.getVillageName()); + if (mapping.getServiceID() != null) { // fields are already populated above
1009-1011: No-op assignment forteleConsultationThe null check is fine, but
setTeleConsultation(getTeleConsultation())again assigns a value to itself and achieves nothing.
Either drop the whole block or perform a meaningful transformation (e.g., trim, normalise to upper-case, etc.).- if (null != mapping.getTeleConsultation()) - mapping.setTeleConsultation(mapping.getTeleConsultation()); + // value already present; remove unless additional processing is requiredsrc/main/java/com/iemr/admin/data/employeemaster/V_Userservicerolemapping.java (1)
150-158: Remove commented legacy field & document new one
- Keeping the old
isSanjeevanifield in comments clutters the entity and may confuse future maintainers.- Consider adding a brief Javadoc on
teleConsultationexplaining allowed values (e.g.,"Y" / "N"or a code list) because the type changed fromBooleantoString.- /* - * @Expose - * @Column(name="isSanjeevani") - * private Boolean isSanjeevani; - */ + /** + * Indicates tele-consultation eligibility. + * Expected values: "Y", "N", or empty. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/iemr/admin/controller/employeemaster/EmployeeMasterController.java(2 hunks)src/main/java/com/iemr/admin/data/employeemaster/M_UserServiceRoleMapping2.java(1 hunks)src/main/java/com/iemr/admin/data/employeemaster/V_Userservicerolemapping.java(1 hunks)src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java(1 hunks)src/main/java/com/iemr/admin/to/employeemaster/Priveleges1097_2.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (java)
- GitHub Check: Package-test
🔇 Additional comments (1)
src/main/java/com/iemr/admin/data/employeemaster/M_UserServiceRoleMapping2.java (1)
108-112: Column definition forteleConsultationlacks length / constraintBy default Hibernate maps a
Stringtovarchar(255).
If the underlying column in your migration / DB script is shorter (many existing tables usechar(1)orvarchar(10)for yes/no flags), inserts will fail. Declare an explicit length to document the contract:-@Column(name = "teleConsultation") -private String teleConsultation; +@Column(name = "teleConsultation", length = 10, nullable = true) +private String teleConsultation;Adjust the length to match the DDL.
ravishanigarapu
requested review from
drtechie,
helenKaryamsetty and
vanitha1822
| @Expose | ||
| @Column(name="isSanjeevani") | ||
| private Boolean isSanjeevani; | ||
| @Column(name="teleConsultation") |
There was a problem hiding this comment.
Comment says isSanjeevani but the boolean is teleConsultation.
Also the attribute isSanjeevani is not needed?
If you are adding new attributes, can you also raise a PR for AMRIT-DB
| private Boolean inbound; | ||
| private Boolean outbound; | ||
| private Boolean isSanjeevani; | ||
| //private Boolean isSanjeevani; |
There was a problem hiding this comment.
What is the impact of removing this?
Can you remove commented out code.
Should this attribute be removed from table also?
There was a problem hiding this comment.
yes we are removing this column from table also
| mapping.setIsSanjeevani(false); | ||
|
|
||
| } | ||
| if(null != mapping.getTeleConsultation()) |
There was a problem hiding this comment.
are there some curly braces missing here {}
not following where this if block ends.
There was a problem hiding this comment.
If we are not adding curly braces{} this condition will apply for next line alone.
There was a problem hiding this comment.
Hmm. That sort of conditional statement is awful for readability.
| if(previl1.getIsSanjeevani() != null) { | ||
| resDataMap1.setIsSanjeevani(previl1.getIsSanjeevani()); | ||
| if(previl1.getTeleConsultation() != null) { | ||
| resDataMap1.setTeleConsultation(previl1.getTeleConsultation()); |
|
|
@mo839639 please raise PR for AMRIT-DB |
2 participants
📋 Description
JIRA ID: AMM-1536
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit