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

[#174877080] Upgrade to italia-utils 5.x #711

Merged
merged 13 commits into from
Oct 27, 2020
Merged

Conversation

balanza
Copy link
Contributor

@balanza balanza commented Sep 18, 2020

This PR upgrades italia-utils to its latest major version. The following changes are included:

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2020

Codecov Report

Merging #711 into master will increase coverage by 2.02%.
The diff coverage is 80.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
+ Coverage   78.99%   81.02%   +2.02%     
==========================================
  Files          65       68       +3     
  Lines        2119     2219     +100     
  Branches      342      373      +31     
==========================================
+ Hits         1674     1798     +124     
+ Misses        433      408      -25     
- Partials       12       13       +1     
Impacted Files Coverage Δ
src/server.ts 0.00% <0.00%> (ø)
src/services/bonusService.ts 75.00% <ø> (ø)
src/services/pagoPAProxyService.ts 94.44% <ø> (ø)
src/services/userDataProcessingService.ts 96.77% <ø> (ø)
src/utils/responses.ts 91.30% <ø> (ø)
src/strategies/bearerBPDTokenStrategy.ts 41.17% <41.17%> (ø)
src/strategies/bearerMyPortalTokenStrategy.ts 41.17% <41.17%> (ø)
src/config.ts 79.43% <45.45%> (-2.88%) ⬇️
src/types/commons.ts 80.00% <50.00%> (-20.00%) ⬇️
src/clients/api.ts 71.42% <75.00%> (+24.62%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e09751...5e081c3. Read the comment docs.

@pagopa-github-bot
Copy link
Contributor

pagopa-github-bot commented Sep 18, 2020

Affected stories

  • 🌟 #174877080: Come DEV voglio pote utilizzare i client delle API generati sul backend dell'app

Generated by 🚫 dangerJS

Comment on lines 6 to 13
/* function SubscriptionKeyHeaderProducer<P>(
token: string
): RequestHeaderProducer<P, "X-Functions-Key" | "Ocp-Apim-Subscription-Key"> {
return () => ({
"Ocp-Apim-Subscription-Key": token,
"X-Functions-Key": token
});
}
} */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gunzip @francescopersico please take a look here:
the former code used to pass token to both X-Functions-Key andOcp-Apim-Subscription-Key header. With the latest implementation, only the latter is valued. Which is the correct configuration?

Copy link
Contributor

Choose a reason for hiding this comment

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

we must update the specs on io-functions-app to take only X-Functions-Key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -28,7 +28,7 @@ export const toInitializedProfile = (
blocked_inbox_or_channels: profile.blocked_inbox_or_channels,
date_of_birth:
user.date_of_birth !== undefined
? formatDate(user.date_of_birth)
? new Date(formatDate(user.date_of_birth))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gunzip please double-check this change as we need to be sure that the type-change is invariant for the API behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you try to test this locally using io-mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically the output format now changes from 1991-12-12 to 1991-12-12T00:00:00.000Z. The client app ignores this value anyway.

@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #711 into master will increase coverage by 1.26%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
+ Coverage   79.67%   80.94%   +1.26%     
==========================================
  Files          68       68              
  Lines        2298     2225      -73     
  Branches      370      370              
==========================================
- Hits         1831     1801      -30     
+ Misses        454      411      -43     
  Partials       13       13              
Impacted Files Coverage Δ
src/services/bonusService.ts 75.00% <ø> (ø)
src/services/pagoPAProxyService.ts 94.44% <ø> (ø)
src/services/userDataProcessingService.ts 96.77% <ø> (ø)
src/clients/api.ts 71.42% <75.00%> (+24.62%) ⬆️
src/clients/bonus.ts 85.71% <75.00%> (+31.86%) ⬆️
src/clients/pagopa.ts 100.00% <100.00%> (+47.36%) ⬆️
src/services/messagesService.ts 81.66% <100.00%> (ø)
src/services/profileService.ts 96.42% <100.00%> (ø)
src/types/profile.ts 91.66% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8028b3d...b2e7987. Read the comment docs.

@balanza balanza marked this pull request as ready for review October 20, 2020 13:09
} {
const options = {
): Client<"SubscriptionKey"> {
return createClient<"SubscriptionKey">({
Copy link
Contributor

Choose a reason for hiding this comment

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

this must be x-functions-key (Ocp-Apim-Subscription-Key is unused)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually is. The referenced api spec is

  SubscriptionKey:
    type: apiKey
    name: X-Functions-Key
    in: header

Under the hood, the generated client does the following:

headers: ({ SubscriptionKey }: { SubscriptionKey: string }) => ({
      "X-Functions-Key": `Bearer ${SubscriptionKey}`
    }),

Copy link
Contributor

Choose a reason for hiding this comment

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

that's right, not easy to grasp from the code here :-) (maybe we could add a comment?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here

@balanza balanza requested a review from gunzip October 27, 2020 09:07
const options = {
): Client {
return createClient({
basePath: "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this param override the spec defined base path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, generated client use the value from the spec as default. This basically tells the client to ignore such value.
Please note that the full host+basepath has been passed by the constructor, which is not touched by this PR

export const PAGOPA_CLIENT = new PagoPAClientFactory(

Copy link
Contributor

@BurnedMarshal BurnedMarshal left a comment

Choose a reason for hiding this comment

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

lgtm

@balanza balanza merged commit fa0147d into master Oct 27, 2020
@balanza balanza deleted the 174877080-upgrade-io-utils branch October 27, 2020 13:18
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.

6 participants