#1501 - Process ECE Response file part 2 #1947
Conversation
dheepak-aot
requested review from
guru-aot,
ann-aot,
andrewsignori-aot,
sh16011993,
andrepestana-aot and
JasonCTang
...essors/schedulers/institution-integration/ece-response/ece-response-integration.scheduler.ts
Outdated
Show resolved
Hide resolved
| * exist before downloading it. | ||
| * @returns parsed records from the file. | ||
| */ | ||
| protected async downloadResponseFileLines( |
There was a problem hiding this comment.
Using the overload signature just for the sake of the optional parameter can be avoided in this scenario IMO.
There was a problem hiding this comment.
The over load is actually used to return different types here.
The main intention of using overload is the return type is now changed to <string[] | false> from <string[]>
So most of the method consumers are using a forEach on the method return and we need to type cast everywhere if we are not using the overload.
There was a problem hiding this comment.
but the 2nd overload signature is the same as the implementation signature right? so, does it make sense? and 1st overload signature is covered in the 2nd overload signature.
There was a problem hiding this comment.
As per my understanding and what I saw(in one of the sync up with @andrewsignori-aot) if we use override to an implementation, the implementation becomes as good as private method, i.e it cannot be accessed directly from outside the class. Hence we need an override to the actual method with same arguments and return type.
There was a problem hiding this comment.
When overloading the first signature is not supposed to be exposed. If I am not wrong it does not even show in the VS Code IntelliSense.
I missed the part that the first and the second methods have different return types, so I am good.
sources/packages/backend/libs/integrations/src/services/ssh/sftp-integration-base.ts
Outdated
Show resolved
Hide resolved
.../integrations/src/institution-integration/ece-integration/ece-response.processing.service.ts
Show resolved
Hide resolved
.../integrations/src/institution-integration/ece-integration/ece-response.processing.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/integrations/src/constants/system-configurations-constants.ts
Show resolved
Hide resolved
.../backend/libs/integrations/src/services/institution-location/institution-location.service.ts
Outdated
Show resolved
Hide resolved
.../integrations/src/institution-integration/ece-integration/ece-response.processing.service.ts
Outdated
Show resolved
Hide resolved
.../integrations/src/institution-integration/ece-integration/ece-response.processing.service.ts
Show resolved
Hide resolved
...integrations/src/institution-integration/ece-integration/ece-response.integration.service.ts
Outdated
Show resolved
Hide resolved
.../backend/libs/integrations/src/services/institution-location/institution-location.service.ts
Outdated
Show resolved
Hide resolved
| } | ||
| if (hasErrors) { | ||
| throw new Error( | ||
| `The file consists invalid data and cannot be processed.`, |
sources/packages/backend/libs/integrations/src/services/ssh/sftp-integration-base.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for doing all the changes, looks good 👍
If possible let's ensure that the team is on the same page about this one #1947 (comment)
|
Kudos, SonarCloud Quality Gate passed!
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
Process ECE Response file part 2
Covered in the PR
downloadResponseFileLinesin SFTPIntegrationBase to optionally check if the file exists before downloading it.To be covered in upcoming PR