fix(person): flip family_member on both endpoints when a family-graph relationship is added
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m39s
CI / OCR Service Tests (pull_request) Successful in 20s
CI / Backend Unit Tests (pull_request) Failing after 3m45s
CI / fail2ban Regex (pull_request) Successful in 46s
CI / Semgrep Security Scan (pull_request) Successful in 20s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m4s
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m39s
CI / OCR Service Tests (pull_request) Successful in 20s
CI / Backend Unit Tests (pull_request) Failing after 3m45s
CI / fail2ban Regex (pull_request) Successful in 46s
CI / Semgrep Security Scan (pull_request) Successful in 20s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m4s
The canonical importer creates persons via PersonRegisterImporter first (no family_member set) and then upserts them via PersonTreeImporter, but mergeCanonical never propagates family_member to existing persons — so persons with imported relationships ended up flagged family_member=false and never appeared in /api/persons family filters or the family-network view. RelationshipService is documented as the owner of the family_member flag, so the fix lives there: addRelationship now sets family_member=true on both endpoints whenever the relation type is PARENT_OF / SPOUSE_OF / SIBLING_OF (the same set getFamilyNetwork filters by). Non-family types (FRIEND/COLLEAGUE/EMPLOYER/DOCTOR/NEIGHBOR/OTHER) leave the flag alone — a family doctor isn't a family member. Extracted the type list as a FAMILY_RELATION_TYPES constant and reused it in getFamilyNetwork for a single source of truth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -31,6 +31,12 @@ import java.util.UUID;
|
|||||||
@RequiredArgsConstructor
|
@RequiredArgsConstructor
|
||||||
public class RelationshipService {
|
public class RelationshipService {
|
||||||
|
|
||||||
|
// Single source of truth for which relationship types are part of the family graph.
|
||||||
|
// Consulted by addRelationship (to set family_member on both endpoints) and by
|
||||||
|
// getFamilyNetwork (to filter the edges returned). FRIEND/COLLEAGUE/etc. are excluded.
|
||||||
|
private static final List<RelationType> FAMILY_RELATION_TYPES =
|
||||||
|
List.of(RelationType.PARENT_OF, RelationType.SPOUSE_OF, RelationType.SIBLING_OF);
|
||||||
|
|
||||||
private final PersonRelationshipRepository relationshipRepository;
|
private final PersonRelationshipRepository relationshipRepository;
|
||||||
private final PersonService personService;
|
private final PersonService personService;
|
||||||
private final RelationshipInferenceService inferenceService;
|
private final RelationshipInferenceService inferenceService;
|
||||||
@@ -64,7 +70,7 @@ public class RelationshipService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
List<PersonRelationship> familyEdges = relationshipRepository.findAllByRelationTypeIn(
|
List<PersonRelationship> familyEdges = relationshipRepository.findAllByRelationTypeIn(
|
||||||
List.of(RelationType.PARENT_OF, RelationType.SPOUSE_OF, RelationType.SIBLING_OF));
|
FAMILY_RELATION_TYPES);
|
||||||
|
|
||||||
List<RelationshipDTO> edges = new ArrayList<>();
|
List<RelationshipDTO> edges = new ArrayList<>();
|
||||||
for (PersonRelationship r : familyEdges) {
|
for (PersonRelationship r : familyEdges) {
|
||||||
@@ -105,15 +111,23 @@ public class RelationshipService {
|
|||||||
.notes(blankToNull(dto.notes()))
|
.notes(blankToNull(dto.notes()))
|
||||||
.build();
|
.build();
|
||||||
|
|
||||||
|
PersonRelationship saved;
|
||||||
try {
|
try {
|
||||||
// saveAndFlush so the unique_rel constraint violates synchronously and is
|
// saveAndFlush so the unique_rel constraint violates synchronously and is
|
||||||
// caught here, not at commit time outside the @Transactional boundary.
|
// caught here, not at commit time outside the @Transactional boundary.
|
||||||
return toDTO(relationshipRepository.saveAndFlush(rel));
|
saved = relationshipRepository.saveAndFlush(rel);
|
||||||
} catch (DataIntegrityViolationException e) {
|
} catch (DataIntegrityViolationException e) {
|
||||||
throw DomainException.conflict(
|
throw DomainException.conflict(
|
||||||
ErrorCode.DUPLICATE_RELATIONSHIP,
|
ErrorCode.DUPLICATE_RELATIONSHIP,
|
||||||
"Relationship already exists for (" + personId + ", " + relatedPerson.getId() + ", " + dto.relationType() + ")");
|
"Relationship already exists for (" + personId + ", " + relatedPerson.getId() + ", " + dto.relationType() + ")");
|
||||||
}
|
}
|
||||||
|
// Family-graph edges imply both endpoints are family members. Idempotent: the
|
||||||
|
// setter is a no-op when the person is already flagged, so re-imports stay clean.
|
||||||
|
if (FAMILY_RELATION_TYPES.contains(dto.relationType())) {
|
||||||
|
personService.setFamilyMember(person.getId(), true);
|
||||||
|
personService.setFamilyMember(relatedPerson.getId(), true);
|
||||||
|
}
|
||||||
|
return toDTO(saved);
|
||||||
}
|
}
|
||||||
|
|
||||||
@Transactional
|
@Transactional
|
||||||
|
|||||||
@@ -23,6 +23,8 @@ import java.util.UUID;
|
|||||||
import static org.assertj.core.api.Assertions.assertThat;
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||||
import static org.mockito.ArgumentMatchers.any;
|
import static org.mockito.ArgumentMatchers.any;
|
||||||
|
import static org.mockito.ArgumentMatchers.anyBoolean;
|
||||||
|
import static org.mockito.ArgumentMatchers.eq;
|
||||||
import static org.mockito.Mockito.never;
|
import static org.mockito.Mockito.never;
|
||||||
import static org.mockito.Mockito.verify;
|
import static org.mockito.Mockito.verify;
|
||||||
import static org.mockito.Mockito.when;
|
import static org.mockito.Mockito.when;
|
||||||
@@ -148,6 +150,50 @@ class RelationshipServiceTest {
|
|||||||
assertThat(result.notes()).isEqualTo("first born");
|
assertThat(result.notes()).isEqualTo("first born");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void addRelationship_marks_both_endpoints_as_family_member_when_type_is_family() {
|
||||||
|
// Creating a family-graph edge (PARENT_OF / SPOUSE_OF / SIBLING_OF) must mark both
|
||||||
|
// endpoints as family members so they appear in findAllFamilyMembers and the network.
|
||||||
|
// This is what makes the canonical importer's relationships actually show up in the UI.
|
||||||
|
when(personService.getById(alice.getId())).thenReturn(alice);
|
||||||
|
when(personService.getById(bob.getId())).thenReturn(bob);
|
||||||
|
when(relationshipRepository.existsByPersonIdAndRelatedPersonIdAndRelationType(
|
||||||
|
bob.getId(), alice.getId(), RelationType.PARENT_OF)).thenReturn(false);
|
||||||
|
when(relationshipRepository.saveAndFlush(any())).thenAnswer(inv -> {
|
||||||
|
PersonRelationship r = inv.getArgument(0);
|
||||||
|
r.setId(UUID.randomUUID());
|
||||||
|
r.setCreatedAt(Instant.now());
|
||||||
|
return r;
|
||||||
|
});
|
||||||
|
|
||||||
|
var dto = new CreateRelationshipRequest(bob.getId(), RelationType.PARENT_OF, null, null, null);
|
||||||
|
service.addRelationship(alice.getId(), dto);
|
||||||
|
|
||||||
|
verify(personService).setFamilyMember(alice.getId(), true);
|
||||||
|
verify(personService).setFamilyMember(bob.getId(), true);
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void addRelationship_does_not_flip_family_member_for_non_family_type() {
|
||||||
|
// FRIEND / COLLEAGUE / EMPLOYER / DOCTOR / NEIGHBOR / OTHER are NOT family-graph
|
||||||
|
// edges (see getFamilyNetwork's filter), so addRelationship must leave family_member
|
||||||
|
// alone — a doctor of the family is not a family member.
|
||||||
|
when(personService.getById(alice.getId())).thenReturn(alice);
|
||||||
|
when(personService.getById(bob.getId())).thenReturn(bob);
|
||||||
|
when(relationshipRepository.saveAndFlush(any())).thenAnswer(inv -> {
|
||||||
|
PersonRelationship r = inv.getArgument(0);
|
||||||
|
r.setId(UUID.randomUUID());
|
||||||
|
r.setCreatedAt(Instant.now());
|
||||||
|
return r;
|
||||||
|
});
|
||||||
|
|
||||||
|
var dto = new CreateRelationshipRequest(bob.getId(), RelationType.FRIEND, null, null, null);
|
||||||
|
service.addRelationship(alice.getId(), dto);
|
||||||
|
|
||||||
|
verify(personService, never()).setFamilyMember(eq(alice.getId()), anyBoolean());
|
||||||
|
verify(personService, never()).setFamilyMember(eq(bob.getId()), anyBoolean());
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void deleteRelationship_succeeds_when_viewpoint_is_object() {
|
void deleteRelationship_succeeds_when_viewpoint_is_object() {
|
||||||
UUID relId = UUID.randomUUID();
|
UUID relId = UUID.randomUUID();
|
||||||
|
|||||||
Reference in New Issue
Block a user