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.

Sign up for GitHub

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

stock sender receiver validation changes #957

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

stock sender receiver validation changes #957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions health-services/stock/src/main/java/org/egov/stock/service/FacilityService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.egov.common.models.stock.SenderReceiverType;
import org.egov.common.models.stock.Stock;
import org.egov.common.models.stock.StockReconciliation;
import org.egov.common.models.stock.TransactionType;
import org.egov.stock.config.StockConfiguration;
import org.springframework.stereotype.Service;
import org.springframework.util.CollectionUtils;
Expand Down Expand Up @@ -96,10 +97,10 @@ public <T> Map<String, List<String>> validateProjectFacilityMappings(List<T> ent

Stock stock = (Stock) entity;

if (SenderReceiverType.WAREHOUSE.equals(stock.getSenderType())) {
facilityIds.add(stock.getSenderId());
}
if (SenderReceiverType.WAREHOUSE.equals(stock.getReceiverType())) {
if (SenderReceiverType.WAREHOUSE.equals(stock.getSenderType()) && TransactionType.DISPATCHED.equals(stock.getTransactionType())) {
facilityIds.add(stock.getSenderId());
}
if (SenderReceiverType.WAREHOUSE.equals(stock.getReceiverType()) && TransactionType.RECEIVED.equals(stock.getTransactionType())) {
Comment on lines +100 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

⚠️ Potential issue

Handle case when facilityIds is empty to prevent unnecessary service call

If facilityIds is empty, searchLimit will be zero, and the service request may return no results or might not behave as expected. This could lead to unnecessary service calls or unexpected responses.

Consider adding a check to handle the case when facilityIds is empty before proceeding with the service request to avoid unnecessary calls:

if (facilityIds.isEmpty()) {
    // Handle the empty case appropriately
    return Collections.emptyMap();
}

Would you like assistance in implementing this change or opening a GitHub issue to track this task?

facilityIds.add(stock.getReceiverId());
}
}
Expand Down
21 changes: 13 additions & 8 deletions health-services/stock/src/main/java/org/egov/stock/util/ValidatorUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.egov.common.models.stock.SenderReceiverType;
import org.egov.common.models.stock.Stock;
import org.egov.common.models.stock.StockReconciliation;
import org.egov.common.models.stock.TransactionType;
import org.egov.common.service.UserService;
import org.egov.stock.service.FacilityService;
import org.egov.tracer.model.CustomException;
Expand Down Expand Up @@ -140,7 +141,7 @@ private static void validateAndEnrichStaffIds(RequestInfo requestInfo, UserServi

/**
* Private method to enrich facility id and staff id
*
*
* @param validStockEntities
* @param facilityIds
* @param staffIds
Expand All @@ -166,9 +167,9 @@ private static void enrichFaciltyAndStaffIdsFromStock(List<Stock> validStockEnti
}

/**
*
*
* creates the error map from the stock objects with invalid party ids
*
*
* @param errorDetailsMap
* @param validStockEntities
* @param invalidStaffIds
Expand Down Expand Up @@ -205,7 +206,7 @@ private static <T> void enrichErrorMapFromInvalidPartyIds(Map<T, List<Error>> er

/**
* method to populate error details map
*
*
* @param <T>
* @param errorDetailsMap
* @param entity
Expand All @@ -218,7 +219,7 @@ private static <T> void getIdForErrorFromMethod(Map<T, List<Error>> errorDetails
}

/**
*
*
* @param <R>
* @param <T>
* @param request
Expand Down Expand Up @@ -269,13 +270,17 @@ private static <T> void enrichErrorForStock(List<Stock> validEntities,
String receiverId = stock.getReceiverId();

List<String> facilityIds = ProjectFacilityMappingOfIds.get(stock.getReferenceId());
if (!(SenderReceiverType.WAREHOUSE.equals(stock.getSenderType()) && TransactionType.DISPATCHED.equals(stock.getTransactionType()))
&& !(SenderReceiverType.WAREHOUSE.equals(stock.getReceiverType()) && TransactionType.RECEIVED.equals(stock.getTransactionType()))) {
continue;
}
Comment on lines +273 to +276
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM with suggestion: Transaction type checks added, but consider logic refinement.

The addition of transaction type checks in the enrichErrorForStock method provides more granular control over error population. However, the current implementation with the continue statement might skip valid checks for non-WAREHOUSE sender/receiver types.

Consider refactoring the logic to ensure all valid combinations are checked. For example:

if (SenderReceiverType.WAREHOUSE.equals(stock.getSenderType()) && TransactionType.DISPATCHED.equals(stock.getTransactionType())) {
    if (!facilityIds.contains(senderId)) {
        populateErrorForStock(stock, senderId, errorDetailsMap);
    }
} else if (SenderReceiverType.WAREHOUSE.equals(stock.getReceiverType()) && TransactionType.RECEIVED.equals(stock.getTransactionType())) {
    if (!facilityIds.contains(receiverId)) {
        populateErrorForStock(stock, receiverId, errorDetailsMap);
    }
}

This approach ensures that all relevant checks are performed without potentially skipping valid scenarios.

Also applies to: 279-283

if (!CollectionUtils.isEmpty(facilityIds)) {

if (SenderReceiverType.WAREHOUSE.equals(stock.getSenderType()) && !facilityIds.contains(senderId)) {
if (SenderReceiverType.WAREHOUSE.equals(stock.getSenderType()) && !facilityIds.contains(senderId) && TransactionType.DISPATCHED.equals(stock.getTransactionType())) {
populateErrorForStock(stock, senderId, errorDetailsMap);
}

if (SenderReceiverType.WAREHOUSE.equals(stock.getReceiverType()) && !facilityIds.contains(receiverId))
if (SenderReceiverType.WAREHOUSE.equals(stock.getReceiverType()) && !facilityIds.contains(receiverId)&& TransactionType.RECEIVED.equals(stock.getTransactionType()))
populateErrorForStock(stock, receiverId, errorDetailsMap);
} else {
populateErrorForStock(stock, senderId + " and " + receiverId, errorDetailsMap);
Expand Down Expand Up @@ -309,7 +314,7 @@ private static <T> void enrichErrorForStockReconciliation(List<StockReconciliati
populateErrorForStockReconciliation(stockReconciliation, errorDetailsMap);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider removing unnecessary blank line.

The added blank line doesn't significantly improve readability as it's between two method definitions that are already separated. Consider removing this line to maintain consistent spacing throughout the file.

@SuppressWarnings("unchecked")
private static <T> void populateErrorForStockReconciliation(StockReconciliation stockReconciliation,
Map<T, List<Error>> errorDetailsMap) {
Expand Down
Loading