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.

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

chore: cleanup erpadapterrequest #578

Conversation

eschrewe
Copy link
Contributor

@eschrewe eschrewe commented Sep 6, 2024

Description

  • added validation for currently unsupported request types
  • fixed erp response url
  • fixed bug in PartnerControllerTest (missing mock)
  • solves Refactor ErpAdapterService #419

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).

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 a lot (also for fixing the PartnerControllerTest)! Checked the issue again: Currently there is still the need to provide tests for the mapping of responses.
Note: as this PR is concise I could also split the issue into two tickets. Please let me know what you prefer :)

@eschrewe
Copy link
Contributor Author

After taking a closer look, I noticed that the Asset Type validation was not working correctly in the Unit tests, so I did some refactoring here.

Regarding the issue of adding additional tests for the mapping of responses, I think it'd be best if we wait for detailed feedback from the other team, that is currently supposed to work on the erp-adapter project (Gerhard). Maybe they come up with a request to change to API a bit, or something like that...

@tom-rm-meyer-ISST
Copy link
Contributor

After taking a closer look, I noticed that the Asset Type validation was not working correctly in the Unit tests, so I did some refactoring here.

Regarding the issue of adding additional tests for the mapping of responses, I think it'd be best if we wait for detailed feedback from the other team, that is currently supposed to work on the erp-adapter project (Gerhard). Maybe they come up with a request to change to API a bit, or something like that...

One could argue for both as likely changes won't be fundamental (I expect). As proposed I've split #419 into #584 and #583. This issue solves #584

Is this pr ready for review? :)

@tom-rm-meyer-ISST tom-rm-meyer-ISST linked an issue Sep 11, 2024 that may be closed by this pull request
@eschrewe
Copy link
Contributor Author

After taking a closer look, I noticed that the Asset Type validation was not working correctly in the Unit tests, so I did some refactoring here.
Regarding the issue of adding additional tests for the mapping of responses, I think it'd be best if we wait for detailed feedback from the other team, that is currently supposed to work on the erp-adapter project (Gerhard). Maybe they come up with a request to change to API a bit, or something like that...

One could argue for both as likely changes won't be fundamental (I expect). As proposed I've split #419 into #584 and #583. This issue solves #584

Is this pr ready for review? :)

Yes

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.

LGTM, thanks for contributing!

@tom-rm-meyer-ISST tom-rm-meyer-ISST merged commit 2e57489 into eclipse-tractusx:main Sep 20, 2024
13 checks passed
@tom-rm-meyer-ISST tom-rm-meyer-ISST deleted the chore/erpadapterrequesttypevalidation branch September 20, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor erp adapter request type
2 participants