[MODORDERS-1114] Add transferRequests flag to validate binding #936
Conversation
Saba-Zedginidze-EPAM
force-pushed
the
MODORDERS-1114
branch
2 times, most recently
from
35b352a
to
39d8e77
Compare
Saba-Zedginidze-EPAM
force-pushed
the
MODORDERS-1114
branch
from
1f7db2a
to
59457a3
Compare
| Map<String, Piece> processedPiecesForPoLine = StreamEx | ||
| .of(piecesGroupedByPoLine.getOrDefault(poLineId, Collections.emptyList())) | ||
| .toMap(Piece::getId, piece -> piece); | ||
| Map<String, Piece> processedPiecesForPoLine = getProcessedPiecesForPoLine(poLineId, piecesGroupedByPoLine); |
There was a problem hiding this comment.
some unused imports are introduced, you can find them in sonar report:
https://sonarcloud.io/project/issues?id=org.folio%3Amod-orders&pullRequest=936&resolved=false&sinceLeakPeriod=true
| .toMap(Piece::getId, piece -> piece); | ||
| } | ||
|
|
||
| protected Map<String, Integer> getEmptyResultCounts() { |
There was a problem hiding this comment.
Maybe (if map is not modified later)
return Map.of(
ProcessingStatus.Type.SUCCESS.toString(), 0,
ProcessingStatus.Type.FAILURE.toString(), 0
);
There was a problem hiding this comment.
The values are updated, that's why didn't use .of()
There was a problem hiding this comment.
Actually map is modified later. Why we have Map<String, int> instead of Map<ProcessingStatus.Type, int>?
There was a problem hiding this comment.
That's a good point. I just refactored some irrelevant code and didn't go much into details. Will modify
| @@ -783,6 +784,30 @@ protected void addErrorForUpdatingItem(Piece piece, Throwable e) { | |||
| } | |||
| } | |||
|
|
|||
| protected Map<String, Piece> getProcessedPiecesForPoLine(String poLineId, Map<String, List<Piece>> piecesGroupedByPoLine) { | |||
There was a problem hiding this comment.
This group of 4 methods are misplaced in "Errors" section. Please check comments after dashes in this class:
Actually this class is already big and hard to manage. We can easily move new methods to corresponding classes but maybe this type of refactoring increases the scope a bit.
There was a problem hiding this comment.
Will move above the errors, thanks. I guess yes, the class is pretty big and can be refactored, unsure if in scope of this.
| logger.warn("checkRequestsForPieceItems:: Found outstanding requests on items with ids: {}", items); | ||
| throw new HttpException(RestConstants.VALIDATION_ERROR, ErrorCodes.REQUESTS_ACTION_REQUIRED); | ||
| } | ||
| else if (bindPiecesCollection.getRequestsAction().equals(TRANSFER)) { |
There was a problem hiding this comment.
Why do you need else if if you throw an error otherwise?
It's better to handle edge case first:
if (items.isEmpty()) {
return Future.succeededFuture();
}
and then continue with general case.
I'd write:
if (!items.isEmpty() && Objects.isNull(bindPiecesCollection.getRequestsAction())) {
logger.warn("checkRequestsForPieceItems:: Found outstanding requests on items with ids: {}", items);
throw new HttpException(RestConstants.VALIDATION_ERROR, ErrorCodes.REQUESTS_ACTION_REQUIRED);
}
if (!items.isEmpty() && bindPiecesCollection.getRequestsAction().equals(TRANSFER) && !tenantToItem.keySet().isEmpty()) {
logger.warn("checkRequestsForPieceItems:: All pieces do not have the same receivingTenantId: {}", tenantToItem.keySet());
throw new HttpException(RestConstants.VALIDATION_ERROR, ErrorCodes.PIECES_HAVE_DIFFERENT_RECEIVING_TENANT_IDS);
}
return Future.succeededFuture();
});
This way I see a 2 cases where you throw an error but it's up to you. Main thing is to not keep many lines of code indented as this increases a complexity.
There was a problem hiding this comment.
Correct, better to get rid of nested conditions.
| return StreamEx.ofValues(piecesGroupedByPoLine).flatMap(List::stream); | ||
| } | ||
|
|
||
| protected Map<String, List<String>> mapTenantIdsToItemIds(Map<String, List<Piece>> piecesGroupedByPoLine, RequestContext requestContext) { |
There was a problem hiding this comment.
This is much like to what I did in getItemRecrods: lines 544-551. Maybe that place can reuse this?
|
Purpose
[MODORDERS-1114] Enhance bind-pieces endpoint with flag to transfer circulation requests
Approach