Bugfix 13763 enable the pathogen test result field #13765
Conversation
Fatigue symptom for all countries Enabling the test result field.
…usceptibilityForm - for DrugSusceptibilityForm, added a conditional exit to prevent enabling susceptility contents if country is not Luxembourg - In PathogenTestForm, moved the call to updateDrugSusceptibilityFields to the end of the listener chain
- Tuberculosis/Latent Tuberculosis is only visible for Luxembourg - IPI/IMI drug susceptibility is visible for all countries
- also added the DrugSusceptibilityForm changes
WalkthroughThis PR adds serogroup specification field mappings to ExternalMessageMapper, removes Switzerland-only restrictions for fatigue in SymptomsDto, refactors PathogenTestForm with new visibility configuration maps for multiple test-related fields, adds UI components for serology data, improves code formatting in PathogenTestController, and introduces country-specific gating logic in DrugSusceptibilityForm for TB-related drug susceptibility visibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (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 |
raulbob
deleted the
bugfix-13763-enable-the-pathogen-test-result-field
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
sormas-ui/src/main/java/de/symeda/sormas/ui/therapy/DrugSusceptibilityForm.java (1)
300-332: Visibility logic is clear; double‑check whether clearing hidden fields for non‑Lux TB/LTBI is intendedThe new
updateFieldsVisibilitycontrol flow looks good overall:
- Early hiding + early returns keep the method readable and avoid nested
ifs.- Restricting visibility to
ANTIBIOTIC_SUSCEPTIBILITYand driving field selection viaAnnotationFieldHelper.getFieldNamesWithMatchingDiseaseAndTestAnnotationsis consistent with the annotation‑based pattern elsewhere.- Country gating for TB/LTBI using
FacadeProvider.getConfigFacade().isConfiguredCountry(CountryHelper.COUNTRY_CODE_LUXEMBOURG)matches the comment intent (only Luxembourg shows TB/LTBI drug susceptibility fields).- Heading visibility tied to
!applicableFieldIds.isEmpty()is straightforward.One behavioral nuance to be aware of:
- At Line 301 you call
FieldHelper.hideFieldsNotInList(getFieldGroup(), List.of(), true);, which (withclearOnHidden = true) clears values for all fields that become hidden.- For non‑Lux TB/LTBI, the subsequent country check returns early (Lines 318–322), so all drug susceptibility fields remain hidden and cleared.
- This means that simply visiting/updating the form for TB/LTBI in a non‑Lux country can wipe existing drug‑susceptibility values on save, not just hide the inputs.
If the requirement is purely “do not allow editing / do not show the fields” but keep any already‑stored values, consider avoiding clearing in that code path, for example:
public void updateFieldsVisibility(Disease disease, PathogenTestType pathogenTestType) { - FieldHelper.hideFieldsNotInList(getFieldGroup(), List.of(), true); + // Reset visibility without clearing values; we'll clear selectively when needed. + FieldHelper.hideFieldsNotInList(getFieldGroup(), List.of(), false); formHeadingLabel.setVisible(false); @@ - if (!FacadeProvider.getConfigFacade().isConfiguredCountry(CountryHelper.COUNTRY_CODE_LUXEMBOURG) - && (disease == Disease.TUBERCULOSIS || disease == Disease.LATENT_TUBERCULOSIS)) { - //quit, we do not want to show the fields if TUBERCULOSIS/LATENT_TUBERCULOSIS and we are not in Luxembourg - return; - } + if (!FacadeProvider.getConfigFacade().isConfiguredCountry(CountryHelper.COUNTRY_CODE_LUXEMBOURG) + && (disease == Disease.TUBERCULOSIS || disease == Disease.LATENT_TUBERCULOSIS)) { + // Quit: keep everything hidden, but retain existing values + return; + } @@ - if (!applicableFieldIds.isEmpty()) { - FieldHelper.showOnlyFields(getFieldGroup(), applicableFieldIds, true); - } + if (!applicableFieldIds.isEmpty()) { + // Here it's likely fine to clear hidden fields because we're explicitly scoping which fields apply + FieldHelper.showOnlyFields(getFieldGroup(), applicableFieldIds, true); + }If, instead, the business rule is to actively drop these values for non‑Lux TB/LTBI, the current implementation already does that; you may just want to capture it explicitly in the comments or requirements.
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java (4)
141-221: RESULT_FIELD_DECISION_MAP and visibility condition maps look correct but can be tightenedThe new
RESULT_FIELD_DECISION_MAPand the various*_VISIBILITY_CONDITIONSmaps correctly encode the intended disease/test-type/result combinations and are used consistently later (e.g., in the test type listener and FieldHelper calls).Two optional improvements for robustness and readability:
- Consider typing the result map as
Map<Disease, List<PathogenTestType>>and usingCollections.unmodifiableList(...)for the values, to avoid exposing mutableArrayListvia a public static constant.- Likewise, if possible, specialize the visibility maps away from
Map<Object, List<Object>>to more specific generic types (or at least document expected key/value types) to help future maintainers.
118-121: Serogroup specification UI wiring is consistent; minor naming and initial-visibility considerationsThe new serogroup specification row in the layout and the fields
seroGrpSepcCB/seroGrpSpecTxtare correctly:
- Added to the HTML layout (
SERO_GROUP_SPECIFICATION+SERO_GROUP_SPECIFICATION_TEXT).- Instantiated in
addFieldswith appropriate types.- Synced in
setValuewith read-only checks.- Controlled via
FieldHelper.setVisibleWhenfor IMI andSeroGroupSpecification.OTHER, as expected.Two small follow-ups:
- The variable name
seroGrpSepcCBappears to have a typo ("Sepc"). Renaming toseroGrpSpecCBwould improve clarity.seroGrpSpecTxtis not explicitly set to invisible before thesetVisibleWhenrule (unlike e.g.seroTypingMethodText). IfFieldHelper.setVisibleWhendoes not reset the initial visibility, you may briefly see the text field before a value is selected. Aligning its initialization with the other conditional fields would avoid any flicker:- seroGrpSpecTxt = addField(PathogenTestDto.SERO_GROUP_SPECIFICATION_TEXT, TextField.class); + seroGrpSpecTxt = addField(PathogenTestDto.SERO_GROUP_SPECIFICATION_TEXT, TextField.class); + seroGrpSpecTxt.setVisible(false);Also applies to: 242-244, 596-601, 1056-1077
1147-1153: Disease variant listener now drives test result; confirm UX intentThe disease-variant value-change listener now:
- Forces
testResultFieldtoPOSITIVEwhen a non-nullDiseaseVariantis selected.- Clears
testResultFieldwhen the variant is cleared.- Toggles the details field based on
HAS_DETAILS.This is logically consistent (a specific variant implies a positive test), but it also means clearing the variant will clear any manually entered test result, even if the user set NEGATIVE/NOT_APPLICABLE explicitly.
Please confirm this UX is intended; if not, you may want to only auto-set the result when it was empty before, and avoid clearing a non-empty result when the variant is removed.
1273-1280: Placeholder TestTypeValueChangeListener is empty; remove or document usage
TestTypeValueChangeListeneris now an empty inner class with a TODO stub and no logic.If nothing outside this class relies on its presence, consider removing it entirely to reduce noise. If external code still references it (as the comment suggests), it might be worth adding a brief Javadoc explaining that it’s intentionally a no-op compatibility shim.
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestController.java (1)
383-399: Case result selection and disease-variant update prompting: behavior change is subtle, verify intentThe refactor in
handleAssociatedCaseandcheckForDiseaseVariantUpdate:
- Picks a single
resultedPathogenTest(positive preferred over negative) and only prompts the user once for updating the sample result.- Uses
!DataHelper.equal(test.getTestedDiseaseVariant(), caze.getDiseaseVariant())as the sole difference check before callingisNotYetRelatedDiseaseVariant(test)and potentially showing the variant update dialog.This means you will now also prompt when:
- The case has a non-null disease variant, and
- The new test’s
testedDiseaseVariantis null, but it’s not already associated (perisNotYetRelatedDiseaseVariant).The dialog text has been updated to handle
[no disease variant], so clearing a case’s variant via a newer test is now possible.Please confirm this is intentional; if you only want to prompt when a non-null variant is proposed from the test, you likely need to keep an explicit
test.getTestedDiseaseVariant() != nullguard in the condition.Also applies to: 550-569
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sormas-api/src/main/java/de/symeda/sormas/api/externalmessage/processing/ExternalMessageMapper.java(1 hunks)sormas-api/src/main/java/de/symeda/sormas/api/symptoms/SymptomsDto.java(0 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestController.java(8 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java(12 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/therapy/DrugSusceptibilityForm.java(2 hunks)
💤 Files with no reviewable changes (1)
- sormas-api/src/main/java/de/symeda/sormas/api/symptoms/SymptomsDto.java
🧰 Additional context used
🧬 Code graph analysis (2)
sormas-ui/src/main/java/de/symeda/sormas/ui/therapy/DrugSusceptibilityForm.java (3)
sormas-api/src/main/java/de/symeda/sormas/api/FacadeProvider.java (1)
FacadeProvider(128-599)sormas-api/src/main/java/de/symeda/sormas/api/utils/AnnotationFieldHelper.java (1)
AnnotationFieldHelper(26-95)sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java (1)
FieldHelper(56-1270)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestController.java (2)
sormas-api/src/main/java/de/symeda/sormas/api/DiseaseHelper.java (1)
DiseaseHelper(32-92)sormas-api/src/main/java/de/symeda/sormas/api/utils/DataHelper.java (1)
DataHelper(60-502)
⏰ 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 (27)
- GitHub Check: SORMAS CI
- GitHub Check: android app test (28)
- GitHub Check: android app test (26)
🔇 Additional comments (6)
sormas-ui/src/main/java/de/symeda/sormas/ui/therapy/DrugSusceptibilityForm.java (1)
34-37: New imports are appropriate and scoped to the new gating logic
CountryHelperandFacadeProviderare correctly introduced and only used for the configured‑country check; no unused or redundant imports here.sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java (3)
313-357: Result auto-selection and drug susceptibility visibility: behavior looks coherent, but confirm edge casesThe extended
updateDrugSusceptibilityFieldSpecificationsplus the newRESULT_FIELD_DECISION_MAPusage in the test type listener together implement:
- Environment samples (
disease == null) skipping any result auto-changes while still updating drug susceptibility field visibility.- Clearing
testResultFieldwhentestTypebecomes null.- Luxembourg-only defaults for TB and invasive diseases (NOT_APPLICABLE / POSITIVE) via
updateDrugSusceptibilityFieldSpecifications.- General auto-positives for selected disease/test-type combinations via
RESULT_FIELD_DECISION_MAP, with more specific Luxembourg rules taking precedence afterward.This is a sensible structure and avoids touching environment-sample results.
Two things to double-check:
- For diseases not in
RESULT_FIELD_DECISION_MAP, each test type change now clearstestResultFieldunconditionally. That matches the prior pattern for some flows but will force the user to re-enter a result whenever they change the test type. Confirm this is intended for all diseases not listed in the map.- For Luxembourg TB types,
RESULT_FIELD_DECISION_MAPdoes nothing (no TB entries), and the country-specific logic inupdateDrugSusceptibilityFieldSpecificationssets the final result — which looks correct and non-conflicting.Also applies to: 1155-1227
406-418: New genotype and serogroup value synchronization looks safeThe extended
setValuenow guards all newly synced fields (genoTypingResultTextTF,seroGrpSepcCB,seroGrpSpecTxt) withisReadOnlychecks before setting values. This aligns with the existing pattern and should avoid read-only write attempts when loading an existing test.
569-592: Visibility rules for TB- and genotyping-related fields are consistent with new condition mapsThe updated visibility wiring:
- Uses
RIFAMPICIN_RESISTANT_VISIBILITY_CONDITIONS,TEST_SCALE_VISIBILITY_CONDITIONS,STRAIN_CALL_STATUS_VISIBILITY_CONDITIONS, andSPECIE_VISIBILITY_CONDITIONSfor Luxembourg TB fields.- Keeps the MIRU pattern profile using a local dependency map, as before.
- Adds
PCR_TEST_SPECIFICATION_VISIBILITY_CONDITIONSforPCR_TEST_SPECIFICATIONacross countries.- Controls
GENOTYPE_RESULTandGENOTYPE_RESULT_TEXTvia disease/test/result conditions and theGenoTypeResult.OTHERsentinel.All of this is consistent with the static maps and should improve maintainability of field visibility. No functional issues spotted.
Also applies to: 983-983, 1080-1109
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestController.java (2)
130-135: Event disease resolution for associated event participant looks goodThe refactored block now:
- Loads the
EventParticipantDtoby UUID.- Uses its
EventReferenceDtoto fetch the fullEventDtoand read the disease.This is clearer than mixing references and full DTOs and ensures you always use the event’s current disease. No issues here.
332-338: Drug susceptibility clearing logic: confirm intended scope for latent TBThe new logic in
savePathogenTests:
- Treats
luxTBas “Luxembourg and tested disease is exactlyDisease.TUBERCULOSIS”.- Uses
DiseaseHelper.checkDiseaseIsInvasiveBacterialDiseasesfor IMI/IPI.- Clears
p.setDrugSusceptibility(null)whentestType == ANTIBIOTIC_SUSCEPTIBILITYbut neitherluxTBnorinvasiveDiseaseis true.This ensures antibiotic susceptibility is only persisted for:
- TB in Luxembourg (not latent TB), and
- Invasive meningococcal/pneumococcal infections in all countries.
Behavior seems coherent, but please confirm:
- That susceptibility for
Disease.LATENT_TUBERCULOSISshould indeed be dropped even in Luxembourg.- That there are no other diseases where ANTIBIOTIC_SUSCEPTIBILITY should be retained.
| Mapping.of( | ||
| pathogenTest::setSeroGroupSpecification, | ||
| pathogenTest.getSeroGroupSpecification(), | ||
| sourceTestReport.getSeroGroupSpecification(), | ||
| PathogenTestDto.SERO_GROUP_SPECIFICATION), | ||
| Mapping.of( | ||
| pathogenTest::setSeroGroupSpecificationText, | ||
| pathogenTest.getSeroGroupSpecificationText(), | ||
| sourceTestReport.getSeroGroupSpecificationText(), | ||
| PathogenTestDto.SERO_GROUP_SPECIFICATION), |
There was a problem hiding this comment.
Wrong UI field key for serogroup specification text mapping
The mapping that sets setSeroGroupSpecificationText uses PathogenTestDto.SERO_GROUP_SPECIFICATION as the UI field path instead of the dedicated SERO_GROUP_SPECIFICATION_TEXT constant. This will mark the wrong field as changed and can confuse any UI logic driven by changedFields.
Please switch the field path to the text constant:
- Mapping.of(
- pathogenTest::setSeroGroupSpecificationText,
- pathogenTest.getSeroGroupSpecificationText(),
- sourceTestReport.getSeroGroupSpecificationText(),
- PathogenTestDto.SERO_GROUP_SPECIFICATION),
+ Mapping.of(
+ pathogenTest::setSeroGroupSpecificationText,
+ pathogenTest.getSeroGroupSpecificationText(),
+ sourceTestReport.getSeroGroupSpecificationText(),
+ PathogenTestDto.SERO_GROUP_SPECIFICATION_TEXT),📝 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.
| Mapping.of( | |
| pathogenTest::setSeroGroupSpecification, | |
| pathogenTest.getSeroGroupSpecification(), | |
| sourceTestReport.getSeroGroupSpecification(), | |
| PathogenTestDto.SERO_GROUP_SPECIFICATION), | |
| Mapping.of( | |
| pathogenTest::setSeroGroupSpecificationText, | |
| pathogenTest.getSeroGroupSpecificationText(), | |
| sourceTestReport.getSeroGroupSpecificationText(), | |
| PathogenTestDto.SERO_GROUP_SPECIFICATION), | |
| Mapping.of( | |
| pathogenTest::setSeroGroupSpecification, | |
| pathogenTest.getSeroGroupSpecification(), | |
| sourceTestReport.getSeroGroupSpecification(), | |
| PathogenTestDto.SERO_GROUP_SPECIFICATION), | |
| Mapping.of( | |
| pathogenTest::setSeroGroupSpecificationText, | |
| pathogenTest.getSeroGroupSpecificationText(), | |
| sourceTestReport.getSeroGroupSpecificationText(), | |
| PathogenTestDto.SERO_GROUP_SPECIFICATION_TEXT), |
🤖 Prompt for AI Agents
In
sormas-api/src/main/java/de/symeda/sormas/api/externalmessage/processing/ExternalMessageMapper.java
around lines 345 to 354, the Mapping that sets setSeroGroupSpecificationText
incorrectly uses PathogenTestDto.SERO_GROUP_SPECIFICATION as the UI field path;
change that mapping to use the dedicated
PathogenTestDto.SERO_GROUP_SPECIFICATION_TEXT constant so the correct UI field
is marked as changed. Ensure only the field path constant is updated for the
setSeroGroupSpecificationText Mapping and leave the rest of the mapping
parameters unchanged.
Fixed hopefully the pathogen test form
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.