[#IP-324] Change fp-ts version to 2.x and removed old italia-commons uses #154

gquadrati · 2021-09-03T10:38:32Z

List of Changes

upgrade fp-ts
upgrade io-ts
upgrade @pagopa/io-function-commons
upgrade express and @types/express
upgrade @pagopa/openapi-codegen-ts
refactor all code to handle new fp-ts constructs
refactor tests
healthcheck now uses semigroups to collect all errors and not just the first one

Motivation and Context

Migrate to fp-ts v2

How Has This Been Tested?

alredy present unit tests

Screenshots (if appropriate):

Types of changes

Chore (nothing changes by a user perspective)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

pagopa-github-bot · 2021-09-06T13:19:55Z

	Warnings
⚠️	This PR changes a total of 2814 LOCs, that is more than a reasonable size of 250. Consider splitting the pull request into smaller ones.
⚠️	Please include a Pivotal story at the beginning of the PR title (see below).

@pagopa/danger-custom-rules

Author: Federico Feroldi

Description: Shared rules for DangerJS, used in the Digital Citizenship projects

Homepage: https://github.com/pagopa/danger-plugin-digitalcitizenship#readme


Created	7 months ago
Last Updated	7 months ago
License	MIT
Maintainers	5
Releases	2
Direct Dependencies	`auto-changelog`, `danger-plugin-yarn` and `pivotaljs`

README

Custom DangerJS rules plugin

Custom rules over Danger CI automation.

Features

Cross-link with PivotalTracker stories, based on PR title
Warn when a PR has no description
Perform sanity checks on yarn.lock
Warn if npm-related files are added (as we use yarn)

@pagopa/express-azure-functions

Author: https://pagopa.gov.it

Description: Express adapter for Azure Functions

Homepage: https://github.com/pagopa/io-functions-express#readme


Created	7 months ago
Last Updated	7 months ago
License	MIT
Maintainers	5
Releases	1
Direct Dependencies

README

io-functions-express

Express adapter for Azure Functions.

Mostly a porting to TypeScript from azure-function-express.

New dependencies added: @pagopa/express-azure-functions and @pagopa/danger-custom-rules.

Example of PR titles that include pivotal stories:

single story: [#123456] my PR title
multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS against e9611ff

CreateMessage/handler.ts

CreateNotificationActivity/handler.ts

AleDore · 2021-09-06T13:31:23Z

StoreMessageContentActivity/handler.ts

-  taskEither
-    .of<ServicePreferenceError, ServicesPreferencesMode>(
+  pipe(
+    TE.of<ServicePreferenceError, ServicesPreferencesMode>(


Do we need to write explicit types here?

Unfortunately in this cases TS cannot infer the Left type (since it's not defined), so we need to explicit it

balanza · 2021-09-07T08:36:07Z

/azp run

azure-pipelines · 2021-09-07T08:36:17Z

No pipelines are associated with this pull request.

WebhookNotificationActivity/handler.ts

utils/healthcheck.ts

utils/profile.ts

AleDore · 2021-09-07T09:17:26Z

Out of the scope of this PR, we must fix node version in .node-version and .nvmrc to 14.16.0

BurnedMarshal · 2021-09-07T10:13:22Z

CreateMessage/handler.ts

+    return pipe(
+      // check whether the client can create a message for the recipient
+      TE.fromEither(
+        canWriteMessage(auth.groups, authorizedRecipients, fiscalCode)
+      ),
+      // Verify if the Service has the required quality to sent message
+      TE.chain(_ =>
+        disableIncompleteServices &&
+        !incompleteServiceWhitelist.includes(serviceId) &&
+        !authorizedRecipients.has(fiscalCode)
+          ? TE.fromEither(
+              pipe(
+                ValidService.decode(userAttributes.service),
+                E.bimap(
                  _1 => ResponseErrorForbiddenNotAuthorizedForRecipient,
                  _1 => true
                )
              )
-            : fromEither(right(true))
-        )
-        .chain(_ =>
-          // check whether the client can provide default addresses
-          fromEither(canDefaultAddresses(messagePayload))
-        )
-        .chain(_ =>
-          // check whether the client can ask for payment
-          fromEither(
-            canPaymentAmount(
-              messagePayload.content,
-              service.maxAllowedPaymentAmount
            )
+          : TE.right(true)
+      ),
+      TE.chainW(_ =>
+        // check whether the client can provide default addresses
+        TE.fromEither(canDefaultAddresses(messagePayload))
+      ),
+      TE.chainW(_ =>
+        // check whether the client can ask for payment
+        TE.fromEither(
+          canPaymentAmount(
+            messagePayload.content,
+            service.maxAllowedPaymentAmount
          )
        )
-        .chain(_ =>
-          // create a Message document in the database
-          createMessageDocument(
-            messageId,
-            messageModel,
-            auth.userId,
-            fiscalCode,
-            messagePayload.time_to_live,
-            serviceId
-          )
+      ),
+      TE.chainW(_ =>


What do you think if we use only one TE.fromEither

pipe( // check whether the client can create a message for the recipient canWriteMessage(auth.groups, authorizedRecipients, fiscalCode), // Verify if the Service has the required quality to sent message E.chain(_ => disableIncompleteServices && !incompleteServiceWhitelist.includes(serviceId) && !authorizedRecipients.has(fiscalCode) ? pipe( ValidService.decode(userAttributes.service), E.bimap( _1 => ResponseErrorForbiddenNotAuthorizedForRecipient, _1 => true ) ) : E.right(true) ), E.chainW(_ => // check whether the client can provide default addresses canDefaultAddresses(messagePayload) ), E.chainW(_ => // check whether the client can ask for payment canPaymentAmount( messagePayload.content, service.maxAllowedPaymentAmount ) ), TE.fromEither, TE.chainW(_ =>

This way we are making that step synchronous, right?

You are right, all the steps before TE.taskEither will be executed on the pipe creation and not when the TaskEither is executed. IMHO is not a problem if the pipe is used inside the execution context. Do you think this pattern could generate unwanted behaviors?

The only one I see is not taking full advantages of node event loop

gquadrati · 2021-09-07T10:14:10Z

Out of the scope of this PR, we must fix node version in .node-version and .nvmrc to 14.16.0

Yes, I've found it set to a higher values, so I thought it was ok

CreateService/handler.ts

BurnedMarshal

LGTM apart a few stylistic comments. Thanks @gquadrati for the amazing job.

GetMessage/handler.ts

WebhookNotificationActivity/__tests__/handler.test.ts

utils/healthcheck.ts

fabriziopapi and others added 2 commits September 3, 2021 16:54

change fp-ts versione to 2.x and removed old italia-commons uses

21920cb

[#IP-324] update tests + libraries

30c405b

gquadrati force-pushed the IP-324--migrate-fp-ts branch from b1eb10d to 30c405b Compare September 3, 2021 15:02

gquadrati marked this pull request as ready for review September 3, 2021 15:08

gquadrati requested review from AleDore, balanza, BurnedMarshal and francescopersico as code owners September 3, 2021 15:08

[#IP-324] update danger rules library

28a1b48

gquadrati changed the title ~~[IP-324] Change fp-ts versione to 2.x and removed old italia-commons uses~~ [IP-324] Change fp-ts version to 2.x and removed old italia-commons uses Sep 3, 2021

gquadrati changed the title ~~[IP-324] Change fp-ts version to 2.x and removed old italia-commons uses~~ [#IP-324] Change fp-ts version to 2.x and removed old italia-commons uses Sep 6, 2021

[#IP-324] upgrade danger

ce0587e

gquadrati force-pushed the IP-324--migrate-fp-ts branch from 55c3b6c to ce0587e Compare September 6, 2021 13:17

pagopa deleted a comment from pagopa-github-bot Sep 6, 2021

AleDore reviewed Sep 6, 2021

View reviewed changes

[#IP-324] refactor based on code review

5cb0e0b

AleDore reviewed Sep 7, 2021

View reviewed changes

BurnedMarshal reviewed Sep 7, 2021

View reviewed changes

[#IP-324] refactor based on code review

0dcbb5d

BurnedMarshal reviewed Sep 7, 2021

View reviewed changes

CreateService/handler.ts Outdated Show resolved Hide resolved

BurnedMarshal approved these changes Sep 7, 2021

View reviewed changes

GetMessage/handler.ts Show resolved Hide resolved

WebhookNotificationActivity/__tests__/handler.test.ts Outdated Show resolved Hide resolved

utils/healthcheck.ts Outdated Show resolved Hide resolved

gquadrati added 2 commits September 7, 2021 20:26

[#IP-324] refactor based on code review

a889d06

[#IP-324] move to TE.ApplicativePar

e9611ff

fabriziopapi approved these changes Sep 9, 2021

View reviewed changes

gquadrati merged commit 63a18d4 into master Sep 9, 2021

gquadrati deleted the IP-324--migrate-fp-ts branch September 9, 2021 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#IP-324] Change fp-ts version to 2.x and removed old italia-commons uses #154

[#IP-324] Change fp-ts version to 2.x and removed old italia-commons uses #154

gquadrati commented Sep 3, 2021 •

edited

Loading

pagopa-github-bot commented Sep 6, 2021 •

edited

Loading

Custom DangerJS rules plugin

Features

io-functions-express

AleDore Sep 6, 2021

gquadrati Sep 6, 2021

balanza commented Sep 7, 2021

azure-pipelines bot commented Sep 7, 2021

AleDore commented Sep 7, 2021

BurnedMarshal Sep 7, 2021

gquadrati Sep 7, 2021

BurnedMarshal Sep 8, 2021

gquadrati Sep 8, 2021

gquadrati commented Sep 7, 2021

BurnedMarshal left a comment

[#IP-324] Change fp-ts version to 2.x and removed old italia-commons uses #154

[#IP-324] Change fp-ts version to 2.x and removed old italia-commons uses #154

Conversation

gquadrati commented Sep 3, 2021 • edited Loading

List of Changes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

pagopa-github-bot commented Sep 6, 2021 • edited Loading

Custom DangerJS rules plugin

Features

io-functions-express

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balanza commented Sep 7, 2021

azure-pipelines bot commented Sep 7, 2021

AleDore commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gquadrati commented Sep 7, 2021

BurnedMarshal left a comment

Choose a reason for hiding this comment

gquadrati commented Sep 3, 2021 •

edited

Loading

pagopa-github-bot commented Sep 6, 2021 •

edited

Loading