Conversation
WalkthroughAdjusts disease defaults and pathogen-test associations, adds RSV variant captions and database entry, extends date-comparison validator API and switches several validators to inclusive mode, and introduces RSV-specific UI caption/visibility logic plus dynamic hospitalization-reason validation for selected diseases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PathogenTestForm
participant TestDateField
participant Validator
User->>PathogenTestForm: Open form
PathogenTestForm->>TestDateField: Initialize validators (date-only or time-aware based on config)
User->>TestDateField: Enter date/time
TestDateField->>PathogenTestForm: valueChange
PathogenTestForm->>Validator: replace/add DateComparisonValidator (dateOnly true/false)
TestDateField->>Validator: validate
Validator-->>User: validation result
sequenceDiagram
participant User
participant CaseDataForm
participant DiseaseSelect
participant VariantField
User->>CaseDataForm: Select disease via DiseaseSelect
DiseaseSelect->>CaseDataForm: selected disease
alt disease == RESPIRATORY_SYNCYTIAL_VIRUS
CaseDataForm->>VariantField: set RSV captions
CaseDataForm->>VariantField: set visible = true
else
CaseDataForm->>VariantField: set visible = false
end
sequenceDiagram
participant User
participant HospitalizationForm
participant AdmittedField
participant CurrentlyHospitalizedField
participant ReasonField
HospitalizationForm->>AdmittedField: attach listener (for GIARDIASIS/CRYPTOSPORIDIOSIS)
HospitalizationForm->>CurrentlyHospitalizedField: attach listener
User->>AdmittedField: choose Yes/No
AdmittedField->>ReasonField: set required true/false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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/samples/AbstractSampleForm.java (1)
284-290: Critical: Validation logic inverted - shipmentDate must be after sampleDateField, not before.The change from
falsetotruefor theearlierOrSameparameter inverts the validation logic. Based on theDateComparisonValidatorimplementation:
earlierOrSame=truevalidates thatshipmentDate <= sampleDateField(shipment before/equal to sample)earlierOrSame=falsevalidates thatshipmentDate >= sampleDateField(shipment after/equal to sample)The error message on Line 290 states "afterDate", indicating that shipmentDate should be after or equal to sampleDateField. The correct parameter value should be
false, nottrue.Apply this diff to fix the validation logic:
shipmentDate.addValidator( new DateComparisonValidator( shipmentDate, sampleDateField, - true, + false, false, I18nProperties.getValidationError(Validations.afterDate, shipmentDate.getCaption(), sampleDateField.getCaption())));
🧹 Nitpick comments (5)
sormas-api/src/main/java/de/symeda/sormas/api/symptoms/SymptomsDto.java (1)
2527-2531: Functional change looks good; consider optional style consistency improvement.The addition of Luxembourg to the countries where the
fatiguefield is visible is correct and aligns with the PR objectives.However, there's a minor style inconsistency: this annotation uses fully qualified names (
CountryHelper.COUNTRY_CODE_SWITZERLAND,CountryHelper.COUNTRY_CODE_LUXEMBOURG), while other similar annotations in the same file use static imports (e.g., lines 2266-2268 useCOUNTRY_CODE_GERMANYandCOUNTRY_CODE_SWITZERLANDwithout theCountryHelper.prefix). SinceCOUNTRY_CODE_SWITZERLANDis already statically imported at line 21, consider:
- Using the simple name
COUNTRY_CODE_SWITZERLAND(without prefix) here- Adding a static import for
COUNTRY_CODE_LUXEMBOURGat the top of the fileThis would improve consistency with other annotations like
feelingIll(line 2266) andfastHeartRate(line 2277).Apply this diff for consistency (if desired):
+import static de.symeda.sormas.api.CountryHelper.COUNTRY_CODE_LUXEMBOURG;@HideForCountriesExcept(countries = { - CountryHelper.COUNTRY_CODE_SWITZERLAND, - CountryHelper.COUNTRY_CODE_LUXEMBOURG }) + COUNTRY_CODE_SWITZERLAND, + COUNTRY_CODE_LUXEMBOURG })sormas-api/src/main/resources/captions.properties (1)
1152-1154: Align helper label with updated field meaning.Field now reads “Suspected main mode of transmission”; consider updating the adjacent helper to match for consistency.
Apply:
-EpiData.modeOfTransmissionType= Specify mode of transmission +EpiData.modeOfTransmissionType= Specify suspected main mode of transmissionsormas-api/src/main/java/de/symeda/sormas/api/Disease.java (1)
78-79: Confirm intent: PARAINFLUENZA_1_4 defaultActive → false.This changes defaults for new configs only; existing instances keep prior setting. If you intend to disable it for existing deployments, add a DB/config migration and release note. Otherwise, consider a short comment in the changelog to avoid surprises.
sormas-api/src/main/resources/enum.properties (1)
778-780: Shortened ExposureType captions — check sentence composition and locales.
- If UI builds sentences like “Exposure to {caption}”, shortening to “Food”/“Recreational water” is fine; if it relied on captions including “Exposure to …”, adjust those templates accordingly.
- Please propagate the caption changes to other locale bundles to prevent mixed-language UI.
sormas-backend/src/main/resources/sql/sormas_schema.sql (1)
14824-14828: Consider idempotency safeguards for data insertion.The INSERT statement unconditionally adds a new row to the
customizableenumvaluetable without checking whether this record already exists. For robustness in multi-environment deployments or re-applied migrations, consider using an idempotent approach:-- Option 1: Check and insert only if not exists INSERT INTO customizableenumvalue(id, uuid, changedate, creationdate, datatype, value, caption, diseases) SELECT nextval('entity_seq'), generate_base32_uuid(), now(), now(), 'DISEASE_VARIANT', 'INDETERMINATE', 'Indeterminate', 'RESPIRATORY_SYNCYTIAL_VIRUS' WHERE NOT EXISTS (SELECT 1 FROM customizableenumvalue WHERE datatype = 'DISEASE_VARIANT' AND value = 'INDETERMINATE'); -- Option 2: Use ON CONFLICT if supported (PostgreSQL 9.5+) INSERT INTO customizableenumvalue(id, uuid, changedate, creationdate, datatype, value, caption, diseases) VALUES (nextval('entity_seq'), generate_base32_uuid(), now(), now(), 'DISEASE_VARIANT', 'INDETERMINATE', 'Indeterminate', 'RESPIRATORY_SYNCYTIAL_VIRUS') ON CONFLICT (datatype, value) DO NOTHING;This prevents duplicate-key or constraint violations during repeated migration runs (e.g., in CI/CD pipelines or rollback-retry scenarios).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
sormas-api/src/main/java/de/symeda/sormas/api/Disease.java(1 hunks)sormas-api/src/main/java/de/symeda/sormas/api/i18n/Captions.java(1 hunks)sormas-api/src/main/java/de/symeda/sormas/api/sample/PathogenTestType.java(1 hunks)sormas-api/src/main/java/de/symeda/sormas/api/symptoms/SymptomsDto.java(1 hunks)sormas-api/src/main/resources/captions.properties(3 hunks)sormas-api/src/main/resources/enum.properties(1 hunks)sormas-backend/src/main/resources/sql/sormas_schema.sql(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/events/EventDataForm.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/hospitalization/HospitalizationForm.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/samples/AbstractSampleForm.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java(8 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/samples/pathogentestlink/PathogenTestListEntry.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/utils/DateComparisonValidator.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/i18n/I18nProperties.java (1)
I18nProperties(39-536)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/pathogentestlink/PathogenTestListEntry.java (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/CssStyles.java (1)
CssStyles(26-539)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java (3)
sormas-api/src/main/java/de/symeda/sormas/api/i18n/I18nProperties.java (1)
I18nProperties(39-536)sormas-app/app/src/main/java/de/symeda/sormas/app/util/DateFormatHelper.java (1)
DateFormatHelper(12-59)sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java (1)
FieldHelper(56-1270)
sormas-api/src/main/java/de/symeda/sormas/api/symptoms/SymptomsDto.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/CountryHelper.java (1)
CountryHelper(22-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: android app test (26)
- GitHub Check: android app test (27)
- GitHub Check: android app test (28)
- GitHub Check: SORMAS CI
🔇 Additional comments (12)
sormas-api/src/main/resources/captions.properties (1)
1875-1889: Verify RSV keys are fully wired and localized.Confirm:
- Captions.java defines PathogenTest_rsv_testedDiseaseVariant and PathogenTest_rsv_testedDiseaseVariantDetails pointing to these keys.
- All non‑EN locale bundles add matching entries to avoid fallback English.
- UI shows RSV subtype only when Disease = RESPIRATORY_SYNCYTIAL_VIRUS; otherwise generic variant labels still apply.
sormas-ui/src/main/java/de/symeda/sormas/ui/hospitalization/HospitalizationForm.java (1)
249-260: LGTM! Dynamic validation logic is correctly implemented.The dual value change listeners appropriately handle the requirement for
hospitalizationReasonbased on either admission or current hospitalization status for GIARDIASIS and CRYPTOSPORIDIOSIS cases. Each listener independently evaluates its field without conflicts.sormas-ui/src/main/java/de/symeda/sormas/ui/utils/DateComparisonValidator.java (1)
62-86: LGTM! New constructor properly documented and implemented.The new overloaded constructor with the
dateOnlyparameter enables time-aware date comparisons. Documentation is clear, and all fields are correctly initialized.sormas-ui/src/main/java/de/symeda/sormas/ui/events/EventDataForm.java (1)
262-263: LGTM! Date-only validation is appropriate for investigation dates.Passing
truefor thedateOnlyparameter ensures that the investigation date validators perform date-only comparisons, which is suitable for these fields where time precision is typically not required.sormas-ui/src/main/java/de/symeda/sormas/ui/samples/pathogentestlink/PathogenTestListEntry.java (1)
106-109: LGTM! Styling enhancement appropriately emphasizes disease variant.Adding
CssStyles.LABEL_CRITICALto the disease variant label makes this important medical information more visually prominent, which is appropriate for RSV-related enhancements.sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java (1)
479-482: Verify cross-domain caption usage for RSV disease variant fields.The code uses
PathogenTestcaptions (PathogenTest_rsv_testedDiseaseVariantandPathogenTest_rsv_testedDiseaseVariantDetails) forCaseDataDtofields. While this may be intentional for consistency across RSV-related forms, cross-domain caption usage can create maintenance challenges if the PathogenTest captions are updated without considering their usage in the Case domain.Confirm that using PathogenTest captions in the Case form is the intended design pattern for RSV disease variants. If these captions should be semantically identical across domains, consider documenting this relationship or creating shared caption constants.
sormas-api/src/main/java/de/symeda/sormas/api/i18n/Captions.java (1)
2278-2279: The RSV constants are auto-generated from captions.properties and do not require changes.The constants at lines 2278-2279 are legitimately generated from keys in
sormas-api/src/main/resources/captions.properties(lines 1875 and 1888). Both generic disease variant fields (PathogenTest.testedDiseaseVariant,PathogenTest.testedDiseaseVariantDetails) and RSV-specific variants (PathogenTest.rsv.testedDiseaseVariant,PathogenTest.rsv.testedDiseaseVariantDetails) exist in the properties file as separate entries. TheI18nConstantGeneratorconverts all property keys to Java constants by replacing dots with underscores, so these constants are correctly generated and the@Generatedannotation is appropriate.Likely an incorrect or invalid review comment.
sormas-backend/src/main/resources/sql/sormas_schema.sql (1)
14824-14828: SQL migration syntax and schema correctness verified.The INSERT statement is correct and follows established patterns:
'DISEASE_VARIANT'is a valid datatype value (stored as varchar in the datatype column, used consistently throughout the codebase)- RESPIRATORY_SYNCYTIAL_VIRUS is the correct disease identifier (matches exact Disease enum constants used for previous RSV variants)
- Single disease value format is correct (established pattern for both RSV and Influenza variants, not comma-separated)
- Schema version progression from 595 to 596 is sequential and correct
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java (3)
379-385: RSV caption configuration looks good.The
diseaseVariantFieldis initially hidden (line 379), and RSV-specific captions are set when the disease is RSV (lines 382-385). The field's visibility is properly controlled later through the RSV subtype dependencies (lines 1074-1083).
521-525: LGTM: Improved formatting.The multiline formatting of
Arrays.asListimproves readability without changing functionality.
1074-1083: LGTM: RSV subtype dependencies correctly implemented.The visibility dependencies for
TESTED_DISEASE_VARIANTare properly configured to show the field only when the disease is RSV, the test type is sequencing-related (SEQUENCING or WHOLE_GENOME_SEQUENCING), and the test result is POSITIVE. This aligns with the API changes inPathogenTestType.java.sormas-api/src/main/java/de/symeda/sormas/api/sample/PathogenTestType.java (1)
162-167: Incorrect review comment — no diseases were removed from SEQUENCING.The git diff confirms that SEQUENCING originally supported only
Disease.RESPIRATORY_SYNCYTIAL_VIRUSandDisease.MEASLES. The change adds support forDisease.INVASIVE_PNEUMOCOCCAL_INFECTIONandDisease.INVASIVE_MENINGOCOCCAL_INFECTION—it does not removeGIARDIASISorCRYPTOSPORIDIOSIS, which were never part of SEQUENCING's@Diseasesannotation to begin with.Additionally, there was no
hide = trueparameter on the original SEQUENCING annotation to remove. This is a purely additive change with no breaking impact.Likely an incorrect or invalid review comment.
| testDateField.removeAllValidators(); | ||
| testDateField.addValidator( | ||
| new DateComparisonValidator( | ||
| testDateField, | ||
| this::getSampleDate, | ||
| false, | ||
| false, | ||
| true, | ||
| I18nProperties.getValidationError( | ||
| Validations.afterDateWithDate, | ||
| testDateField.getCaption(), | ||
| I18nProperties.getPrefixCaption(SampleDto.I18N_PREFIX, SampleDto.SAMPLE_DATE_TIME), | ||
| DateFormatHelper.formatDate(getSampleDate())))); | ||
| testDateField.addValueChangeListener(e -> { | ||
| boolean hasTime = !getSampleDate().toInstant().atZone(ZoneId.systemDefault()).toLocalDate().equals(LocalTime.MIDNIGHT); | ||
| if (hasTime) { | ||
| testDateField.removeAllValidators(); | ||
| testDateField.addValidator( | ||
| new DateComparisonValidator( | ||
| testDateField, | ||
| this::getSampleDate, | ||
| false, | ||
| false, | ||
| false, | ||
| I18nProperties.getValidationError( | ||
| Validations.afterDateWithDate, | ||
| testDateField.getCaption(), | ||
| I18nProperties.getPrefixCaption(SampleDto.I18N_PREFIX, SampleDto.SAMPLE_DATE_TIME), | ||
| DateFormatHelper.formatLocalDateTime(getSampleDate())))); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical: Type mismatch in date-time comparison logic.
Line 350 contains a critical logic error:
boolean hasTime = !getSampleDate().toInstant().atZone(ZoneId.systemDefault()).toLocalDate().equals(LocalTime.MIDNIGHT);This compares a LocalDate object with LocalTime.MIDNIGHT, which are incompatible types and will never be equal. The condition will always evaluate to true, breaking the intended dynamic validator adjustment.
Apply this fix:
- boolean hasTime = !getSampleDate().toInstant().atZone(ZoneId.systemDefault()).toLocalDate().equals(LocalTime.MIDNIGHT);
+ boolean hasTime = !getSampleDate().toInstant().atZone(ZoneId.systemDefault()).toLocalTime().equals(LocalTime.MIDNIGHT);🤖 Prompt for AI Agents
In sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java
around lines 336 to 366, the hasTime check incorrectly compares a LocalDate to
LocalTime.MIDNIGHT causing the condition to always be true; replace the
toLocalDate() call with toLocalTime() so you check the time-of-day portion
(e.g., hasTime =
!getSampleDate().toInstant().atZone(ZoneId.systemDefault()).toLocalTime().equals(LocalTime.MIDNIGHT)),
leaving the rest of the validator logic intact.
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13643 |
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13643 |
Fixes #
Summary by CodeRabbit
New Features
Improvements
Updates