#13711 - Fixed type of contact checkboxes to allow multiple selections#13778
#13711 - Fixed type of contact checkboxes to allow multiple selections#13778roldy merged 6 commits intodevelopmentfrom
Conversation
roldy
requested review from
KarnaiahPesula,
SORMAS-JanBoehme,
obinna-h-n and
raulbob
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughConverts contact proximity from a single value to a multi-valued Set across API DTOs, JPA entity mapping, backend services/transformers, REST schemas, UI, tests, and adds post-query proximity population by contact IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI (Grid / Form)"
participant Facade as "ContactFacadeEjb"
participant Service as "ContactService"
participant DB as "Database"
UI->>Facade: Request contacts (index/export/detailed)
Facade->>DB: Execute query selecting contact fields + contact.id
DB-->>Facade: Return rows (element-collection proximities not in multiselect)
Facade->>Service: getContactProximitiesByContactIds(listOfIds)
Service->>DB: Fetch Contact entities by IDs (includes contact_contactproximities)
DB-->>Service: Return Contacts with proximities
Service-->>Facade: Map<contactId, Set<ContactProximity)>
Facade->>Facade: Populate DTOs via setContactProximities(map.get(id))
Facade-->>UI: Return DTOs with contactProximities populated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
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/contact/ContactListCriteriaBuilder.java (1)
186-199: CONTACT_PROXIMITIES cannot be sorted using the generic contact.get(sortProperty.propertyName) approach.The
contactProximitiesfield is an@ElementCollection(fetch = FetchType.LAZY)of enum values (line 416 in Contact.java), which JPA cannot directly select or sort on. Attempting to sort by this field will cause a runtime exception. The code must either:
- Remove
ContactIndexDto.CONTACT_PROXIMITIESfrom the sortable cases, or- Provide alternative sorting logic that handles ElementCollections appropriately (e.g., sorting by count, existence, or a related scalar field)
🧹 Nitpick comments (18)
sormas-backend/src/main/resources/sql/sormas_schema.sql (3)
15101-15104: Clarify intent of WHERE IS NOT NULL condition in data migration.The migration query only copies non-NULL proximity values to the new join table. Verify that contacts with NULL proximity are intentionally excluded—if any contacts should have their NULL values preserved or handled differently, this migration would silently lose that information.
Additionally, consider whether the migration should be wrapped in a transaction or include explicit logging for visibility into how many records were migrated vs. skipped.
15086-15099: Review cascading delete trigger logic for potential redundancy or conflicts.The migration creates or drops three separate
DELETEtriggers on thecontacttable:
delete_history_trigger_contact_contactproximities(lines 15086-15089)delete_history_trigger(lines 15091-15094)delete_history_trigger_contacts_visits(lines 15096-15099)In PostgreSQL, multiple
DELETEtriggers can coexist and will all fire on deletion. Verify that:
- All three triggers are necessary and intentional (not redundant).
- They don't conflict or cause unintended cascading behavior.
- The order of execution (if order-dependent) is preserved or immaterial.
This pattern suggests possible schema drift or trigger duplication that could become fragile as the codebase evolves.
15067-15084: Verify temporal tracking compatibility in the join table.The join table includes a
sys_periodcolumn and aversioningtrigger for temporal history tracking. Confirm that:
- The temporal
tstzrangesemantics (open-ended upper bound for current rows) are correctly applied during initial insert (line 15101).- The history table
contact_contactproximities_historyis correctly populated by the versioning trigger during subsequent updates or deletes.- Querying current vs. historical proximity values across both the main table and history table yields the expected results.
If this temporal design is new or differs from other multi-valued relationships in the schema, consider documenting the pattern for consistency.
sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactService.java (2)
943-947: Follow-up suppression logic correctly generalized to multiple proximitiesUsing
Set<ContactProximity> contactProximities = contact.getContactProximities();and suppressing follow-up when the set is non-empty andnoneMatch(ContactProximity::hasFollowUp)preserves the old semantics while supporting multiple proximity types:
- Any proximity that requires follow-up keeps the contact in follow-up.
- Only when all selected proximities have no follow-up (or disease has no follow-up) is follow-up disabled.
Given
getContactProximities()now returns an empty set instead of null, the explicit null-check is redundant but harmless.
2033-2059: New getContactProximitiesByContactIds helper is functionally correct; consider minor refinementsThe helper correctly:
- Short-circuits null/empty input with an empty
Map.- Retrieves
Contactentities for the given IDs.- Maps each ID to a defensive
HashSetcopy of itscontactProximities(or an empty set).Two minor refinements you might consider:
- Return
Collections.emptyMap()instead ofnew HashMap<>()for the empty-input case, unless callers rely on mutability.- Document or watch usage for large ID batches, since loading full
Contactentities just to read an element collection may be heavier than a more targeted query with joins, if this ever becomes performance‑critical.sormas-backend/src/main/java/de/symeda/sormas/backend/contact/Contact.java (1)
20-23: JPA element-collection mapping for contactProximities is sound and matches the new modelThe changes cleanly migrate from a single proximity to a collection:
CONTACT_PROXIMITIESconstant reflects the plural field name used throughout DTOs and UI.Set<ContactProximity> contactProximitiesmapped via@ElementCollection(fetch = FetchType.LAZY),@Enumerated(EnumType.STRING), and@CollectionTable(name = "contact_contactproximities", ...)with@Column(name = "contactproximity", nullable = false)is a standard pattern for enum collections.getContactProximities()lazily initializes the set, ensuring non-null collections and simplifying downstream logic such asnoneMatch(ContactProximity::hasFollowUp)inContactService.setContactProximitiesstraightforwardly replaces the collection.Optionally, you could:
- Initialize
contactProximitiesat declaration (= new HashSet<>()) and drop the lazy-init in the getter.- Decide whether
setContactProximities(null)should be treated as “no proximities” (normalize to an empty set in the setter) to make the API more robust to callers.Also applies to: 71-71, 172-172, 414-427
sormas-backend/src/test/java/de/symeda/sormas/backend/contact/ContactFacadeEjbTest.java (1)
2350-2586: New contactProximities tests are thorough and correctly exercise the multi-select behaviorThe added tests:
- Validate persistence and round-trip retrieval of:
- Multiple proximities (
TOUCHED_FLUID,PHYSICAL_CONTACT,SAME_ROOM).- An explicitly empty set.
- A single value (
CLOSE_CONTACT).- Assert that proximities are exposed correctly in:
ContactIndexDto(index list).ContactIndexDetailedDto(detailed index list).- Verify update semantics when adding additional proximities to an existing contact.
This gives good coverage of the new multi-valued model across core retrieval paths. If you want to reduce duplication, the repeated RDCF/user/case/contact setup could be factored into a small helper, but that’s purely a test-maintainability nicety.
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactSelectionGrid.java (1)
44-55: Grid now binds to CONTACT_PROXIMITIES, consistent with SimilarContactDtoUpdating the column to
SimilarContactDto.CONTACT_PROXIMITIEScorrectly aligns the grid with the new collection-based field onSimilarContactDto. This ensures proximities are visible in the selection grid alongside other contact attributes.If the default
Set.toString()rendering is not user-friendly enough, you might later introduce a custom renderer (e.g., comma-separated, localized captions) for this column, but the binding itself is correct.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactSelectionField.java (2)
3-3: Consider avoiding wildcard imports.Replacing explicit imports with a wildcard import (
com.vaadin.ui.*) reduces IDE clarity and can make it harder to identify which specific classes are used.
160-166: Consider i18n for proximity values and use String.join for clarity.The proximity concatenation uses
Object::toStringwhich produces non-localized enum names. Additionally, the reduce operation can be simplified.Consider this approach:
- final Label lblContactProximity = new Label( - referenceContact.getContactProximities() != null && !referenceContact.getContactProximities().isEmpty() - ? referenceContact.getContactProximities().stream().map(Object::toString).reduce((a, b) -> a + ", " + b).orElse("") - : ""); + final Label lblContactProximity = new Label( + referenceContact.getContactProximities() != null && !referenceContact.getContactProximities().isEmpty() + ? referenceContact.getContactProximities().stream() + .map(p -> p.toString()) + .collect(Collectors.joining(", ")) + : "");Note: For proper internationalization, consider using
I18nProperties.getEnumCaption(proximity)instead oftoString()to display localized enum values, similar to patterns used elsewhere in the UI layer.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/SourceContactListEntry.java (1)
93-103: Apply i18n and consider extracting duplicate concatenation logic.This proximity concatenation logic duplicates the pattern in
ContactSelectionField.java(lines 160-163) and similarly lacks internationalization.Consider:
- Using
String.join(", ", ...)orCollectors.joining(", ")instead of reduce for clarity- Applying
I18nProperties.getEnumCaption(proximity)for localized enum values- Extracting the concatenation logic into a shared utility method to eliminate duplication
Example:
if (contact.getContactProximities() != null && !contact.getContactProximities().isEmpty()) { - String proximitiesString = contact.getContactProximities().stream() - .map(Object::toString) - .reduce((a, b) -> a + ", " + b) - .orElse(""); + String proximitiesString = contact.getContactProximities().stream() + .map(p -> I18nProperties.getEnumCaption(p)) + .collect(Collectors.joining(", ")); Label lblContactProximity = new Label(StringUtils.abbreviate(proximitiesString, 50));sormas-ui/src/main/java/de/symeda/sormas/ui/contact/AbstractContactGrid.java (1)
41-41: Avoid wildcard imports for clarity.The wildcard import
de.symeda.sormas.ui.utils.*reduces code clarity and may pull in unnecessary classes. Consider explicit imports for only the classes used from this package.sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDto.java (1)
82-82: Consider initializingcontactProximitiesto an empty set.The field is declared but not initialized, leaving it
nulluntil explicitly set viasetContactProximities. While the javadoc explains deferred population, returningnullfromgetContactProximities()may cause NPEs in callers that iterate without null-checking. Consider initializing toCollections.emptySet()ornew HashSet<>()for defensive programming.- private Set<ContactProximity> contactProximities; + private Set<ContactProximity> contactProximities = new HashSet<>();sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCreateForm.java (2)
23-23: Avoid wildcard imports for better code clarity.The wildcard import
com.vaadin.v7.ui.*and others at lines 44-49 reduce code clarity. Consider explicit imports to make dependencies clearer.
358-364: Null-safe handling of proximities inupdateContactProximity.The code properly handles the case where
getValue()might not be aSet, defaulting tonull. Consider usingCollections.emptySet()as the fallback for safer iteration downstream.private void updateContactProximity() { Object valueObj = contactProximities.getValue(); @SuppressWarnings("unchecked") - Set<ContactProximity> value = valueObj instanceof Set ? (Set<ContactProximity>) valueObj : null; + Set<ContactProximity> value = valueObj instanceof Set ? (Set<ContactProximity>) valueObj : Collections.emptySet(); FieldHelper.updateEnumData( contactProximities, Arrays.asList(ContactProximity.getValues(disease, FacadeProvider.getConfigFacade().getCountryLocale()))); contactProximities.setValue(value); }sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java (1)
31-31: Consider avoiding wildcard imports.Wildcard imports (
import com.vaadin.v7.ui.*;) can introduce naming conflicts and make dependencies less explicit. If the project's coding guidelines permit them, this is acceptable, but explicit imports are generally preferred for maintainability.sormas-api/src/main/java/de/symeda/sormas/api/contact/SimilarContactDto.java (1)
90-96: Inconsistent null-handling compared toContactDto.
ContactDto.getContactProximities()lazily initializes an emptyHashSetif null, butSimilarContactDto.getContactProximities()returns the field directly, which may benull. Consider aligning the approach for consistency:public Set<ContactProximity> getContactProximities() { + if (contactProximities == null) { + contactProximities = new HashSet<>(); + } return contactProximities; }sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactFacadeEjb.java (1)
32-32: Consider using explicit imports instead of wildcards.Wildcard imports (e.g.,
import de.symeda.sormas.api.contact.*;) can reduce code clarity and may cause naming conflicts. Explicit imports make dependencies clearer and IDE navigation more precise.Also applies to: 62-62, 64-64, 67-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactDto.java(4 hunks)sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactExportDto.java(5 hunks)sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDetailedDto.java(3 hunks)sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDto.java(7 hunks)sormas-api/src/main/java/de/symeda/sormas/api/contact/SimilarContactDto.java(6 hunks)sormas-backend/src/main/java/de/symeda/sormas/backend/contact/Contact.java(5 hunks)sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactFacadeEjb.java(12 hunks)sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactIndexDetailedDtoResultTransformer.java(2 hunks)sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactIndexDtoResultTransformer.java(2 hunks)sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactListCriteriaBuilder.java(2 hunks)sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactService.java(3 hunks)sormas-backend/src/main/resources/sql/sormas_schema.sql(2 hunks)sormas-backend/src/test/java/de/symeda/sormas/backend/contact/ContactFacadeEjbTest.java(1 hunks)sormas-backend/src/test/resources/docgeneration/emailTemplates/contacts/ContactEmail.cmp(1 hunks)sormas-backend/src/test/resources/docgeneration/emailTemplates/contacts/ContactEmail.txt(1 hunks)sormas-rest/swagger.json(3 hunks)sormas-rest/swagger.yaml(3 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/AbstractContactGrid.java(4 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCaseConversionSelectionGrid.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCreateForm.java(11 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java(8 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactGridDetailed.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactSelectionField.java(2 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactSelectionGrid.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/SourceContactListEntry.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/components/linelisting/contactfield/ContactLineField.java(1 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/components/linelisting/layout/LineListingLayout.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCaseConversionSelectionGrid.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/contact/SimilarContactDto.java (1)
SimilarContactDto(12-157)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactSelectionGrid.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/contact/SimilarContactDto.java (1)
SimilarContactDto(12-157)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactSelectionField.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/contact/SimilarContactDto.java (1)
SimilarContactDto(12-157)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactGridDetailed.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDetailedDto.java (1)
ContactIndexDetailedDto(20-183)
sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactListCriteriaBuilder.java (1)
sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDto.java (1)
ContactIndexDto(35-485)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/AbstractContactGrid.java (2)
sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDto.java (1)
ContactIndexDto(35-485)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/contact/SourceContactListEntry.java (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/CssStyles.java (1)
CssStyles(26-546)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCreateForm.java (2)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/LayoutUtil.java (1)
LayoutUtil(30-418)sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java (1)
ContactDataForm(68-1149)
sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactFacadeEjb.java (2)
sormas-api/src/main/java/de/symeda/sormas/api/utils/fieldaccess/checkers/AnnotationBasedFieldAccessChecker.java (1)
AnnotationBasedFieldAccessChecker(23-68)sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDto.java (1)
ContactIndexDto(35-485)
⏰ 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). (5)
- GitHub Check: android app test (26)
- GitHub Check: SORMAS CI
- GitHub Check: android app test (28)
- GitHub Check: android app test (27)
- GitHub Check: Lint Code Base
🔇 Additional comments (39)
sormas-backend/src/main/resources/sql/sormas_schema.sql (1)
15107-15108: Ensure DROP COLUMN IF EXISTS is intentional and safe.The use of
DROP COLUMN IF EXISTSsuggests this migration may be idempotent or re-runnable. Verify:
- Whether re-running the migration script is a supported use case for this codebase.
- Whether any dependent views, stored procedures, or application code still reference the old
contactproximitycolumn and could break.- Whether the
IF EXISTSclauses prevent proper error detection if the migration partially fails.If this migration should run only once, consider removing
IF EXISTSto fail loudly if the column has already been dropped, signaling a deployment or script-ordering issue.sormas-backend/src/test/resources/docgeneration/emailTemplates/contacts/ContactEmail.cmp (1)
19-21: Empty-properties marker change looks fine; just confirm dual markers are intentionalSwitching line 20 to
[]is consistent with representing empty collections and aligns with the updated text template. With./.still present on line 21, please confirm both markers are expected in this comparison template and not a leftover from the old format.sormas-backend/src/test/resources/docgeneration/emailTemplates/contacts/ContactEmail.txt (1)
21-26: Template variable rename correctly reflects collection-based proximitiesUsing
$contact_contactProximitieshere matches the newcontactProximitiesfield and constant naming in the API/DTOs. This looks correct; just ensure any remaining$contact_contactProximityplaceholders in other templates were also updated.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/components/linelisting/layout/LineListingLayout.java (1)
5-5: Correctly adapts line-listing typeOfContact to the new contactProximities setWrapping the single
typeOfContactvalue intoCollections.singleton(...)and assigning it tocontactProximitiesis a clean way to bridge the existing single-select line-listing UI with the new multi-valued backend model. The null guard avoids accidentally overwriting with an empty set.Also applies to: 25-25, 199-202
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/components/linelisting/contactfield/ContactLineField.java (1)
112-119: Caption key updated to CONTACT_PROXIMITIES and is consistent with the new field nameUsing
ContactDto.CONTACT_PROXIMITIESfor thetypeOfContactcaption keeps this line-listing field aligned with the renamed contact property and i18n keys. The control itself remains a single-selectComboBox<ContactProximity>, which is fine for now given that line listing was already single-select; just confirm that you don’t also want multi-select behavior in this specific UI context, now that the backend supports it.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCaseConversionSelectionGrid.java (1)
39-39: LGTM: Constant name updated to reflect multi-proximity support.The change from
CONTACT_PROXIMITYtoCONTACT_PROXIMITIESaligns with the API-wide migration to support multiple proximity values per contact.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactGridDetailed.java (1)
42-42: LGTM: Column reference updated for multi-proximity support.The constant name change from
CONTACT_PROXIMITYtoCONTACT_PROXIMITIEScorrectly updates the column insertion logic to align with the new multi-value proximity field.sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactListCriteriaBuilder.java (1)
111-111: Post-query enrichment pattern correctly implements contactProximities population.The query selection changed from
contact.get(Contact.CONTACT_PROXIMITY)tocontact.get(Contact.ID)becausecontactProximitiesis an ElementCollection that cannot be selected in multiselect queries. The implementation correctly populates it separately after querying usingContactService.getContactProximitiesByContactIds(), as seen inContactFacadeEjb.java(lines 1283–1287 and 1340–1344). The pattern is intentional and documented in the code.sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactIndexDtoResultTransformer.java (1)
49-49: Verify alignment between ContactIndexDto constructor, query projection, and result transformer casting.The tuple element cast changed from
ContactProximitytoLongat line 49. Confirmed this aligns with:
- The
ContactIndexDtoconstructor (line 123) which expectsLong idat this parameter position- The query projection in
ContactListCriteriaBuilder(line 112) which selectscontact.get(Contact.ID)The change is correct and properly aligned across all three components.
sormas-rest/swagger.yaml (1)
11642-11660: AllcontactProximityreferences have been properly converted tocontactProximitiesacross the swagger.yaml file.The changes are correct and complete:
- No orphaned singular
contactProximityproperty definitions remain- All 14 enum values match exactly with the Java
ContactProximityenum definition- All three Contact-related schemas (ContactDto, ContactIndexDetailedDto, ContactIndexDto) have been consistently updated
- The OpenAPI/Swagger syntax is correct
The conversion aligns with the PR objective to enable multiple proximity selections and maintains consistency across the API specification.
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/AbstractContactGrid.java (2)
166-177: LGTM! Renderer implementation is correct.The renderer properly handles null and empty sets, uses localized enum captions via
I18nProperties.getEnumCaption, and joins multiple proximities with commas. The null/empty check prevents NPEs.
224-224: Column reference correctly updated.The column reference is properly updated to
CONTACT_PROXIMITIESto align with the new multi-value data model.sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDto.java (2)
116-119: Javadoc clearly explains the design decision.Good documentation explaining why
contactProximitiesis populated separately due to JPA ElementCollection limitations.
473-479: Newidaccessors support the post-query population pattern.The
getId()andsetId()methods enable the backend to populatecontactProximitiesafter the initial query using the contact's ID.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactCreateForm.java (3)
86-86: Type change fromNullableOptionGrouptoOptionGroupis correct.The change to
OptionGroup(withsetMultiSelect(true)) properly enables multiple checkbox selections, which is the core fix for issue #13711.
193-205: Multi-select implementation correctly enables multiple proximity selections.The implementation:
- Uses
OptionGroupwithsetMultiSelect(true)to allow multiple selections- Properly handles the value as a
Set<ContactProximity>in the value change listener- Includes appropriate type checking (
instanceof Set) before castingThis directly addresses the bug where checkboxes behaved like radio buttons.
329-332: Delegation toContactDataForm.deduceContactCategorypromotes code reuse.Good refactoring to delegate category deduction logic to
ContactDataForm, avoiding duplication and ensuring consistent behavior across forms.sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactIndexDetailedDto.java (2)
69-69: Constructor parameter correctly updated toLong id.The parameter change from
ContactProximitytoLong idaligns with the parent classContactIndexDtoand the overall migration to post-query proximity population.
84-88: Super constructor call properly passesidparameter.The super call correctly passes the
idparameter toContactIndexDto, maintaining consistency with the parent class's updated signature.sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactExportDto.java (4)
123-123: Field type correctly changed toSet<ContactProximity>.The field declaration aligns with the multi-value proximity model. Note that like
ContactIndexDto, this field starts asnulluntil populated post-query.
250-253: Clear javadoc explaining deferred population pattern.Good documentation explaining why
contactProximitiescannot be selected directly in JPA queries due to ElementCollection limitations.
308-308: Inline comment clarifies intentional design.The comment clearly indicates that
contactProximitiesis intentionally not set in the constructor and will be populated separately.
610-614: Export annotation and getter correctly updated.The
@ExportPropertyannotation correctly referencesContactDto.CONTACT_PROXIMITIES, and the return type is properly changed toSet<ContactProximity>. The backend export logic properly handles Set serialization—ContactFacadeEjbexplicitly populatescontactProximitiesviagetContactProximitiesByContactIds(), which returnsMap<Long, Set<ContactProximity>>, confirming the field integrates seamlessly into the export flow.sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactIndexDetailedDtoResultTransformer.java (1)
53-53: Tuple mapping correctly updated forLong idparameter.The cast to
(Long)aligns with theContactIndexDetailedDtoconstructor signature where theidfield is positioned beforeContactClassification. TheContactProximitiesare populated separately after query execution through a post-processing step that uses the contact IDs retrieved from the DTO.sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java (2)
268-271: Multi-select OptionGroup setup looks correct.The field is properly configured with
setMultiSelect(true)and the horizontal style is removed for better UX with multiple checkboxes.
755-789: Static helper method is a good pattern for testability.Extracting
deduceContactCategoryas a package-visible static method allows unit testing the category deduction logic independently of the UI form.sormas-api/src/main/java/de/symeda/sormas/api/contact/ContactDto.java (2)
548-553: Lazy initialization in getter is acceptable but mutates state.The getter initializes
contactProximitiesif null. This pattern prevents NPEs when iterating over the collection, but be aware that callinggetContactProximities()modifies internal state—subsequent serialization or comparison may behave differently than expected if the getter was never called.
87-87: API constant rename is a breaking change.Renaming
CONTACT_PROXIMITYtoCONTACT_PROXIMITIESis a public API change. Ensure all consumers (including external integrations and REST clients) are updated accordingly.#!/bin/bash # Search for any remaining references to the old constant name rg -n "CONTACT_PROXIMITY[^I]" --type javasormas-api/src/main/java/de/symeda/sormas/api/contact/SimilarContactDto.java (2)
150-156: Newidfield getter/setter looks correct.The
idfield enables post-query population of proximities by contact ID. This is a reasonable pattern when proximities are fetched separately to avoid N+1 query issues.
44-64: No changes needed – constructor behavior is intentional.
contactProximitiesis not initialized in the constructor by design. Element collections cannot be selected in JPA multiselect queries, socontactProximitiesis populated separately inContactFacadeEjb.getMatchingContacts()viasetContactProximities()before returning. This pattern matchesContactIndexDtoandContactExportDto. Callers properly null-check before accessing the field, and the field is always populated before objects are exposed externally.sormas-rest/swagger.json (2)
13980-13989: Consistency verified across ContactIndexDto and ContactIndexDetailedDto.The schema changes in hunks 2 and 3 maintain consistency with hunk 1. All three Contact-related DTOs correctly define
contactProximitiesas an array of enum strings with the same 14 proximity types.Also applies to: 14151-14160
13633-13639: Schema migration from singular to plural is complete; document breaking API change.The migration from
contactProximitytocontactProximitiesis complete across all affected schemas (ContactDto, ContactIndexDto, ContactIndexDetailedDto). All three schema definitions are consistent in structure and no stale singular references remain.This is a breaking change for REST clients expecting the old
contactProximityproperty. Consider documenting the breaking change in release notes and providing a migration guide for API consumers.sormas-backend/src/main/java/de/symeda/sormas/backend/contact/ContactFacadeEjb.java (7)
753-758: LGTM! Proximity loading pattern correctly handles JPA limitation.The separate query to populate
contactProximitiesis a proper workaround for JPA's ElementCollection limitation in multiselect queries. The use ofgetOrDefaultwith an empty HashSet ensures safe handling of missing entries.
1282-1288: LGTM! Consistent proximity loading implementation.The proximity population follows the same correct pattern established in
getExportList, maintaining consistency across the codebase.
1339-1345: LGTM! Proximity loading consistently applied.
1502-1502: LGTM! Entity mapping correctly updated for collection.The change from singular to plural correctly reflects the new collection-based proximity model.
1854-1854: LGTM! Excellent defensive copy implementation.Creating a new
HashSetprevents unintended sharing of mutable collection state between the entity and DTO. The null-safe check is also appropriate.
2080-2080: LGTM! Matching contacts query correctly adapted.The query selection change from
Contact.PROXIMITYtoContact.IDaligns with the separate proximity loading pattern, maintaining consistency across all query methods.Also applies to: 2092-2098
2363-2363: LGTM! Completeness calculation correctly handles collection.The condition properly checks both null and emptiness, ensuring that only contacts with actual proximity data contribute to the completeness score.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java(8 hunks)sormas-ui/src/main/java/de/symeda/sormas/ui/utils/NullableOptionGroup.java(1 hunks)
⏰ 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). (5)
- GitHub Check: android app test (28)
- GitHub Check: android app test (27)
- GitHub Check: android app test (26)
- GitHub Check: Lint Code Base
- GitHub Check: SORMAS CI
🔇 Additional comments (4)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/NullableOptionGroup.java (1)
74-80: LGTM! Well-designed helper method.The
setNullableValuemethod properly handles setting a single value on this multi-select component by wrapping it in a singleton set. This complements the existinggetNullableValuegetter and enables clean single-value semantics where needed (e.g., setting contact category from multiple proximities).sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java (3)
267-270: LGTM! Correctly implements multi-select proximity.The field is properly configured as an
OptionGroupwithsetMultiSelect(true), enabling the checkbox behavior described in issue #13711. Removing the horizontal styling is appropriate for a multi-select checkbox group.
102-102: LGTM! References consistently updated.All references to the contact proximity field have been correctly updated from the singular
contactProximityto the pluralcontactProximities, maintaining consistency throughout the form layout, styling, and data population logic.Also applies to: 697-697, 888-888
750-788: Confirm: Past issue resolved and German system coverage is complete, but unmapped enum values pose maintenance risk.The refactoring correctly handles multiple proximities:
- Line 785 correctly uses
setNullableValue(highestRiskCategory)instead of passing a Set tosetValue, resolving the critical bug from the previous review- Static helper method
deduceContactCategoryis well-designed for testability- Null and empty checks are present
For German systems (where this method is used), all
ContactProximityvalues returned byContactProximity.getValues()for CORONAVIRUS are covered:
- FACE_TO_FACE_LONG, TOUCHED_FLUID, AEROSOL → HIGH_RISK
- MEDICAL_UNSAFE → HIGH_RISK_MED
- MEDICAL_LIMITED → MEDIUM_RISK_MED
- SAME_ROOM, FACE_TO_FACE_SHORT, MEDICAL_SAME_ROOM → LOW_RISK
- MEDICAL_SAFE, MEDICAL_DISTANT → NO_RISK
However, four
ContactProximityenum values remain unmapped: PHYSICAL_CONTACT, CLOTHES_OR_OTHER, CLOSE_CONTACT, and AIRPLANE. If these are ever passed to the method (via API or future changes), the contact category will silently remain unchanged. Consider either mapping these values or adding a default case with explicit handling and a comment explaining why they are excluded.
| contactProximities.addValueChangeListener(e -> { | ||
| if (!getInternalValue().getContactProximities().equals(e.getProperty().getValue()) || contactCategory.isModified()) { | ||
| updateContactCategory((Set<ContactProximity>) contactProximities.getValue()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B2 -A2 'getContactProximities\(\)' --type=java -g '!*Test.java'Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 8605
🏁 Script executed:
cat -n sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java | sed -n '250,300p'Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 2865
🏁 Script executed:
rg -n 'addValueChangeListener' sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java -A5 | head -50Repository: SORMAS-Foundation/SORMAS-Project
Length of output: 2334
Add null safety check for form internal value in listener.
The comparison at line 276 chains calls without checking if getInternalValue() is null. While getContactProximities() has defensive initialization in its getter, the setter can explicitly set the value to null (as seen in ContactFacadeEjb.java:1854), and more importantly, getInternalValue() itself may be null during form initialization or if the form is not properly bound. This pattern should be defensive, as shown in similar listeners elsewhere in the file (e.g., line 452: if (getValue() != null)).
Wrap the comparison with a null check: if (getInternalValue() != null && !getInternalValue().getContactProximities().equals(...) || ...)
🤖 Prompt for AI Agents
In sormas-ui/src/main/java/de/symeda/sormas/ui/contact/ContactDataForm.java
around lines 275-279, the listener calls
getInternalValue().getContactProximities() without guarding against a null
internal value; update the if condition to check getInternalValue() != null
before accessing its fields and ensure correct grouping so the overall condition
becomes: (getInternalValue() != null &&
!getInternalValue().getContactProximities().equals(e.getProperty().getValue()))
|| contactCategory.isModified(); this prevents NPE during form initialization or
when the internal value is explicitly set to null.
… to prevent potential NullPointerExceptions during form initialization or when the form is not properly bound
There was a problem hiding this comment.
It seems that the formatter has collapsed package imports. Please check because with the eclipse formatter it should not happen.
There was a problem hiding this comment.
I think the modifications for DTOs are causing a test to fail:
Error: InfraValidationSoundnessIncomingTest>InfraValidationSoundnessTest.testShareContactValidation:654->InfraValidationSoundnessTest.assertValidationDto:541->InfraValidationSoundnessTest.getExpected:521->InfraValidationSoundnessTest.getInfraPaths:468->InfraValidationSoundnessTest.buildDtoTree:238->InfraValidationSoundnessTest.buildDtoTree:238->InfraValidationSoundnessTest.buildDtoTree:228 » NotImplemented Ping @JonasCir
Error: InfraValidationSoundnessOutgoingTest>InfraValidationSoundnessTest.testShareContactValidation:654->InfraValidationSoundnessTest.assertValidationDto:541->InfraValidationSoundnessTest.getExpected:521->InfraValidationSoundnessTest.getInfraPaths:468->InfraValidationSoundnessTest.buildDtoTree:238->InfraValidationSoundnessTest.buildDtoTree:238->InfraValidationSoundnessTest.buildDtoTree:228 » NotImplemented Ping @JonasCir
There was a problem hiding this comment.
Actionable comments posted: 0
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/test/java/de/symeda/sormas/backend/sormastosormas/validation/InfraValidationSoundnessTest.java (1)
380-418: MissingSet→HashSetconversion will cause instantiation failure.The traversal logic at lines 403-418 now correctly supports
Set, but the instantiation logic at lines 380-383 only handlesList→ArrayListconversion. When aSetfield isnulland needs to be created, line 396 will throwNoSuchMethodExceptionbecauseSetis an interface with no constructor.🐛 Proposed fix
Class<?> erasedType = currentField.getType().getErasedType(); if (erasedType == List.class) { erasedType = ArrayList.class; } +if (erasedType == Set.class) { + erasedType = HashSet.class; +}
# Conflicts: # sormas-backend/src/main/resources/sql/sormas_schema.sql
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13778 |
3 participants
Fixes #13711
Summary by CodeRabbit
New Features
APIs & Exports
Database
Backend
UI
Tests
✏️ Tip: You can customize this high-level summary in your review settings.