-
Notifications
You must be signed in to change notification settings - Fork 23
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
feature: #586 bpdm lookup functionality #939
feature: #586 bpdm lookup functionality #939
Conversation
# Conflicts: # tx-backend/src/main/java/org/eclipse/tractusx/traceability/bpn/domain/service/BpnRepository.java # tx-backend/src/main/java/org/eclipse/tractusx/traceability/bpn/infrastructure/repository/BpnRepositoryImpl.java
…ature/586-bpdm-lookup
# Conflicts: # CHANGELOG.md # docs/api/traceability-foss-backend.json # tx-backend/openapi/traceability-foss-backend.json
This reverts commit 789b673.
This reverts commit 1eaf015.
# Conflicts: # tx-backend/src/main/java/org/eclipse/tractusx/traceability/assets/infrastructure/base/irs/model/response/mapping/submodel/BatchMapper.java # tx-backend/src/main/java/org/eclipse/tractusx/traceability/assets/infrastructure/base/irs/model/response/mapping/submodel/JustInSequenceMapper.java # tx-backend/src/main/java/org/eclipse/tractusx/traceability/assets/infrastructure/base/irs/model/response/mapping/submodel/SerialPartMapper.java # tx-backend/src/main/java/org/eclipse/tractusx/traceability/assets/infrastructure/base/irs/model/response/relationship/Aspect.java
This reverts commit 1a6c657.
This reverts commit 83c995a.
This reverts commit 5421616.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ds-lcapellino please find my comments.
...raceability/assets/infrastructure/base/irs/model/response/mapping/submodel/MapperHelper.java
Outdated
Show resolved
Hide resolved
@@ -87,6 +87,13 @@ public void updateManufacturers(Map<String, String> bpns) { | |||
} | |||
} | |||
|
|||
@Override | |||
public BpnEntity save(BusinessPartnerResponse businessPartner) { | |||
String value = businessPartner.getNames() == null ? null : businessPartner.getNames().get(0).getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you clarify with product owner if it is okay from business die to use first element out of the response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I migrated the feature from IRS to TraceX and there this was basically the case. However, I adapted this line to check for empty Strings and use the first not empty value.
But we should talk to PO about potential risks with this implementation.
...ain/java/org/eclipse/tractusx/traceability/bpn/infrastructure/repository/BpnServiceImpl.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public String findByBpn(String bpn) { | ||
String manufacturerName = bpnRepository.findManufacturerName(bpn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to return an optional within bpnResository.findManufacturerName(bpn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since currently have no placeholder designed for empty manufacturer names, I think it is okay to return a null value and also store in the database. With an Optional, we check if the Optional is empty and would then store a null value in the database. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend using an Optional as return type. Then another developer has full control over the flow and knows that this variable might be not existing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this method wasn't introduced to my PR, I would recommend adding this to #440. I will comment the issue accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you.
...src/main/java/org/eclipse/tractusx/traceability/common/config/RestTemplateConfiguration.java
Outdated
Show resolved
Hide resolved
Hey @ds-mwesener, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This PR introduces functionality to resolve the manufacturer name by a given BPN. Since IRS is planning to remove this feature on their side, TraceX must resolve BPN by its own.
If the BPDM service is not available, a null value will be inserted for the manufacturer name.
Persistence of BPNs and manufacturer names is respected in this PR.
resolves #586
Added
Pre-review checks
Please ensure to do as many of the following checks as possible, before asking for committer review: