MCBFF-38: Update hardcoded effective location during checkin #47

imerabishvili · 2025-01-22T17:54:12Z

Purpose

https://folio-org.atlassian.net/browse/MCBFF-38

Approach

Obtain proper effective location based on item's effectiveLocationId and tenant and update required fields in response.

TODOS and Open Questions

Learning

roman-barannyk · 2025-01-24T12:39:50Z

src/test/java/org/folio/circulationbff/api/CheckInApiTest.java

+    var request = new CheckInRequest()
+      .itemBarcode("test_barcode")
+      .checkInDate(new Date())
+      .servicePointId(randomUUID());
+
+    givenCirculationCheckinSucceed(request);
+
+    var checkinItem = new Item()
+      .id("itemId")
+      .copyNumber("copyNumber")
+      .effectiveLocationId( "effectiveLocationId");
+
+    givenSearchInstanceReturnsItem(TENANT_ID_COLLEGE, checkinItem);
+
+    givenCurrentTenantIsConsortium();


Suggested change

var request = new CheckInRequest()

.itemBarcode("test_barcode")

.checkInDate(new Date())

.servicePointId(randomUUID());

givenCirculationCheckinSucceed(request);

var checkinItem = new Item()

.id("itemId")

.copyNumber("copyNumber")

.effectiveLocationId( "effectiveLocationId");

givenSearchInstanceReturnsItem(TENANT_ID_COLLEGE, checkinItem);

givenCurrentTenantIsConsortium();

var request = new CheckInRequest()

.itemBarcode("test_barcode")

.checkInDate(new Date())

.servicePointId(randomUUID());

givenCirculationCheckinSucceed(request);

var checkinItem = new Item()

.id("itemId")

.copyNumber("copyNumber")

.effectiveLocationId( "effectiveLocationId");

givenSearchInstanceReturnsItem(TENANT_ID_COLLEGE, checkinItem);

givenCurrentTenantIsConsortium();

Aren't empty lines OK? I see them in other places and in my opinion they are improving readability.

They enhance readability by splitting certain logic blocks of code, but I don't think it’s necessary to place them after every line...

roman-barannyk · 2025-01-24T12:41:06Z

src/test/java/org/folio/circulationbff/api/CheckInApiTest.java

+    var request = new CheckInRequest()
+      .itemBarcode("test_barcode")
+      .checkInDate(new Date())
+      .servicePointId(randomUUID());
+
+    givenCirculationCheckinSucceed(request);
+
+    var searchResponse = new SearchInstances().instances(List.of());
+    wireMockServer.stubFor(WireMock.get(urlMatching("/search/instances.*"))
+      .willReturn(jsonResponse(searchResponse, SC_OK)));
+


Suggested change

var request = new CheckInRequest()

.itemBarcode("test_barcode")

.checkInDate(new Date())

.servicePointId(randomUUID());

givenCirculationCheckinSucceed(request);

var searchResponse = new SearchInstances().instances(List.of());

wireMockServer.stubFor(WireMock.get(urlMatching("/search/instances.*"))

.willReturn(jsonResponse(searchResponse, SC_OK)));

var request = new CheckInRequest()

.itemBarcode("test_barcode")

.checkInDate(new Date())

.servicePointId(randomUUID());

givenCirculationCheckinSucceed(request);

var searchResponse = new SearchInstances().instances(List.of());

wireMockServer.stubFor(WireMock.get(urlMatching("/search/instances.*"))

.willReturn(jsonResponse(searchResponse, SC_OK)));

roman-barannyk · 2025-01-24T12:42:22Z

src/main/java/org/folio/circulationbff/service/impl/CheckInServiceImpl.java

