FIX: NgSurvey integration fixes#14056
Conversation
…RMAS-Project into fix/ng-survey-integration-deployment
|
Warning Review limit reached
More reviews will be available in 8 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSurvey response handling now rejects empty external survey IDs, deduplicates report IDs during ingestion, saves reprocessed messages even on failure, updates correction UI feedback, and adds a schema migration that removes survey configuration entries. ChangesSurvey response and survey configuration flow
Estimated review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 3
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/externalmessage/ExternalMessageService.java (1)
331-340: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDuplicate
reportIdresolution is nondeterministic.Line 340 keeps the first tuple on collisions, but this query has no
ORDER BY, so the surviving UUID/status depends on database row order. If the samereportIdexists once asUNPROCESSEDand once asPROCESSED, the overwrite path can pick the wrong record and skip the intended refresh.🤖 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/externalmessage/ExternalMessageService.java` around lines 331 - 340, The reportId-to-message mapping in ExternalMessageService is resolving duplicates nondeterministically because the Collectors.toMap merge function keeps an arbitrary first tuple while the query has no ordering. Update the query/stream handling around the multiselect and toMap collection so duplicate reportId entries are resolved deterministically, preferring the intended ExternalMessageStatus (for example, PROCESSED over UNPROCESSED) using a clear comparator or explicit post-query selection rather than relying on database row order.
🤖 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/externalmessage/ExternalMessageFacadeEjb.java`:
- Around line 1037-1048: The reprocessing flow in ExternalMessageFacadeEjb still
rethrows from the InterruptedException and ExecutionException catches, so the
method never returns the saved DTO to SurveyResponseFailureEditor. Change the
try/catch/finally structure around
automaticSurveyResponseProcessor.processSurveyResponses(...) and
save(externalMessage) so the edited response is always persisted and the method
returns the saved result even when reprocessing fails, instead of propagating a
RuntimeException from this path.
- Around line 313-345: The survey response filtering in ExternalMessageFacadeEjb
should discard null entries before the later reuse of the same list, since the
current reportIds stream only filters nulls for collection and the subsequent
surveyResponses.forEach and final stream still dereference dto.getReportId().
Update the processing in the method that builds reportIds and reuses
surveyResponses so the working list itself is null-safe before any later
iteration, preventing NPEs when a batch contains a null ExternalMessageDto.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/externalmessage/surveyresponse/SurveyResponseDetailsWindow.java`:
- Around line 213-215: The new action can open an empty
SurveyResponseFailureEditor because the enablement logic in
SurveyResponseDetailsWindow now allows opening when PatchResponse is not applied
even if there are no failures. Update the
canProcess/responseReceivedButNotApplied condition so the action is only enabled
when there is at least one failure to display, and align the summary text with
the same applied/not-applied state used by SurveyResponseFailureEditor and
PatchResponse.
---
Outside diff comments:
In
`@sormas-backend/src/main/java/de/symeda/sormas/backend/externalmessage/ExternalMessageService.java`:
- Around line 331-340: The reportId-to-message mapping in ExternalMessageService
is resolving duplicates nondeterministically because the Collectors.toMap merge
function keeps an arbitrary first tuple while the query has no ordering. Update
the query/stream handling around the multiselect and toMap collection so
duplicate reportId entries are resolved deterministically, preferring the
intended ExternalMessageStatus (for example, PROCESSED over UNPROCESSED) using a
clear comparator or explicit post-query selection rather than relying on
database row order.
🪄 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: af7fb66d-82c1-4dd5-82f9-e6506f2ac86e
📒 Files selected for processing (8)
sormas-api/src/main/java/de/symeda/sormas/api/i18n/Strings.javasormas-api/src/main/resources/strings.propertiessormas-backend/src/main/java/de/symeda/sormas/backend/common/CriteriaBuilderHelper.javasormas-backend/src/main/java/de/symeda/sormas/backend/externalmessage/ExternalMessageFacadeEjb.javasormas-backend/src/main/java/de/symeda/sormas/backend/externalmessage/ExternalMessageService.javasormas-backend/src/main/java/de/symeda/sormas/backend/survey/SurveyFacadeEjb.javasormas-ui/src/main/java/de/symeda/sormas/ui/externalmessage/surveyresponse/SurveyResponseDetailsWindow.javasormas-ui/src/main/java/de/symeda/sormas/ui/externalmessage/surveyresponse/SurveyResponseFailureEditor.java
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/resources/sql/sormas_schema.sql`:
- Around line 16420-16426: The EXTERNAL_MESSAGES migration currently replaces
the entire properties JSON in featureconfiguration, which overwrites existing
settings for all deployments. Update the SQL to modify only the ngSurvey-related
flag instead of rebuilding properties in the update for EXTERNAL_MESSAGES,
preserving any existing keys and values. Use the featureconfiguration properties
column and the EXTERNAL_MESSAGES featuretype condition to locate the change, and
if a reset is still needed, guard it behind a legacy ngSurvey-specific check
rather than applying it globally.
🪄 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: 0c34e49b-ac8a-47cc-b1f4-36bcaa9845e0
📒 Files selected for processing (2)
sormas-backend/src/main/java/de/symeda/sormas/backend/survey/SurveyTokenFacadeEjb.javasormas-backend/src/main/resources/sql/sormas_schema.sql
✅ Files skipped from review due to trivial changes (1)
- sormas-backend/src/main/java/de/symeda/sormas/backend/survey/SurveyTokenFacadeEjb.java
| UPDATE featureconfiguration | ||
| SET properties = json_build_object( | ||
| 'FETCH_MODE', false, | ||
| 'FORCE_AUTOMATIC_PROCESSING', true, | ||
| 'SURVEY_FETCH_ENABLED', false | ||
| ) | ||
| WHERE featuretype = 'EXTERNAL_MESSAGES'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve existing EXTERNAL_MESSAGES settings instead of replacing them.
Line 16420 rewrites the whole properties JSON, so every upgrade now forces FETCH_MODE=false and SURVEY_FETCH_ENABLED=false globally. That changes existing external-message integrations outside the ngSurvey fix scope and can silently disable deployments that already rely on fetch mode. Patch only the flag this PR needs, or gate the reset behind a legacy ngSurvey-specific condition.
Suggested SQL
UPDATE featureconfiguration
-SET properties = json_build_object(
- 'FETCH_MODE', false,
- 'FORCE_AUTOMATIC_PROCESSING', true,
- 'SURVEY_FETCH_ENABLED', false
-)
+SET properties = COALESCE(properties, '{}'::jsonb) || jsonb_build_object(
+ 'FORCE_AUTOMATIC_PROCESSING', true
+)
WHERE featuretype = 'EXTERNAL_MESSAGES';📝 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.
| UPDATE featureconfiguration | |
| SET properties = json_build_object( | |
| 'FETCH_MODE', false, | |
| 'FORCE_AUTOMATIC_PROCESSING', true, | |
| 'SURVEY_FETCH_ENABLED', false | |
| ) | |
| WHERE featuretype = 'EXTERNAL_MESSAGES'; | |
| UPDATE featureconfiguration | |
| SET properties = COALESCE(properties, '{}'::jsonb) || jsonb_build_object( | |
| 'FORCE_AUTOMATIC_PROCESSING', true | |
| ) | |
| WHERE featuretype = 'EXTERNAL_MESSAGES'; |
🤖 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/resources/sql/sormas_schema.sql` around lines 16420 -
16426, The EXTERNAL_MESSAGES migration currently replaces the entire properties
JSON in featureconfiguration, which overwrites existing settings for all
deployments. Update the SQL to modify only the ngSurvey-related flag instead of
rebuilding properties in the update for EXTERNAL_MESSAGES, preserving any
existing keys and values. Use the featureconfiguration properties column and the
EXTERNAL_MESSAGES featuretype condition to locate the change, and if a reset is
still needed, guard it behind a legacy ngSurvey-specific check rather than
applying it globally.
1 participant
Fixes #13832
Summary by CodeRabbit
New Features
Bug Fixes