[#176068662] Enable check on EmailValidation Orchestrator #132
Conversation
| context.log.error(`${logPrefix}|ERROR=${String(err)}`); | ||
| throw new Error(String(err)); | ||
| }) | ||
| .fold<ReturnTypes>(identity, identity) |
There was a problem hiding this comment.
you can fold here to ResponseSuccessAccepted() so you do it once
Codecov Report
@@ Coverage Diff @@
## master #132 +/- ##
==========================================
+ Coverage 82.25% 82.53% +0.28%
==========================================
Files 28 29 +1
Lines 817 853 +36
Branches 82 86 +4
==========================================
+ Hits 672 704 +32
- Misses 140 145 +5
+ Partials 5 4 -1
Continue to review full report at Codecov.
|
AleDore
requested review from
balanza,
BurnedMarshal and
francescopersico
as code owners
| export const makeStartEmailValidationProcessOrchestratorId = ( | ||
| fiscalCode: FiscalCode, | ||
| email: EmailAddress | ||
| ) => hashCreator(`${fiscalCode}-${email}`); |
There was a problem hiding this comment.
what about adding the day (dd/mm/yyyy) to the hash ? in this way if the orchestrator stucks the user can retry the day after cc @balanza
There was a problem hiding this comment.
The idea is good, I'm just a little reluctant in introducing time assumptions without make them parametrisable.
We can have troubles testing the feature if we hardcode such behaviour.
(this is a comment of https://github.com/pagopa/io-functions-app/pull/132/files#r540826663, github doesn't make it clear)
There was a problem hiding this comment.
you can pass a parameter with the date having a default value of Date.now() if you're worried about testing.
| ) | ||
| .mapLeft(err => { | ||
| context.log.error(`${logPrefix}|ERROR=${String(err)}`); | ||
| throw new Error(String(err)); |
There was a problem hiding this comment.
Not a big fan of this throw... We should return a response error here
There was a problem hiding this comment.
Yep... but API specs does not provide 500 at the moment. For instance, if we add 500 to API specs we must modify also io-backend and io-app.
There was a problem hiding this comment.
we must add 500 to fn-app API specs, the backend seems to alreaady handle it here:
https://github.com/pagopa/io-backend/blob/master/api_backend.yaml#L345
| isOrchestratorRunning(dfClient, orchId).foldTaskEither< | ||
| Error, | ||
| IResponseSuccessAccepted | ||
| >(fromLeft, _ => |
There was a problem hiding this comment.
No need to cast the left type, we can just use chain here:
| isOrchestratorRunning(dfClient, orchId).foldTaskEither< | |
| Error, | |
| IResponseSuccessAccepted | |
| >(fromLeft, _ => | |
| isOrchestratorRunning(dfClient, orchId).chain(_ => |
| export const makeStartEmailValidationProcessOrchestratorId = ( | ||
| fiscalCode: FiscalCode, | ||
| email: EmailAddress | ||
| ) => hashCreator(`${fiscalCode}-${email}`); |
There was a problem hiding this comment.
The idea is good, I'm just a little reluctant in introducing time assumptions without make them parametrisable.
We can have troubles testing the feature if we hardcode such behaviour.
(this is a comment of https://github.com/pagopa/io-functions-app/pull/132/files#r540826663, github doesn't make it clear)
| .mapLeft(err => { | ||
| context.log.error(`${logPrefix}|ERROR=${String(err)}`); | ||
| throw new Error(String(err)); |
There was a problem hiding this comment.
I'd remove this part since any error here is already logged with the function name
| _.isRunning | ||
| ? taskEither.of(void 0) |
There was a problem hiding this comment.
that would be better to chain fromPredicate here instead of using taskEither.of(void 0)
I'm on it :) |
It works! It can be merged :) |
This PR allow to check if there is already a running orchestrator for the id composed by
fiscalCode-email, in order to avoid multiple running for emailValidationProcess on the same email.