Feat/error handling on material refresh #995
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
.../src/main/java/org/eclipse/tractusx/puris/backend/masterdata/domain/model/RefreshResult.java
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Looks great overall, thanks! Found a few messages to improve consistency.
Further I would refactor to harmonize validations with non-reported entities.
One further question: How have you tested this? I wasn't able to produce an error visible to the frontend but found one in the logs.
I tried manipulated the customer backends db to have a negative demand and then refreshed as a supplier:
docker exec -it postgres-all /bin/sh
psql -d customer-puris -U <see environement for PG_USER>
select * from own_demand
demand_category_code | measurement_unit | quantity | day | partner_uuid | uuid | demand_location_bpns | material_own_material_number | supplier_location_bpns | last_updated_on_date_time
----------------------+------------------+----------+---------------------+--------------------------------------+--------------------------------------+----------------------+------------------------------+------------------------+---------------------------
0 | 0 | 500 | 2025-10-06 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 8e7b9212-46f8-4d56-99bf-911f10dbbdeb | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.253
1 | 0 | 510 | 2025-10-07 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 15a21a49-1a37-4cbb-9c73-4ca306c94fcf | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.601
2 | 0 | 500 | 2025-10-08 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | e71bd1d2-4deb-4031-9ce6-2e3a1e9ba348 | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.733
2 | 0 | 500 | 2025-10-09 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 0f0511ed-8c7e-43d9-8d5f-577b8afa4ada | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.9
2 | 0 | 400 | 2025-10-10 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 2be2e08b-9f00-47a9-bd2a-b2bcc40e858e | BPNS4444444444XX | MNR-7307-AU340474.002 | | 2025-10-06 17:07:39.037
1 | 0 | 810 | 2025-10-17 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 0fd76321-9f5f-432e-8092-4a83b120ce65 | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:42.321
**0 | 0 | 0 | 2025-10-07 22:00:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | ded65399-796e-466f-925f-64088c4208c3 | BPNS4444444444XX | MNR-7307-AU340474.002 | | 2025-10-06 17:24:09.274**
UPDATE own_demand SET quantity = -1 where uuid = 'ded65399-796e-466f-925f-64088c4208c3';
UPDATE 1
puris_customer= commit;
select * from own_demand;
demand_category_code | measurement_unit | quantity | day | partner_uuid | uuid | demand_location_bpns | material_own_material_number | supplier_location_bpns | last_updated_on_date_time
----------------------+------------------+----------+---------------------+--------------------------------------+--------------------------------------+----------------------+------------------------------+------------------------+---------------------------
0 | 0 | 500 | 2025-10-06 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 8e7b9212-46f8-4d56-99bf-911f10dbbdeb | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.253
1 | 0 | 510 | 2025-10-07 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 15a21a49-1a37-4cbb-9c73-4ca306c94fcf | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.601
2 | 0 | 500 | 2025-10-08 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | e71bd1d2-4deb-4031-9ce6-2e3a1e9ba348 | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.733
2 | 0 | 500 | 2025-10-09 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 0f0511ed-8c7e-43d9-8d5f-577b8afa4ada | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:38.9
2 | 0 | 400 | 2025-10-10 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 2be2e08b-9f00-47a9-bd2a-b2bcc40e858e | BPNS4444444444XX | MNR-7307-AU340474.002 | | 2025-10-06 17:07:39.037
1 | 0 | 810 | 2025-10-17 17:07:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | 0fd76321-9f5f-432e-8092-4a83b120ce65 | BPNS4444444444XX | MNR-7307-AU340474.002 | BPNS1234567890ZZ | 2025-10-06 17:07:42.321
**0 | 0 | -1 | 2025-10-07 22:00:00 | b5a7a28c-8d37-4a78-a0b7-9b8746f2ba70 | ded65399-796e-466f-925f-64088c4208c3 | BPNS4444444444XX | MNR-7307-AU340474.002 | | 2025-10-06 17:24:09.274**
Frontend renders fine on both sides. Supplier side has no demands at all.
| ### Added | ||
|
|
||
| - / | ||
| - Added improved error messages for refresh materials ([#995](https://github.com/eclipse-tractusx/puris/pull/995)) |
There was a problem hiding this comment.
Please update to latest changelog version (changed meanwhile) and consider this a major version.
Please also update:
- frontend/package.json
- frontend/package-lock.json
- backend/pom.xml
- charts/Chart.yaml
- charts/README.md
- docs/admin/Migration_Guide
There was a problem hiding this comment.
Please merge main / bump to major version
| delivery.getCustomerOrderPositionNumber() == null && | ||
| delivery.getSupplierOrderNumber() == null | ||
| )); | ||
| return validateWithDetails(delivery).isEmpty(); |
There was a problem hiding this comment.
All checks in this method are the same as in OwnDeliveryService.validateWithDetails except for the own partner check. Doesn't it make sense to refactor and applying the same approach as in item stock using basicValidation and [specific validations](https://github.com/eclipse-tractusx/puris/blob/main/backend/src/main/java/org/eclipse/tractusx/puris/backend/stock/logic/service/MaterialItemStockService.java#L47s?
I know it has not yet been this way but could be a good moment to do it.
| if (!partner.equals(demandPartner) || !material.equals(demandMaterial)) { | ||
| log.warn("Received inconsistent data from " + partner.getBpnl() + "\n" + demands); | ||
| return; | ||
| errors.add(new RefreshError(List.of("Received inconsistent data: partner or material mismatch"))); |
There was a problem hiding this comment.
Please harmonize the error messages across apiServices. This message is more specific with regard to the message (great) but lacks the bpnl / ownMaterialNumber. Please combine.
| if (demand.getQuantity() <= 0) { | ||
| errors.add("Quantity must be greater than 0."); |
There was a problem hiding this comment.
| if (demand.getQuantity() <= 0) { | |
| errors.add("Quantity must be greater than 0."); | |
| if (demand.getQuantity() < 0) { | |
| errors.add("Quantity must be greater than or equal to 0."); |
you flipped the less / more sign
| errors.add("Missing demand location BPNS."); | ||
| } | ||
| if (demand.getPartner().equals(ownPartnerEntity)) { | ||
| errors.add("Partner cannot be the same entity."); |
There was a problem hiding this comment.
| errors.add("Partner cannot be the same entity."); | |
| errors.add("Partner cannot be the same as own partner entity."); |
Same as own demand service
| errors.add("Missing stock location BPNA."); | ||
| } | ||
| if (daysOfSupply.getPartner().equals(ownPartnerEntity)) { | ||
| errors.add("Partner cannot be the same entity."); |
There was a problem hiding this comment.
| errors.add("Partner cannot be the same entity."); | |
| errors.add("Partner cannot be the same as own partner entity."); |
| site.getBpns().equals(daysOfSupply.getStockLocationBPNS()) || | ||
| site.getAddresses().stream().noneMatch(address -> address.getBpna().equals(daysOfSupply.getStockLocationBPNA())) | ||
| )) { | ||
| errors.add("Invalid days of supply: stock location is not valid."); |
There was a problem hiding this comment.
| errors.add("Invalid days of supply: stock location is not valid."); | |
| errors.add(String.format("Invalid days of supply: stock location '%s' and or stock address '%s' don't belong to each other or partner '%s'.", daysOfSupply.getStockLocationBPNS(), daysOfSupply.getStockLocationBPNA(), daysOfSupply.getPartner().getBpnl())); |
Please consider a more detailed error description for easier understanding
| site.getBpns().equals(daysOfSupply.getStockLocationBPNS()) || | ||
| site.getAddresses().stream().noneMatch(address -> address.getBpna().equals(daysOfSupply.getStockLocationBPNA())) | ||
| )) { | ||
| errors.add("Invalid days of supply: stock location is not valid."); |
There was a problem hiding this comment.
| errors.add("Invalid days of supply: stock location is not valid."); | |
| errors.add(String.format("Invalid days of supply: stock location '%s' and or stock address '%s' don't belong to each other or partner '%s'.", daysOfSupply.getStockLocationBPNS(), daysOfSupply.getStockLocationBPNA(), daysOfSupply.getPartner().getBpnl())); |
Please consider a more detailed error message to improve understandability.
| return validateWithDetails(daysOfSupply).isEmpty(); | ||
| } | ||
|
|
||
| public List<String> validateWithDetails(ReportedSupplierSupply daysOfSupply) { |
There was a problem hiding this comment.
Please also consider refactoring here. This time it's the exact same validation.
...n/java/org/eclipse/tractusx/puris/backend/demand/logic/services/DemandRequestApiService.java
Show resolved
Hide resolved
|
Hi Tom! I use DBeaver and insert invalid values to existing entries to test Let me know how it goes after this update please :) |
There was a problem hiding this comment.
There have been two smaller things left.
I tried it out and it worked fine.
But I detected one issue: We use the same error messages as for the excel import and we can't identify the entry that is matched with the error message. Thus I walked through all error messages and suggested more appropriate error messages.
I would be grateful, if you could adapt them. Please also consider the stock service.
| if (delivery.getPartner() == null) { | ||
| errors.add("Missing partner."); | ||
| } | ||
| // errors.addAll(validateResponsibility(delivery)); |
There was a problem hiding this comment.
| // errors.addAll(validateResponsibility(delivery)); |
| return basicValidation(delivery).isEmpty() && validateOwnPartner(delivery).isEmpty() | ||
| && validateOwnResponsibility(delivery).isEmpty(); |
There was a problem hiding this comment.
| return basicValidation(delivery).isEmpty() && validateOwnPartner(delivery).isEmpty() | |
| && validateOwnResponsibility(delivery).isEmpty(); | |
| return validateWithDetails(delivery).isEmpty(); |
As it's the same validation I would reuse it same as before.
|
|
||
| public boolean validate(ReportedDelivery delivery) { | ||
| return validateWithDetails(delivery).isEmpty(); | ||
| return basicValidation(delivery).isEmpty() && validateReportedResponsibility(delivery).isEmpty(); |
There was a problem hiding this comment.
| return basicValidation(delivery).isEmpty() && validateReportedResponsibility(delivery).isEmpty(); | |
| return validateWithDetails(delivery).isEmpty(); |
Same as before. If you had a specific thought, feel free to let me know - maybe I'm missing something :)
| if ((demand.getSupplierLocationBpns() != null && | ||
| ownPartnerEntity.getSites().stream().noneMatch(site -> site.getBpns().equals(demand.getSupplierLocationBpns()))) | ||
| || demand.getPartner().getSites().stream().noneMatch(site -> site.getBpns().equals(demand.getDemandLocationBpns()))) { | ||
| errors.add("Invalid demand: supplier or demand location is not valid."); |
There was a problem hiding this comment.
| errors.add("Invalid demand: supplier or demand location is not valid."); | |
| errors.add("Supplier or demand location is not valid."); |
increase consistency
| @Override | ||
| public boolean validate(OwnDemand demand) { | ||
| return validateWithDetails(demand).isEmpty(); | ||
| return true; |
There was a problem hiding this comment.
| return true; | |
| return validateWithDetails(demand).isEmpty(); |
Likely a debugging issue
| break; | ||
| case CUSTOMER: | ||
| if (!delivery.getMaterial().isProductFlag()) { | ||
| errors.add("Material must have product flag for customer responsibility."); |
There was a problem hiding this comment.
| errors.add("Material must have product flag for customer responsibility."); | |
| errors.add(String.format("Material '%s' must be configured as product via flag (incoterm '%s' with customer responsibility).", delivery.getMaterial().getOwnMaterialNumber(), delivery.getIncoterm().getValue())); |
| errors.add("Material must have material flag for supplier responsibility."); | ||
| } | ||
| if (delivery.getPartner().getSites().stream().noneMatch(site -> site.getBpns().equals(delivery.getOriginBpns()))) { | ||
| errors.add("Origin BPNA must match one of the partner entity's site' address BPNAs for supplier responsibility."); |
There was a problem hiding this comment.
| errors.add("Origin BPNA must match one of the partner entity's site' address BPNAs for supplier responsibility."); | |
| errors.add(String.format("Origin BPNA '%s' not configured for site '%s' of partner '%s' (incoterm '%s' with supplier responsibility).", delivery.getOriginBpna(), delivery.getOriginBpns(), delivery.getPartner().getBpnl(), delivery.getIncoterm().getValue())); |
| errors.add("Origin BPNA must match one of the partner entity's site' address BPNAs for supplier responsibility."); | ||
| } | ||
| if (ownPartnerEntity.getSites().stream().noneMatch(site -> site.getBpns().equals(delivery.getDestinationBpns()))) { | ||
| errors.add("Destination BPNA must match one of the partner entity's site' address BPNAs for supplier responsibility."); |
There was a problem hiding this comment.
| errors.add("Destination BPNA must match one of the partner entity's site' address BPNAs for supplier responsibility."); | |
| errors.add(String.format("Destination BPNA '%s' not configured for site '%s' of partner '%s' (incoterm '%s' with supplier responsibility).", delivery.getDestinationBpna(), delivery.getDestinationBpns(), delivery.getPartner().getBpnl(), delivery.getIncoterm().getValue())); |
| errors.add("Material must have product flag for customer responsibility."); | ||
| } | ||
| if (ownPartnerEntity.getSites().stream().noneMatch(site -> site.getBpns().equals(delivery.getOriginBpns()))) { | ||
| errors.add("Site BPNS must match one of the own partner entity's site BPNS for customer responsibility."); |
There was a problem hiding this comment.
| errors.add("Site BPNS must match one of the own partner entity's site BPNS for customer responsibility."); | |
| errors.add(String.format("Origin BPNS '%s' must match one of the own partner entity's site BPNS (incoterm '%s' with customer responsibility).", delivery.getOriginBpns(), delivery.getIncoterm().getValue())); |
| errors.add("Site BPNS must match one of the own partner entity's site BPNS for customer responsibility."); | ||
| } | ||
| if (delivery.getPartner().getSites().stream().noneMatch(site -> site.getBpns().equals(delivery.getDestinationBpns()))) { | ||
| errors.add("Site BPNA must match one of the partner entity's site' address BPNAs for customer responsibility."); |
There was a problem hiding this comment.
| errors.add("Site BPNA must match one of the partner entity's site' address BPNAs for customer responsibility."); | |
| errors.add(String.format("Destination BPNA '%s' not configured for site '%s' of partner '%s' (incoterm '%s' with supplier responsibility).", delivery.getDestinationBpna(), delivery.getDestinationBpns(), delivery.getPartner().getBpnl(), delivery.getIncoterm().getValue())); |
There was a problem hiding this comment.
There have been two smaller things left.
I tried it out and it worked fine.
But I detected one issue: We use the same error messages as for the excel import and we can't identify the entry that is matched with the error message. Thus I walked through all error messages and suggested more appropriate error messages.
I would be grateful, if you could adapt them. Please also consider the stock service.
2 participants
Description
Related issue: #985
Pre-review checks
Please ensure to do as many of the following checks as possible, before asking for committer review: