AMM-1002 : Preprod- Unable to separate a Customization field for #134
Conversation
different-2 service line in single provider
WalkthroughThis pull request introduces a new field, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/main/java/com/iemr/common/data/customization/SectionFieldsMappingDTO.java (1)
17-17: LGTM! Consider adding documentation and validation.The addition of
projectIdaligns with the requirement to separate customization fields. However, consider these improvements:+ /** Project identifier used to separate customization fields for different service lines */ + @JsonProperty(required = false) // if nullable + @Min(value = 1, message = "Project ID must be positive") private Integer projectId;src/main/java/com/iemr/common/repo/customization/SectionAndFieldsMappingRepo.java (1)
19-21: Document the fallback behavior for projectId=0.The query correctly implements a fallback mechanism where
projectId=0represents global settings. This aligns with the PR objective of separating customization fields while maintaining backward compatibility.Add a comment explaining the fallback behavior:
+// Fetches mappings for specific project/provider or falls back to global settings (projectId=0/serviceProviderId=0) @Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.sectionId = :sectionId AND (sfm.serviceProviderId = :serviceProviderId OR sfm.serviceProviderId= 0) AND (sfm.projectId = :projectId OR sfm.projectId= 0)")src/main/java/com/iemr/common/service/customization/CustomizationServiceImpl.java (1)
Line range hint
183-377: Consider adding transaction boundariesThe changes effectively solve the field separation issue by introducing projectId context. However, consider adding @transactional annotations to methods that perform multiple database operations (save, update) to ensure data consistency in case of failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/main/java/com/iemr/common/data/customization/SectionAndFieldsMapping.java (1 hunks)
- src/main/java/com/iemr/common/data/customization/SectionFieldsMappingDTO.java (1 hunks)
- src/main/java/com/iemr/common/repo/customization/SectionAndFieldsMappingRepo.java (1 hunks)
- src/main/java/com/iemr/common/service/customization/CustomizationServiceImpl.java (3 hunks)
🔇 Additional comments (6)
src/main/java/com/iemr/common/data/customization/SectionFieldsMappingDTO.java (1)
Line range hint
1-19: Verify data migration strategy.Since this appears to be a schema change, ensure there's a plan for handling existing records in the database.
src/main/java/com/iemr/common/repo/customization/SectionAndFieldsMappingRepo.java (1)
19-27: Verify the impact of projectId changes.The addition of projectId parameter affects multiple queries. Let's verify that all callers are updated accordingly.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information:
All callers are properly updated with projectId parameter
The code search shows that both repository methods
findSectionIdAndSectionNameAndServiceProviderIdandgetByFieldNameare called correctly inCustomizationServiceImpl.javawith the projectId parameter being passed appropriately:
getByFieldNameis called withsectionFieldsMappingDTO.getProjectId()findSectionIdAndSectionNameAndServiceProviderIdis called withsectionAndFieldsMapping.getProjectId()🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for method calls to ensure all callers are updated with projectId parameter rg -A 2 "findSectionIdAndSectionNameAndServiceProviderId|getByFieldName" --type java # Search for any remaining queries in the repo that might need projectId ast-grep --pattern 'SELECT sfm FROM SectionAndFieldsMapping sfm WHERE $$$'Length of output: 2036
src/main/java/com/iemr/common/data/customization/SectionAndFieldsMapping.java (2)
99-102: LGTM! The projectId field implementation looks correct.The field follows consistent patterns with other ID fields in the class and will help separate customization fields per project.
Let's verify the database schema changes:
#!/bin/bash # Description: Check if database migration scripts exist for the ProjectID column # Look for migration files containing ProjectID rg -i "projectid|project_id" --type sql
99-102: Verify impact on existing records.Since this is a new column in an existing table, ensure there's a strategy for handling existing records.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have sufficient information about how projectId is handled in the codebase.
The projectId column addition is properly handled with fallback mechanisms
The codebase shows proper handling of the projectId column through:
- Default fallback to projectId=0 in queries (e.g.
OR sp.projectId = 0)- Null checks and initialization of projectId in service layer
- Consistent usage across related entities (ServicelineCustomization, SectionProjectMapping, etc.)
The existing records are safely handled through the OR conditions in repository queries that fall back to projectId=0 when querying data:
(sfm.projectId = :projectId OR sfm.projectId= 0)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for repository and service layer handling of projectId # Check repository methods using projectId ast-grep --pattern 'interface $_ extends JpaRepository<$_, $_> { $$$ findBy$$$ProjectId$$$ $$$ }' # Check service layer handling rg -A 5 "projectId" --type javaLength of output: 12446
src/main/java/com/iemr/common/service/customization/CustomizationServiceImpl.java (2)
183-188: LGTM: Proper validation and uniqueness check for projectIdThe changes correctly enforce projectId presence and ensure field name uniqueness within each project context, which addresses the core issue of separating customization fields.
191-191: LGTM: Proper projectId assignmentThe projectId is correctly assigned from the DTO to the mapping object.
| @Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.fieldName = :fieldName and sfm.serviceProviderId = :serviceProviderId and sfm.projectId = :projectId") | ||
| List<SectionAndFieldsMapping> getByFieldName(@Param("fieldName") String fieldName,@Param("serviceProviderId") Integer serviceProviderId,@Param("projectId") Integer projectId); |
There was a problem hiding this comment.
Inconsistent fallback behavior between queries.
While the first query allows fallback to global settings (projectId=0), this query enforces strict matching. This inconsistency could lead to unexpected behavior where some queries find global settings and others don't.
Consider applying the same fallback pattern:
-@Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.fieldName = :fieldName and sfm.serviceProviderId = :serviceProviderId and sfm.projectId = :projectId")
+@Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.fieldName = :fieldName AND (sfm.serviceProviderId = :serviceProviderId OR sfm.serviceProviderId = 0) AND (sfm.projectId = :projectId OR sfm.projectId = 0)")📝 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.
| @Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.fieldName = :fieldName and sfm.serviceProviderId = :serviceProviderId and sfm.projectId = :projectId") | |
| List<SectionAndFieldsMapping> getByFieldName(@Param("fieldName") String fieldName,@Param("serviceProviderId") Integer serviceProviderId,@Param("projectId") Integer projectId); | |
| @Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.fieldName = :fieldName AND (sfm.serviceProviderId = :serviceProviderId OR sfm.serviceProviderId = 0) AND (sfm.projectId = :projectId OR sfm.projectId = 0)") | |
| List<SectionAndFieldsMapping> getByFieldName(@Param("fieldName") String fieldName,@Param("serviceProviderId") Integer serviceProviderId,@Param("projectId") Integer projectId); |
| @Column(name = "ProjectID") | ||
| @Expose | ||
| private Integer projectId; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation constraints.
To ensure data integrity, consider adding validation annotations:
@Column(name = "ProjectID")
@Expose
+@NotNull(message = "Project ID is required")
+@Min(value = 1, message = "Project ID must be positive")
private Integer projectId;📝 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.
| @Column(name = "ProjectID") | |
| @Expose | |
| private Integer projectId; | |
| @Column(name = "ProjectID") | |
| @Expose | |
| @NotNull(message = "Project ID is required") | |
| @Min(value = 1, message = "Project ID must be positive") | |
| private Integer projectId; | |
| sectionAndFieldsMapping.getServiceProviderId(), sectionAndFieldsMapping.getProjectId()); | ||
|
|
||
| String sectionName = sectionMasterCustomizationRepo.findSectionName(sectionAndFieldsMapping.getSectionId()); | ||
| Map<String, Object> responseMap = new HashMap<>(); | ||
| responseMap.put("sectionId", sectionAndFieldsMapping.getSectionId()); | ||
| responseMap.put("sectionName", sectionName); | ||
| responseMap.put("serviceProviderId", sectionAndFieldsMapping.getServiceProviderId()); | ||
|
|
||
| responseMap.put("projectId", sectionAndFieldsMapping.getProjectId()); |
There was a problem hiding this comment.
Add null check for projectId
While the changes correctly include projectId in the query and response, consider adding a null check for projectId to prevent potential NullPointerException.
- .findSectionIdAndSectionNameAndServiceProviderId(sectionAndFieldsMapping.getSectionId(),
- sectionAndFieldsMapping.getServiceProviderId(), sectionAndFieldsMapping.getProjectId());
+ if (sectionAndFieldsMapping.getProjectId() == null) {
+ throw new IllegalArgumentException("ProjectId is required");
+ }
+ .findSectionIdAndSectionNameAndServiceProviderId(sectionAndFieldsMapping.getSectionId(),
+ sectionAndFieldsMapping.getServiceProviderId(), sectionAndFieldsMapping.getProjectId());Committable suggestion was skipped due to low confidence.
| if(sectionAndFieldsMapping.getProjectId() != null) | ||
| response.setProjectId(sectionAndFieldsMapping.getProjectId()); |
There was a problem hiding this comment.
Validate projectId before update
While the conditional update of projectId is correct, consider validating that the new projectId exists in the projects table to maintain data integrity.
- if(sectionAndFieldsMapping.getProjectId() != null)
- response.setProjectId(sectionAndFieldsMapping.getProjectId());
+ if(sectionAndFieldsMapping.getProjectId() != null) {
+ // Validate projectId exists
+ ProjectCustomization project = projectCustomizationRepo.findById(sectionAndFieldsMapping.getProjectId())
+ .orElseThrow(() -> new IllegalArgumentException("Invalid projectId"));
+ response.setProjectId(sectionAndFieldsMapping.getProjectId());
+ }Committable suggestion was skipped due to low confidence.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/iemr/common/repo/customization/SectionAndFieldsMappingRepo.java (1)
19-28: Consider adding documentation about project-service line relationship.The addition of
projectIdeffectively solves the service line separation issue. However, consider adding documentation (either in code comments or README) explaining:
- The relationship between projects and service lines
- The significance of projectId=0 as a fallback
- When and how the fallback mechanism should be used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/com/iemr/common/data/customization/SectionAndFieldsMapping.java (2 hunks)
- src/main/java/com/iemr/common/repo/customization/SectionAndFieldsMappingRepo.java (1 hunks)
- src/main/java/com/iemr/common/service/customization/CustomizationServiceImpl.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/iemr/common/data/customization/SectionAndFieldsMapping.java
- src/main/java/com/iemr/common/service/customization/CustomizationServiceImpl.java
🔇 Additional comments (1)
src/main/java/com/iemr/common/repo/customization/SectionAndFieldsMappingRepo.java (1)
27-28: Reference existing comment about inconsistent fallback behavior.The inconsistency in fallback behavior between queries is still present, as noted in the previous review. The first query allows fallback to global settings (projectId=0) while this one doesn't.
| @Query("SELECT sfm FROM SectionAndFieldsMapping sfm WHERE sfm.fieldName = :fieldName AND (sfm.serviceProviderId = :serviceProviderId OR sfm.serviceProviderId = 0) AND (sfm.projectId = :projectId OR sfm.projectId = 0)") | ||
| List<SectionAndFieldsMapping> findSectionIdAndSectionNameAndServiceProviderId( | ||
| @Param("sectionId") Integer sectionId, @Param("serviceProviderId") Integer serviceProviderId); | ||
| @Param("sectionId") Integer sectionId, @Param("serviceProviderId") Integer serviceProviderId, @Param("projectId") Integer projectId); |
There was a problem hiding this comment.
Method name doesn't match its implementation.
The method name suggests it finds by sectionId and sectionName, but the query filters by fieldName. This mismatch could lead to confusion. Consider renaming the method to better reflect its functionality.
-List<SectionAndFieldsMapping> findSectionIdAndSectionNameAndServiceProviderId(
+List<SectionAndFieldsMapping> findByFieldNameWithFallback(Committable suggestion was skipped due to low confidence.
different-2 service line in single provider
📋 Description
JIRA ID: AMM-1002
Preprod- Unable to separate a Customization field for different 2 service line in single provider
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
projectIdfield to enhance data mapping for sections and fields.projectId, improving specificity in operations related to project and section mappings.Bug Fixes
projectIdis provided when saving section and field mappings, enhancing data integrity.projectIdin data retrieval processes to prevent errors.Documentation
projectIdfor clearer context in returned data.