Merged
Conversation
- Add null safety checks in CaseManagementPrint getAllNoteIds methods - Fix copy-paste bug in filter_issues session attribute check - Add defensive null handling for NoteSelectionResult and note collections - Improve role validation with proper null checks in filterNotes1 method - Ensure methods return empty arrays instead of null to prevent NPE
Reviewer's GuideThis PR refactors CaseManagementPrint to fetch and filter notes directly from the manager using Java streams and cleans up session-based filtering, while also introducing null/format validation and logging when resolving note roles in CaseManagementManagerImpl. Class diagram for updated CaseManagementPrint and CaseManagementManagerImplclassDiagram
class CaseManagementPrint {
+doPrint(LoggedInInfo, Integer, boolean, ...)
-getAllNoteIds(LoggedInInfo, HttpServletRequest, String)
-getAllNoteIdsWithinDateRange(LoggedInInfo, HttpServletRequest, String, Date, Date)
}
class CaseManagementManagerImpl {
+filterNotes1(String, Collection<EChartNoteEntry>)
}
class CaseManagementNote {
+getId()
+getProviderNo()
+getObservation_date()
+isLocked()
+isArchived()
+getRole()
+getType()
}
class CaseManagementNoteExt {
+getKeyVal()
}
class NoteSelectionResult {
+getNotes()
}
class NoteDisplay {
+getNoteId()
}
class NoteDisplayLocal {
}
class Secrole {
+getName()
}
class RoleCache {
+getRole(Long)
}
CaseManagementPrint --> CaseManagementManagerImpl : uses
CaseManagementPrint --> CaseManagementNote : uses
CaseManagementPrint --> CaseManagementNoteExt : uses
CaseManagementPrint --> NoteSelectionResult : uses
CaseManagementPrint --> NoteDisplay : uses
NoteDisplayLocal --|> NoteDisplay
CaseManagementManagerImpl --> Secrole : uses
CaseManagementManagerImpl --> RoleCache : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The doPrint method is very large and handles multiple responsibilities (note retrieval, filtering, integration); consider extracting these into smaller helper methods for better readability and maintainability.
- getAllNoteIds and getAllNoteIdsWithinDateRange share nearly identical logic for building criteria and collecting IDs; extract the common code into a single parameterized method to reduce duplication.
- The enhanced null and format checks around role resolution in filterNotes1 are helpful, but consider moving the parsing and lookup logic into a dedicated utility to simplify the main filtering method.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The doPrint method is very large and handles multiple responsibilities (note retrieval, filtering, integration); consider extracting these into smaller helper methods for better readability and maintainability.
- getAllNoteIds and getAllNoteIdsWithinDateRange share nearly identical logic for building criteria and collecting IDs; extract the common code into a single parameterized method to reduce duplication.
- The enhanced null and format checks around role resolution in filterNotes1 are helpful, but consider moving the parsing and lookup logic into a dedicated utility to simplify the main filtering method.
## Individual Comments
### Comment 1
<location> `src/main/java/ca/openosp/openo/casemgmt/service/CaseManagementManagerImpl.java:1479` </location>
<code_context>
if (cmNote.getType().equals("local_note")) {
noteRole = cmNote.getRole();
- noteRoleName = RoleCache.getRole(Long.valueOf(noteRole)).getName().toLowerCase();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the nested role name extraction logic into a helper method to flatten and clarify the loop body.
You’ve added a lot of nested checks/logging around turning a raw `String` into a lowercase role name. You can collapse all of that into one small helper and keep the loop body flat. For example:
```java
// in your class
private String safeGetRoleName(String roleIdStr) {
if (roleIdStr == null || roleIdStr.trim().isEmpty()) {
logger.debug("Note has null or empty role");
return "";
}
try {
Long roleId = Long.valueOf(roleIdStr);
logger.debug("Looking up role ID: {}", roleId);
Secrole secrole = RoleCache.getRole(roleId);
if (secrole != null) {
String name = secrole.getName().toLowerCase();
logger.debug("Found role: {} for ID: {}", name, roleId);
return name;
}
} catch (NumberFormatException e) {
logger.warn("Invalid role format: '{}' - not a valid Long", roleIdStr);
}
return "";
}
```
Then simplify your loop to:
```java
for (EChartNoteEntry cmNote : notes) {
if (!cmNote.getType().equals("local_note") &&
!cmNote.getType().equals("remote_note")) {
filteredNotes.add(cmNote);
continue;
}
String noteRoleName;
if (cmNote.getType().equals("local_note")) {
noteRoleName = safeGetRoleName(cmNote.getRole());
} else { // remote_note
noteRoleName = cmNote.getRole();
}
ProgramAccess pa = (ProgramAccess) programAccessMap
.get("read " + noteRoleName + " notes");
if (pa != null && (pa.isAllRoles() || isRoleIncludedInAccess(pa, role))) {
// ...
}
}
```
This preserves all logging, exception handling, and behavior but removes deep nesting and makes the loop body much clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/ca/openosp/openo/casemgmt/service/CaseManagementManagerImpl.java
Show resolved
Hide resolved
This was
linked to
issues
Sep 29, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes made
CaseManagementPrint.doPrint().CaseManagementPrint.caseManagementMgrdirectly, instead of using session attributes. These attributes are not consistently set between theCaseManagementViewandCaseManagementEntrystruts actions.CaseManagementManagerImplwhen getting role and roleName.Additional changes as of 29/09/2025:
CaseManagementPrint.doPrint().Summary by Sourcery
Improve the eChart notes printing workflow by refactoring note retrieval and filtering logic using streams, and harden role resolution in CaseManagementManagerImpl with null and format checks
Bug Fixes:
Enhancements: