fix(access-types): Fix updater not returned on get collection

NEWS.md

@@ -13,6 +13,7 @@
 ### Bug fixes
 * Fix offset handling when retrieving holdings from HoldingsIQ. ([MODKBEKBJ-825](https://folio-org.atlassian.net/browse/MODKBEKBJ-825))
 * Fix error messages format received from HoldingsIQ. ([MODKBEKBJ-826](https://folio-org.atlassian.net/browse/MODKBEKBJ-826))
+* Fix updater not returned on get access-types collection. ([MODKBEKBJ-842](https://folio-org.atlassian.net/browse/MODKBEKBJ-842))
 
 ### Tech Dept
 * Description ([ISSUE](https://folio-org.atlassian.net/browse/ISSUE))

src/main/java/org/folio/service/accesstypes/AccessTypesServiceImpl.java

@@ -239,7 +239,7 @@ private CompletableFuture<List<AccessType>> populateUserMetadata(Map<String, Str
                                                                    List<AccessType> accessTypes) {
     var usersIds = accessTypes.stream()
       .map(AccessType::getMetadata)
-      .map(metadata -> Arrays.asList(metadata.getCreatedByUserId(), metadata.getUpdatedByUsername()))
+      .map(metadata -> Arrays.asList(metadata.getCreatedByUserId(), metadata.getUpdatedByUserId()))
       .flatMap(List::stream)
       .filter(Objects::nonNull)
       .distinct()

src/test/java/org/folio/rest/impl/EholdingsAccessTypesImplIntegrationTest.java

@@ -1,5 +1,8 @@
 package org.folio.rest.impl;
 
+import static com.github.tomakehurst.wiremock.client.WireMock.containing;
+import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo;
 import static org.folio.HttpStatus.SC_BAD_REQUEST;
 import static org.folio.HttpStatus.SC_CREATED;
 import static org.folio.HttpStatus.SC_NOT_FOUND;
@@ -9,6 +12,7 @@
 import static org.folio.repository.accesstypes.AccessTypeMappingsTableConstants.ACCESS_TYPES_MAPPING_TABLE_NAME;
 import static org.folio.repository.accesstypes.AccessTypesTableConstants.ACCESS_TYPES_TABLE_NAME;
 import static org.folio.repository.kbcredentials.KbCredentialsTableConstants.KB_CREDENTIALS_TABLE_NAME;
+import static org.folio.service.users.UsersLookUpService.USERS_ENDPOINT;
 import static org.folio.util.AccessTypesTestUtil.ACCESS_TYPES_PATH;
 import static org.folio.util.AccessTypesTestUtil.KB_CREDENTIALS_ACCESS_TYPES_ENDPOINT;
 import static org.folio.util.AccessTypesTestUtil.KB_CREDENTIALS_ACCESS_TYPE_ID_ENDPOINT;
@@ -71,23 +75,26 @@ void tearDown() {
 
   @Test
   void shouldReturnAccessTypeCollectionOnGet() {
-    List<AccessType> testAccessTypes = testData(credentialsId);
-    final String id0 = insertAccessType(testAccessTypes.get(0), vertx);
-    final String id1 = insertAccessType(testAccessTypes.get(1), vertx);
+    var testAccessTypes = testData(credentialsId);
+    // First access type is created by John and last updated by Jane (both served by the mocked /users lookup).
+    final var id0 = insertAccessType(testAccessTypes.get(0), JOHN_ID, JANE_ID, vertx);
+    final var id1 = insertAccessType(testAccessTypes.get(1), vertx);
 
-    AccessTypeCollection actual = getWithStatus(ACCESS_TYPES_PATH, SC_OK)
-      .as(AccessTypeCollection.class);
+    var actual = getWithStatus(ACCESS_TYPES_PATH, SC_OK).as(AccessTypeCollection.class);
 
     assertEquals(Integer.valueOf(2), actual.getMeta().getTotalResults());
     assertEquals(2, actual.getData().size());
-    assertThat(actual.getData().get(0), allOf(
-      hasProperty("id", equalTo(id0)),
-      allOf(hasProperty("attributes", notNullValue()), hasProperty("metadata", notNullValue()))
-    ));
-    assertThat(actual.getData().get(1), allOf(
-      hasProperty("id", equalTo(id1)),
-      allOf(hasProperty("attributes", notNullValue()), hasProperty("metadata", notNullValue()))
-    ));
+    var first = actual.getData().get(0);
+    assertThat(first, allOf(hasProperty("id", equalTo(id0)),
+      hasProperty("attributes", notNullValue()), hasProperty("metadata", notNullValue())));
+    assertThat(actual.getData().get(1), allOf(hasProperty("id", equalTo(id1)),
+      hasProperty("attributes", notNullValue()), hasProperty("metadata", notNullValue())));
+    assertEquals("John", first.getCreator().getFirstName());
+    assertEquals("Doe", first.getCreator().getLastName());
+    assertEquals("firstname_test", first.getUpdater().getFirstName());
+    assertEquals("lastname_test", first.getUpdater().getLastName());
+    // Regression guard: the updater's id (updatedByUserId) must be part of the /users query.
+    wm.verify(getRequestedFor(urlPathEqualTo(USERS_ENDPOINT)).withQueryParam("query", containing(JANE_ID)));
   }
 
   @Test

src/test/java/org/folio/service/accesstypes/AccessTypesServiceImplTest.java

@@ -0,0 +1,97 @@
+package org.folio.service.accesstypes;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.folio.util.TestUtil.result;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyList;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.commons.collections4.map.CaseInsensitiveMap;
+import org.folio.holdingsiq.model.RequestContext;
+import org.folio.okapi.common.XOkapiHeaders;
+import org.folio.repository.accesstypes.AccessTypesRepository;
+import org.folio.repository.accesstypes.DbAccessType;
+import org.folio.rest.jaxrs.model.AccessType;
+import org.folio.rest.jaxrs.model.AccessTypeCollection;
+import org.folio.rest.jaxrs.model.Metadata;
+import org.folio.service.users.User;
+import org.folio.service.users.UsersLookUpService;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import org.springframework.core.convert.converter.Converter;
+
+@MockitoSettings(strictness = Strictness.WARN)
+@ExtendWith(MockitoExtension.class)
+class AccessTypesServiceImplTest {
+
+  private static final String TENANT_ID = "test";
+  private static final Map<String, String> HEADERS = new CaseInsensitiveMap<>(Map.of(
+    XOkapiHeaders.TENANT, TENANT_ID,
+    XOkapiHeaders.URL, "http://test.url"
+  ));
+
+  @InjectMocks
+  private AccessTypesServiceImpl accessTypesService;
+
+  @Mock
+  private UsersLookUpService usersLookUpService;
+  @Mock
+  private AccessTypesRepository repository;
+  @Mock
+  private Converter<List<AccessType>, AccessTypeCollection> accessTypeCollectionConverter;
+  @Mock
+  private Converter<DbAccessType, AccessType> accessTypeFromDbConverter;
+
+  @Test
+  void findByCredentialsIdShouldPopulateUpdaterFromUpdatedByUserId() {
+    var credentialsId = UUID.randomUUID();
+    var creatorId = UUID.randomUUID();
+    var updaterId = UUID.randomUUID();
+
+    // Access type created by one user and last updated by a different user.
+    var accessType = new AccessType().withMetadata(new Metadata()
+      .withCreatedByUserId(creatorId.toString())
+      .withUpdatedByUserId(updaterId.toString()));
+    var dbAccessType = DbAccessType.builder().id(UUID.randomUUID()).credentialsId(credentialsId).build();
+    when(repository.findByCredentialsId(credentialsId, TENANT_ID))
+      .thenReturn(completedFuture(List.of(dbAccessType)));
+    when(accessTypeFromDbConverter.convert(dbAccessType)).thenReturn(accessType);
+    when(usersLookUpService.lookUpUsers(anyList(), any(RequestContext.class)))
+      .thenReturn(completedFuture(List.of(user(creatorId, "creator"), user(updaterId, "updater"))));
+    when(accessTypeCollectionConverter.convert(anyList())).thenReturn(new AccessTypeCollection());
+
+    result(accessTypesService.findByCredentialsId(credentialsId.toString(), HEADERS));
+
+    // The updater's id must be part of the users lookup - regression guard for the
+    // updatedByUsername/updatedByUserId mix-up that left updater unresolved.
+    var idsCaptor = ArgumentCaptor.forClass(List.class);
+    verify(usersLookUpService).lookUpUsers(idsCaptor.capture(), any(RequestContext.class));
+    assertThat(idsCaptor.getValue()).contains(creatorId, updaterId);
+    // Enrichment mutates the access types in place before they are converted to the collection.
+    assertThat(accessType.getUpdater()).isNotNull()
+      .extracting("firstName", "lastName").containsExactly("updater_firstname", "updater_lastname");
+    assertThat(accessType.getMetadata().getUpdatedByUsername()).isEqualTo("updater");
+    assertThat(accessType.getCreator()).isNotNull()
+      .extracting("firstName").isEqualTo("creator_firstname");
+  }
+
+  private User user(UUID id, String prefix) {
+    return User.builder()
+      .id(id.toString())
+      .userName(prefix)
+      .firstName(prefix + "_firstname")
+      .lastName(prefix + "_lastname")
+      .build();
+  }
+}

src/test/java/org/folio/util/AccessTypesTestUtil.java

@@ -80,6 +80,11 @@ public static List<AccessType> insertAccessTypes(List<AccessType> items, Vertx v
   }
 
   public static String insertAccessType(AccessType accessType, Vertx vertx) {
+    return insertAccessType(accessType, JOHN_ID, null, vertx);
+  }
+
+  public static String insertAccessType(AccessType accessType, String createdByUserId, String updatedByUserId,
+                                        Vertx vertx) {
     CompletableFuture<RowSet<Row>> future = new CompletableFuture<>();
 
     String query = prepareQuery(upsertAccessTypeQuery(), accessTypesTestTable());
@@ -91,9 +96,9 @@ public static String insertAccessType(AccessType accessType, Vertx vertx) {
       accessType.getAttributes().getName(),
       accessType.getAttributes().getDescription(),
       OffsetDateTime.now(),
-      toUUID(JOHN_ID),
-      null,
-      null
+      toUUID(createdByUserId),
+      updatedByUserId == null ? null : OffsetDateTime.now(),
+      toUUID(updatedByUserId)
     );
 
     PostgresClient.getInstance(vertx).execute(query, params, event -> future.complete(null));