Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion descriptors/ModuleDescriptor-template.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the same for item

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need user-tenants.item.get?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@imerabishvili imerabishvili Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"search.instances.collection.get",
"inventory-storage.items.item.get",
"inventory-storage.service-points.item.get"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the same permissions for collections

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

]
}
]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package org.folio.circulationbff.service;

import org.folio.circulationbff.domain.dto.Item;
import org.folio.circulationbff.domain.dto.ServicePoint;

public interface InventoryService {
Item fetchItem(String id);
ServicePoint fetchServicePoint(String id);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import java.util.Collection;

import org.folio.circulationbff.domain.dto.BffSearchInstance;
import org.folio.circulationbff.domain.dto.SearchInstance;

public interface SearchService {
SearchInstance findInstanceByItemId(String itemId);
Collection<BffSearchInstance> findInstances(String query);
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.folio.circulationbff.service;

public interface UserTenantsService {
String getCurrentTenant();
String getCentralTenant();
boolean isCentralTenant();
boolean isCentralTenant(String tenantId);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,26 +1,76 @@
package org.folio.circulationbff.service.impl;

import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import org.folio.circulationbff.client.feign.CheckInClient;
import org.folio.circulationbff.domain.dto.CheckInRequest;
import org.folio.circulationbff.domain.dto.CheckInResponse;
import org.folio.circulationbff.domain.dto.SearchItem;
import org.folio.circulationbff.service.CheckInService;
import org.folio.circulationbff.service.InventoryService;
import org.folio.circulationbff.service.SearchService;
import org.folio.circulationbff.service.UserTenantsService;
import org.folio.spring.service.SystemUserScopedExecutionService;
import org.springframework.stereotype.Service;

import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import java.util.Objects;

@Service
@RequiredArgsConstructor
@Log4j2
public class CheckInServiceImpl implements CheckInService {

private final CheckInClient checkInClient;
private final SearchService searchService;
private final UserTenantsService userTenantsService;
private final InventoryService inventoryService;
private final SystemUserScopedExecutionService executionService;

@Override
public CheckInResponse checkIn(CheckInRequest request) {
log.info("checkIn: checking in item with barcode {} on service point {}",
request::getItemBarcode, request::getServicePointId);
return checkInClient.checkIn(request);
request.getItemBarcode(), request.getServicePointId());
var response = checkInClient.checkIn(request);
var item = response.getItem();
var servicePointName = getEffectiveLocationServicePoint(item.getId());
if (servicePointName != null) {
var slipContextItem = response.getStaffSlipContext().getItem();
slipContextItem.setEffectiveLocationPrimaryServicePointName(servicePointName);
slipContextItem.toServicePoint(servicePointName);
}
return response;
}

private String getEffectiveLocationServicePoint(String itemId) {
log.info("getEffectiveLocationServicePoint: itemId {}", itemId);
var instance = searchService.findInstanceByItemId(itemId);
if (instance == null) {
log.info("getEffectiveLocationServicePoint: instance not found");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return null;
}
var itemTenantId = instance.getItems()
.stream()
.filter(item -> item.getId().equals(itemId))
.findFirst()
.map(SearchItem::getTenantId)
.orElse(null);
if (Objects.equals(itemTenantId, userTenantsService.getCurrentTenant())) {
log.info("getEffectiveLocationServicePoint: same tenant case {}", itemTenantId);
var item = inventoryService.fetchItem(itemId);
var servicePoint = inventoryService.fetchServicePoint(item.getEffectiveLocationId());
return servicePoint.getName();
} else {
log.info("getEffectiveLocationServicePoint: cross tenant case {}", itemTenantId);
var item = executionService.executeSystemUserScoped(
itemTenantId,
() -> inventoryService.fetchItem(itemId)
);
var servicePoint = executionService.executeSystemUserScoped(
itemTenantId,
() -> inventoryService.fetchServicePoint(item.getEffectiveLocationId())
);
return servicePoint.getName();
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.folio.circulationbff.service.impl;

import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import org.folio.circulationbff.client.feign.ItemStorageClient;
import org.folio.circulationbff.client.feign.ServicePointClient;
import org.folio.circulationbff.domain.dto.Item;
import org.folio.circulationbff.domain.dto.ServicePoint;
import org.folio.circulationbff.service.InventoryService;
import org.springframework.stereotype.Service;

@Service
@RequiredArgsConstructor
@Log4j2
public class InventoryServiceImpl implements InventoryService {
private final ItemStorageClient itemClient;
private final ServicePointClient servicePointClient;

@Override
public Item fetchItem(String id) {
log.info("fetchItem:: fetching item {}", id);
return itemClient.findItem(id);
}

@Override
public ServicePoint fetchServicePoint(String id) {
log.info("fetchServicePoint:: fetching service point {}", id);
return servicePointClient.findServicePoint(id);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.util.function.Function;
import java.util.stream.Collectors;

import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
import org.folio.circulationbff.client.feign.HoldingsStorageClient;
import org.folio.circulationbff.client.feign.InstanceStorageClient;
Expand Down Expand Up @@ -71,6 +72,17 @@ public class SearchServiceImpl implements SearchService {
private final BulkFetchingService fetchingService;
private final SearchInstanceMapper searchInstanceMapper;

@Override
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

if (CollectionUtils.isEmpty(searchResult.getInstances())) {
return null;
}
return searchResult.getInstances().get(0);
}

@Override
public Collection<BffSearchInstance> findInstances(String query) {
log.info("findInstances:: searching instances by query: {}", query);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ public class UserTenantsServiceImpl implements UserTenantsService {

private final UserTenantsClient userTenantsClient;

@Override
public String getCurrentTenant() {
UserTenant firstUserTenant = getFirstUserTenant();
if (firstUserTenant == null) {
log.info("getCurrentTenant:: failed to fetch user tenants");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@roman-barannyk roman-barannyk Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed duplicate logging in the class

return null;
}
String currentTenantId = firstUserTenant.getTenantId();
log.info("getCurrentTenant:: currentTenantId={}", currentTenantId);
return currentTenantId;
}

@Override
public String getCentralTenant() {
UserTenant firstUserTenant = getFirstUserTenant();
Expand Down
134 changes: 130 additions & 4 deletions src/main/resources/swagger.api/schemas/response/check-in-response.yaml
Original file line number Diff line number Diff line change
@@ -1,8 +1,134 @@
type: object
description: Check-in response
properties:
toServicePoint:
description: Name of the service point the item should be routed to
type: string
item:
description: Item data
type: object
properties:
id:
description: Item ID
type: string
instanceId:
description: Related Instance Id
type: string
holdingsRecordId:
description: Related holding record id
type: string
title:
description: Related title
type: string
barcode:
description: Barcode of the item
type: string
enumeration:
description: Enumeration of the item
type: string
volume:
description: Volume of the item
type: string
chronology:
description: Chronology of the item
type: string
displaySummary:
description: Display summary of the item
type: string
copyNumber:
description: Copy number of the item
type: string
callNumber:
description: Call number of the item
type: string
additionalProperties: true
staffSlipContext:
description: Staff slips data
type: object
properties:
item:
description: Staff slips item data
type: object
properties:
title:
description: Title of the instance record
type: string
primaryContributor:
description: Primary contributor name from the instance record
type: string
allContributors:
description: List of contributor names from the instance record concatenated with semicolon
type: string
barcode:
description: Barcode of the item
type: string
status:
description: Status of the item
type: string
enumeration:
description: Enumeration of the item
type: string
volume:
description: Volume of the item
type: string
chronology:
description: Chronology of the item
type: string
yearCaption:
description: Year caption of the item
type: string
materialType:
description: Material type of the item
type: string
loanType:
description: Loan type of the item
type: string
copy:
description: Copy number of the item
type: string
numberOfPieces:
description: Number of item pieces
type: string
displaySummary:
description: Display summary of the item
type: string
descriptionOfPieces:
description: Description of item pieces
type: string
effectiveLocationSpecific:
description: Name of the effective location
type: string
effectiveLocationLibrary:
description: Library name of the effective location
type: string
effectiveLocationCampus:
description: Campus name of the effective location
type: string
effectiveLocationInstitution:
description: Institution name of the effective location
type: string
effectiveLocationDiscoveryDisplayName:
description: Discovery display name of the effective location
type: string
effectiveLocationPrimaryServicePointName:
description: Primary service point name of the effective location
type: string
callNumber:
description: Call number of the item
type: string
callNumberPrefix:
description: Prefix of the item's call number
type: string
callNumberSuffix:
description: Suffix of the item's call number
type: string
lastCheckedInDateTime:
description: Last checked in date of the item
type: string
format: date-time
toServicePoint:
description: Destination service point of the item
type: string
fromServicePoint:
description: Last checked in service point of the item
type: string
additionalProperties: true

additionalProperties: true
additionalProperties: true
Loading
Loading