[#IC-49] Porting SpecialServices to new ProcessMessage flow #172

BurnedMarshal · 2021-11-22T09:41:48Z

List of Changes

Upgrade io-functions-commons dependency for SpecialServices
CreateMessage flow now have serviceCategory to handle Standard and Special service preferences check.
Only admin can create and edit SpecialService properties.

Motivation and Context

The io-functions-services should handle Standard and Special services. This PR upgrade specifications and models to do that.
When a message is created for a Special Service to evaluate if INBOX is enabled for a User, Activation model is used to retrieve Special Service activation.

How Has This Been Tested?

unit test

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)

pagopa-github-bot · 2021-11-22T09:44:11Z

	Warnings
⚠️	This PR changes a total of 430 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).

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 f2482cb

ProcessMessage/handler.ts

UpdateService/handler.ts

openapi/index.yaml

balanza · 2021-11-22T11:41:18Z

package.json

@@ -20,7 +20,7 @@
    "generate": "npm-run-all generate:*",
    "generate:definitions": "rimraf ./generated/definitions && shx mkdir -p ./generated/definitions && gen-api-models --api-spec ./openapi/index.yaml --no-strict --out-dir ./generated/definitions",
    "generate:api-notifications": "rimraf ./generated/notifications && shx mkdir -p ./generated/notifications && gen-api-models --api-spec https://raw.githubusercontent.com/pagopa/io-backend/master/api_notifications.yaml --out-dir ./generated/notifications --response-decoders --request-types",
-    "generate:api-admin": "rimraf generated/api-admin && shx mkdir -p generated/api-admin && gen-api-models --api-spec https://raw.githubusercontent.com/pagopa/io-functions-admin/master/openapi/index.yaml --no-strict --out-dir generated/api-admin --request-types --response-decoders --client",
+    "generate:api-admin": "rimraf generated/api-admin && shx mkdir -p generated/api-admin && gen-api-models --api-spec https://raw.githubusercontent.com/pagopa/io-functions-admin/IC-40-special-service-model-upgrade/openapi/index.yaml --no-strict --out-dir generated/api-admin --request-types --response-decoders --client",


We won't merge a specific branch. Consider:

generate code from master (as it used to be)

generate code from a specific version (good)

import sdk package (better)

You have right. This should be changed to master when the pagopa/io-functions-admin#172 is merged.
If we want to move to sdk import version I think is better do the refactory on another PR.

Fixed in 988385e

…rvice discrimination

BurnedMarshal · 2021-11-22T14:22:25Z

override with what? (why?) it's not very clear form the comment how this works

Sorry, I try to explain it better

BurnedMarshal · 2021-11-22T14:35:34Z

override with what? (why?) it's not very clear form the comment how this works

Sorry, I try to explain it better

@gunzip I've fixed the comments 2ca3a1a. Tell me if now is more clear.

gunzip · 2021-11-22T14:48:30Z

ProcessMessage/handler.ts

+ * A missing Activation is equal to an INACTIVE one.
+ *
+ * @param lActivation
+ * @returns


that would be nice to describe why we return two lists (left and right part of TaskEither) of BlockedInboxOrChannelEnum

If you think that is more clear we can return a TaskEither<never, ReadonlyArray<BlockedInboxOrChannelEnum>> changing the last part of the chain like this:

TE.chainW( TE.fromPredicate(hasActiveActivation => hasActiveActivation, identity) ), TE.fold( () => TE.of( blockedInboxOrChannel.includes(BlockedInboxOrChannelEnum.INBOX) ? blockedInboxOrChannel : [...blockedInboxOrChannel, BlockedInboxOrChannelEnum.INBOX] ), () => TE.of( blockedInboxOrChannel.filter( el => el !== BlockedInboxOrChannelEnum.INBOX ) ) )

a comment was added in 0b1169e

it depends on the usage, if you only use one side returning a Task would be better (or using never).

I prefer the function to return a Task<ReadonlyArray<BlockedInboxOrChannelEnum>> too

TaskEither was replaced by a Task in 06a1b5c

ProcessMessage/handler.ts

gunzip · 2021-11-22T15:18:16Z

ProcessMessage/handler.ts

+                // If the service has category equals to SPECIAL, the INBOX value in blockedInboxOrChannel depends from the Activation
+                // value. If the SPECIAL service is ACTIVE (exists an activation for couple user/service with ACTIVE status)
+                // we remove INBOX from blockedInboxOrChannel, otherwise we add the INBOX value.
+                return getActivationForSpecialServices(lActivation)({


Suggested change

return getActivationForSpecialServices(lActivation)({

return getBlockedInboxesForSpecialService(lActivation)({

Fixed in 0b1169e

gunzip · 2021-11-22T15:18:47Z

ProcessMessage/handler.ts

+                return getActivationForSpecialServices(lActivation)({
+                  blockedInboxOrChannel,
+                  context,
+                  createdMessageEvent,


it looks like you only need the senderServiceId

Fixed in 0b1169e

Co-authored-by: Danilo Spinelli <gunzip@users.noreply.github.com>

gquadrati

LGTM

gquadrati · 2021-11-24T09:11:19Z

ProcessMessage/handler.ts

+ * A missing Activation is equal to an INACTIVE one.
+ *
+ * @param lActivation
+ * @returns


I prefer the function to return a Task<ReadonlyArray<BlockedInboxOrChannelEnum>> too

…Service

balanza · 2021-12-01T10:24:07Z

UpdateService/handler.ts

-            ...servicePayload.service_metadata,
-            token_name: adb2cTokenName
-          }
+          // Only Admins can change service category and custom_special_flow name


It's not clear to me how "being admin" is ensured in this flow

Good point. I've tried to extend the comment in f2482cb

sonarqubecloud · 2021-12-01T10:53:33Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

BurnedMarshal requested a review from balanza November 22, 2021 09:41

BurnedMarshal requested a review from a team as a code owner November 22, 2021 09:41

BurnedMarshal requested a review from infantesimone November 22, 2021 09:41

BurnedMarshal mentioned this pull request Nov 22, 2021

[#IC-49] Update models and specs for special services #166

Closed

6 tasks

balanza suggested changes Nov 22, 2021

View reviewed changes

BurnedMarshal added 4 commits November 22, 2021 15:17

[#IC-49] Porting SpecialServices to new ProcessMessage flow

3b56ea4

[#IC-49] Remove manual casting for ServiceMetadata

5605f86

[#IC-49] Extract sub-pipe for Activation check in a new function

f444621

[#IC-49] Add comment inside blockedInboxOrChannels pipe on Special se…

15c1873

…rvice discrimination

BurnedMarshal force-pushed the IC-49-special-services-bis branch from b2e90ed to 15c1873 Compare November 22, 2021 14:17

[#IC-49] Fix comments

2ca3a1a

typo

65430ec

BurnedMarshal force-pushed the IC-49-special-services-bis branch from b01aa13 to 65430ec Compare November 22, 2021 14:37

gunzip reviewed Nov 22, 2021

View reviewed changes

BurnedMarshal and others added 3 commits November 22, 2021 19:37

Update ProcessMessage/handler.ts

f5b41c1

Co-authored-by: Danilo Spinelli <gunzip@users.noreply.github.com>

Update ProcessMessage/handler.ts

c4b4597

Co-authored-by: Danilo Spinelli <gunzip@users.noreply.github.com>

[#IC-49] Rename method, rationalize parametes, add comment

0b1169e

gquadrati approved these changes Nov 24, 2021

View reviewed changes

BurnedMarshal added 2 commits November 30, 2021 14:47

[#IC-49] Update admin client specs to master branch

988385e

[#IC-49] Replace TaskEither with Task for getBlockedInboxesForSpecial…

06a1b5c

…Service

BurnedMarshal requested review from gquadrati, balanza and gunzip November 30, 2021 14:58

balanza reviewed Dec 1, 2021

View reviewed changes

[#IC-49] Extend comment to explain how admins can edit SpecialServices

f2482cb

BurnedMarshal merged commit 2a71f91 into master Dec 1, 2021

BurnedMarshal deleted the IC-49-special-services-bis branch December 1, 2021 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#IC-49] Porting SpecialServices to new ProcessMessage flow #172

[#IC-49] Porting SpecialServices to new ProcessMessage flow #172

BurnedMarshal commented Nov 22, 2021

pagopa-github-bot commented Nov 22, 2021 •

edited

Loading

balanza Nov 22, 2021

BurnedMarshal Nov 22, 2021

BurnedMarshal Nov 30, 2021

BurnedMarshal commented Nov 22, 2021

BurnedMarshal commented Nov 22, 2021

gunzip Nov 22, 2021

BurnedMarshal Nov 22, 2021

BurnedMarshal Nov 22, 2021

gunzip Nov 23, 2021

gquadrati Nov 24, 2021

BurnedMarshal Nov 30, 2021

gunzip Nov 22, 2021

BurnedMarshal Nov 22, 2021

gunzip Nov 22, 2021

BurnedMarshal Nov 22, 2021

gquadrati left a comment

gquadrati Nov 24, 2021

balanza Dec 1, 2021

BurnedMarshal Dec 1, 2021

sonarqubecloud bot commented Dec 1, 2021

	return getActivationForSpecialServices(lActivation)({
	return getBlockedInboxesForSpecialService(lActivation)({

[#IC-49] Porting SpecialServices to new ProcessMessage flow #172

[#IC-49] Porting SpecialServices to new ProcessMessage flow #172

Conversation

BurnedMarshal commented Nov 22, 2021

List of Changes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

pagopa-github-bot commented Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurnedMarshal commented Nov 22, 2021

BurnedMarshal commented Nov 22, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gquadrati left a comment

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

sonarqubecloud bot commented Dec 1, 2021

pagopa-github-bot commented Nov 22, 2021 •

edited

Loading