From b69776810e1db550075cc31115f8fd3c573673ae Mon Sep 17 00:00:00 2001 From: Wei Qing <48304907+weiquu@users.noreply.github.com> Date: Sat, 24 Feb 2024 13:00:35 +0900 Subject: [PATCH] [#12048] Resolve merge conflicts (#12776) * remove extra methods * remove method * remove extra check * fix failing tests * fix 2/3 tests * merge in GetFeedbackSessionActionTest from pr12773 --- .../it/ui/webapi/GetCoursesActionIT.java | 6 +- .../webapi/BasicCommentSubmissionAction.java | 18 --- .../webapi/BasicFeedbackSubmissionAction.java | 112 +++++++++--------- .../CreateFeedbackResponseCommentAction.java | 2 - .../webapi/SubmitFeedbackResponsesAction.java | 32 ++--- .../sqlui/webapi/GetCourseActionTest.java | 3 +- .../GetFeedbackResponsesActionTest.java | 24 ---- .../webapi/GetFeedbackSessionActionTest.java | 12 +- 8 files changed, 71 insertions(+), 138 deletions(-) diff --git a/src/it/java/teammates/it/ui/webapi/GetCoursesActionIT.java b/src/it/java/teammates/it/ui/webapi/GetCoursesActionIT.java index d052607ca49..408821c38b7 100644 --- a/src/it/java/teammates/it/ui/webapi/GetCoursesActionIT.java +++ b/src/it/java/teammates/it/ui/webapi/GetCoursesActionIT.java @@ -145,8 +145,10 @@ private void verifySameCourseData(CourseData actualCourse, Course expectedCourse private void verifySameCourseDataStudent(CourseData actualCourse, Course expectedCourse) { assertEquals(actualCourse.getCourseId(), expectedCourse.getId()); assertEquals(actualCourse.getCourseName(), expectedCourse.getName()); - assertEquals(actualCourse.getCreationTimestamp(), 0); - assertEquals(actualCourse.getDeletionTimestamp(), 0); + assertEquals(actualCourse.getCreationTimestamp(), expectedCourse.getCreatedAt().toEpochMilli()); + if (expectedCourse.getDeletedAt() != null) { + assertEquals(actualCourse.getDeletionTimestamp(), expectedCourse.getDeletedAt().toEpochMilli()); + } assertEquals(actualCourse.getTimeZone(), expectedCourse.getTimeZone()); } diff --git a/src/main/java/teammates/ui/webapi/BasicCommentSubmissionAction.java b/src/main/java/teammates/ui/webapi/BasicCommentSubmissionAction.java index 14f13aeb58d..186a44b2140 100644 --- a/src/main/java/teammates/ui/webapi/BasicCommentSubmissionAction.java +++ b/src/main/java/teammates/ui/webapi/BasicCommentSubmissionAction.java @@ -18,24 +18,6 @@ abstract class BasicCommentSubmissionAction extends BasicFeedbackSubmissionActio static final String FEEDBACK_RESPONSE_COMMENT_EMPTY = "Comment cannot be empty"; - /** - * Validates the questionType of the corresponding question. - */ - void validQuestionForCommentInSubmission(FeedbackQuestionAttributes feedbackQuestion) { - if (!feedbackQuestion.getQuestionDetailsCopy().isFeedbackParticipantCommentsOnResponsesAllowed()) { - throw new InvalidHttpParameterException("Invalid question type for comment in submission"); - } - } - - /** - * Validates the questionType of the corresponding question. - */ - void validQuestionForCommentInSubmission(FeedbackQuestion feedbackQuestion) { - if (!feedbackQuestion.getQuestionDetailsCopy().isFeedbackParticipantCommentsOnResponsesAllowed()) { - throw new InvalidHttpParameterException("Invalid question type for comment in submission"); - } - } - /** * Validates comment doesn't exist of corresponding response. */ diff --git a/src/main/java/teammates/ui/webapi/BasicFeedbackSubmissionAction.java b/src/main/java/teammates/ui/webapi/BasicFeedbackSubmissionAction.java index 95cb30dfbda..566db1f25c3 100644 --- a/src/main/java/teammates/ui/webapi/BasicFeedbackSubmissionAction.java +++ b/src/main/java/teammates/ui/webapi/BasicFeedbackSubmissionAction.java @@ -129,25 +129,6 @@ void checkAccessControlForStudentFeedbackSubmission( } } - /** - * Checks the access control for student feedback result. - */ - void checkAccessControlForStudentFeedbackResult( - StudentAttributes student, FeedbackSessionAttributes feedbackSession) throws UnauthorizedAccessException { - if (student == null) { - throw new UnauthorizedAccessException("Trying to access system using a non-existent student entity"); - } - - String previewAsPerson = getRequestParamValue(Const.ParamsNames.PREVIEWAS); - - if (StringHelper.isEmpty(previewAsPerson)) { - gateKeeper.verifyAccessible(student, feedbackSession); - verifyMatchingGoogleId(student.getGoogleId()); - } else { - checkAccessControlForPreview(feedbackSession, false); - } - } - /** * Checks the access control for student feedback submission. */ @@ -185,6 +166,25 @@ void checkAccessControlForStudentFeedbackSubmission(Student student, FeedbackSes } } + /** + * Checks the access control for student feedback result. + */ + void checkAccessControlForStudentFeedbackResult( + StudentAttributes student, FeedbackSessionAttributes feedbackSession) throws UnauthorizedAccessException { + if (student == null) { + throw new UnauthorizedAccessException("Trying to access system using a non-existent student entity"); + } + + String previewAsPerson = getRequestParamValue(Const.ParamsNames.PREVIEWAS); + + if (StringHelper.isEmpty(previewAsPerson)) { + gateKeeper.verifyAccessible(student, feedbackSession); + verifyMatchingGoogleId(student.getGoogleId()); + } else { + checkAccessControlForPreview(feedbackSession, false); + } + } + /** * Gets the instructor involved in the submission process. */ @@ -241,6 +241,43 @@ void checkAccessControlForInstructorFeedbackSubmission( } } + /** + * Checks the access control for instructor feedback submission. + */ + void checkAccessControlForInstructorFeedbackSubmission( + Instructor instructor, FeedbackSession feedbackSession) throws UnauthorizedAccessException { + if (instructor == null) { + throw new UnauthorizedAccessException("Trying to access system using a non-existent instructor entity"); + } + + String moderatedPerson = getRequestParamValue(Const.ParamsNames.FEEDBACK_SESSION_MODERATED_PERSON); + String previewAsPerson = getRequestParamValue(Const.ParamsNames.PREVIEWAS); + + if (!StringHelper.isEmpty(moderatedPerson)) { + gateKeeper.verifyLoggedInUserPrivileges(userInfo); + gateKeeper.verifyAccessible( + sqlLogic.getInstructorByGoogleId(feedbackSession.getCourse().getId(), userInfo.getId()), + feedbackSession, Const.InstructorPermissions.CAN_MODIFY_SESSION_COMMENT_IN_SECTIONS); + } else if (!StringHelper.isEmpty(previewAsPerson)) { + gateKeeper.verifyLoggedInUserPrivileges(userInfo); + gateKeeper.verifyAccessible( + sqlLogic.getInstructorByGoogleId(feedbackSession.getCourse().getId(), userInfo.getId()), + feedbackSession, Const.InstructorPermissions.CAN_MODIFY_SESSION); + } else { + gateKeeper.verifySessionSubmissionPrivilegeForInstructor(feedbackSession, instructor); + if (instructor.getAccount() != null) { + if (userInfo == null) { + // Instructor is associated to an account; even if registration key is passed, do not allow access + throw new UnauthorizedAccessException("Login is required to access this feedback session"); + } else if (!userInfo.id.equals(instructor.getAccount().getGoogleId())) { + // Logged in instructor is not the same as the instructor registered for the given key, + // do not allow access + throw new UnauthorizedAccessException("You are not authorized to access this feedback session"); + } + } + } + } + /** * Checks the access control for instructor feedback result. */ @@ -288,43 +325,6 @@ private void checkAccessControlForPreview(FeedbackSessionAttributes feedbackSess } } - /** - * Checks the access control for instructor feedback submission. - */ - void checkAccessControlForInstructorFeedbackSubmission( - Instructor instructor, FeedbackSession feedbackSession) throws UnauthorizedAccessException { - if (instructor == null) { - throw new UnauthorizedAccessException("Trying to access system using a non-existent instructor entity"); - } - - String moderatedPerson = getRequestParamValue(Const.ParamsNames.FEEDBACK_SESSION_MODERATED_PERSON); - String previewAsPerson = getRequestParamValue(Const.ParamsNames.PREVIEWAS); - - if (!StringHelper.isEmpty(moderatedPerson)) { - gateKeeper.verifyLoggedInUserPrivileges(userInfo); - gateKeeper.verifyAccessible( - sqlLogic.getInstructorByGoogleId(feedbackSession.getCourse().getId(), userInfo.getId()), - feedbackSession, Const.InstructorPermissions.CAN_MODIFY_SESSION_COMMENT_IN_SECTIONS); - } else if (!StringHelper.isEmpty(previewAsPerson)) { - gateKeeper.verifyLoggedInUserPrivileges(userInfo); - gateKeeper.verifyAccessible( - sqlLogic.getInstructorByGoogleId(feedbackSession.getCourse().getId(), userInfo.getId()), - feedbackSession, Const.InstructorPermissions.CAN_MODIFY_SESSION); - } else { - gateKeeper.verifySessionSubmissionPrivilegeForInstructor(feedbackSession, instructor); - if (instructor.getAccount() != null) { - if (userInfo == null) { - // Instructor is associated to an account; even if registration key is passed, do not allow access - throw new UnauthorizedAccessException("Login is required to access this feedback session"); - } else if (!userInfo.id.equals(instructor.getAccount().getGoogleId())) { - // Logged in instructor is not the same as the instructor registered for the given key, - // do not allow access - throw new UnauthorizedAccessException("You are not authorized to access this feedback session"); - } - } - } - } - /** * Verifies that it is not a preview request. */ diff --git a/src/main/java/teammates/ui/webapi/CreateFeedbackResponseCommentAction.java b/src/main/java/teammates/ui/webapi/CreateFeedbackResponseCommentAction.java index 02f960ba47b..a7e4deeb41c 100644 --- a/src/main/java/teammates/ui/webapi/CreateFeedbackResponseCommentAction.java +++ b/src/main/java/teammates/ui/webapi/CreateFeedbackResponseCommentAction.java @@ -93,7 +93,6 @@ void checkSpecificAccessControl() throws UnauthorizedAccessException { checkAccessControlForStudentFeedbackSubmission(student, session); - validQuestionForCommentInSubmission(feedbackQuestion); verifyResponseOwnerShipForStudent(student, feedbackResponse, feedbackQuestion); break; case INSTRUCTOR_SUBMISSION: @@ -110,7 +109,6 @@ void checkSpecificAccessControl() throws UnauthorizedAccessException { checkAccessControlForInstructorFeedbackSubmission(instructorAsFeedbackParticipant, session); - validQuestionForCommentInSubmission(feedbackQuestion); verifyResponseOwnerShipForInstructor(instructorAsFeedbackParticipant, feedbackResponse); break; case INSTRUCTOR_RESULT: diff --git a/src/main/java/teammates/ui/webapi/SubmitFeedbackResponsesAction.java b/src/main/java/teammates/ui/webapi/SubmitFeedbackResponsesAction.java index 0f329395de0..9604462e50d 100644 --- a/src/main/java/teammates/ui/webapi/SubmitFeedbackResponsesAction.java +++ b/src/main/java/teammates/ui/webapi/SubmitFeedbackResponsesAction.java @@ -14,7 +14,6 @@ import teammates.common.datatransfer.attributes.FeedbackSessionAttributes; import teammates.common.datatransfer.attributes.InstructorAttributes; import teammates.common.datatransfer.attributes.StudentAttributes; -import teammates.common.datatransfer.questions.FeedbackQuestionType; import teammates.common.datatransfer.questions.FeedbackResponseDetails; import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; @@ -361,15 +360,6 @@ private JsonResult handleDataStoreExecute(FeedbackQuestionAttributes feedbackQue FeedbackResponsesRequest submitRequest = getAndValidateRequestBody(FeedbackResponsesRequest.class); log.info(JsonUtils.toCompactJson(submitRequest)); - if (isSingleRecipientSubmission) { - // only keep the response for the recipient when the request is a single-recipient submission - List responseRequests = submitRequest.getResponses(); - submitRequest.setResponses( - responseRequests.stream() - .filter(r -> recipientId.equals(r.getRecipient())) - .collect(Collectors.toList())); - } - for (String recipient : submitRequest.getRecipients()) { if (!recipientsOfTheQuestion.containsKey(recipient)) { throw new InvalidOperationException( @@ -435,23 +425,17 @@ private JsonResult handleDataStoreExecute(FeedbackQuestionAttributes feedbackQue .validateResponsesDetails(responseDetails, numRecipients); if (!questionSpecificErrors.isEmpty()) { - throw new InvalidHttpRequestBodyException(String.join("\n", questionSpecificErrors)); + throw new InvalidHttpRequestBodyException(questionSpecificErrors.toString()); } - if (!isSingleRecipientSubmission) { - List recipients = submitRequest.getRecipients(); - List feedbackResponsesToDelete = existingResponsesPerRecipient.entrySet().stream() - .filter(entry -> !recipients.contains(entry.getKey())) - .map(entry -> entry.getValue()) - .collect(Collectors.toList()); + List recipients = submitRequest.getRecipients(); + List feedbackResponsesToDelete = existingResponsesPerRecipient.entrySet().stream() + .filter(entry -> !recipients.contains(entry.getKey())) + .map(entry -> entry.getValue()) + .collect(Collectors.toList()); - for (FeedbackResponseAttributes feedbackResponse : feedbackResponsesToDelete) { - logic.deleteFeedbackResponseCascade(feedbackResponse.getId()); - } - } else if (submitRequest.getRecipients().isEmpty() && existingResponsesPerRecipient.containsKey(recipientId)) { - // delete a single recipient submission - FeedbackResponseAttributes feedbackResponseToDelete = existingResponsesPerRecipient.get(recipientId); - logic.deleteFeedbackResponseCascade(feedbackResponseToDelete.getId()); + for (FeedbackResponseAttributes feedbackResponse : feedbackResponsesToDelete) { + logic.deleteFeedbackResponseCascade(feedbackResponse.getId()); } List output = new ArrayList<>(); diff --git a/src/test/java/teammates/sqlui/webapi/GetCourseActionTest.java b/src/test/java/teammates/sqlui/webapi/GetCourseActionTest.java index 74cafbd1579..2f493a8bac1 100644 --- a/src/test/java/teammates/sqlui/webapi/GetCourseActionTest.java +++ b/src/test/java/teammates/sqlui/webapi/GetCourseActionTest.java @@ -167,7 +167,6 @@ void testExecute_asInstructor_success() { @Test void testExecute_asStudentHideCreatedAtAndDeletedAt_success() { Course course = new Course("course-id", "name", Const.DEFAULT_TIME_ZONE, "institute"); - course.setCreatedAt(Instant.parse("2022-01-01T00:00:00Z")); course.setCreatedAt(Instant.parse("2023-01-01T00:00:00Z")); when(mockLogic.getCourse(course.getId())).thenReturn(course); @@ -181,7 +180,7 @@ void testExecute_asStudentHideCreatedAtAndDeletedAt_success() { CourseData actionOutput = (CourseData) getJsonResult(getCourseAction).getOutput(); Course expectedCourse = course; - expectedCourse.setCreatedAt(Instant.ofEpochMilli(0)); + expectedCourse.setCreatedAt(Instant.parse("2023-01-01T00:00:00Z")); expectedCourse.setDeletedAt(Instant.ofEpochMilli(0)); assertEquals(JsonUtils.toJson(new CourseData(expectedCourse)), JsonUtils.toJson(actionOutput)); diff --git a/src/test/java/teammates/ui/webapi/GetFeedbackResponsesActionTest.java b/src/test/java/teammates/ui/webapi/GetFeedbackResponsesActionTest.java index cef1ec33611..2ed15c8de65 100644 --- a/src/test/java/teammates/ui/webapi/GetFeedbackResponsesActionTest.java +++ b/src/test/java/teammates/ui/webapi/GetFeedbackResponsesActionTest.java @@ -145,30 +145,6 @@ protected void testExecute_commentSubmission_shouldGetCommentsSuccessfully() thr verifyFeedbackCommentEquals(comment1FromStudent1, actualResponses.get(0).getGiverComment()); } - @Test - protected void testExecute_getTextQuestionType_shouldGetCommentsSuccessfully() throws Exception { - DataBundle dataBundle = loadDataBundle("/FeedbackResponseCommentCRUDTest.json"); - removeAndRestoreDataBundle(dataBundle); - - StudentAttributes student1InCourse1 = dataBundle.students.get("student1InCourse1"); - FeedbackQuestionAttributes qn7InSession1 = dataBundle.feedbackQuestions.get("qn7InSession1"); - FeedbackResponseAttributes response1ForQ7 = dataBundle.feedbackResponses.get("response1ForQ7"); - FeedbackResponseCommentAttributes comment3FromStudent1 = - dataBundle.feedbackResponseComments.get("comment3FromStudent1"); - - loginAsStudent(student1InCourse1.getGoogleId()); - String[] params = { - Const.ParamsNames.FEEDBACK_QUESTION_ID, qn7InSession1.getId(), - Const.ParamsNames.INTENT, Intent.STUDENT_SUBMISSION.toString(), - }; - FeedbackResponsesData actualData = getFeedbackResponse(params); - List actualResponses = actualData.getResponses(); - - assertEquals(1, actualResponses.size()); - verifyFeedbackResponseEquals(response1ForQ7, actualResponses.get(0)); - verifyFeedbackCommentEquals(comment3FromStudent1, actualResponses.get(0).getGiverComment()); - } - @Test @Override protected void testAccessControl() { diff --git a/src/test/java/teammates/ui/webapi/GetFeedbackSessionActionTest.java b/src/test/java/teammates/ui/webapi/GetFeedbackSessionActionTest.java index 769a9d8809c..94ccaff5134 100644 --- a/src/test/java/teammates/ui/webapi/GetFeedbackSessionActionTest.java +++ b/src/test/java/teammates/ui/webapi/GetFeedbackSessionActionTest.java @@ -1257,7 +1257,7 @@ protected void testAccessControl_instructorResult() throws Exception { ______TS("Only instructor with correct privilege can access"); verifyOnlyInstructorsOfTheSameCourseWithCorrectCoursePrivilegeCanAccess( - Const.InstructorPermissions.CAN_VIEW_SESSION_IN_SECTIONS, params + Const.InstructorPermissions.CAN_SUBMIT_SESSION_IN_SECTIONS, params ); ______TS("Instructor moderates instructor submission with correct privilege will pass"); @@ -1268,21 +1268,13 @@ protected void testAccessControl_instructorResult() throws Exception { verifyOnlyInstructorsOfTheSameCourseWithCorrectCoursePrivilegeCanAccess( Const.InstructorPermissions.CAN_MODIFY_SESSION_COMMENT_IN_SECTIONS, params); - ______TS("Instructor previews instructor submission with correct privilege will pass"); + ______TS("Instructor preview instructor result with correct privilege will pass"); String[] previewInstructorSubmissionParams = generateParameters(feedbackSession, Intent.INSTRUCTOR_SUBMISSION, "", "", instructor1OfCourse1.getEmail()); verifyOnlyInstructorsOfTheSameCourseWithCorrectCoursePrivilegeCanAccess( Const.InstructorPermissions.CAN_MODIFY_SESSION, previewInstructorSubmissionParams); - - ______TS("Instructor previews instructor result with correct privilege will pass"); - - String[] previewInstructorResultParams = - generateParameters(feedbackSession, Intent.INSTRUCTOR_RESULT, - "", "", instructor1OfCourse1.getEmail()); - verifyOnlyInstructorsOfTheSameCourseWithCorrectCoursePrivilegeCanAccess( - Const.InstructorPermissions.CAN_MODIFY_SESSION, previewInstructorResultParams); } private String[] generateParameters(FeedbackSessionAttributes session, Intent intent,