+    log.info("getEffectiveLocationServicePoint: itemId {}", itemId);
+    var instance = searchService.findInstanceByItemId(itemId);
+    if (instance == null) {
+      log.info("getEffectiveLocationServicePoint: instance not found");


Suggested change

log.info("getEffectiveLocationServicePoint: instance not found");

log.warn("getEffectiveLocationServicePoint: instance not found");

roman-barannyk · 2025-01-24T12:45:13Z

src/main/java/org/folio/circulationbff/service/impl/SearchServiceImpl.java

+  public SearchInstance findInstanceByItemId(String itemId) {
+    log.info("findInstanceByItemId:: itemId {}", itemId);
+    String query = String.format("items.id==%s&expandAll=true", itemId);
+    SearchInstances searchResult = searchClient.findInstances(query, true);


Suggested change

SearchInstances searchResult = searchClient.findInstances(query, true);

SearchInstances searchInstances = searchClient.findInstances(query, true);

roman-barannyk · 2025-01-24T13:00:09Z

descriptors/ModuleDescriptor-template.json

@@ -150,7 +150,11 @@
            "circulation-bff.loans.check-in-by-barcode.execute"
          ],
          "modulePermissions": [
-            "circulation.check-in-by-barcode.post"
+            "circulation.check-in-by-barcode.post",
+            "user-tenants.collection.get",


please add the same for item

Why we need user-tenants.item.get?

There's a rule that if we add a permission for an item, we need to add a similar one for the collection, and vice versa

Could not find user-tenants.item.get anywhere in folio-org. Does it exists?

you are right, it exists only for consortia.user-tenants.item.get, so ignore it then

roman-barannyk · 2025-01-24T13:00:26Z

descriptors/ModuleDescriptor-template.json

+            "inventory-storage.items.item.get",
+            "inventory-storage.service-points.item.get"


please add the same permissions for collections

roman-barannyk · 2025-01-24T13:02:15Z

src/main/java/org/folio/circulationbff/service/impl/UserTenantsServiceImpl.java

+  public String getCurrentTenant() {
+    UserTenant firstUserTenant = getFirstUserTenant();
+    if (firstUserTenant == null) {
+      log.info("getCurrentTenant:: failed to fetch user tenants");


Suggested change

log.info("getCurrentTenant:: failed to fetch user tenants");

log.warn("getCurrentTenant:: failed to fetch user tenants");

Good point. But then we need to change this for similar lines in this file right?

It appears that the absence of the first user-tenant is unexpected behavior, and should be logged with warning level. This approach should be applied to all similar cases

fixed duplicate logging in the class

…ging

roman-barannyk · 2025-01-24T15:01:03Z

src/test/java/org/folio/circulationbff/service/SearchServiceTest.java

+    when(searchClient.findInstances(query, true))
+      .thenReturn(mockSearchResponse);


Suggested change

when(searchClient.findInstances(query, true))

.thenReturn(mockSearchResponse);

when(searchClient.findInstances(query, true)).thenReturn(mockSearchResponse);

roman-barannyk · 2025-01-24T15:02:01Z

src/test/java/org/folio/circulationbff/service/SearchServiceTest.java

+    SearchInstance response = searchService.findInstanceByItemId(itemId);
+    assertEquals(response, instance);


Suggested change

SearchInstance response = searchService.findInstanceByItemId(itemId);

assertEquals(response, instance);

assertEquals(searchService.findInstanceByItemId(itemId), instance);

sonarqubecloud · 2025-01-24T15:23:33Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

MCBFF-38: Update hardcoded effective location during checkin

131db36

imerabishvili force-pushed the MCBFF-38 branch from f662700 to 131db36 Compare January 23, 2025 11:58

roman-barannyk reviewed Jan 24, 2025

View reviewed changes

MCBFF-38: fix search query

ae4a787

imerabishvili force-pushed the MCBFF-38 branch from d6e1f53 to ae4a787 Compare January 24, 2025 13:13

MCBFF-38: removed empty lines, added permissions, fixed duplicate log…

c70dcf4

…ging

roman-barannyk reviewed Jan 24, 2025

View reviewed changes

MCBFF-38: given-when-then comments removed

0e725cd

imerabishvili and others added 2 commits January 24, 2025 19:33

MCBFF-38: try to fix allowed SPs

a3daf0d

MCBFF-38 Refactor allowed SP logic

0d9bd39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MCBFF-38: Update hardcoded effective location during checkin #47

MCBFF-38: Update hardcoded effective location during checkin #47

imerabishvili commented Jan 22, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025 •

edited

Loading

roman-barannyk Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025 •

edited

Loading

imerabishvili Jan 24, 2025

roman-barannyk Jan 24, 2025

roman-barannyk Jan 24, 2025

sonarqubecloud bot commented Jan 24, 2025

	log.info("getEffectiveLocationServicePoint: instance not found");
	log.warn("getEffectiveLocationServicePoint: instance not found");

	SearchInstances searchResult = searchClient.findInstances(query, true);
	SearchInstances searchInstances = searchClient.findInstances(query, true);

		"inventory-storage.items.item.get",
		"inventory-storage.service-points.item.get"

	log.info("getCurrentTenant:: failed to fetch user tenants");
	log.warn("getCurrentTenant:: failed to fetch user tenants");

		when(searchClient.findInstances(query, true))
		.thenReturn(mockSearchResponse);

		SearchInstance response = searchService.findInstanceByItemId(itemId);
		assertEquals(response, instance);

	SearchInstance response = searchService.findInstanceByItemId(itemId);
	assertEquals(response, instance);
	assertEquals(searchService.findInstanceByItemId(itemId), instance);

MCBFF-38: Update hardcoded effective location during checkin #47

Are you sure you want to change the base?

MCBFF-38: Update hardcoded effective location during checkin #47

Conversation

imerabishvili commented Jan 22, 2025

Purpose

Approach

TODOS and Open Questions

Learning

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imerabishvili Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-barannyk Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 24, 2025

Quality Gate passed

imerabishvili Jan 24, 2025 •

edited

Loading

roman-barannyk Jan 24, 2025 •

edited

Loading