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

Jump to bottom

feat: implemented days of supply edc integration #694

Merged
merged 5 commits into from
Dec 20, 2024
Merged

feat: implemented days of supply edc integration #694

merged 5 commits into from
Dec 20, 2024

Conversation

ReneSchroederLJ
Copy link
Member

@ReneSchroederLJ ReneSchroederLJ commented Nov 27, 2024 •
edited
Loading

Description

  • add SAMM models for Days of Supply
  • implemented logic for registering the corresponding Assets and Contract
  • implemented the request service for exchanging Days of Supply through the EDC

reolves #312

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

  • DEPENDENCIES are up-to-date. Dash license tool. Committers can open IP issues for restricted libs.
  • Copyright and license header are present on all affected files
  • If helm chart has been changed, the chart version has been bumped to either next major, minor or patch level (compared to released chart).

@ReneSchroederLJ ReneSchroederLJ marked this pull request as ready for review November 27, 2024 17:16
tom-rm-meyer-ISST
tom-rm-meyer-ISST requested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your contribution, and sorry for the late review. Got some smaller changes. Likely we should instead of solving a few things open issues to do a refactoring.

The updates broke the following test cases of the bruno collection

  • Test_02-EDC/Check from Customer POV/Query catalog for Submodel Assets
  • Test_02-EDC/Check from Supplier POV/Query catalog for Submodel Assets
    The test, Test_01-MAD/01_01-Init/Customer/MAD/Get All Partners has been missed by me during a recent fix.
  • Also the ShellDescriptors should be checked regarding the new submodels

I'm wondering if this should also be handled with a separate pr and if we should do the PR also against a feature branch. Then we could handle the quick things in this pr. Additional stuff (update bruno collection, some javadoc) on the branch, too and we just merge the feature fully. Feel free to also reach out to me via MS teams to quickly align here.

backend/src/main/java/org/eclipse/tractusx/puris/backend/common/edc/domain/model/AssetType.java Outdated Show resolved Hide resolved
...main/java/org/eclipse/tractusx/puris/backend/common/edc/logic/service/EdcAdapterService.java Outdated Show resolved Hide resolved
backend/src/main/java/org/eclipse/tractusx/puris/backend/common/util/VariablesService.java Outdated Show resolved Hide resolved
charts/puris/Chart.yaml Outdated Show resolved Hide resolved
...a/org/eclipse/tractusx/puris/backend/supply/controller/DaysOfSupplyRequestApiController.java Outdated Show resolved Hide resolved
...main/java/org/eclipse/tractusx/puris/backend/supply/logic/service/CustomerSupplyService.java Show resolved Hide resolved
...a/org/eclipse/tractusx/puris/backend/supply/logic/service/DaysOfSupplyRequestApiService.java Show resolved Hide resolved
...a/org/eclipse/tractusx/puris/backend/supply/logic/service/DaysOfSupplyRequestApiService.java
if (direction == DirectionCharacteristic.OUTBOUND) {
List<List<OwnSupplierSupply>> suppliesBySite = new ArrayList<>();
for (var site : sites) {
var supplierSupply = supplierSupplyService.calculateSupplierDaysOfSupply(
Copy link
Contributor

Choose a reason for hiding this comment

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

I very often mix up the perspective. Some Javadoc would be nice, if this doesn't explode the pr length. Else it could be a follow up issue. Also e.g. Javadoc in Controller SupplyController is confusing. Endpoint description for method getCustomerDaysOfSupply leaves room to interpretation. It's the endpoint to calculate your own days of supply with direction inbound for a supplier partner.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the handling of DirectionCharacteristic and the role perspectives need to be checked separately. While developing other features and investigating bugs I came across potential issues with inconsistencies in the logic. We should try to make an effort to make sure it's always consistent and more transparent.

...a/org/eclipse/tractusx/puris/backend/supply/logic/service/DaysOfSupplyRequestApiService.java
for (var reportedCustomerSupply : reportedCustomerSupplies) {
var supplyPartner = reportedCustomerSupply.getPartner();
var supplyMaterial = reportedCustomerSupply.getMaterial();
if (!partner.equals(supplyPartner) || !material.equals(supplyMaterial)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, regarding my comment in DaysOfSupplySammMapper): we're consistent with other handlings. I'm wondering if we should handle this validation within the samm mapper or the validation of the entity. Currently it's to some extent distributed and I don't remember a reason (could be historic growing). But this refactoring is not part of this pr.

...main/java/org/eclipse/tractusx/puris/backend/supply/logic/service/SupplierSupplyService.java Outdated Show resolved Hide resolved
tom-rm-meyer-ISST
tom-rm-meyer-ISST requested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating. Just a small version change missing. You can directly commit via suggestion! Re-request review for merge

backend/src/main/java/org/eclipse/tractusx/puris/backend/common/edc/domain/model/AssetType.java Outdated Show resolved Hide resolved
...main/java/org/eclipse/tractusx/puris/backend/supply/logic/service/CustomerSupplyService.java Show resolved Hide resolved
...ain/java/org/eclipse/tractusx/puris/backend/supply/logic/adapter/DaysOfSupplySammMapper.java Show resolved Hide resolved
...ain/java/org/eclipse/tractusx/puris/backend/supply/logic/adapter/DaysOfSupplySammMapper.java Show resolved Hide resolved
tom-rm-meyer-ISST
tom-rm-meyer-ISST approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, issues for remaining comments will be creaed by @ReneSchroederLJ (feel free to link the pr and close the issues).

Helm-Chart Link is ignored on purpose. Version Bump will be done during release.

@tom-rm-meyer-ISST tom-rm-meyer-ISST merged commit 1bf86a2 into eclipse-tractusx:main Dec 20, 2024
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-rm-meyer-ISST tom-rm-meyer-ISST tom-rm-meyer-ISST approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ReneSchroederLJ @tom-rm-meyer-ISST