[#172621882] Get all data for a specific user #42

balanza · 2020-05-11T10:10:23Z

Implementation of part of the Activit handles needed to complete the task.

Things to note:

some feature may fit better into io-functions-commons, like mocks. We might move them there after this activity
NotificationModel has been extended to support a specific use case. This as well might be moved into commons.

pagopa-github-bot · 2020-05-11T10:11:50Z

Affected stories

🌟 #172621882: Come CIT, quando esprimo la volontà di scaricare i miei dati, voglio ricevere un link per il download dei dati entro 7gg dalla richiesta

Generated by 🚫 dangerJS

AleDore · 2020-05-11T12:08:03Z

SetUserDataProcessingStatusActivity/handler.ts

+    tryCatch(
+      () =>
+        userDataProcessingModel.createOrUpdateByNewOne({
+          ...currentRecord,
+          status: nextStatus
+        }),
+      (err: Error) => {
+        return ActivityResultQueryFailure.encode({
+          kind: "QUERY_FAILURE",
+          query: "userDataProcessingModel.createOrUpdateByNewOne",
+          reason: err.message
+        });
+      }


Can we use fromQueryEither in this case?

I agree, but still fromQueryEither isn't as generic as to be placed in a dedicated module. I'll just copy it over though, because I think it's more eloquent

codecov-io · 2020-05-11T12:46:51Z

Codecov Report

Merging #42 into master will decrease coverage by 1.80%.
The diff coverage is 77.98%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   88.81%   87.00%   -1.81%     
==========================================
  Files          21       24       +3     
  Lines         608      762     +154     
  Branches       37       49      +12     
==========================================
+ Hits          540      663     +123     
- Misses         67       98      +31     
  Partials        1        1

Impacted Files	Coverage Δ
CreateService/handler.ts	`85.18% <0.00%> (ø)`
UpdateService/handler.ts	`90.47% <0.00%> (ø)`
GetService/handler.ts	`86.95% <50.00%> (+3.62%)`	⬆️
ExtractUserDataActivity/handler.ts	`73.00% <73.00%> (ø)`
SetUserDataProcessingStatusActivity/handler.ts	`88.09% <88.09%> (ø)`
utils/userData.ts	`100.00% <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 a06e8f6...919c475. Read the comment docs.

gunzip · 2020-05-11T13:03:29Z

ExtractUserDataActivity/notification.ts

+            value: fiscalCode
+          }
+        ],
+        query: `SELECT * FROM m WHERE m.fiscal_code = @fiscalCode`


since the partitionKey of the notifications collection is messageId this results in a cross partition query which should be avoided (see https://docs.microsoft.com/it-it/azure/cosmos-db/how-to-query-container#cross-partition-query).

you should include a condition on messageId so: get all users messages then, for each retrieved message, loop to get the message-status and the relative nofitications; then, for each retrieved notification, loop to get then the notification-status.

gunzip · 2020-05-11T13:05:40Z

SetUserDataProcessingStatusActivity/handler.ts

+ * The validation process use a `id and validator` strategy.
+ *
+ * The `id` is generated using an ulid generator and is used when searching
+ * a specific ValidationToken entity in the table storage.
+ *
+ * For the `validator` we use a random-bytes generator. This `validator` value is
+ * hashed using the `sha256` strategy and then stored in the entity as `validatorHash`
+ *
+ * Each token has also a `InvalidAfter` field to set the token lifetime.


probably we should remove this comments? :)

gunzip · 2020-05-12T13:02:47Z

ExtractUserDataActivity/handler.ts

+ *
+ * @returns the same document without db metadata
+ */
+const fromRetrievedDbDocument = <T>(doc: T & RetrievedDocumentT): T => {


every model has a class method toBaseType method which does exactly the same and does not depend on cosmosdb internal, consider using that one and remove this one
ie. https://github.com/pagopa/io-functions-commons/blob/6b60b5cc5fb840ff397f2ebc3261a3c7408b2168/src/models/profile.ts#L129

it seems to to be used by anyone. It's passed to the base class which exposes it as protected, then never actually used.
https://github.com/pagopa/io-functions-commons/blob/master/src/utils/documentdb_model.ts#L31

If we cannot use that, I suggest to remove this method anyway and just use
t.exact(Message).encode(o); inline when needed

gunzip · 2020-05-12T13:07:17Z

ExtractUserDataActivity/handler.ts

+const appendContentToMessage = (
+  messageFromDb: RetrievedMessageWithoutContent,
+  content: MessageContent
+): MessageWithContent => ({


why do we need to merge the message with its content ?
it's better to just save them in a separate blob

good, even easier

the structure of the zip should mimic the internal representation (that won't happen, but suppose you have to restore all data fom the content of the zip :)

This includes also document-related data? I mean: should we export also RetrievedDocument-related fields?

no since they're not needed to rebuild the whole thing

gunzip · 2020-05-12T13:12:18Z

ExtractUserDataActivity/handler.ts

+        .map(([notificationId, channel]) =>
+          fromQueryEither(
+            () =>
+              notificationStatusModel.findOneNotificationStatusByNotificationChannel(


why do we need to differentiate by channel?

no, but I wanted to use the actual method which queries by statusId, and calculates the statusId by its channel

gunzip · 2020-05-12T13:15:27Z

ExtractUserDataActivity/handler.ts

+    ReadonlyArray<RetrievedNotification>
+  > => {
+    if (messages.length) {
+      // this spread is needed as typescript wouldn't recognize messages[0] to be defined otherwise


it's not clear, why do we need this pattern here ? consider using a NonEmptyArray instead and avoid to call the same code twice

NonEmptyArray.decode(messages)... or if (NonEmptyArray.is(message)) ...

"fp-ts/lib/NonEmptyArray"

didn't know that, thanks

are you sure it works like that? it doesn't seem to be a decoder though.

Anyway, I think it doesn't solved the issue: to pass an arbitrary list of taskEither to a sequenceT to resolve them in parallel: https://github.com/pagopa/io-functions-admin/pull/42/files/98dd1f4ddb37bed424eb078bd0549cd0a9c064b9#diff-6c3441a7b004efdd3512746300508009R339

I mean: you can just use NonEmptyArray.fromArray(messages).map(...) instead of check messages length and separate head and tail (it's not clear why do you want to do that). you can use sequence (instead of sequenceT) to process them in parallel

https://grossbart.github.io/fp-ts-recipes/#/async?a=work-with-a-list-of-tasks-in-parallel

gunzip · 2020-05-12T15:29:37Z

ExtractUserDataActivity/handler.ts

+        arrayOfArray =>
+          fromEither(
+            right(
+              arrayOfArray.reduce(


you can use array.flatten from fp-ts

I think it doesn't support readonly arrays (introduced in v2.5)

gunzip · 2020-05-12T15:30:04Z

ExtractUserDataActivity/handler.ts

+    ReadonlyArray<RetrievedNotification>
+  > => {
+    if (messages.length) {
+      // this spread is needed as typescript wouldn't recognize messages[0] to be defined otherwise


I mean: you can just use NonEmptyArray.fromArray(messages).map(...) instead of check messages length and separate head and tail (it's not clear why do you want to do that). you can use sequence (instead of sequenceT) to process them in parallel

https://grossbart.github.io/fp-ts-recipes/#/async?a=work-with-a-list-of-tasks-in-parallel

ExtractUserDataActivity/handler.ts

gunzip · 2020-05-13T08:06:57Z

ExtractUserDataActivity/handler.ts

+      .foldTaskEither(
+        e => fromEither(left(e)),
+        arrayOfArray =>
+          // tslint:disable-next-line: readonly-array
+          fromEither(right(flatten(arrayOfArray as RetrievedNotification[][])))
+      );


since you are converting back to taskEither, maybe I'm wrong but it looks like you can just use map(flatten) here

you're right, foldTaskEither come from a version of the code which has been refactored now. No more reason for that, thanks

gunzip · 2020-05-13T08:08:26Z

ExtractUserDataActivity/handler.ts

+          () => messageModel.getContentFromBlob(blobService, messageId),
+          "messageModel.getContentFromBlob"
+        ).foldTaskEither<ActivityResultQueryFailure, MessageContentWithId>(
+          failure => fromEither(left(failure)),


when the left value is unchanged, probably you can chain or map instead of folding

In this case it's not trivial, because the right path may become left if Option is none.

gunzip · 2020-05-13T08:10:43Z

ExtractUserDataActivity/handler.ts

+                switch (channel) {
+                  case NotificationChannelEnum.EMAIL:
+                  case NotificationChannelEnum.WEBHOOK:
+                    return [notificationId, channel];
+                  default:
+                    assertNever(channel);


cannot ...Object.values(NotificationChannelEnum).map(channel => [notificationId, channel])
just work without the switch here?

Yes, but this way I could implement an exhaustive check that will force us to update this function if in the future we add/remove options in NotificationChannelEnum

why do we want to be forced to update this method in case?

Maybe we'll add support for another notification channel

and why .map(channel => [notificationId, channel]) won't suffice in that case?

gunzip · 2020-05-13T08:11:13Z

ExtractUserDataActivity/handler.ts

+        ActivityResultQueryFailure,
+        ReadonlyArray<NotificationStatus>
+      >(
+        e => fromEither(left(e)),


map / chain ?

gunzip · 2020-05-13T08:14:04Z

ExtractUserDataActivity/handler.ts

+        })
+      )
+      .chain(({ fiscalCode }) => queryAllUserData(fiscalCode))
+      .map(allUserData =>


when you have map + mapLeft you can use bimap to handle both

ExtractUserDataActivity/index.ts

ExtractUserDataActivity/notification.ts

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

…tions-admin into 172621882-accesso-ai-dati

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

…tions-admin into 172621882-accesso-ai-dati

balanza added 5 commits May 7, 2020 19:30

update libs

88113b6

implemented SetUserDataProcessingStatusActivityHandler

72a3437

Merge branch 'master' into 172621882-accesso-ai-dati

42d6145

ExtractUserDataActivity

1cab5c3

better comments

a497dbb

balanza requested review from gunzip and AleDore May 11, 2020 10:10

balanza self-assigned this May 11, 2020

balanza changed the title ~~[#172621882] accesso ai dati~~ [#172621882] access to data for citizens May 11, 2020

balanza added 3 commits May 11, 2020 12:32

better failure handling

ff9a056

fix exhaustive check

3b800f8

fix lint

691b857

AleDore reviewed May 11, 2020

View reviewed changes

refactor

87d4032

gunzip reviewed May 11, 2020

View reviewed changes

balanza added 3 commits May 12, 2020 11:15

retrieving notifications using message id

1febd9a

fix comments

6a5092b

add query notification status

98dd1f4

balanza marked this pull request as ready for review May 12, 2020 13:18

balanza requested review from cloudify and francescopersico as code owners May 12, 2020 13:18

balanza requested a review from gunzip May 12, 2020 13:19

gunzip reviewed May 12, 2020

View reviewed changes

balanza added 4 commits May 12, 2020 18:34

message blobs as separate entities

31a6259

use t.exact instead of manually skims types

e2b542c

refactor sequences

a876b03

using flatten

975e3e5

avoid extract webhook urls

4b0cf46

balanza closed this May 13, 2020

balanza reopened this May 13, 2020

balanza removed request for cloudify and francescopersico May 13, 2020 07:32

refactor

0bd30e8

gunzip requested changes May 13, 2020

View reviewed changes

gunzip changed the title ~~[#172621882] access to data for citizens~~ [#172621882] Get all data from database and storage for a specific user May 13, 2020

gunzip changed the title ~~[#172621882] Get all data from database and storage for a specific user~~ [#172621882] Get all data for a specific user May 13, 2020

balanza and others added 11 commits May 13, 2020 12:12

Update ExtractUserDataActivity/index.ts

e6cdd0f

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

Merge branch '172621882-accesso-ai-dati' of github.com:pagopa/io-func…

bffd99e

…tions-admin into 172621882-accesso-ai-dati

using bimap

c4fb9ae

Update ExtractUserDataActivity/handler.ts

caf973f

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

Update ExtractUserDataActivity/notification.ts

daadc33

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

Update ExtractUserDataActivity/handler.ts

ab9cd6b

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

Update ExtractUserDataActivity/handler.ts

44ce7be

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

refactor

3b55c55

Merge branch '172621882-accesso-ai-dati' of github.com:pagopa/io-func…

36d8728

…tions-admin into 172621882-accesso-ai-dati

refactor

eec952f

added message status to export

919c475

gunzip approved these changes May 13, 2020

View reviewed changes

gunzip merged commit fdfc8e9 into master May 13, 2020

gunzip deleted the 172621882-accesso-ai-dati branch May 13, 2020 16:13

balanza restored the 172621882-accesso-ai-dati branch May 14, 2020 20:31

[#172621882] Get all data for a specific user #42

[#172621882] Get all data for a specific user #42

Conversation

balanza commented May 11, 2020 • edited Loading

pagopa-github-bot commented May 11, 2020 • edited Loading

Affected stories

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 11, 2020 • edited Loading

Codecov Report

gunzip May 11, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunzip May 12, 2020 • edited Loading

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

gunzip May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunzip May 12, 2020 • edited Loading

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

balanza May 13, 2020 • edited Loading

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

balanza commented May 11, 2020 •

edited

Loading

pagopa-github-bot commented May 11, 2020 •

edited

Loading

codecov-io commented May 11, 2020 •

edited

Loading

gunzip May 11, 2020 •

edited

Loading

gunzip May 12, 2020 •

edited

Loading

gunzip May 12, 2020 •

edited

Loading

gunzip May 12, 2020 •

edited

Loading

balanza May 13, 2020 •

edited

Loading