[#171683039] Import and staticize municipalities from csv #59

AleDore · 2020-07-09T15:40:24Z

Municipalities porting and refactor

pagopa-github-bot · 2020-07-09T15:41:35Z

Affected stories

🌟 #171683039: Come APP voglio poter accedere al dettaglio delle municipalità

New dependencies added: csv-parse, node-fetch and @types/node-fetch.

csv-parse

Author: David Worms

Description: CSV parsing implementing the Node.js `stream.Transform` API

Homepage: https://csv.js.org/parse/


Created	over 6 years ago
Last Updated	2 days ago
License	MIT
Maintainers	1
Releases	99
Direct Dependencies
Keywords	csv, parse, parser, convert, tsv and stream

README

CSV Parser for Node.js

Part of the CSV module, this project is a parser converting CSV text input into arrays or objects. It implements the Node.js stream.Transform API. It also provides a simple callback-based API for convenience. It is both extremely easy to use and powerful. It was first released in 2010 and is used against big data sets by a large community.

Documentation

Features

Follow the Node.js streaming API
Simplicity with the optional callback API
Support delimiters, quotes, escape characters and comments
Line breaks discovery
Support big datasets
Complete test coverage and samples for inspiration
No external dependencies
Work nicely with the csv-generate, stream-transform and csv-stringify packages
MIT License

node-fetch

Author: David Frank

Description: A light-weight module that brings window.fetch to node.js

Homepage: https://github.com/bitinn/node-fetch


Created	over 5 years ago
Last Updated	about 1 month ago
License	MIT
Maintainers	4
Releases	58
Direct Dependencies
Keywords	fetch, http and promise

@types/node-fetch

Author: Unknown

Description: TypeScript definitions for node-fetch

Homepage: http://npmjs.com/package/@types/node-fetch


Created	almost 4 years ago
Last Updated	3 months ago
License	MIT
Maintainers	1
Releases	33
Direct Dependencies	`@types/node` and `form-data`

README

Installation

npm install --save @types/node-fetch

Summary

This package contains type definitions for node-fetch (https://github.com/bitinn/node-fetch).

Details

Files were exported from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node-fetch.

Additional Details

Last updated: Thu, 23 Apr 2020 19:30:58 GMT
Dependencies: @types/form-data, @types/node
Global values: none

Credits

These definitions were written by Torsten Werner, Niklas Lindgren, Vinay Bedre, Antonio Román, Andrew Leedham, Jason Li, Brandon Wilson, Steve Faulkner, ExE Boss, Alex Savin, Alexis Tyler, and Jakub Kisielewski.

Generated by 🚫 dangerJS

codecov-commenter · 2020-07-09T15:41:50Z

Codecov Report

Merging #59 into master will decrease coverage by 1.31%.
The diff coverage is 89.01%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   86.15%   84.83%   -1.32%     
==========================================
  Files          25       31       +6     
  Lines         816     1108     +292     
  Branches       50       64      +14     
==========================================
+ Hits          703      940     +237     
- Misses        112      167      +55     
  Partials        1        1

Impacted Files	Coverage Δ
SendUserDataDownloadMessageActivity/handler.ts	`29.72% <ø> (ø)`
utils/file.ts	`84.00% <84.00%> (ø)`
...eleteOrchestrator/GetUserDataProcessing/handler.ts	`88.46% <88.46%> (ø)`
ExtractUserDataActivity/handler.ts	`80.09% <100.00%> (+2.62%)`	⬆️
utils/conversions.ts	`88.63% <100.00%> (ø)`
UserDataDownloadOrchestrator/handler.ts	`92.59% <0.00%> (ø)`
utils/appinsights.ts	`84.21% <0.00%> (ø)`
UserDataProcessingTrigger/index.ts	`100.00% <0.00%> (ø)`
... and 2 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 cdae189...5b65a46. Read the comment docs.

gunzip · 2020-07-10T06:59:14Z

package.json

    "io-ts": "1.8.5",
    "italia-ts-commons": "^8.1.0",
    "randomstring": "^1.1.5",
+    "request": "^2.88.2",


may we use node-fetch instead of request ? it comes from io-functions-commons so we can avoid adding a new dep (moreover, request package is obsolete)

gunzip · 2020-07-10T07:01:25Z

utils/file.ts

+    () => getBlobAsText(blobService, containerName, blobName).then(e => e),
+    toError
+  ).foldTaskEither(
+    err => fromEither(left(err)),


Suggested change

err => fromEither(left(err)),

fromLeft,

gunzip · 2020-07-10T07:02:21Z

utils/file.ts

+  tryCatch(
+    () => getBlobAsText(blobService, containerName, blobName).then(e => e),
+    toError
+  ).foldTaskEither(


why we need this call to foldTaskEither here? it looks like it's mapping identities

getBlobAsText returns Promise<Either<Error, BlobResult>> so I've used the same code to turn it into a TaskEither -> see

io-functions-admin/ExtractUserDataActivity/handler.ts

Line 161 in cdae189

const fromPromiseEither = <L, R>(

gunzip · 2020-07-10T07:02:40Z

utils/file.ts

+      ).then(e => e),
+    toError
+  ).foldTaskEither(
+    err => fromEither(left(err)),


same as previous

gunzip · 2020-07-10T07:58:59Z

utils/municipality.ts

+        _.foldL(
+          () => fromLeft(new Error("Municipalities with catastale is empty")),
+          __ =>
+            parseCsv(__, optionMunicipalitiesWithCatastale).foldTaskEither(


gunzip · 2020-07-10T08:02:56Z

utils/municipality.ts

+  )
+    .chain(rawFile =>
+      rawFile.foldL(
+        () => fromEither(leftE(new Error("abolished municipalities is empty"))),


fromEither(left) === fromLeft()

gunzip · 2020-07-10T08:08:03Z

utils/municipality.ts

+    .chain(result =>
+      array.sequence(taskEither)(
+        result
+          .filter(r => r[9] !== "")


a comment here on what's happening would be useful or better you can isolate this filter into its own method with a meaningful name

gunzip · 2020-07-10T08:09:26Z

utils/municipality.ts

+                ),
+              municipality =>
+                serializeMunicipalityToJson(blobService, {
+                  codiceCatastale: r[9],


maybe an object with a map between indexex (integers) and column names (constants) can be useful here

balanza · 2020-07-10T07:41:02Z

utils/municipality.ts

+  if (record.length < 13) {
+    return leftE([
+      {
+        context: [],
+        value: "record has not the right length"
+      }
+    ]);
+  }


Do we need this? I think just the validation below is enough (it would return a well-formed validation error indicating that some values are undefined).
So you can keep all the parsing logic in the Municipality decoder

I think that this check is trying to avoid column shifting problems, because if a record length is larger than expected, we can assign a wrong value to a property. Moreover we use a positional notation to extract values from csv files.

if a record length is larger than expected

Actually the check is for records shorter than expected, isn't it?

Yes, but the logic is the same, maybe we can turn this check into record.length !== <expected_record_length>

balanza · 2020-07-10T08:03:34Z

utils/municipality.ts

+          __ =>
+            parseCsv(__, optionMunicipalitiesWithCatastale).foldTaskEither(
+              e => fromEither(leftE(e)),
+              matrixString => fromEither(rightE(matrixString))


Suggested change

matrixString => fromEither(rightE(matrixString))

matrixString => taskEither.of(matrixString)

balanza · 2020-07-10T08:04:10Z

utils/municipality.ts

+          () => fromLeft(new Error("Municipalities with catastale is empty")),
+          __ =>
+            parseCsv(__, optionMunicipalitiesWithCatastale).foldTaskEither(
+              e => fromEither(leftE(e)),


Suggested change

e => fromEither(leftE(e)),

e => fromLeft(e),

balanza · 2020-07-10T08:10:43Z

utils/municipality.ts

+    .foldTaskEither<Error, Map<string, string>>(
+      err => fromLeft(err),
+      municipalitiesCatastaleRows =>
+        fromEither(
+          rightE(
+            municipalitiesCatastaleRows.reduce(
+              (map: Map<string, string>, row) => {
+                map.set(row[1].toLowerCase(), row[0]);
+                return map;
+              },
+              new Map<string, string>()
+            )
+          )
+        )
+    );


Is foldTaskEither needed? as you are not lifting left type, I think you can use map

.map( municipalitiesCatastaleRows => municipalitiesCatastaleRows.reduce( (map: Map<string, string>, row) => { map.set(row[1].toLowerCase(), row[0]); return map; }, new Map<string, string>() ) );

(to be verified)

I' ve changed it into a chain

balanza · 2020-07-10T08:12:04Z

utils/municipality.ts

+    );
+
+const fromAbolishedMunicipalityToSerializableMunicipality = (
+  abolishedMunicipality: t.TypeOf<typeof AbolishedMunicipality>,


Suggested change

abolishedMunicipality: t.TypeOf<typeof AbolishedMunicipality>,

abolishedMunicipality: AbolishedMunicipality,

balanza · 2020-07-10T08:16:42Z

utils/municipality.ts

+  return {
+    codiceCatastale,
+    municipality: {
+      codiceProvincia: "",
+      codiceRegione: "",
+      denominazione: abolishedMunicipality.comune,
+      denominazioneInItaliano: abolishedMunicipality.comune,
+      denominazioneRegione: "",
+      siglaProvincia: abolishedMunicipality.provincia
+    }
+  } as ISerializableMunicipality;
+};


I'm afraid of this cast. The object isn't recognised as ISerializableMunicipality because we're not sure municipality is a Municipality.

Consider decode the municipality field value:

municipality: Municipality.decode({ codiceProvincia: "", codiceRegione: "", denominazione: abolishedMunicipality.comune, denominazioneInItaliano: abolishedMunicipality.comune, denominazioneRegione: "", siglaProvincia: abolishedMunicipality.provincia }).getOrElseL(_ => { throw new Error(/* err message */); }

Or even, why not make ISerializableMunicipality a decoder itself?

balanza · 2020-07-10T08:21:31Z

utils/municipality.ts

+  )
+    .chain(rawFile =>
+      rawFile.foldL(
+        () => fromEither(leftE(new Error("abolished municipalities is empty"))),


Suggested change

() => fromEither(leftE(new Error("abolished municipalities is empty"))),

() => fromLeft(new Error("abolished municipalities is empty")),

balanza · 2020-07-10T08:25:34Z

utils/municipality.ts

+    .chain(rawFile =>
+      rawFile.foldL(
+        () => fromEither(leftE(new Error("abolished municipalities is empty"))),
+        json => fromEither(rightE(JSON.parse(json)))


Suggested change

json => fromEither(rightE(JSON.parse(json)))

json => taskEither.of(JSON.parse(json)))

Besides of that, be careful because JSON.parse can throw! Consider use parseJSON

gunzip

lgtm apart from minor changes

gunzip · 2020-07-10T13:31:36Z

UpdateMunicipalities/function.json

+{
+  "bindings": [
+    {
+      "schedule": "0 */15 * * * *",


how many times is this supposed to run ? once a day suffices

Which hour?

I've set every day at 01:00 AM

gunzip · 2020-07-10T13:32:30Z

UpdateMunicipalities/handler.ts

+  )
+    .foldTaskEither<Error, void>(
+      err => {
+        context.log.error(`Cannot update municipalities| ${err}`);


may we prefix the error with the name of the function? moreover get rid of the space after '|'

gunzip · 2020-07-10T13:33:09Z

UpdateMunicipalities/handler.ts

+    exportCurrentMunicipalities(blobService),
+    exportForeignMunicipalities(blobService)
+  )
+    .foldTaskEither<Error, void>(


it looks like mapLeft() would suffice here (instead of folding)

gunzip · 2020-07-10T13:34:29Z

UpdateMunicipalities/index.ts

+
+import { getUpdateMunicipalitiesHandler } from "./handler";
+
+const storageConnectionString = getRequiredStringEnv("StorageConnection");


Suggested change

const storageConnectionString = getRequiredStringEnv("StorageConnection");

const storageConnectionString = getRequiredStringEnv("AssetsStorageConnection");

gunzip · 2020-07-10T13:44:01Z

utils/municipality.ts

+      denominazioneRegione: "",
+      siglaProvincia: abolishedMunicipality.provincia
+    }
+  }).getOrElseL(_ => {


may we just return the either and skip all the invalid entries with a warning?

gunzip · 2020-07-10T13:44:18Z

utils/municipality.ts

+        abolishedMunArray
+          .filter(am => municipalityToCatastale.has(am.comune.toLowerCase()))
+          .map(am =>
+            fromAbolishedMunicipalityToSerializableMunicipality(


this may throw currently, see comment above

gunzip · 2020-07-10T13:48:08Z

utils/municipality.ts

+    )
+    .chain(result =>
+      array.sequence(taskEither)(
+        filterEmptyCodiceCatastaleRecords(result, 9 as NonNegativeInteger).map(


why "9" ? I guess the index value must be hardcoded into filterEmptyCodiceCatastaleRecords instead of being taken as paramter

agree, fixed

gunzip · 2020-07-10T13:48:48Z

why draft?=

AleDore · 2020-07-17T21:53:18Z

UpdateMunicipalities/handler.ts

  blobService: BlobService
 ) => async (context: Context): Promise<unknown> => {
-  return sequenceT(taskEither)(
+  const partialResults = sequenceT(taskEitherSeq)(


I've to refactor this way (but it's not the only way I think), in order to avoid Task maximum number exceeded. It tooks about 20 seconds to write about 8300 blob. All feedbacks are welcome! @gunzip @balanza

sonarqubecloud · 2021-11-30T08:39:25Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

balanza · 2022-01-10T16:22:03Z

@AleDore I assume this to not be actual anymore

AleDore · 2022-01-10T18:54:25Z

Yes it is correct! Inviato da iPhone

…

Il giorno 10 gen 2022, alle ore 17:22, Emanuele De Cupis ***@***.***> ha scritto: @AleDore I assume this to not be actual anymore — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

municipality utilities

498f690

gunzip reviewed Jul 10, 2020

View reviewed changes

balanza reviewed Jul 10, 2020

View reviewed changes

refactor for review + timeTrigger function

480e6d0

AleDore requested review from balanza and gunzip July 10, 2020 13:04

gunzip reviewed Jul 10, 2020

View reviewed changes

AleDore added 2 commits July 10, 2020 17:14

added tests on file utils

605c26f

minor changes

8f093f8

AleDore marked this pull request as ready for review July 10, 2020 17:00

AleDore requested review from cloudify and francescopersico as code owners July 10, 2020 17:00

AleDore requested a review from gunzip July 10, 2020 17:00

gunzip changed the title ~~[#171683039] - Porting of municipalities exports from io-service-metadata~~ [#171683039] Import and staticize municipalities from csv Jul 13, 2020

AleDore added 2 commits July 17, 2020 23:49

fix large number of TaskEither

f6385f9

fix lint

5b65a46

AleDore commented Jul 17, 2020

View reviewed changes

gunzip added the on-hold label Nov 17, 2020

balanza marked this pull request as draft January 10, 2022 16:21

	matrixString => fromEither(rightE(matrixString))
	matrixString => taskEither.of(matrixString)

	abolishedMunicipality: t.TypeOf<typeof AbolishedMunicipality>,
	abolishedMunicipality: AbolishedMunicipality,

	() => fromEither(leftE(new Error("abolished municipalities is empty"))),
	() => fromLeft(new Error("abolished municipalities is empty")),

	json => fromEither(rightE(JSON.parse(json)))
	json => taskEither.of(JSON.parse(json)))


		import { getUpdateMunicipalitiesHandler } from "./handler";

		const storageConnectionString = getRequiredStringEnv("StorageConnection");

[#171683039] Import and staticize municipalities from csv #59

Are you sure you want to change the base?

[#171683039] Import and staticize municipalities from csv #59

Uh oh!

Conversation

AleDore commented Jul 9, 2020

Uh oh!

pagopa-github-bot commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Affected stories

CSV Parser for Node.js

Documentation

Features

Installation

Summary

Details

Additional Details

Credits

Uh oh!

codecov-commenter commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AleDore Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gunzip left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pagopa-github-bot commented Jul 9, 2020 •

edited

Loading

codecov-commenter commented Jul 9, 2020 •

edited

Loading

AleDore Jul 10, 2020 •

edited

Loading

gunzip left a comment •

edited

Loading

AleDore Jul 10, 2020 •

edited

Loading

AleDore Jul 17, 2020 •

edited

Loading