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.

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 GetHasResponsesActionTest #13289

Merged

Conversation

BunnyHoppp
Copy link
Contributor

Part of #12048

Outline of Solution

Change GetHasResponsesActionTest.java to ensure compatibility with the PostgreSQL database following the database migration.

BunnyHoppp and others added 4 commits March 25, 2025 17:27
Commit current changes before refactoring
GetHasResponsesAction
Fix the path for execute method for
migrated courses
Test execute method with some positive
and negative test cases

Test checkSpecificAccessControl method
with some positive and negative test
cases
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 nites, rest of the test lgtm!

@jasonqiu212 jasonqiu212 added the s.Ongoing The PR is being worked on by the author(s) label Mar 30, 2025
@mingyang143 mingyang143 added s.ToReview The PR is waiting for review(s) s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) s.ToReview The PR is waiting for review(s) labels Mar 30, 2025
@mingyang143 mingyang143 self-requested a review March 30, 2025 05:39
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.

LGTM

@domoberzin domoberzin added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 30, 2025
@domoberzin domoberzin merged commit 2c1409b into TEAMMATES:master Mar 30, 2025
11 checks passed
Copy link
Contributor

@mingyang143 mingyang143 left a comment

Choose a reason for hiding this comment

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

Other than the comments, the rest LGTM

}

@Test
void testAccessControl_nonInstructor_cannotAccessResponses() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test can be renamed to testAccessControl_nonInstructorWithInstructorEntityParams_cannotAccessResponses for more clarity. We can also add another similar test for students with the name testAccessControl_nonStudentWithStudentEntityParams_cannotAccessResponses.


verifyCannotAccess(params);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can add a test method with the name testAccessControl_invalidParams_throwsInvalidHttpParameterException, with the following tests:

    void testAccessControl_invalidParams_throwsInvalidHttpParameterException() {
        loginAsStudent(typicalStudent.getGoogleId());
        // Missing Entity Type in params
        String[] params = new String[] {
                Const.ParamsNames.COURSE_ID, typicalCourse.getId(),
        };
        verifyHttpParameterFailureAcl(params);

        // Missing Course ID in params
        String[] params1 = new String[] {
                Const.ParamsNames.ENTITY_TYPE, Const.EntityType.STUDENT,
                Const.ParamsNames.FEEDBACK_SESSION_NAME, typicalFeedbackSession.getName(),
        };
        verifyHttpParameterFailureAcl(params1);
        logoutUser();

        loginAsInstructor(typicalInstructor.getGoogleId());
        // Missing course ID and feedback session id in params but with feedback session name in params
        String[] params2 = new String[] {
                Const.ParamsNames.FEEDBACK_SESSION_NAME, typicalFeedbackSession.getName(),
                Const.ParamsNames.ENTITY_TYPE, Const.EntityType.INSTRUCTOR,
        };
        verifyHttpParameterFailureAcl(params2);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants