#13754 - Changed the storage of phone notification#13833
#13754 - Changed the storage of phone notification#13833raulbob wants to merge 2 commits intodevelopmentfrom
Conversation
…eports Implement phone notification reporting functionality that allows phone notifications for cases through the use of surveillance reporting system. What's New: - Users are now createing phone notification reports for cases through the case notification panel - Phone notification report can hold now additional potential fields - Treatment information is now stored in the surveillance report - Diagnostic dates is now recorded in the surveillance report - Phone notifications can be viewed and edited by authorized users - Read-only view mode displays a close button instead of save/discard options Key Changes: - New PHONE_NOTIFICATION reporting type added to surveillance reports - Treatment fields added to surveillance report tracking - Improved UI forms for creating and editing phone notifications - Phone notifications are properly integrated into the surveillance report list - Allows additional fields to be potentially added to phone notifications
📝 WalkthroughWalkthroughRefactors notifications to create/edit PHONE_NOTIFICATION surveillance reports, adds treatment fields to SurveillanceReport/DTO, moves doctor-declaration diagnosis/treatment updates into doctor-declaration message flow, and updates UI components/controllers to operate on SurveillanceReportDto instead of CaseDataDto. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Notifier UI
participant Ctrl as CaseNotifierSideViewController
participant Facade as SurveillanceReportFacadeEjb
participant DB as Database
User->>UI: Click "Create phone notification"
UI->>Ctrl: createPhoneNotification(caseRef)
Ctrl->>Facade: create SurveillanceReport(type=PHONE_NOTIFICATION)
Facade->>DB: INSERT surveillancereport
DB-->>Facade: created
Ctrl->>UI: openEditWindow(surveillanceReport)
User->>UI: Fill treatment/diagnosis, Submit
UI->>Ctrl: updateSurveillanceReportFromForm(surveillanceReport)
Ctrl->>Facade: update surveillanceReport (treatment, dates)
Facade->>DB: UPDATE surveillancereport
DB-->>Facade: updated
Ctrl->>UI: navigate back to case / refresh view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewContent.java (1)
135-147:⚠️ Potential issue | 🔴 CriticalPotential
NullPointerExceptionongetAgentLastName().toUpperCase().The outer condition uses
||, so the block is entered if eitheragentFirstNameoragentLastNameis non-blank. However, line 144 unconditionally callsnotifier.getAgentLastName().toUpperCase(Locale.ROOT), which will throw an NPE if onlyagentFirstNameis populated andagentLastNameisnull. The same applies togetAgentFirstName()which would concatenate as the literal string"null".🐛 Proposed fix — null-safe agent name formatting
- Label agentNameLabel = new Label(notifier.getAgentFirstName() + " " + notifier.getAgentLastName().toUpperCase(Locale.ROOT)); + String agentFirst = notifier.getAgentFirstName() != null ? notifier.getAgentFirstName() : ""; + String agentLast = notifier.getAgentLastName() != null ? notifier.getAgentLastName().toUpperCase(Locale.ROOT) : ""; + Label agentNameLabel = new Label((agentFirst + " " + agentLast).trim());
🤖 Fix all issues with AI agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierForm.java`:
- Around line 242-273: In CaseNotifierForm, remove the duplicate population of
diagnosticDateField and the early notificationDateField assignment: instead of
setting diagnosticDateField from surveillanceReport.getDateOfDiagnosis() in the
first if-block and again later, and setting notificationDateField to
LocalDate.now() only to be overwritten, consolidate so that
notificationDateField is set to surveillanceReport.getReportDate() (converted to
LocalDate) when surveillanceReport != null, otherwise set it to
java.time.LocalDate.now(); set diagnosticDateField only once from
surveillanceReport.getDateOfDiagnosis() when surveillanceReport != null; keep
the treatmentGroup logic as-is.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java`:
- Around line 65-75: The branch wrongly treats cases with
hasPhoneNotification=true but hasNotifier=false as "create" cases; change the
conditional logic in CaseNotifierSideViewComponent so hasPhoneNotification is
checked first and, if true, render the edit/update UI instead of the create UI
(i.e., do not execute the createPhoneNotification/create button path when
hasPhoneNotification is true), otherwise fall back to the existing
hasNoNotificationReports || !hasNotifier check; update the code paths around
controller.createPhoneNotification and addCreateButton to ensure the edit UI
(the counterpart to addCreateButton) is shown for existing phone notifications
and navigation (ControllerProvider.getCaseController().navigateToCase) remains
the same after edits.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewContent.java`:
- Around line 111-115: The code in CaseNotifierSideViewContent sets
notificationDateField from surveillanceReport.getReportDate() or falls back to
notifier.getChangeDate() without guarding notifier.getChangeDate() for null,
which can cause a NullPointerException; update the block around
notificationDateField.setValue(...) to check notifier.getChangeDate() != null
before calling .toInstant() (or use Optional) and if both dates are null call
notificationDateField.setValue(null) or leave the field unchanged—ensure you
reference surveillanceReport, surveillanceReport.getReportDate(),
notifier.getChangeDate(), and notificationDateField when applying the null
guard.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java`:
- Around line 101-109: The method hasNoNotificationReports currently calls
FacadeProvider.getSurveillanceReportFacade().count(caseCriteria) twice and
discards the first result; change it to call count only once by storing the
result in a local variable (e.g., int count =
FacadeProvider.getSurveillanceReportFacade().count(caseCriteria)) and then
return count == 0. Update the method in CaseNotifierSideViewController to remove
the redundant count() invocation and use the stored variable when returning.
- Around line 111-117: The method hasOnlyPhoneNotificationReports currently
returns true when there are zero PHONE_NOTIFICATION reports and only queries
phone reports; change it to compute both the number of PHONE_NOTIFICATION
reports and the total number of reports for the given case and return true only
when totalCount > 0 and phoneCount == totalCount. Use SurveillanceReportCriteria
(setReportingType(ReportingType.PHONE_NOTIFICATION)) to get phoneCount and a
criteria without reportingType (or with no filter) to get totalCount via
FacadeProvider.getSurveillanceReportFacade().count(...), and update
hasOnlyPhoneNotificationReports(CaseReferenceDto caze) to return totalCount > 0
&& phoneCount == totalCount.
- Around line 250-265: The code currently picks existingReports.get(0) which is
unordered; change selection to deterministically use the newest
PHONE_NOTIFICATION report by reportDate (either call the facade method that
returns the newest phone notification report, e.g.,
getNewestPhoneNotificationReport(caze.toReference()), or compute the max from
existingReports using a comparator on SurveillanceReportDto.getReportDate()) so
surveillanceReport is always the most recent report rather than an arbitrary
list element in CaseNotifierSideViewController.
- Around line 335-370: The updateSurveillanceReportFromForm method currently
ignores cleared date fields because it only updates surveillanceReport when
notifierForm.getNotificationDate() or getDiagnosticDate() are non-null; modify
updateSurveillanceReportFromForm to explicitly set
surveillanceReport.setReportDate(null) when notifierForm.getNotificationDate()
== null and surveillanceReport.setDateOfDiagnosis(null) when
notifierForm.getDiagnosticDate() == null so that cleared values from the
CaseNotifierForm are persisted (locate logic around
getNotificationDate/getDiagnosticDate and surveillanceReport setters).
🧹 Nitpick comments (8)
sormas-backend/src/main/resources/sql/sormas_schema.sql (1)
15250-15250: Consider whetherDEFAULT falseis appropriate for existing surveillance reports.When this migration runs, all existing rows in
surveillancereports(andsurveillancereports_history) will gettreatmentnotapplicable = false. This is semantically correct if "false" means "treatment applicability was not explicitly marked as N/A" — i.e., the field is unset/default. Just confirm this aligns with the application's interpretation, sincefalsecould also be read as "treatment IS applicable," which is a different assertion for pre-existing reports that had no notion of this field.If the intent is "unknown/unset," using
NULL(no default) might be more semantically precise, consistent with the other two new columns.Consider using NULL default for consistency
-ALTER TABLE surveillancereports ADD COLUMN treatmentnotapplicable boolean DEFAULT false; +ALTER TABLE surveillancereports ADD COLUMN treatmentnotapplicable boolean; ... -ALTER TABLE surveillancereports_history ADD COLUMN treatmentnotapplicable boolean DEFAULT false; +ALTER TABLE surveillancereports_history ADD COLUMN treatmentnotapplicable boolean;sormas-api/src/main/java/de/symeda/sormas/api/caze/surveillancereport/SurveillanceReportDto.java (1)
87-91: Consider using boxedBooleanfortreatmentNotApplicablefor consistency and null-safety.The field uses primitive
boolean, which defaults tofalseand cannot represent an "unset" state. This means existing surveillance reports (before the migration) will implicitly havetreatmentNotApplicable = false, and you lose the ability to distinguish "not applicable = false" from "never set". The other treatment fields (treatmentStarted,treatmentStartDate) are nullable; this one stands out as always having a value.If this is intentional (i.e.,
falseis the correct default for all existing records), this is fine — but worth confirming.sormas-ui/src/main/java/de/symeda/sormas/ui/caze/surveillancereport/SurveillanceReportList.java (1)
162-176: PHONE_NOTIFICATION button is added but always disabled — consider not adding it at all.The action button for
PHONE_NOTIFICATIONentries is always created withfalse(disabled), making it non-interactive. If the intent is to prevent editing from the side view (as the comment states), it may be cleaner to skip adding the action button entirely for phone notifications, rather than showing a disabled edit icon.Also, minor typo on line 163: "frome" → "from".
Suggested approach
if (ReportingType.PHONE_NOTIFICATION.equals(report.getReportingType())) { - // we do not allow editing frome the side view because we cannot reload the case page + // we do not allow editing from the side view because we cannot reload the case page // phone notifications are only editable from the case page notification box - listEntry.addActionButton( - report.getUuid(), - (Button.ClickListener) event -> ControllerProvider.getCaseNotifierSideViewController() - .editPhoneNotification(caseRef, this::reload, false), - false); + // No action button: phone notifications are edited from the case notification panel } else {sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java (1)
41-41: Non-serializable controller stored as a field in a Vaadin component.
CaseNotifierSideViewControllerobtained fromControllerProvideris stored as a transient field. Whiletransientprevents serialization issues, accessingcontrollerafter session deserialization would yieldnull, causing NPEs on user interaction. This is acceptable if Vaadin session serialization is not used, but worth being aware of.sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewContent.java (1)
60-69: Constructor allowsnullsurveillanceReportbut the class uses it with null-guards — verify intent.The constructor does not validate against a
nullsurveillanceReport(unlikeCaseNotifierFormwhich throwsIllegalArgumentException). The code handles nulls at usage sites (lines 111, 120, 154, 179), so this appears intentional. However, the inconsistency withCaseNotifierFormis worth noting — if callers always provide a non-null report, you could simplify by adding a null check here and removing the scattered guards.sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierForm.java (1)
342-367: Validation only checks three required fields — consider whether diagnostic date needs validation.The diagnostic date is user-editable but not validated here. If a diagnostic date is required by business rules, it should be checked. If it's optional, this is fine as-is.
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java (2)
55-69:getNotifierComponentacceptsSurveillanceReportDtobut does not null-check it.The method validates
cazeandcaze.getNotifier()but passessurveillanceReportstraight through toCaseNotifierSideViewContent.CaseNotifierSideViewContenthandles null internally, so this may be intentional, but it's inconsistent with the other parameter checks in this method.
351-356: Auto-populatingtreatmentStartDatewithnew Date()may be surprising.When the treatment option is
YESandtreatmentStartDateis currentlynull, the code auto-fills it with the current timestamp. This could be inaccurate if the actual treatment start date differs. Consider whether this should be leftnullfor the user to fill in, or at least documented in the UI.
|
|
||
| // Set diagnostic date if available | ||
| if (surveillanceReport != null && surveillanceReport.getDateOfDiagnosis() != null) { | ||
| diagnosticDateField.setValue(surveillanceReport.getDateOfDiagnosis().toInstant().atZone(ZoneId.systemDefault()).toLocalDate()); | ||
| } | ||
| } else { | ||
| // For new notifier, set notification date to current date | ||
| notificationDateField.setValue(java.time.LocalDate.now()); | ||
| } | ||
|
|
||
| treatmentGroup.clear(); | ||
|
|
||
| if (caze != null) { | ||
| if (caze.isTreatmentNotApplicable()) { | ||
| if (surveillanceReport != null) { | ||
| // Read treatment values from surveillance report | ||
| if (surveillanceReport.isTreatmentNotApplicable()) { | ||
| treatmentGroup.setValue(TreatmentOption.NOT_APPLICABLE); | ||
| } else { | ||
| final YesNoUnknown treatmentStarted = caze.getTreatmentStarted(); | ||
| final YesNoUnknown treatmentStarted = surveillanceReport.getTreatmentStarted(); | ||
| if (treatmentStarted != null) { | ||
| treatmentGroup.setValue(TreatmentOption.forValue(treatmentStarted)); | ||
| } | ||
| } | ||
|
|
||
| // Set notification date from report date | ||
| if (surveillanceReport.getReportDate() != null) { | ||
| notificationDateField.setValue(surveillanceReport.getReportDate().toInstant().atZone(ZoneId.systemDefault()).toLocalDate()); | ||
| } | ||
|
|
||
| // Set diagnostic date if available | ||
| if (surveillanceReport.getDateOfDiagnosis() != null) { | ||
| diagnosticDateField.setValue(surveillanceReport.getDateOfDiagnosis().toInstant().atZone(ZoneId.systemDefault()).toLocalDate()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate diagnostic date population — the same field is set twice from the same source.
diagnosticDateField is set from surveillanceReport.getDateOfDiagnosis() at lines 244–246, and then again at lines 271–273. Since surveillanceReport is guaranteed non-null by the constructor, the first assignment (lines 244–246) is always overwritten by the second. Similarly, notificationDateField set from the notifier's creation/change date (lines 237–241) is immediately overwritten by surveillanceReport.getReportDate() at lines 266–268.
Consider removing the redundant assignments in the first block (lines 237–246) and consolidating the logic.
♻️ Proposed consolidation
if (notifier != null) {
firstNameField.setValue(notifier.getFirstName() != null ? notifier.getFirstName() : "");
lastNameField.setValue(notifier.getLastName() != null ? notifier.getLastName() : "");
registrationNumberField.setValue(notifier.getRegistrationNumber() != null ? notifier.getRegistrationNumber() : "");
phoneField.setValue(notifier.getPhone() != null ? notifier.getPhone() : "");
emailField.setValue(notifier.getEmail() != null ? notifier.getEmail() : "");
addressField.setValue(notifier.getAddress() != null ? notifier.getAddress() : "");
agentFirstNameField.setValue(notifier.getAgentFirstName() != null ? notifier.getAgentFirstName() : "");
agentLastNameField.setValue(notifier.getAgentLastName() != null ? notifier.getAgentLastName() : "");
// Set notification date to notifier's creation/modification date
if (notifier.getCreationDate() != null) {
notificationDateField.setValue(notifier.getCreationDate().toInstant().atZone(ZoneId.systemDefault()).toLocalDate());
} else if (notifier.getChangeDate() != null) {
notificationDateField.setValue(notifier.getChangeDate().toInstant().atZone(ZoneId.systemDefault()).toLocalDate());
}
-
- // Set diagnostic date if available
- if (surveillanceReport != null && surveillanceReport.getDateOfDiagnosis() != null) {
- diagnosticDateField.setValue(surveillanceReport.getDateOfDiagnosis().toInstant().atZone(ZoneId.systemDefault()).toLocalDate());
- }
} else {
// For new notifier, set notification date to current date
notificationDateField.setValue(java.time.LocalDate.now());
}🤖 Prompt for AI Agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierForm.java`
around lines 242 - 273, In CaseNotifierForm, remove the duplicate population of
diagnosticDateField and the early notificationDateField assignment: instead of
setting diagnosticDateField from surveillanceReport.getDateOfDiagnosis() in the
first if-block and again later, and setting notificationDateField to
LocalDate.now() only to be overwritten, consolidate so that
notificationDateField is set to surveillanceReport.getReportDate() (converted to
LocalDate) when surveillanceReport != null, otherwise set it to
java.time.LocalDate.now(); set diagnosticDateField only once from
surveillanceReport.getDateOfDiagnosis() when surveillanceReport != null; keep
the treatmentGroup logic as-is.
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java
Outdated
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewContent.java
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java
Show resolved
Hide resolved
| private void updateSurveillanceReportFromForm(SurveillanceReportDto surveillanceReport, CaseNotifierForm notifierForm) { | ||
| // Update report date from notification date | ||
| final LocalDate notificationDate = notifierForm.getNotificationDate(); | ||
| if (notificationDate != null) { | ||
| surveillanceReport.setReportDate(Date.from(notificationDate.atStartOfDay(ZoneId.systemDefault()).toInstant())); | ||
| } | ||
|
|
||
| if (selectedOption.equals(TreatmentOption.YES)) { | ||
| caze.setTreatmentStarted(YesNoUnknown.YES); | ||
| caze.setTreatmentNotApplicable(false); | ||
| if (caze.getTreatmentStartDate() == null) { | ||
| caze.setTreatmentStartDate(new java.util.Date()); | ||
| } | ||
| return; | ||
| // Update diagnosis date if provided | ||
| final LocalDate diagnosticDate = notifierForm.getDiagnosticDate(); | ||
| if (diagnosticDate != null) { | ||
| surveillanceReport.setDateOfDiagnosis(Date.from(diagnosticDate.atStartOfDay(ZoneId.systemDefault()).toInstant())); | ||
| } | ||
|
|
||
| if (selectedOption.equals(TreatmentOption.NO)) { | ||
| caze.setTreatmentStarted(YesNoUnknown.NO); | ||
| caze.setTreatmentNotApplicable(false); | ||
| caze.setTreatmentStartDate(null); | ||
| return; | ||
| } | ||
|
|
||
| if (selectedOption.equals(TreatmentOption.NOT_APPLICABLE)) { | ||
| caze.setTreatmentNotApplicable(true); | ||
| caze.setTreatmentStarted(null); | ||
| caze.setTreatmentStartDate(null); | ||
| return; | ||
| } | ||
|
|
||
| if (selectedOption.equals(TreatmentOption.UNKNOWN)) { | ||
| caze.setTreatmentStarted(YesNoUnknown.UNKNOWN); | ||
| caze.setTreatmentNotApplicable(false); | ||
| caze.setTreatmentStartDate(null); | ||
| // Update treatment based on selected option | ||
| final TreatmentOption selectedOption = notifierForm.getSelectedTreatmentOption(); | ||
| if (selectedOption != null) { | ||
| if (selectedOption.equals(TreatmentOption.YES)) { | ||
| surveillanceReport.setTreatmentStarted(YesNoUnknown.YES); | ||
| surveillanceReport.setTreatmentNotApplicable(false); | ||
| if (surveillanceReport.getTreatmentStartDate() == null) { | ||
| surveillanceReport.setTreatmentStartDate(new Date()); | ||
| } | ||
| } else if (selectedOption.equals(TreatmentOption.NO)) { | ||
| surveillanceReport.setTreatmentStarted(YesNoUnknown.NO); | ||
| surveillanceReport.setTreatmentNotApplicable(false); | ||
| surveillanceReport.setTreatmentStartDate(null); | ||
| } else if (selectedOption.equals(TreatmentOption.NOT_APPLICABLE)) { | ||
| surveillanceReport.setTreatmentNotApplicable(true); | ||
| surveillanceReport.setTreatmentStarted(null); | ||
| surveillanceReport.setTreatmentStartDate(null); | ||
| } else if (selectedOption.equals(TreatmentOption.UNKNOWN)) { | ||
| surveillanceReport.setTreatmentStarted(YesNoUnknown.UNKNOWN); | ||
| surveillanceReport.setTreatmentNotApplicable(false); | ||
| surveillanceReport.setTreatmentStartDate(null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clearing dates is not handled — user-removed values won't be persisted.
If a user clears diagnosticDate (line 344) or notificationDate (line 338), the if (date != null) guards prevent updating the report, so the old value remains. This means users cannot remove a previously set date.
Consider explicitly setting null on the report when the form field is empty.
🔧 Proposed fix
private void updateSurveillanceReportFromForm(SurveillanceReportDto surveillanceReport, CaseNotifierForm notifierForm) {
// Update report date from notification date
final LocalDate notificationDate = notifierForm.getNotificationDate();
if (notificationDate != null) {
surveillanceReport.setReportDate(Date.from(notificationDate.atStartOfDay(ZoneId.systemDefault()).toInstant()));
}
// Update diagnosis date if provided
final LocalDate diagnosticDate = notifierForm.getDiagnosticDate();
- if (diagnosticDate != null) {
- surveillanceReport.setDateOfDiagnosis(Date.from(diagnosticDate.atStartOfDay(ZoneId.systemDefault()).toInstant()));
- }
+ surveillanceReport.setDateOfDiagnosis(
+ diagnosticDate != null
+ ? Date.from(diagnosticDate.atStartOfDay(ZoneId.systemDefault()).toInstant())
+ : null);🤖 Prompt for AI Agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java`
around lines 335 - 370, The updateSurveillanceReportFromForm method currently
ignores cleared date fields because it only updates surveillanceReport when
notifierForm.getNotificationDate() or getDiagnosticDate() are non-null; modify
updateSurveillanceReportFromForm to explicitly set
surveillanceReport.setReportDate(null) when notifierForm.getNotificationDate()
== null and surveillanceReport.setDateOfDiagnosis(null) when
notifierForm.getDiagnosticDate() == null so that cleared values from the
CaseNotifierForm are persisted (locate logic around
getNotificationDate/getDiagnosticDate and surveillanceReport setters).
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13833 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java`:
- Around line 77-91: When hasPhoneNotification is true but hasNotifier is false,
avoid calling controller.getNotifierComponent(caze, phoneNotificationReport) and
creating the edit button because getNotifierComponent throws
IllegalArgumentException when caze.getNotifier() is null; change the branch in
CaseNotifierSideViewComponent to check both hasPhoneNotification and hasNotifier
before calling getNotifierComponent and
ButtonHelper.createIconButton/editPhoneNotification, and in the fallback
(hasPhoneNotification && !hasNotifier) render a safe alternative such as a
"Create notifier" button via addCreateButton or a descriptive label indicating
inconsistent data so no null notifier is dereferenced and editPhoneNotification
is not invoked with a null notifier.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java`:
- Around line 89-99: NullPointerException can occur when comparing reports with
a null reportDate; update both getOldestDoctorDeclarationReport and
getNewestPhoneNotificationReport to use a null-safe comparator (e.g.,
Comparator.comparing(SurveillanceReportDto::getReportDate,
Comparator.nullsFirst/Last(Comparator.naturalOrder()))) or filter out null
reportDate entries before calling min/max so
Comparator.comparing(SurveillanceReportDto::getReportDate) cannot receive null
values; ensure you adjust the tie-breaking/null-ordering choice consistently for
both methods.
- Around line 109-121: The method hasOnlyPhoneNotificationReports currently
returns true when both phoneRepCount and caseRepCount are zero; update its
return logic to ensure there is at least one report by requiring caseRepCount >
0 (or phoneRepCount > 0) in addition to equality. Specifically, in
hasOnlyPhoneNotificationReports use the existing phoneRepCount and caseRepCount
variables (and the existing SurveillanceReportCriteria /
ReportingType.PHONE_NOTIFICATION logic) but change the final check to verify
that reports exist (e.g., caseRepCount > 0) AND phoneRepCount == caseRepCount so
the method only returns true when all existing reports are phone notifications
and there is at least one report.
🧹 Nitpick comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java (1)
99-113: Silent no-ops in button click handler may confuse users.When the button is clicked, if the report or external message is
null(lines 102-108), the handler returns silently with no user feedback. Consider showing a notification or disabling the button when no external message is available.
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java
Show resolved
Hide resolved
| public SurveillanceReportDto getOldestDoctorDeclarationReport(CaseReferenceDto caze) { | ||
|
|
||
| SurveillanceReportCriteria criteria = new SurveillanceReportCriteria(); | ||
| criteria.caze(caze); | ||
| criteria.setReportingType(ReportingType.DOCTOR); | ||
|
|
||
| List<SurveillanceReportDto> reports = FacadeProvider.getSurveillanceReportFacade().getIndexList(criteria, null, null, null); | ||
| // Get reports with DOCTOR reporting type | ||
| SurveillanceReportCriteria doctorCriteria = new SurveillanceReportCriteria(); | ||
| doctorCriteria.caze(caze); | ||
| doctorCriteria.setReportingType(ReportingType.DOCTOR); | ||
| List<SurveillanceReportDto> doctorReports = FacadeProvider.getSurveillanceReportFacade().getIndexList(doctorCriteria, null, null, null); | ||
|
|
||
| // Filter to get the oldest report | ||
| return reports.stream() | ||
| .min(Comparator.comparing(SurveillanceReportDto::getReportDate)) // Assuming getDate() returns the report date | ||
| .orElse(null); | ||
| return doctorReports.stream().min(Comparator.comparing(SurveillanceReportDto::getReportDate)).orElse(null); | ||
| } |
There was a problem hiding this comment.
Potential NPE in comparators if reportDate is null on any report.
Both getOldestDoctorDeclarationReport (line 98) and getNewestPhoneNotificationReport (line 143) use Comparator.comparing(SurveillanceReportDto::getReportDate) which will throw NullPointerException if any report has a null reportDate.
Proposed fix — use null-safe comparator
- return doctorReports.stream().min(Comparator.comparing(SurveillanceReportDto::getReportDate)).orElse(null);
+ return doctorReports.stream().min(Comparator.comparing(SurveillanceReportDto::getReportDate, Comparator.nullsLast(Comparator.naturalOrder()))).orElse(null);- return phoneReports.stream().max(Comparator.comparing(SurveillanceReportDto::getReportDate)).orElse(null);
+ return phoneReports.stream().max(Comparator.comparing(SurveillanceReportDto::getReportDate, Comparator.nullsFirst(Comparator.naturalOrder()))).orElse(null);Also applies to: 137-144
🤖 Prompt for AI Agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java`
around lines 89 - 99, NullPointerException can occur when comparing reports with
a null reportDate; update both getOldestDoctorDeclarationReport and
getNewestPhoneNotificationReport to use a null-safe comparator (e.g.,
Comparator.comparing(SurveillanceReportDto::getReportDate,
Comparator.nullsFirst/Last(Comparator.naturalOrder()))) or filter out null
reportDate entries before calling min/max so
Comparator.comparing(SurveillanceReportDto::getReportDate) cannot receive null
values; ensure you adjust the tie-breaking/null-ordering choice consistently for
both methods.
| public boolean hasOnlyPhoneNotificationReports(CaseReferenceDto caze) { | ||
| // Get reports with PHONE_NOTIFICATION reporting type | ||
| final SurveillanceReportCriteria phoneCriteria = new SurveillanceReportCriteria(); | ||
| phoneCriteria.caze(caze); | ||
| phoneCriteria.setReportingType(ReportingType.PHONE_NOTIFICATION); | ||
| final long phoneRepCount = FacadeProvider.getSurveillanceReportFacade().count(phoneCriteria); | ||
|
|
||
| final SurveillanceReportCriteria caseCriteria = new SurveillanceReportCriteria(); | ||
| caseCriteria.caze(caze); | ||
| final long caseRepCount = FacadeProvider.getSurveillanceReportFacade().count(caseCriteria); | ||
|
|
||
| return phoneRepCount == caseRepCount; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check where hasOnlyPhoneNotificationReports is called to assess impact
rg -n 'hasOnlyPhoneNotificationReports' --type=javaRepository: SORMAS-Foundation/SORMAS-Project
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Search for method usage in all files, not just Java
rg -n 'hasOnlyPhoneNotificationReports' --type-list
# Check all file types
rg 'hasOnlyPhoneNotificationReports'Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 652
🏁 Script executed:
#!/bin/bash
# Search for method usage in all files without type restrictions
rg 'hasOnlyPhoneNotificationReports' -iRepository: SORMAS-Foundation/SORMAS-Project
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Read the file around the method definition with context
head -n 150 sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java | tail -n +1Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 6417
🏁 Script executed:
#!/bin/bash
# Search for test files related to CaseNotifierSideViewController
find . -name "*CaseNotifierSideViewController*" -o -name "*Test.java" | grep -i "notifier" | head -20Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Search for method calls in test files
grep -n 'hasOnlyPhoneNotificationReports' ./sormas-backend/src/test/java/de/symeda/sormas/backend/person/notifier/NotifierCaseIntegrationTest.java ./sormas-backend/src/test/java/de/symeda/sormas/backend/person/notifier/NotifierEjbTest.javaRepository: SORMAS-Foundation/SORMAS-Project
Length of output: 58
🏁 Script executed:
#!/bin/bash
# Check if this class is referenced or used as a component/endpoint
rg 'CaseNotifierSideViewController' --type=javaRepository: SORMAS-Foundation/SORMAS-Project
Length of output: 1201
🏁 Script executed:
#!/bin/bash
# Check if hasOnlyPhoneNotificationReports is called in the files that use the controller
rg 'hasOnlyPhoneNotificationReports' sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewComponent.java sormas-ui/src/main/java/de/symeda/sormas/ui/caze/surveillancereport/SurveillanceReportList.javaRepository: SORMAS-Foundation/SORMAS-Project
Length of output: 58
🏁 Script executed:
#!/bin/bash
# Let's check the git history to see if this method is newly added
cd sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier
git log --oneline -n 20 CaseNotifierSideViewController.java 2>/dev/null | head -10Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Check the diff for the recent commit to see when hasOnlyPhoneNotificationReports was added
git show --stat 354f6857e6 | head -50Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 2986
🏁 Script executed:
#!/bin/bash
# Check the actual changes to CaseNotifierSideViewController in this commit
git show 354f6857e6 -- sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java | head -200Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 8694
hasOnlyPhoneNotificationReports returns true when there are zero reports, contradicting its semantic meaning.
When both phoneRepCount and caseRepCount are 0, the comparison phoneRepCount == caseRepCount evaluates to true. The method name implies that at least one phone notification report should exist. Add a check to ensure reports actually exist:
Proposed fix
public boolean hasOnlyPhoneNotificationReports(CaseReferenceDto caze) {
// Get reports with PHONE_NOTIFICATION reporting type
final SurveillanceReportCriteria phoneCriteria = new SurveillanceReportCriteria();
phoneCriteria.caze(caze);
phoneCriteria.setReportingType(ReportingType.PHONE_NOTIFICATION);
final long phoneRepCount = FacadeProvider.getSurveillanceReportFacade().count(phoneCriteria);
final SurveillanceReportCriteria caseCriteria = new SurveillanceReportCriteria();
caseCriteria.caze(caze);
final long caseRepCount = FacadeProvider.getSurveillanceReportFacade().count(caseCriteria);
- return phoneRepCount == caseRepCount;
+ return caseRepCount > 0 && phoneRepCount == caseRepCount;
}📝 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.
| public boolean hasOnlyPhoneNotificationReports(CaseReferenceDto caze) { | |
| // Get reports with PHONE_NOTIFICATION reporting type | |
| final SurveillanceReportCriteria phoneCriteria = new SurveillanceReportCriteria(); | |
| phoneCriteria.caze(caze); | |
| phoneCriteria.setReportingType(ReportingType.PHONE_NOTIFICATION); | |
| final long phoneRepCount = FacadeProvider.getSurveillanceReportFacade().count(phoneCriteria); | |
| final SurveillanceReportCriteria caseCriteria = new SurveillanceReportCriteria(); | |
| caseCriteria.caze(caze); | |
| final long caseRepCount = FacadeProvider.getSurveillanceReportFacade().count(caseCriteria); | |
| return phoneRepCount == caseRepCount; | |
| } | |
| public boolean hasOnlyPhoneNotificationReports(CaseReferenceDto caze) { | |
| // Get reports with PHONE_NOTIFICATION reporting type | |
| final SurveillanceReportCriteria phoneCriteria = new SurveillanceReportCriteria(); | |
| phoneCriteria.caze(caze); | |
| phoneCriteria.setReportingType(ReportingType.PHONE_NOTIFICATION); | |
| final long phoneRepCount = FacadeProvider.getSurveillanceReportFacade().count(phoneCriteria); | |
| final SurveillanceReportCriteria caseCriteria = new SurveillanceReportCriteria(); | |
| caseCriteria.caze(caze); | |
| final long caseRepCount = FacadeProvider.getSurveillanceReportFacade().count(caseCriteria); | |
| return caseRepCount > 0 && phoneRepCount == caseRepCount; | |
| } |
🤖 Prompt for AI Agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java`
around lines 109 - 121, The method hasOnlyPhoneNotificationReports currently
returns true when both phoneRepCount and caseRepCount are zero; update its
return logic to ensure there is at least one report by requiring caseRepCount >
0 (or phoneRepCount > 0) in addition to equality. Specifically, in
hasOnlyPhoneNotificationReports use the existing phoneRepCount and caseRepCount
variables (and the existing SurveillanceReportCriteria /
ReportingType.PHONE_NOTIFICATION logic) but change the final check to verify
that reports exist (e.g., caseRepCount > 0) AND phoneRepCount == caseRepCount so
the method only returns true when all existing reports are phone notifications
and there is at least one report.
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13833 |
2 participants
Fixes #13754
Implement phone notification reporting functionality that allows phone notifications for cases through the use of surveillance reporting system.
What's New:
Key Changes:
Summary by CodeRabbit
New Features
Improvements