-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#12048] Migrate tests for GetCoursesActionTest #13232
base: master
Are you sure you want to change the base?
[#12048] Migrate tests for GetCoursesActionTest #13232
Conversation
…S#13200) * Migrate DeleteAccountActionTest * Fix checkstyle * Modify tests in DeleteAccountActionTest.java * Update method name in DeleteAccountActionTest
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.
Good work just some small changes:
Lets add one more access control test case for unregistered users using loginAsUnregistered
Const.ParamsNames.ENTITY_TYPE, Const.EntityType.INSTRUCTOR, | ||
Const.ParamsNames.COURSE_STATUS, Const.CourseStatus.ACTIVE, | ||
}; | ||
GetCoursesAction a = getAction(params); |
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.
nit: lets use the full name for the variable, action
to make it easier to follow
}; | ||
GetCoursesAction a = getAction(params); | ||
JsonResult result = a.execute(); | ||
CoursesData x = (CoursesData) result.getOutput(); |
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.
nit: similarly, lets use a more descriptive variable name here like data
JsonResult result = a.execute(); | ||
CoursesData x = (CoursesData) result.getOutput(); | ||
assertEquals(1, x.getCourses().size()); | ||
assertEquals(stubCourse.getId(), x.getCourses().get(0).getCourseId()); |
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.
Similarly for student lets add 1 more assertion here for this:
coursesDataList.forEach(CourseData::hideInformationForStudent); |
assertEquals(stubCourseList.size(), x.getCourses().size()); | ||
assertEquals(stubCourseList.get(0).getId(), x.getCourses().get(0).getCourseId()); |
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.
Lets add 1 more assertion to check for the privileges that are set in the action here:
teammates/src/main/java/teammates/ui/webapi/GetCoursesAction.java
Lines 157 to 173 in d1930b5
coursesDataList.forEach(courseData -> { | |
if (sqlCourseIdToInstructor.containsKey(courseData.getCourseId())) { | |
Instructor instructor = sqlCourseIdToInstructor.get(courseData.getCourseId()); | |
if (instructor == null) { | |
return; | |
} | |
InstructorPermissionSet privilege = constructInstructorPrivileges(instructor, null); | |
courseData.setPrivileges(privilege); | |
} else { | |
InstructorAttributes instructor = courseIdToInstructor.get(courseData.getCourseId()); | |
if (instructor == null) { | |
return; | |
} | |
InstructorPermissionSet privilege = constructInstructorPrivileges(instructor, null); | |
courseData.setPrivileges(privilege); | |
} | |
}); |
assertEquals(stubCourseList.size(), x.getCourses().size()); | ||
assertEquals(stubCourseList.get(0).getId(), x.getCourses().get(0).getCourseId()); |
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.
Lets add the assertion to check for the privileges here too
List<Instructor> instructorListStub; | ||
Instructor instructorStub; |
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 thinking if we should store these as attributes of the main class and init them using @BeforeMethod
like in previous PR
This would help to simplify some of the mocking logic for eg., when(mockLogic.getInstructorsForGoogleId(stubInstructorObject.getInstructorStub().getGoogleId())).thenReturn(stubInstructorObject.getInstructorListStub());
to
when(mockLogic.getInstructorsForGoogleId(stubInstructor.getGoogleId())).thenReturn(stubInstructorList);
Part of #12048
Outline of Solution
Change GetCoursesActionTest.java to ensure compatibility with the PostgreSQL database following the database migration.