-
Notifications
You must be signed in to change notification settings - Fork 2
PIN-7516 GET /eservices/{eServiceId}/descriptors/{descriptorId}/(certified,verified,declared)Attributes #2476
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
base: release/2.8.0
Are you sure you want to change the base?
PIN-7516 GET /eservices/{eServiceId}/descriptors/{descriptorId}/(certified,verified,declared)Attributes #2476
Conversation
}; | ||
|
||
// eslint-disable-next-line max-params | ||
async function retrieveEServiceDescriptorAttributes( |
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.
Implementation looks good 🚀
I think the code is clear enough to remove most of the comments. I would add comments only to explain the reason for a step that is not already straightforward with variable/function names
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.
Align also with @crasherbit since part of this implementation could be extracted into a utility function to also create the returning value of the POST calls, since they return the same object.
...ages/m2m-gateway/test/integration/eservices/getEserviceDescriptorCertifiedAttributes.test.ts
Show resolved
Hide resolved
...ages/m2m-gateway/test/integration/eservices/getEserviceDescriptorCertifiedAttributes.test.ts
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.
Looks good 🚀
But there's a bug in the totalCount computation
Plus, left some minor suggestions/quick fixes
packages/m2m-gateway/test/api/eservices/getEserviceDescriptorCertifiedAttributes.test.ts
Outdated
Show resolved
Hide resolved
...ages/m2m-gateway/test/integration/eservices/getEserviceDescriptorCertifiedAttributes.test.ts
Outdated
Show resolved
Hide resolved
...ages/m2m-gateway/test/integration/eservices/getEserviceDescriptorCertifiedAttributes.test.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.
Super!
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.
minor change required, but spec LG
match(error.code) | ||
.with( | ||
"eserviceDescriptorNotFound", | ||
"eserviceDescriptorAttributeNotFound", |
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.
if an attribute id exists in a descriptor and this attribute is not found in the registry, it is considered an internal error
"eserviceDescriptorAttributeNotFound", |
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.
Something wrong with the new base branch, since I see a lot of unrelated changes! It's because it was based on develop where more stuff was merged after the release branch was created...
Maybe the simplest thing to do is to merge develop
again in release/2.8.0
, it shouldn't be an issue. Otherwise, you need to re-apply only the changes related to this feature and it's not straightforward
4610492
to
56dd195
Compare
PIN-7516
SRS
Description
Implemented
GET /eservices/{eServiceId}/descriptors/{descriptorId}/(certified,verified,declared)Attributes
routes in m2m gatewayImplementation
m2mGatewayApi.yml
Double-check errors
Testing
Checklist: