#13754 - Changed the storage of phone notification#13833
#13754 - Changed the storage of phone notification#13833raulbob merged 3 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 notifier flows to create/edit PHONE_NOTIFICATION surveillance reports; adds treatment fields to SurveillanceReport DTO/entity; moves diagnosis/treatment population into doctor-declaration message flow; updates API, persistence, UI components, and 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: save/update surveillanceReport (treatment, dates)
Facade->>DB: UPDATE surveillancereport
DB-->>Facade: updated
Ctrl-->>UI: navigate back / refresh case view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 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.
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierForm.java
Show resolved
Hide resolved
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
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java
Show resolved
Hide resolved
|
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
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
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13833 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java`:
- Around line 272-275: getNewestPhoneNotificationReport can return null if all
PHONE_NOTIFICATION reports have null reportDate, but the caller branch (when
existingReports.isEmpty() is false) assigns that null to surveillanceReport and
later triggers an NPE; update the code so that when
getNewestPhoneNotificationReport(...) yields null you fall back to a non-null
report (for example the first element of existingReports) before assigning
surveillanceReport. Locate the call site in CaseNotifierSideViewController where
surveillanceReport is set and add the fallback logic, or alter
getNewestPhoneNotificationReport to return a non-null/Optional and ensure the
caller uses existingReports.get(0) as a fallback when the stream finds no dated
report.
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/notifier/CaseNotifierSideViewController.java
Show resolved
Hide resolved
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13833 |
3 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