-
Notifications
You must be signed in to change notification settings - Fork 1
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
IMN-657 GET EService descriptor by ID #712
Conversation
356946c
to
2c04084
Compare
ec02c5e
to
62f2849
Compare
62f2849
to
56a0277
Compare
packages/bff/src/model/api/converters/catalogClientApiConverter.ts
Outdated
Show resolved
Hide resolved
packages/bff/src/model/api/converters/catalogClientApiConverter.ts
Outdated
Show resolved
Hide resolved
packages/bff/src/model/api/converters/catalogClientApiConverter.ts
Outdated
Show resolved
Hide resolved
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.
Left some minor comments
const descriptorAttributes = { | ||
certified: [ | ||
toBffCatalogApiDescriptorAttribute( | ||
attributes, | ||
descriptor.attributes.certified.flat() | ||
), | ||
], | ||
declared: [ | ||
toBffCatalogApiDescriptorAttribute( | ||
attributes, | ||
descriptor.attributes.declared.flat() | ||
), | ||
], | ||
verified: [ | ||
toBffCatalogApiDescriptorAttribute( | ||
attributes, | ||
descriptor.attributes.verified.flat() | ||
), | ||
], | ||
}; |
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.
This could also become a function toBffCatalogApiDescriptorAttributes(descriptorAttributes)
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.
done in this commit
producer: tenantApi.Tenant | ||
): bffApi.ProducerDescriptorEService { | ||
const producerMail = producer.mails.find( | ||
(m) => m.kind === tenantMailKind.ContactEmail |
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 know the values are the same and that TS probably helps with this, but here you are comparing enums of different types: tenantApi.Tenant
and our internal TenantMailKind
. To be safer I would refactor as follows:
(m) => m.kind === tenantMailKind.ContactEmail | |
(m) => m.kind === tenantApi.MailKind.Values.CONTACT_EMAIL |
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.
Furthermore this commit move it to separate file
headers: Headers, | ||
descriptorAttributeIds: string[] | ||
) => { | ||
// Fetched all attributes in a recursive way |
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.
// Fetched all attributes in a recursive way | |
// Fetch all attributes in a recursive way |
descriptorNotFound: "0006", | ||
attributeNotExists: "0008", | ||
invalidEserviceRequester: "0009", |
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.
Doubt: in the BFF do we have to use the same codes used in Scala, being exposed directly to the FE, or can we do like in the processes and redefine our codes from scratch just incrementing whenever we add a new error?
cc @galales
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.
As discussed offline with @Carminepo2 and @beetlecrunch, this code could be different from the Scala codebase
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, so let's do the increment by one since we don't need to adhere to Scala codes:
descriptorNotFound: "0006", | |
attributeNotExists: "0008", | |
invalidEserviceRequester: "0009", | |
descriptorNotFound: "0004", | |
attributeNotExists: "0005", | |
invalidEserviceRequester: "0006", |
export function toBffCatalogApiEserviceRiskAnalysis( | ||
riskAnalysis: catalogApi.EServiceRiskAnalysis | ||
): bffApi.EServiceRiskAnalysis { | ||
const answers: { [key: string]: string[] } = |
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.
Could you reuse type from bffApi.RiskAnalysisForm?
const answers: { [key: string]: string[] } = | |
const answers: bffApi.RiskAnalysisForm["answers"] = |
packages/bff/src/model/api/converters/catalogClientApiConverter.ts
Outdated
Show resolved
Hide resolved
@@ -129,6 +124,36 @@ const getBulkAttributes = async ( | |||
return await attributesBulk(0, []); | |||
}; | |||
|
|||
const getEserviceDesciptor = ( |
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.
const getEserviceDesciptor = ( | |
const retrieveEserviceDescriptor = ( |
We can align this name with the other retrieves like this and we can fix the typo
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.
Done with this commit
const isUpgradable = (agreement: agreementApi.Agreement): boolean => { | ||
const eserviceDescriptor = eservice.descriptors.find( | ||
(e) => e.id === agreement.descriptorId | ||
); | ||
|
||
return ( | ||
eserviceDescriptor !== undefined && | ||
eservice.descriptors | ||
.filter((d) => Number(d.version) > Number(eserviceDescriptor.version)) | ||
.find( | ||
(d) => | ||
(d.state === descriptorApiState.PUBLISHED || | ||
d.state === descriptorApiState.SUSPENDED) && | ||
(agreement.state === agreementApiState.ACTIVE || | ||
agreement.state === agreementApiState.SUSPENDED) | ||
) !== undefined | ||
); | ||
}; |
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.
For reference: an almost the same function is present here in another PR
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.
@Carminepo2 in this commit have introduced a new file mappers.ts
, my intent is to move here the commons function used to pick, transform or evaluate data.
Also in this next PR I'm using this approach, but in this case probably I prefer to place this method in mapper.
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.
this commit move this logic in a specific file
export function toEserviceAttribute( | ||
attributes: catalogApi.Attribute[] | ||
): EServiceAttribute[] { | ||
return attributes.map((attribute) => ({ | ||
...attribute, | ||
id: unsafeBrandId(attribute.id), | ||
})); | ||
} | ||
|
||
export function toDescriptorWithOnlyAttributes( | ||
descriptor: catalogApi.EServiceDescriptor | ||
): DescriptorWithOnlyAttributes { | ||
return { | ||
...descriptor, | ||
attributes: { | ||
certified: descriptor.attributes.certified.map(toEserviceAttribute), | ||
declared: descriptor.attributes.declared.map(toEserviceAttribute), | ||
verified: descriptor.attributes.verified.map(toEserviceAttribute), | ||
}, | ||
}; | ||
} |
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.
We are using runtime resources to change something at type level, I know the performance impact could be negligible but I saw this in a lot of part of the monorepo and I'm wondering if it is worth discussing.
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.
@Carminepo2 good point 👍🏻
unfortunately branded type usages requires mapping like this, so we need to choose what we want encourage. Id as string are very error prone when they are function's parameter, branded type is a sort of guard to avoid bugs, meanwhile every time we convert models it's necessary this wrapping...
I suggest to discuss a possible policy offline.
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.
Works for me! Just left some minor comments but consider it approved on my side :)
packages/bff/src/model/api/converters/catalogClientApiConverter.ts
Outdated
Show resolved
Hide resolved
descriptorNotFound: "0006", | ||
attributeNotExists: "0008", | ||
invalidEserviceRequester: "0009", |
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, so let's do the increment by one since we don't need to adhere to Scala codes:
descriptorNotFound: "0006", | |
attributeNotExists: "0008", | |
invalidEserviceRequester: "0009", | |
descriptorNotFound: "0004", | |
attributeNotExists: "0005", | |
invalidEserviceRequester: "0006", |
Co-authored-by: Eric Camellini <eric.camellini@gmail.com>
Part of BFF Catalog Routes
Closes IMN-657
Description
Implements route
/producers/eservices/:eserviceId/descriptors/:descriptorId
TEST
✅ HTTP Status 200 - BFF returns expected descriptor
✅ HTTP Status 404 - Descriptor not Found
✅ HTTP Status - 403 Invalid Eservice Requester