-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#IOCIT-105] added servicesPreferences to profile data download #191
Conversation
Example of PR titles that include pivotal stories:
Generated by 🚫 dangerJS |
@@ -255,7 +291,8 @@ describe("createExtractUserDataActivityHandler", () => { | |||
notificationStatusModel: notificationStatusModelMock, | |||
profileModel: profileModelMock, | |||
userDataBlobService: blobServiceMock, | |||
userDataContainerName: aUserDataContainerName | |||
userDataContainerName: aUserDataContainerName, | |||
servicePreferencesModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd maintain coherence keeping or servicePreferencesModel: servicePreferencesModel
or servicePreferencesModel
alone. Also mocks have usually a "Mock" suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 1403000
utils/__tests__/userData.test.ts
Outdated
if (E.isRight(result)) { | ||
expect(result.right).toMatchObject(mockedUserData); | ||
expect( | ||
(result.right.servicesPreferences[0] as any).foo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could just check that they are equal to aServicePreference
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 1403000
ExtractUserDataActivity/handler.ts
Outdated
servicePreferencesModel.findAllByFiscalCode(fiscalCode), | ||
flattenAsyncIterator, | ||
asyncIteratorToArray, | ||
promise => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just use fromPromiseEither
or define something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't clear to me what do you mean with fromPromiseEither
. In here we have a Promise<ReadonlyArray<Either>>
and the Either
s are not relevant in the TE transformation. IMHO the tryCatch
is the best method to transform a Promise to a TE where the left value is obtained only on the promise failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i created another function because the promise mentioned above was returning an either[]
. done in 1403000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that there was a utility function above and maybe we could have adapted it to be more general. The function is using tryCatch but taking it outside the pipe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that with a separated function the code is less readable in this case, but i leave on you @michaeldisaro @arcogabbo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On new functions pls add a detailed JS doc :D
Co-authored-by: burnedmarshal <dan.manni@gmail.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, to be sure maybe we can just run it on io-mock if possible and check it works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
i just run it and it works as expected |
List of Changes
Added
servicesPreferences
field toAllUserData
typeI did a mini refactor on the pipeline for the user data fetch replacing some
sequenceS
withbind
structure where parallel tasks wasn't neededMotivation and Context
At the moment the servicesPreferences of a profile are not being added to the data a user downloads their data
How Has This Been Tested?
The entire flow of adding some preferences and downloading the data was tested through the
io-mock
projectTests passing
The delete procedure already eliminates the preferences, so testing wasn't needed in that front.
Screenshots (if appropriate):
Types of changes
Checklist: