🐛: FIX: 14130/14136: ngSurvey: Missing facilities, Enhacements Hospitalization handling, customizable fields#14138
Conversation
- Reversed condition for cron activation - copy-paste issue in log - NPE exception for some occurrences
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBackend changes adjust survey-response handling, customizable-field patch dictionary plumbing, and date patch parsing. They also add EPIDATA patch coverage and update tests for the new date behavior. ChangesSurvey processing fixes
Customizable-field patching
Partial date parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 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-backend/src/main/java/de/symeda/sormas/backend/ReferenceDataValueInstanceProviderImpl.java`:
- Around line 58-63: The FacilityReferenceDto lookup in
ReferenceDataValueInstanceProviderImpl is now ambiguous because it returns all
facilities and the downstream getOne/findAny resolution can select an arbitrary
FacilityDto when external IDs are duplicated. Restrict the facility dictionary
to a unique, deterministic set before mapping in the
FacilityFacade/getAllAfter(DATE_ALL_VALUES) path, and make sure the generic
reference resolution logic used by getOne prefers an unambiguous unique key
rather than caption/externalId matching that can return multiple candidates.
🪄 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: 0dfcb58d-7d78-40ef-a940-3f1232a517da
📒 Files selected for processing (4)
sormas-backend/src/main/java/de/symeda/sormas/backend/ReferenceDataValueInstanceProviderImpl.javasormas-backend/src/main/java/de/symeda/sormas/backend/common/CronService.javasormas-backend/src/main/java/de/symeda/sormas/backend/externalmessage/ExternalMessageFacadeEjb.javasormas-backend/src/main/java/de/symeda/sormas/backend/externalmessage/survey/AutomaticSurveyResponseProcessor.java
…ly and different format
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sormas-backend/src/main/java/de/symeda/sormas/backend/patch/mapping/impl/valuemapper/DatePatchMapper.java (1)
54-56: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPreserve null-date mapping before calling
toString().Line 56 now crashes for null patch values, but null replacements are allowed by the patch flow and should map as successful
nullvalues instead of throwing.🐛 Proposed fix
Object value = request.getValue(); + if (value == null) { + return ValueMappingResult.withData(null); + } + String str = value.toString().trim();🤖 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-backend/src/main/java/de/symeda/sormas/backend/patch/mapping/impl/valuemapper/DatePatchMapper.java` around lines 54 - 56, The DatePatchMapper value handling currently calls toString() on request.getValue() without guarding null, which breaks null patch replacements. Update the DatePatchMapper logic so the null-date mapping is preserved before any toString() call: detect a null value from request.getValue(), return the successful null mapping path, and only trim/parse when the value is non-null. Use the DatePatchMapper mapping flow and the request.getValue() access as the key place to fix this.
🤖 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.
Outside diff comments:
In
`@sormas-backend/src/main/java/de/symeda/sormas/backend/patch/mapping/impl/valuemapper/DatePatchMapper.java`:
- Around line 54-56: The DatePatchMapper value handling currently calls
toString() on request.getValue() without guarding null, which breaks null patch
replacements. Update the DatePatchMapper logic so the null-date mapping is
preserved before any toString() call: detect a null value from
request.getValue(), return the successful null mapping path, and only trim/parse
when the value is non-null. Use the DatePatchMapper mapping flow and the
request.getValue() access as the key place to fix this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d38f944d-fd24-4cbb-a4a0-66ed166bdc9d
📒 Files selected for processing (4)
sormas-backend/src/main/java/de/symeda/sormas/backend/patch/DataPatcherImpl.javasormas-backend/src/main/java/de/symeda/sormas/backend/patch/mapping/impl/valuemapper/DatePatchMapper.javasormas-backend/src/test/java/de/symeda/sormas/backend/patch/mapping/impl/valuemapper/DateMapperTest.javasormas-backend/src/test/java/de/symeda/sormas/patch/DataPatcherImplTest.java
✅ Files skipped from review due to trivial changes (1)
- sormas-backend/src/test/java/de/symeda/sormas/patch/DataPatcherImplTest.java
Pa-Touche
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sormas-backend/src/main/java/de/symeda/sormas/backend/patch/DataPatcherImpl.java (2)
190-216: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCustom exposure fields still need a UUID seed.
buildEntityUuidDictionaryFromonly populates fromentityCacheand only backfillsEPIDATA; a request with onlyCustom.Exposure.*fields can still reachentityUuidDictionary.get(...).get()with no exposure entry and NPE. The broader Immunization example doesn’t apply here; this patcher only maps CASE, EPIDATA, and EXPOSURE. Resolve the exposure target before building this dictionary instead of special-casing only EPIDATA.🤖 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-backend/src/main/java/de/symeda/sormas/backend/patch/DataPatcherImpl.java` around lines 190 - 216, The UUID dictionary builder in DataPatcherImpl only backfills EPIDATA, so Custom.Exposure fields can still miss a UUID seed and later fail when entityUuidDictionary is read. Update buildEntityUuidDictionaryFrom to also resolve and include the EXPOSURE entity UUID from caseData before returning the map, instead of special-casing only CustomizableFieldContext.EPIDATA. Use the existing CustomizableFieldContextPatchMapping and CustomizableContextIndexKey flow so both EPIDATA and EXPOSURE contexts are present whenever requested.
111-115: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep patching idempotent across retries. Moving
patch()toREQUIRES_NEWlets it commit even if survey-message persistence later rolls back, so a retry can apply the same response again; whenreplacementStrategyisALWAYS, that can create duplicate child records. Consider keeping the whole flow in one transaction or adding a deduplication guard.🤖 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-backend/src/main/java/de/symeda/sormas/backend/patch/DataPatcherImpl.java` around lines 111 - 115, The patch flow in DataPatcherImpl.patch is no longer safely idempotent across retries because the REQUIRES_NEW transaction can commit the response before later survey-message persistence rolls back. Keep the full patch-and-persist flow in a single transaction, or add a deduplication guard in patch() and the ALWAYS replacementStrategy path so repeated retries cannot create duplicate child records.
🧹 Nitpick comments (1)
sormas-backend/src/test/java/de/symeda/sormas/patch/DataPatcherImplTest.java (1)
1179-1204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an update-path assertion and group-context "alone" coverage.
This test only covers the initial-write path for EPIDATA (unlike
patch_customizableField_text, which also verifies an override). More importantly, given the DataPatcherImpl fix is EPIDATA-specific, consider adding a test for a group-based context (e.g.,Exposure) patched via customizable fields only, with no companion plain field, to confirm whether the same NPE risk exists there.🤖 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-backend/src/test/java/de/symeda/sormas/patch/DataPatcherImplTest.java` around lines 1179 - 1204, This EPIDATA test only covers the initial create/write path in DataPatcherImplTest.patch_customizableField_text_epidata; add an update-path assertion like the existing patch_customizableField_text test so the field is patched twice and the replacement behavior is verified. Also add a separate customizable-fields-only test for a group-based context such as Exposure using the same patch flow but without any plain companion field, to exercise the path in DataPatcherImpl and confirm the NPE fix does not regress there.
🤖 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.
Outside diff comments:
In
`@sormas-backend/src/main/java/de/symeda/sormas/backend/patch/DataPatcherImpl.java`:
- Around line 190-216: The UUID dictionary builder in DataPatcherImpl only
backfills EPIDATA, so Custom.Exposure fields can still miss a UUID seed and
later fail when entityUuidDictionary is read. Update
buildEntityUuidDictionaryFrom to also resolve and include the EXPOSURE entity
UUID from caseData before returning the map, instead of special-casing only
CustomizableFieldContext.EPIDATA. Use the existing
CustomizableFieldContextPatchMapping and CustomizableContextIndexKey flow so
both EPIDATA and EXPOSURE contexts are present whenever requested.
- Around line 111-115: The patch flow in DataPatcherImpl.patch is no longer
safely idempotent across retries because the REQUIRES_NEW transaction can commit
the response before later survey-message persistence rolls back. Keep the full
patch-and-persist flow in a single transaction, or add a deduplication guard in
patch() and the ALWAYS replacementStrategy path so repeated retries cannot
create duplicate child records.
---
Nitpick comments:
In
`@sormas-backend/src/test/java/de/symeda/sormas/patch/DataPatcherImplTest.java`:
- Around line 1179-1204: This EPIDATA test only covers the initial create/write
path in DataPatcherImplTest.patch_customizableField_text_epidata; add an
update-path assertion like the existing patch_customizableField_text test so the
field is patched twice and the replacement behavior is verified. Also add a
separate customizable-fields-only test for a group-based context such as
Exposure using the same patch flow but without any plain companion field, to
exercise the path in DataPatcherImpl and confirm the NPE fix does not regress
there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aee25f76-b3bd-4f8d-8a8e-86893c4bacd9
📒 Files selected for processing (4)
sormas-backend/src/main/java/de/symeda/sormas/backend/patch/DataPatcherImpl.javasormas-backend/src/main/java/de/symeda/sormas/backend/patch/customizablefield/CustomizableFieldDataPatchRequest.javasormas-backend/src/main/java/de/symeda/sormas/backend/patch/customizablefield/CustomizableFieldDataPatcher.javasormas-backend/src/test/java/de/symeda/sormas/patch/DataPatcherImplTest.java
2 participants
Fixes #14130
Fixes #14136
Summary by CodeRabbit
MM/yyyy(mapped to the first day of the month).