Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#12048] Migrate tests for GetStudentActionTest #13241

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[#12048] Migrate tests for GetStudentActionTest #13241

wants to merge 4 commits into from

Conversation

mingyang143
Copy link
Contributor

@mingyang143 mingyang143 commented Feb 19, 2025 •
edited
Loading

Part of #12048

Outline of Solution
Change GetStudentActionTest.java to ensure compatibility with the PostgreSQL database following the database migration.

@github-actions GitHub Actions
Copy link

Hi @mingyang143, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

@mingyang143 mingyang143 changed the title [#12048] Migrate tests for GetStudentAction [#12048] Migrate tests for GetStudentActionTest Feb 19, 2025
@mingyang143 mingyang143 marked this pull request as ready for review February 19, 2025 17:18
@jasonqiu212 jasonqiu212 self-requested a review March 6, 2025 11:24
jasonqiu212
jasonqiu212 reviewed Mar 6, 2025
View reviewed changes
Copy link
Contributor

@jasonqiu212 jasonqiu212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments on checking the output. rest looks good!

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java Outdated
GetStudentAction action = getAction(params);
StudentData studentData = (StudentData) getJsonResult(action).getOutput();
assertEquals(studentData.getEmail(), stubStudent.getEmail());
assertEquals(studentData.getKey(), null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can replace this with assertNull(studentData.getKey())

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java Outdated
Comment on lines 96 to 97
assertEquals(studentData.getEmail(), stubStudent.getEmail());
assertEquals(studentData.getKey(), null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can follow the old test case and create a private method for checking if the student data matches?

domoberzin
domoberzin requested changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@domoberzin domoberzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks good, would be good to incorporate some of the review comments, thanks for the work!

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java
}

@Test
void testSpecificAccessControl_instructorWithoutPermission_cannotAccess() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please add some blank lines for better readability

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java
/**
* SUT: {@link GetStudentAction}.
*/
public class GetStudentActionTest extends BaseActionTest<GetStudentAction> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please add some blank lines for better readability in some of the test methods

@domoberzin domoberzin added the s.Ongoing The PR is being worked on by the author(s) label Mar 6, 2025
dishenggg
dishenggg requested changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@dishenggg dishenggg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, just some comments

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java Outdated
loginAsInstructor(stubInstructor.getGoogleId());
String[] params = {
Const.ParamsNames.COURSE_ID, stubCourse.getId(),
Const.ParamsNames.STUDENT_ID, stubStudent.getGoogleId(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think the STUDENT_ID param is being used in this action, i tried commenting it out but the test still passes

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java Outdated
loginAsInstructor(stubInstructor.getGoogleId());
String[] params = {
Const.ParamsNames.COURSE_ID, stubCourse.getId(),
Const.ParamsNames.STUDENT_ID, stubStudent.getGoogleId(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here regarding STUDENT_ID

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java Outdated
Comment on lines 283 to 285
String[] params = {
Const.ParamsNames.COURSE_ID, stubCourse.getId(),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the typical instructor access includes STUDENT_EMAIL param lets add it here, we also need it to reach the block that checks if an instructor has access

teammates/src/main/java/teammates/ui/webapi/GetStudentAction.java

Lines 40 to 51 in 9c2d80c

if (studentEmail != null) {
student = sqlLogic.getStudentForEmail(courseId, studentEmail);
if (student == null || userInfo == null || !userInfo.isInstructor) {
throw new UnauthorizedAccessException(UNAUTHORIZED_ACCESS);
}
Instructor instructor = sqlLogic.getInstructorByGoogleId(courseId, userInfo.id);
gateKeeper.verifyAccessible(instructor, sqlLogic.getCourse(courseId),
student.getTeamName(),
Const.InstructorPermissions.CAN_VIEW_STUDENT_IN_SECTIONS);

Without the STUDENT_EMAIL param we are testing this block

teammates/src/main/java/teammates/ui/webapi/GetStudentAction.java

Lines 54 to 61 in 9c2d80c

} else {
if (userInfo == null || !userInfo.isStudent) {
throw new UnauthorizedAccessException(UNAUTHORIZED_ACCESS);
}
student = sqlLogic.getStudentByGoogleId(courseId, userInfo.id);
gateKeeper.verifyAccessible(student, course);
}

src/test/java/teammates/sqlui/webapi/GetStudentActionTest.java Outdated
void testSpecificAccessControl_studentPublicAccess_cannotAccess() {
loginAsStudent(stubStudent.getGoogleId());
when(mockLogic.getCourse(stubCourse.getId())).thenReturn(stubCourse);
when(mockLogic.getStudentByGoogleId(stubCourse.getId(), stubStudent.getGoogleId())).thenReturn(stubStudent);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i understand this test case correctly this should be

Suggested change
when(mockLogic.getStudentByGoogleId(stubCourse.getId(), stubStudent.getGoogleId())).thenReturn(stubStudent);
when(mockLogic.getStudentForEmail(stubCourse.getId(), "randomEmail")).thenReturn(null);;

@mingyang143
Copy link
Contributor Author

Hi @domoberzin, @dishenggg and @jasonqiu212, I have updated the PR according to the changes requested. Let me know if there are anymore issues, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonqiu212 jasonqiu212 Awaiting requested review from jasonqiu212

@domoberzin domoberzin Awaiting requested review from domoberzin

@dishenggg dishenggg Awaiting requested review from dishenggg

Requested changes must be addressed to merge this pull request.

Assignees

@mingyang143 mingyang143

Labels
s.Ongoing The PR is being worked on by the author(s)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mingyang143 @dishenggg @jasonqiu212 @domoberzin