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

PIN 5558 - Get eservice descriptor #1218

Open
wants to merge 2 commits into
base: feature/incaricato
Choose a base branch
from

Conversation

MalpenZibo
Copy link
Collaborator

@MalpenZibo MalpenZibo commented Nov 19, 2024

Closes PIN-5558

Manual test

Delegate without attribute, Delegator without attribute

screenshot-20241120-14:34:59

screenshot-20241120-14:35:16

Delegate without attribute, Delegator with attribute

screenshot-20241120-11:40:29
screenshot-20241120-14:34:32

@MalpenZibo MalpenZibo changed the base branch from develop to feature/incaricato November 19, 2024 16:56
@MalpenZibo MalpenZibo marked this pull request as ready for review November 20, 2024 13:44
@MalpenZibo MalpenZibo requested review from Viktor-K, ecamellini and paologaleotti and removed request for Viktor-K November 20, 2024 13:44
@@ -92,7 +92,7 @@ export function toBffCatalogDescriptorEService(
descriptor: catalogApi.EServiceDescriptor,
producerTenant: tenantApi.Tenant,
agreement: agreementApi.Agreement | undefined,
requesterTenant: tenantApi.Tenant
requesterTenants: tenantApi.Tenant[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment below: since isMine should probably consider only the requester, while hasCertifiedAttributes should consider the requester and the consumer delegators, we can split the two params here

Suggested change
requesterTenants: tenantApi.Tenant[]
requesterTenant: tenantApi.Tenant,
consumerDelegators: tenantApi.Tenant[]

Comment on lines 110 to 115
isMine: requesterTenants.some((t) =>
isRequesterEserviceProducer(t.id, eservice)
),
hasCertifiedAttributes: requesterTenants.some((t) =>
hasCertifiedAttributes(descriptor, t)
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure isMine shall consider the delegators, because it's about the requester being the producer of the eservice, the one that can manage it/edit it, while if I am the delegate of a consumer delegation I want to consume the eservice. Should this maybe be set to true in the case the requester is the delegate of a producer delegation? (to discuss with the "Capofila" team, probably, as a further task).

I would also add a comment to explain why hasCertifiedAttributes checks the delegators, it's hard to understand because the name is misleading now. If you think it makes sense, let's also add the same comment as the API spec's description for the field.

Suggested change
isMine: requesterTenants.some((t) =>
isRequesterEserviceProducer(t.id, eservice)
),
hasCertifiedAttributes: requesterTenants.some((t) =>
hasCertifiedAttributes(descriptor, t)
),
isMine: isRequesterEserviceProducer(requesterTenant.id, eservice),
hasCertifiedAttributes: [requesterTenant, ...consumerDelegators].some((t) =>
hasCertifiedAttributes(descriptor, t)
/* True in case:
- the requester has the certified attributes required to consume the eservice, or
- the requester is the delegated consumer for the eservice and the delegator has the certified attributes required to consume the eservice */
),

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.

2 participants