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

Feat: Move Sharing State Transition Logic to the Sharing State Service #513

Merged

Conversation

nicoprow
Copy link
Contributor

@nicoprow nicoprow commented Oct 6, 2023

Description

Currently, the sharing state service is somewhat of a glorified repository. It just takes a DTO, maps it to the entity and saves whatever is inside. This means the logic of managing sharing state transitions is left to the invoking services.

The goal of this pull request is to move such sharing state transition logic to the sharing state, by this capsulating responsibitilies between the services better.

In addition I added some validation logic for transitioning to shring states making sure essential information for that state is given. At the same time, information that is irrelevant for the transitioned sharing state is being ignored (such as error information for any other state than Error).

Pre-review checks

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

@nicoprow nicoprow self-assigned this Oct 6, 2023
@nicoprow nicoprow added enhancement New feature or request Gate labels Oct 6, 2023
@nicoprow nicoprow changed the title refactor(gate): moved more service logic to the sharing state service Refactor: Move Sharing State Transition Logic to the Sharing State Service Oct 6, 2023
@nicoprow nicoprow force-pushed the refactor/gate/sharing-state-service branch from b0c119b to 637bd3d Compare October 9, 2023 10:50
@nicoprow nicoprow changed the title Refactor: Move Sharing State Transition Logic to the Sharing State Service Feat: Move Sharing State Transition Logic to the Sharing State Service Oct 9, 2023
@nicoprow nicoprow force-pushed the refactor/gate/sharing-state-service branch 2 times, most recently from 53bab1b to cc62eac Compare October 9, 2023 12:49
@nicoprow nicoprow marked this pull request as ready for review October 9, 2023 13:10
@@ -92,6 +91,9 @@ class BusinessPartnerService(

saveChangelog(resolutionResults)

val partners = resolutionResults.map { it.businessPartner }
sharingStateService.setInitial(partners.map { it.externalId }, BusinessPartnerType.ADDRESS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing this with AddressPersistenceService, would it make sense to call sharingStateService.setSuccess(..) instead?
But setting which of the 3 BPNs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it via a simple logic to map from business partner types to the relevant BPN.

Later on, we should move from setting BPN and business partner type explicitely in the sharing state and rather inferring it from the business partner stages. But this can only happen after we integrated the L/S/A endpoints to the generic model, made the external identifiers unique and the business partner type optional in the sharing state.


return page.toDto(page.content.map {
return sharingStatePage.toDto(sharingStatePage.content.map {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use .toPageDto { SharingStateDto(...) } from CommonMappings.kt?

Copy link
Contributor Author

@nicoprow nicoprow Oct 11, 2023

Choose a reason for hiding this comment

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

right, used that mapping instead


fun setSuccess(externalIdBpnDto: Collection<ExternalIdBpnDto>, businessPartnerType: BusinessPartnerType): Collection<SharingState> {
val sharingStates = getOrCreate(externalIdBpnDto.map { it.externalId }, businessPartnerType)
val externalIdByBpn = externalIdBpnDto.associateBy { it.externalId }
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name is confusing.

Suggested change
val externalIdByBpn = externalIdBpnDto.associateBy { it.externalId }
val bpnByExternalId = externalIdBpnDto.associateBy { it.externalId }.mapValues { it.value.bpn }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense

@nicoprow nicoprow force-pushed the refactor/gate/sharing-state-service branch from cc62eac to 08fce18 Compare October 11, 2023 11:25
@nicoprow
Copy link
Contributor Author

Wait until #519 is merged before this should be merged

@nicoprow nicoprow force-pushed the refactor/gate/sharing-state-service branch from 08fce18 to b39f15e Compare October 24, 2023 13:45
 - moved service logic for handling state transitions from other services to the sharing state service
 - added validation for logic when passing transition requests
 - added and adapted tests
@nicoprow nicoprow force-pushed the refactor/gate/sharing-state-service branch from b39f15e to 259e740 Compare October 24, 2023 15:08
@nicoprow nicoprow merged commit 752dc06 into eclipse-tractusx:main Oct 25, 2023
7 checks passed
@nicoprow nicoprow deleted the refactor/gate/sharing-state-service branch October 25, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants