OZG-6867 OZG-7002 Fix adding child groups

src/main/java/de/ozgcloud/admin/keycloak/KeycloakApiService.java

@@ -20,11 +20,17 @@ class KeycloakApiService {
 	}
 
 	public String addGroup(GroupRepresentation groupRepresentation) {
-		try (var response = groupsResource.add(groupRepresentation)) {
+		try (var response = addParentOrChildGroup(groupRepresentation)) {
 			return getAddedResourceIdFromResponse(response);
 		}
 	}
 
+	Response addParentOrChildGroup(GroupRepresentation groupRepresentation) {
+		return groupRepresentation.getParentId() == null ?
+				groupsResource.add(groupRepresentation) :
+				groupsResource.group(groupRepresentation.getParentId()).subGroup(groupRepresentation);
+	}
+
 	String getAddedResourceIdFromResponse(Response response) {
 		if (response.getStatus() == Response.Status.CREATED.getStatusCode()) {
 			return extractResourceIdFromLocationHeader(response.getHeaderString("Location"));

src/main/java/de/ozgcloud/admin/organisationseinheit/SyncService.java

@@ -2,6 +2,7 @@ package de.ozgcloud.admin.organisationseinheit;
 
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Stream;
 
 import org.springframework.stereotype.Service;
 
@@ -89,7 +90,28 @@ class SyncService {
 	}
 
 	public void syncAddedOrganisationsEinheiten(long syncTimestamp) {
-		repository.findAllWithoutSyncResult().forEach(organisationsEinheit -> syncAddedOrganisationsEinheit(syncTimestamp, organisationsEinheit));
+		sortInAdditionOrder(repository.findAllWithoutSyncResult()).forEach(organisationsEinheit -> syncAddedOrganisationsEinheit(syncTimestamp, organisationsEinheit));
+	}
+
+	Stream<OrganisationsEinheit> sortInAdditionOrder(Stream<OrganisationsEinheit> organisationsEinheiten) {
+		return organisationsEinheiten.sorted((org1, org2) -> {
+			if (org1.getParentId() == null && org2.getParentId() == null) {
+				return 0;
+			}
+			if (org1.getParentId() == null) {
+				return -1;
+			}
+			if (org2.getParentId() == null) {
+				return 1;
+			}
+			if (org1.getId().equals(org2.getParentId())) {
+				return -1;
+			}
+			if (org1.getParentId().equals(org2.getId())) {
+				return 1;
+			}
+			return 0;
+		});
 	}
 
 	void syncAddedOrganisationsEinheit(long syncTimestamp, OrganisationsEinheit organisationsEinheit) {

src/test/java/de/ozgcloud/admin/keycloak/GroupRepresentationTestFactory.java

@@ -13,6 +13,7 @@ class GroupRepresentationTestFactory {
 	public static final String ORGANIZATIONS_EINHEIT_ID_ATTRIBUTE = "organisationseinheitId";
 
 	public static final String ID = UUID.randomUUID().toString();
+	public static final String PARENT_ID = UUID.randomUUID().toString();
 	public static final String NAME = LoremIpsum.getInstance().getName();
 	public static final String ORGANISATIONS_EINHEIT_ID = UUID.randomUUID().toString();
 	public static final Map<String, List<String>> ATTRIBUTES = Map.of(ORGANIZATIONS_EINHEIT_ID_ATTRIBUTE, List.of(ORGANISATIONS_EINHEIT_ID));
@@ -43,6 +44,7 @@ class GroupRepresentationTestFactory {
 		if (withId) {
 			groupRepresentation.setId(ID);
 		}
+		groupRepresentation.setParentId(PARENT_ID);
 		groupRepresentation.setName(NAME);
 		groupRepresentation.setAttributes(ATTRIBUTES);
 		groupRepresentation.setSubGroups(List.of(createSubGroup(withId)));
@@ -54,6 +56,7 @@ class GroupRepresentationTestFactory {
 		if (withId) {
 			groupRepresentation.setId(SUB_GROUP_ID);
 		}
+		groupRepresentation.setParentId(ID);
 		groupRepresentation.setName(SUB_GROUP_NAME);
 		groupRepresentation.setAttributes(SUB_GROUP_ATTRIBUTES);
 		return groupRepresentation;

src/test/java/de/ozgcloud/admin/keycloak/KeycloakApiServiceITCase.java

@@ -8,6 +8,7 @@ import java.util.Optional;
 
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.keycloak.admin.client.resource.GroupsResource;
 import org.keycloak.representations.idm.GroupRepresentation;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.test.context.ContextConfiguration;
@@ -25,6 +26,8 @@ class KeycloakApiServiceITCase {
 	private KeycloakApiService service;
 	@Autowired
 	private KeycloakApiProperties properties;
+	@Autowired
+	private GroupsResource groupsResource;
 
 	@Nested
 	class TestGetAllGroups {
@@ -146,7 +149,7 @@ class KeycloakApiServiceITCase {
 		}
 
 		@Test
-		void shouldNotAddSubgroups() {
+		void shouldNotAddSubgroupsOfAddedGroup() {
 			var groupToAdd = createUniqueGroupRepresentation("shouldNotAddSubgroups");
 
 			var groupId = service.addGroup(groupToAdd);
@@ -155,9 +158,23 @@ class KeycloakApiServiceITCase {
 					.asList().isEmpty();
 		}
 
-		private GroupRepresentation createUniqueGroupRepresentation(String suffix) {
+		@Test
+		void shouldAddSubgroupToParent() {
+			var parentToAdd = createUniqueGroupRepresentation("shouldAddSubgroupToParent-parent");
+			var parentId = service.addGroup(parentToAdd);
+			var childToAdd = createUniqueGroupRepresentation("shouldAddSubgroupToParent-child");
+			childToAdd.setParentId(parentId);
+
+			var childId = service.addGroup(childToAdd);
+
+			var subgroupsInKc = groupsResource.group(parentId).getSubGroups(0, Integer.MAX_VALUE, true);
+			assertThat(subgroupsInKc).hasSize(1).first().extracting(GroupRepresentation::getId, GroupRepresentation::getParentId)
+					.contains(childId, parentId);
+		}
+
+		private GroupRepresentation createUniqueGroupRepresentation(String nameSuffix) {
 			// LoremIpsum does not guarantee unique results when called repeatedly
-			var groupName = "%s (%s)".formatted(LoremIpsum.getInstance().getName(), suffix);
+			var groupName = "%s (%s)".formatted(LoremIpsum.getInstance().getName(), nameSuffix);
 			return GroupRepresentationTestFactory.createWithoutId(groupName);
 		}
 

src/test/java/de/ozgcloud/admin/keycloak/KeycloakApiServiceTest.java

@@ -6,6 +6,7 @@ import static org.mockito.Mockito.*;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.keycloak.admin.client.resource.GroupResource;
 import org.keycloak.admin.client.resource.GroupsResource;
 import org.keycloak.representations.idm.GroupRepresentation;
 import org.mockito.InjectMocks;
@@ -34,15 +35,15 @@ class KeycloakApiServiceTest {
 
 		@BeforeEach
 		void init() {
-			when(groupsResource.add(groupRepresentation)).thenReturn(response);
+			doReturn(response).when(service).addParentOrChildGroup(groupRepresentation);
 			doReturn(RESOURCE_ID).when(service).getAddedResourceIdFromResponse(response);
 		}
 
 		@Test
-		void shouldCallGroupsResource() {
+		void shouldAddParentOrChildGroup() {
 			callService();
 
-			verify(groupsResource).add(groupRepresentation);
+			verify(service).addParentOrChildGroup(groupRepresentation);
 		}
 
 		@Test
@@ -64,6 +65,69 @@ class KeycloakApiServiceTest {
 		}
 	}
 
+	@Nested
+	class TestAddParentOrChildGroup {
+
+		private GroupRepresentation groupRepresentation;
+
+		@Nested
+		class OnParentGroup {
+
+			@BeforeEach
+			void init() {
+				groupRepresentation = GroupRepresentationTestFactory.create();
+				groupRepresentation.setParentId(null);
+				when(groupsResource.add(groupRepresentation)).thenReturn(response);
+			}
+
+			@Test
+			void shouldCallGroupsResource() {
+				callService();
+
+				verify(groupsResource).add(groupRepresentation);
+			}
+
+			@Test
+			void shouldReturnResponse() {
+				var serviceResponse = callService();
+
+				assertThat(serviceResponse).isEqualTo(response);
+			}
+		}
+
+		@Nested
+		class OnChildGroup {
+
+			@Mock
+			private GroupResource groupResource;
+
+			@BeforeEach
+			void init() {
+				groupRepresentation = GroupRepresentationTestFactory.create();
+				when(groupsResource.group(GroupRepresentationTestFactory.PARENT_ID)).thenReturn(groupResource);
+				when(groupResource.subGroup(groupRepresentation)).thenReturn(response);
+			}
+
+			@Test
+			void shouldCallGroupResource() {
+				callService();
+
+				verify(groupResource).subGroup(groupRepresentation);
+			}
+
+			@Test
+			void shouldReturnResponse() {
+				var serviceResponse = callService();
+
+				assertThat(serviceResponse).isEqualTo(response);
+			}
+		}
+
+		private Response callService() {
+			return service.addParentOrChildGroup(groupRepresentation);
+		}
+	}
+
 	@Nested
 	class TestGetAddedResourceIdFromResponse {
 

src/test/java/de/ozgcloud/admin/organisationseinheit/SyncServiceTest.java

@@ -5,6 +5,7 @@ import static org.mockito.ArgumentMatchers.*;
 import static org.mockito.Mockito.*;
 
 import java.time.Instant;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
@@ -416,15 +417,21 @@ class SyncServiceTest {
 	}
 
 	@Nested
-	class TestsyncAddedOrganisationsEinheiten {
+	class TestSyncAddedOrganisationsEinheiten {
 
 		private final long syncTimestamp = Instant.now().toEpochMilli();
 		private final OrganisationsEinheit withoutSyncResult1 = OrganisationsEinheitTestFactory.createBuilder().id("A").build();
 		private final OrganisationsEinheit withoutSyncResult2 = OrganisationsEinheitTestFactory.createBuilder().id("B").build();
+		private final OrganisationsEinheit[] unsortedOrganisationsEinheiten = new OrganisationsEinheit[]{withoutSyncResult2, withoutSyncResult1};
+		private final OrganisationsEinheit[] sortedOrganisationsEinheiten = new OrganisationsEinheit[]{withoutSyncResult1, withoutSyncResult2};
+
+		@Captor
+		private ArgumentCaptor<Stream<OrganisationsEinheit>> streamArgumentCaptor;
 
 		@BeforeEach
 		void setUp() {
-			when(repository.findAllWithoutSyncResult()).thenReturn(Stream.of(withoutSyncResult1, withoutSyncResult2));
+			when(repository.findAllWithoutSyncResult()).thenReturn(Stream.of(unsortedOrganisationsEinheiten));
+			doReturn(Stream.of(sortedOrganisationsEinheiten)).when(service).sortInAdditionOrder(any());
 			doNothing().when(service).syncAddedOrganisationsEinheit(anyLong(), any());
 		}
 
@@ -435,6 +442,14 @@ class SyncServiceTest {
 			verify(repository).findAllWithoutSyncResult();
 		}
 
+		@Test
+		void shouldSortInAdditionOrder() {
+			callService();
+
+			verify(service).sortInAdditionOrder(streamArgumentCaptor.capture());
+			assertThat(streamArgumentCaptor.getValue()).containsExactly(unsortedOrganisationsEinheiten);
+		}
+
 		@Test
 		void shouldSynchronizeFirstOrganisationsEinheit() {
 			callService();
@@ -449,11 +464,55 @@ class SyncServiceTest {
 			verify(service).syncAddedOrganisationsEinheit(syncTimestamp, withoutSyncResult2);
 		}
 
+		@Test
+		void shouldSynchronizeInOrder() {
+			var inOrder = inOrder(service);
+
+			callService();
+
+			Arrays.stream(sortedOrganisationsEinheiten).forEach(organisationsEinheit ->
+					inOrder.verify(service).syncAddedOrganisationsEinheit(syncTimestamp, organisationsEinheit));
+		}
+
 		private void callService() {
 			service.syncAddedOrganisationsEinheiten(syncTimestamp);
 		}
 	}
 
+	@Nested
+	class TestSortInAdditionOrder {
+
+		@Test
+		void shouldTopLevelOrgComeFirst() {
+			var orgWithParent = OrganisationsEinheitTestFactory.create();
+			var orgWithoutParent = OrganisationsEinheitTestFactory.createBuilder().parentId(null).build();
+
+			var sorted = service.sortInAdditionOrder(Stream.of(orgWithParent, orgWithoutParent));
+
+			assertThat(sorted).containsExactly(orgWithoutParent, orgWithParent);
+		}
+
+		@Test
+		void shouldParentComeBeforeChild() {
+			var parent = OrganisationsEinheitTestFactory.create();
+			var child = OrganisationsEinheitTestFactory.createBuilder().parentId(parent.getId()).build();
+
+			var sorted = service.sortInAdditionOrder(Stream.of(child, parent));
+
+			assertThat(sorted).containsExactly(parent, child);
+		}
+
+		@Test
+		void shouldPreserveOrderForTopLevelGroups() {
+			var orgWithoutParent1 = OrganisationsEinheitTestFactory.createBuilder().parentId(null).build();
+			var orgWithoutParent2 = OrganisationsEinheitTestFactory.createBuilder().parentId(null).build();
+
+			var sorted = service.sortInAdditionOrder(Stream.of(orgWithoutParent1, orgWithoutParent2));
+
+			assertThat(sorted).containsExactly(orgWithoutParent1, orgWithoutParent2);
+		}
+	}
+
 	@Nested
 	class TestSyncAddedOrganisationsEinheit {