-
Notifications
You must be signed in to change notification settings - Fork 14
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
#2112 - Business validations: Application Bulk Withdrawal #2465
#2112 - Business validations: Application Bulk Withdrawal #2465
Conversation
sources/packages/backend/apps/api/src/app.institutions.module.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/constants/error-code.constants.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/constants/error-code.constants.ts
Outdated
Show resolved
Hide resolved
...rollers/student-scholastic-standings/student-scholastic-standings.institutions.controller.ts
Outdated
Show resolved
Hide resolved
...rollers/student-scholastic-standings/student-scholastic-standings.institutions.controller.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
...application-bulk-withdrawal/application-bulk-withdrawal-import-business-validation.models.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
.../services/application-bulk-withdrawal/application-bulk-withdrawal-import-business.service.ts
Outdated
Show resolved
Hide resolved
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 work on the other dev comments, I do not have anything new.
.../src/services/application-bulk-withdrawal/application-bulk-withdrawal-import-text.service.ts
Show resolved
Hide resolved
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, nice work @sh16011993
validationModel.applicationFound = true; | ||
validationModel.studentSINMatch = | ||
textValidation.textModel.sin === applicationData.sin; | ||
validationModel.hasCorrectInstitutionCode = |
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.
Are we suppose to do further validation, when originator is incorrect?
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 just updated this part of the code.
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.
Thanks for making the changes @sh16011993
LGTM considering the need to wrap up the PR 👍
private async getApplicationValidationData( | ||
applicationNumbers: string[], | ||
institutionId: number, | ||
originator: string, |
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.
minor, if we need this institution code, then maybe instead of originator
, we can name it as institutionCode
in the service, same for line 145
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.
Thanks for making the changes, adjusting the ticket, and making it happen. Looks good 👍
studentScholasticStandings: { | ||
changeType: | ||
StudentScholasticStandingChangeType.StudentWithdrewFromProgram, | ||
}, | ||
}, |
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 am having a hard time understanding the logic, this method is to retrieve the info of the application that are requested for scholastic standing right? then why this logic? so, the ideal scenario is the application won't have any previous scholastic standing, so with this where
condition, those application info won't be retrieving right? Let me know if I am missing anything
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.
Yes. Thanks @ann-aot . Corrected it.
sin: record.student.sinValidation.sin, | ||
locationId: record.location.id, | ||
locationCode: record.location.institutionCode, | ||
hasPreviouslyBeenWithdrawn: !!record.studentScholasticStandings?.filter( |
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 u can use .some()
and it will directly return a 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.
Looks good!
Kudos, SonarCloud Quality Gate passed!
|
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.
Nice effort @sh16011993 👍 thanks for doing the changes 👍
As a part of this PR, the following tasks were completed:
validation-utils
file.Screenshots [validation errors and warnings]: