-
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
#1978 - Request an Offering Change: Student View Request - e2e tests (Part 2) #2240
#1978 - Request an Offering Change: Student View Request - e2e tests (Part 2) #2240
Conversation
...-offering-change-request.students.controller.getApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...-offering-change-request.students.controller.getApplicationOfferingChangeRequest.e2e-spec.ts
Show resolved
Hide resolved
...-offering-change-request.students.controller.getApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...-offering-change-request.students.controller.getApplicationOfferingChangeRequest.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...ing-change-request.students.controller.getApplicationOfferingChangeRequestStatus.e2e-spec.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.
The tests look good. We just need to investigate the memory issue.
...fering-change-request.students.controller.updateApplicationOfferingChangeRequest.e2e-spec.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.
Thanks for doing the changes, just one more comment
@sh16011993 didn't we include e2e test for the offering controller that we added? |
...fering-change-request.students.controller.updateApplicationOfferingChangeRequest.e2e-spec.ts
Show resolved
Hide resolved
...fering-change-request.students.controller.updateApplicationOfferingChangeRequest.e2e-spec.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.
Nice coverage, please take a look at the comments.
I missed it @dheepak-aot . Thanks for pointing it out. I will add it. |
@@ -115,7 +115,7 @@ export class ApplicationOfferingChangeRequestStudentsController extends BaseCont | |||
description: | |||
"Invalid application offering change status or student consent not provided", | |||
}) | |||
async updateApplicationOfferingChangeRequestStatus( | |||
async updateApplicationOfferingChangeRequest( |
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.
👍
let applicationRepo: Repository<Application>; | ||
let assessmentRepo: Repository<StudentAssessment>; | ||
|
||
const deliveryMethod = (offering: EducationProgramOffering) => { |
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 believe that usually, we keep the helper method at the end of the file. If it is something to be reused outside this file it an be moved to a different file/util.
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 doing the changes, just left a minor comment, hence approving it 👍
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
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 @sh16011993 . thanks for doing the changes and fixing the memory issue in the PR
await app?.close(); | ||
}); | ||
|
||
const deliveryMethod = (offering: EducationProgramOffering) => { |
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.
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 job. Left a minor comment.
const savedUser = await db.user.save(createFakeUser()); | ||
const fakeOffering = createFakeEducationProgramOffering({ | ||
auditUser: savedUser, | ||
}); | ||
const offering = await db.educationProgramOffering.save(fakeOffering); | ||
const fakeApplication = createFakeApplication({ student }); | ||
const application = await applicationRepo.save(fakeApplication); | ||
const fakeAssessment = createFakeStudentAssessment({ | ||
auditUser: savedUser, | ||
application, | ||
offering, | ||
}); | ||
fakeApplication.currentAssessment = fakeAssessment; | ||
await applicationRepo.save(fakeApplication); | ||
await assessmentRepo.save(fakeAssessment); |
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.
We could have used the saveFakeApplication with student and it would have replaced all the code here right?
I am not going to call it a blocker but to keep in mind for upcoming e2es.
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 have made the changes @dheepak-aot . Thanks for pointing it out. 👍
|
||
it("Should throw the Not Found (404) exception when the offering summary purpose is not provided.", async () => { | ||
// Arrange | ||
const savedUser = await db.user.save(createFakeUser()); |
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.
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.
Thanks for doing the changes 👍
As a part of this PR, the following task was completed
The below test cases were written for the controller
ApplicationOfferingChangeRequestStudentsController
and controllerEducationProgramOfferingStudentsController
endpoints:getApplicationOfferingChangeRequest
√ Should return the application offering change request details when provided with the application offering change request id.
√ Should throw a HttpStatus Not Found (404) when the application offering change request is not associated with the authenticated student.
getApplicationOfferingChangeRequestStatus
√ Should return the application offering change request status when provided with the application offering change request id.
√ Should throw a HttpStatus Not Found (404) when the application offering status change request is not associated with the authenticated student.
updateApplicationOfferingChangeRequest
√ Should update the application offering change request status for the provided application offering change request id.
√ Should throw a HttpStatus Not Found (404) error when an application offering change update is requested for an application not in completed state.
√ Should throw a HttpStatus Not Found (404) error when an application offering change is not associated with the authenticated student.
√ Should throw a HttpStatus Not Found (404) error when an application offering change is not in a valid status to be updated.
√ Should throw a HttpStatus Unprocessable Entity (422) error when an invalid application offering change request is made.
√ Should throw a HttpStatus Unprocessable Entity (422) error when student consent is not provided.
getOfferingSummaryDetails
√ Should get the offering simplified details, not including the validations, approvals and extensive data when the provided purpose is application-offering-change.
√ Should throw the Not Found (404) exception when the student is not authorized for the provided offering.
√ Should throw the Not Found (404) exception when the offering summary purpose is not provided.
Note
Increased memory heap size to 12288 MB to fix the "heap limit allocation failed - JavaScript heap out of memory" error.