-
Notifications
You must be signed in to change notification settings - Fork 13
#2994 - T4A - Upload File to Student Account - Part 2 #5511
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
Conversation
...backend/apps/queue-consumers/src/processors/t4a/_tests_/e2e/t4a-upload.processor.e2e-spec.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/integrations/src/t4a/t4a-enqueuer.processing.service.ts
Outdated
Show resolved
Hide resolved
| * @returns True if the error is an SSH error with the given code, false otherwise. | ||
| */ | ||
| static hasError(error: unknown, errorCode: SSHErrorCodes): boolean { | ||
| return !!error && (error as SSHError).code === errorCode; |
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.
Was there a reason not to use the class and instance of similar to other errors here?
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.
instance of works when a class is instantiated, similar to what we do in the Web for ApiProcessError.
Here, either the framework throws the error using a class, or the instance of would not work.
Please let me know if I am missing something.
| if (SshService.hasError(error, SSHErrorCodes.NotConnected)) { | ||
| // Stops processing as the SFTP client is disconnected. | ||
| throw new Error("SFTP client disconnected unexpectedly.", { | ||
| cause: error, | ||
| }); | ||
| } |
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.
👍
| } | ||
| if (!students.length) { | ||
| processSummary.warn( | ||
| processSummary.info( |
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.
👍
| /** | ||
| * Validate if a string represents a valid UUID v4. | ||
| */ | ||
| export const uuidV4Matcher: jest.AsymmetricMatcher = { |
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 didn't get the purpose of this matcher. Can you please help me understand?
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.
It asserts if a string is a GUID and also checks the GUID version as 4 (the one that we have been using).
Instead of using an expected.any(String).
dheepak-aot
left a comment
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.
Great coverage of E2E tests. Thanks for making the changes. I would suggest to add the new envs to env-example file.
| * @param errorCode Error code to be checked. | ||
| * @returns True if the error is an SSH error with the given code, false otherwise. | ||
| */ | ||
| static hasError(error: unknown, errorCode: SSHErrorCodes): boolean { |
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.
name could be hasSSHError
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.
It can, but it is a static method, inside a service called SshService, that will be consumed as SshService.hasError. Does it really require SSH? If yes, then the createClient should be renamed to createSSHClient 😉
| CRA_RESPONSE_FOLDER: ${{ secrets.CRA_RESPONSE_FOLDER }} | ||
| CRA_PROGRAM_AREA_CODE: ${{ secrets.CRA_PROGRAM_AREA_CODE }} | ||
| CRA_ENVIRONMENT_CODE: ${{ secrets.CRA_ENVIRONMENT_CODE }} | ||
| T4A_FOLDER: ${{ vars.T4A_FOLDER }} |
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.
Please also update the devops makefile to pass these parameters.
tiago-graf
left a comment
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.
looks great, just a minor comment
|



T4A_FOLDER, as planned, was removed from the constant and converted to an env config.T4A_ARCHIVE_FOLDERwas exposed to allow having all the archived files added to the same shared folder. Currently, all archive operations implicitly look for an "Archive" folder at the same level as the files being processed, which would not work for the T4A and its subfolder, unless an agreement is made to create them ahead of time (either manually or automatically), which is not desired.renameFilefrom the SSH changed to allow using an existing SFTP client.archiveFilechanged to allow it to optionally receive the "absolute" path where the file should be archived.E2E Tests
Queue processor for t4a-upload-enqueuer,
√ Should enqueue 3 batches when the batch size is 100, and there are 2 folders, one with 100 files and the other with 101 files.
√ Should log that no directories were found when no directories were listed from the SFTP.
√ Should not try to add bulk jobs when a directory was found from the SFTP, but no files were found.
Queue processor for t4a-upload,
√ Should download a T4A file from the SFTP, create a file in the student account, upload the file to S3 storage, queue a virus scan, send a notification to the student, and archive the processed file on success when the student has the current SIN valid, and matching the file name.
√ Should find a student match when the SIN is valid for one of the SIN validations entries when the student has multiple valid SIN numbers.
√ Should skip the file upload when a file with the same hash was already uploaded for the same student.
√ Should log a warning and archive the file with a not found student when an invalid SIN is found in the T4A file name.
√ Should log a warning and archive the file when multiple student accounts have the same valid SIN.