Feature 13918 salmonellosis form enhancements#13934
Open
roldy wants to merge 4 commits into
Open
Conversation
…xposure, PathogenTest & Case
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR extends disease-aware visibility filtering infrastructure and sample export enhancements for Salmonellosis. Core changes include a new ChangesSalmonellosis Disease-Aware Visibility and Sample Context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataView.java (1)
147-150: ⚡ Quick winUse the same disease fallback for criteria building as for panel visibility.
Line 255 gates rendering with
resolvedDisease, but the immunization/vaccination criteria still userefreshedContact.getDisease(). For contacts with null disease and case-derived disease, this can produce inconsistent filtering.♻️ Proposed adjustment
- return new ImmunizationListCriteria.Builder(refreshedContact.getPerson()).withDisease(refreshedContact.getDisease()).build(); + Disease criteriaDisease = refreshedContact.getDisease(); + if (criteriaDisease == null && refreshedContact.getCaze() != null) { + CaseDataDto refreshedCase = FacadeProvider.getCaseFacade().getCaseDataByUuid(refreshedContact.getCaze().getUuid()); + criteriaDisease = refreshedCase != null ? refreshedCase.getDisease() : null; + } + return new ImmunizationListCriteria.Builder(refreshedContact.getPerson()).withDisease(criteriaDisease).build();- return new VaccinationCriteria.Builder(refreshedContact.getPerson()).withDisease(refreshedContact.getDisease()) + Disease criteriaDisease = refreshedContact.getDisease() != null + ? refreshedContact.getDisease() + : refreshedCase != null ? refreshedCase.getDisease() : null; + return new VaccinationCriteria.Builder(refreshedContact.getPerson()).withDisease(criteriaDisease) .build()Also applies to: 255-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataView.java` around lines 147 - 150, The criteria building is using refreshedContact.getDisease() while panel visibility uses resolvedDisease (computed from contactDto and caseDto), causing inconsistent filtering; update the criteria construction in ContactDataView to use the same resolvedDisease variable (the one computed from contactDto and caseDto) instead of refreshedContact.getDisease() so immunization/vaccination criteria and the visibility gate both use the same disease fallback logic (ensure any references in methods that build criteria or call into the immunization/vaccination filters use resolvedDisease).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sormas-api/src/main/java/de/symeda/sormas/api/epidata/EpiDataDto.java`:
- Around line 204-209: EpiDataDto now contains a mutable nested field
foodHistory (FoodHistoryDto) but its clone() currently performs a shallow copy;
update EpiDataDto.clone() to deep-copy foodHistory by invoking its copy/clone
constructor or a toDto/deepClone method (handle nulls), so the cloned EpiDataDto
gets an independent FoodHistoryDto instance; also apply the same deep-copy
pattern to the other similar nested fields referenced around the other ranges
(the blocks around lines shown for 448-454 and 463-479) to prevent shared
mutable state between original and clone.
In `@sormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodHistoryDto.java`:
- Around line 57-59: In setConsumedItemsDetails on FoodHistoryDto, normalize the
incoming Map<FoodConsumptionItem,String> instead of assigning it directly: if
the argument is null, set this.consumedItemsDetails to an empty
EnumMap<>(FoodConsumptionItem.class); otherwise create a new
EnumMap<>(FoodConsumptionItem.class) and putAll from the provided map (or copy
directly if it already is an EnumMap) so consumedItemsDetails is never null and
always an EnumMap, preserving DTO invariants.
In `@sormas-api/src/main/java/de/symeda/sormas/api/sample/PathogenTestType.java`:
- Around line 262-284: The formatting method
PathogenTestType.toString(PathogenTestType, String) currently appends the
details text only for OTHER, so new free-text types annotated with
`@RevealsTestTypeText` (e.g., MULTILOCUS_SEQUENCE_TYPING, CGMLST, SNP_TYPING,
SEROTYPING) lose their testTypeText; update toString (and the formatType()
callers) to also include the provided details when the enum constant is
annotated with `@RevealsTestTypeText` (detect via the enum constant's annotation
or an existing helper such as a reveals-test-type flag), i.e., if details is
non-empty and the constant has `@RevealsTestTypeText` then append/format the
details the same way as OTHER.
In `@sormas-backend/src/main/java/de/symeda/sormas/backend/epidata/EpiData.java`:
- Around line 350-357: The OneToOne mapping on EpiData.getFoodHistory()
currently uses cascade = CascadeType.ALL but lacks orphan removal, so when
EpiDataFacadeEjb calls target.setFoodHistory(null) the detached FoodHistory is
not deleted; update the mapping on the getFoodHistory() method in class EpiData
to include orphanRemoval = true (retain existing cascade and fetch settings) so
that removing the reference will delete the orphaned FoodHistory entity.
In `@sormas-backend/src/main/resources/sql/sormas_schema.sql`:
- Around line 16035-16038: The trigger creation for delete_history_trigger on
the composite-PK join table foodhistory_consumeditems is incorrect because the
trigger function is only parameterized by foodhistory_id and will mis-handle
sibling rows; remove the DROP TRIGGER / CREATE TRIGGER statements for
delete_history_trigger referring to foodhistory_consumeditems and do not
register this trigger for that table (or replace with a table-specific history
cleanup that considers both keys) — locate the CREATE TRIGGER ... AFTER DELETE
ON foodhistory_consumeditems and the call to
delete_history_trigger('foodhistory_consumeditems_history','foodhistory_id') and
remove or omit those lines.
---
Nitpick comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataView.java`:
- Around line 147-150: The criteria building is using
refreshedContact.getDisease() while panel visibility uses resolvedDisease
(computed from contactDto and caseDto), causing inconsistent filtering; update
the criteria construction in ContactDataView to use the same resolvedDisease
variable (the one computed from contactDto and caseDto) instead of
refreshedContact.getDisease() so immunization/vaccination criteria and the
visibility gate both use the same disease fallback logic (ensure any references
in methods that build criteria or call into the immunization/vaccination filters
use resolvedDisease).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf685b63-64b2-4c65-8753-4122980c7c9e
⛔ Files ignored due to path filters (1)
sormas-api/src/main/resources/doc/SORMAS_Data_Dictionary.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (33)
sormas-api/src/main/java/de/symeda/sormas/api/caze/CaseDataDto.javasormas-api/src/main/java/de/symeda/sormas/api/epidata/EpiDataDto.javasormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodConsumptionItem.javasormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodHistoryDto.javasormas-api/src/main/java/de/symeda/sormas/api/exposure/ExposureSubSetting.javasormas-api/src/main/java/de/symeda/sormas/api/i18n/Captions.javasormas-api/src/main/java/de/symeda/sormas/api/i18n/Strings.javasormas-api/src/main/java/de/symeda/sormas/api/sample/PathogenTestType.javasormas-api/src/main/java/de/symeda/sormas/api/sample/RevealsTestTypeText.javasormas-api/src/main/java/de/symeda/sormas/api/sample/SampleExportDto.javasormas-api/src/main/java/de/symeda/sormas/api/sample/SampleMaterial.javasormas-api/src/main/resources/captions.propertiessormas-api/src/main/resources/enum.propertiessormas-api/src/main/resources/strings.propertiessormas-backend/src/main/java/de/symeda/sormas/backend/epidata/EpiData.javasormas-backend/src/main/java/de/symeda/sormas/backend/epidata/EpiDataFacadeEjb.javasormas-backend/src/main/java/de/symeda/sormas/backend/epidata/FoodHistory.javasormas-backend/src/main/java/de/symeda/sormas/backend/epidata/FoodHistoryMapper.javasormas-backend/src/main/resources/META-INF/persistence.xmlsormas-backend/src/main/resources/sql/sormas_schema.sqlsormas-backend/src/test/resources/META-INF/persistence.xmlsormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataView.javasormas-ui/src/main/java/de/symeda/sormas/ui/contact/AbstractContactGrid.javasormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataView.javasormas-ui/src/main/java/de/symeda/sormas/ui/epidata/EpiDataForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/epidata/FoodHistoryForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/epidata/FoodHistoryItemsField.javasormas-ui/src/main/java/de/symeda/sormas/ui/exposure/ExposureForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/hospitalization/HospitalizationForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/components/TestMethodComponent.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/CssStyles.javasormas-ui/src/main/webapp/VAADIN/themes/sormas/global.scsssormas-ui/src/test/resources/META-INF/persistence.xml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodHistoryDto.java (1)
57-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against
EnumMapconstruction failure for empty generic maps.Line 59 can throw at runtime when
consumedItemsDetailsis empty and not already anEnumMap. Build with explicit key type, then copy entries.Proposed fix
public void setConsumedItemsDetails(Map<FoodConsumptionItem, String> consumedItemsDetails) { - this.consumedItemsDetails = - consumedItemsDetails == null ? new EnumMap<>(FoodConsumptionItem.class) : new EnumMap<>(consumedItemsDetails); + EnumMap<FoodConsumptionItem, String> normalized = new EnumMap<>(FoodConsumptionItem.class); + if (consumedItemsDetails != null) { + normalized.putAll(consumedItemsDetails); + } + this.consumedItemsDetails = normalized; }In Java, does `new EnumMap<>(map)` throw when `map` is empty and not an EnumMap (e.g., Collections.emptyMap())? Please cite the JDK behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodHistoryDto.java` around lines 57 - 60, In setConsumedItemsDetails in FoodHistoryDto the current new EnumMap<>(consumedItemsDetails) can throw for empty generic maps (e.g., Collections.emptyMap()); instead construct an EnumMap with the key type (new EnumMap<>(FoodConsumptionItem.class)) and then, if the incoming consumedItemsDetails is non-null, call putAll(consumedItemsDetails) to copy entries; preserve the null-to-empty behavior by initializing the field to a new EnumMap<>(FoodConsumptionItem.class) when the argument is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@sormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodHistoryDto.java`:
- Around line 57-60: In setConsumedItemsDetails in FoodHistoryDto the current
new EnumMap<>(consumedItemsDetails) can throw for empty generic maps (e.g.,
Collections.emptyMap()); instead construct an EnumMap with the key type (new
EnumMap<>(FoodConsumptionItem.class)) and then, if the incoming
consumedItemsDetails is non-null, call putAll(consumedItemsDetails) to copy
entries; preserve the null-to-empty behavior by initializing the field to a new
EnumMap<>(FoodConsumptionItem.class) when the argument is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 257eb1d6-d730-4802-aa8e-4378d41c716b
📒 Files selected for processing (9)
sormas-api/src/main/java/de/symeda/sormas/api/epidata/EpiDataDto.javasormas-api/src/main/java/de/symeda/sormas/api/epidata/FoodHistoryDto.javasormas-api/src/main/java/de/symeda/sormas/api/sample/PathogenTestType.javasormas-api/src/main/java/de/symeda/sormas/api/sample/SampleExportDto.javasormas-backend/src/main/java/de/symeda/sormas/backend/epidata/EpiData.javasormas-backend/src/main/java/de/symeda/sormas/backend/sample/SampleFacadeEjb.javasormas-backend/src/main/resources/sql/sormas_schema.sqlsormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataView.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/pathogentestlink/PathogenTestListEntry.java
🚧 Files skipped from review as they are similar to previous changes (2)
- sormas-backend/src/main/java/de/symeda/sormas/backend/epidata/EpiData.java
- sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataView.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13917 #13918
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI/Style Updates