From be2edf0da840ce1a906f8ee6f7adc387e915bc11 Mon Sep 17 00:00:00 2001 From: dao ngoc hieu <53283766+daongochieu2810@users.noreply.github.com> Date: Sun, 18 Jul 2021 17:14:01 +0800 Subject: [PATCH] [#11248] Update instructor's privileges as the role is updated (#11258) * Update privileges in Action class * Refactor UpdateInstructorPrivilegeAction class and add test for UpdateInstructorActionTest * Fix lint * Move method to InstructorsLogic and add test * Update test * Fix lint * Remove space changes * Remove redundant test data * Fix lint * Add description for test cases * Add more test cases * Remove databundle code * Fix lint --- src/main/java/teammates/logic/api/Logic.java | 14 ++++++- .../logic/core/InstructorsLogic.java | 29 +++++++++++++ .../ui/webapi/UpdateInstructorAction.java | 6 +++ .../UpdateInstructorPrivilegeAction.java | 32 +-------------- .../logic/core/InstructorsLogicTest.java | 41 +++++++++++++++++++ .../ui/webapi/UpdateInstructorActionTest.java | 11 ++--- 6 files changed, 96 insertions(+), 37 deletions(-) diff --git a/src/main/java/teammates/logic/api/Logic.java b/src/main/java/teammates/logic/api/Logic.java index 1509b057638..26abac8f931 100644 --- a/src/main/java/teammates/logic/api/Logic.java +++ b/src/main/java/teammates/logic/api/Logic.java @@ -144,6 +144,19 @@ public void putInstructorDocuments(List instructors) { instructorsLogic.putDocuments(instructors); } + /** + * Update instructor being edited to ensure validity of the course. + * + * @see InstructorsLogic#updateToEnsureValidityOfInstructorsForTheCourse(String, InstructorAttributes) + */ + public void updateToEnsureValidityOfInstructorsForTheCourse(String courseId, InstructorAttributes instructorToEdit) { + + Assumption.assertNotNull(courseId); + Assumption.assertNotNull(instructorToEdit); + + instructorsLogic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToEdit); + } + /** * Preconditions:
* * All parameters are non-null. @@ -1345,5 +1358,4 @@ public boolean isStudentsInSameTeam(String courseId, String student1Email, Strin Assumption.assertNotNull(student2Email); return studentsLogic.isStudentsInSameTeam(courseId, student1Email, student2Email); } - } diff --git a/src/main/java/teammates/logic/core/InstructorsLogic.java b/src/main/java/teammates/logic/core/InstructorsLogic.java index 7c27a411648..d4b4fef8b10 100644 --- a/src/main/java/teammates/logic/core/InstructorsLogic.java +++ b/src/main/java/teammates/logic/core/InstructorsLogic.java @@ -13,6 +13,7 @@ import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; import teammates.common.util.Assumption; +import teammates.common.util.Const; import teammates.common.util.Logger; import teammates.storage.api.InstructorsDb; @@ -306,4 +307,32 @@ public void resetInstructorGoogleId(String originalEmail, String courseId) throw } } + /** + * Checks if there are any other registered instructors that can modify instructors. + * If there are none, the instructor currently being edited will be granted the privilege + * of modifying instructors automatically. + * + * @param courseId Id of the course. + * @param instructorToEdit Instructor that will be edited. + * This may be modified within the method. + */ + public void updateToEnsureValidityOfInstructorsForTheCourse(String courseId, InstructorAttributes instructorToEdit) { + List instructors = getInstructorsForCourse(courseId); + int numOfInstrCanModifyInstructor = 0; + InstructorAttributes instrWithModifyInstructorPrivilege = null; + for (InstructorAttributes instructor : instructors) { + if (instructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)) { + numOfInstrCanModifyInstructor++; + instrWithModifyInstructorPrivilege = instructor; + } + } + boolean isLastRegInstructorWithPrivilege = numOfInstrCanModifyInstructor <= 1 + && instrWithModifyInstructorPrivilege != null + && (!instrWithModifyInstructorPrivilege.isRegistered() + || instrWithModifyInstructorPrivilege.googleId + .equals(instructorToEdit.googleId)); + if (isLastRegInstructorWithPrivilege) { + instructorToEdit.privileges.updatePrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR, true); + } + } } diff --git a/src/main/java/teammates/ui/webapi/UpdateInstructorAction.java b/src/main/java/teammates/ui/webapi/UpdateInstructorAction.java index bdeed6a2d3a..31bd093dd7c 100644 --- a/src/main/java/teammates/ui/webapi/UpdateInstructorAction.java +++ b/src/main/java/teammates/ui/webapi/UpdateInstructorAction.java @@ -2,6 +2,7 @@ import org.apache.http.HttpStatus; +import teammates.common.datatransfer.InstructorPrivileges; import teammates.common.datatransfer.attributes.InstructorAttributes; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; @@ -45,6 +46,8 @@ JsonResult execute() { instructorRequest.getRoleName(), instructorRequest.getIsDisplayedToStudent(), instructorRequest.getDisplayName()); + logic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToEdit); + try { InstructorAttributes updatedInstructor; if (instructorRequest.getId() == null) { @@ -55,6 +58,7 @@ JsonResult execute() { .withDisplayedName(instructorToEdit.displayedName) .withIsDisplayedToStudents(instructorToEdit.isDisplayedToStudents) .withRole(instructorToEdit.role) + .withPrivileges(instructorToEdit.privileges) .build()); } else { updatedInstructor = logic.updateInstructorCascade( @@ -65,6 +69,7 @@ JsonResult execute() { .withDisplayedName(instructorToEdit.displayedName) .withIsDisplayedToStudents(instructorToEdit.isDisplayedToStudents) .withRole(instructorToEdit.role) + .withPrivileges(instructorToEdit.privileges) .build()); } InstructorData newInstructorData = new InstructorData(updatedInstructor); @@ -109,6 +114,7 @@ private InstructorAttributes retrieveEditedInstructor(String courseId, String in instructorToEdit.name = SanitizationHelper.sanitizeName(instructorName); instructorToEdit.email = SanitizationHelper.sanitizeEmail(instructorEmail); instructorToEdit.role = SanitizationHelper.sanitizeName(instructorRole); + instructorToEdit.privileges = new InstructorPrivileges(instructorToEdit.role); instructorToEdit.displayedName = SanitizationHelper.sanitizeName(newDisplayedName); instructorToEdit.isDisplayedToStudents = isDisplayedToStudents; diff --git a/src/main/java/teammates/ui/webapi/UpdateInstructorPrivilegeAction.java b/src/main/java/teammates/ui/webapi/UpdateInstructorPrivilegeAction.java index d1e9ccf50fe..e250e20a70f 100644 --- a/src/main/java/teammates/ui/webapi/UpdateInstructorPrivilegeAction.java +++ b/src/main/java/teammates/ui/webapi/UpdateInstructorPrivilegeAction.java @@ -1,6 +1,5 @@ package teammates.ui.webapi; -import java.util.List; import java.util.Map; import org.apache.http.HttpStatus; @@ -64,7 +63,7 @@ JsonResult execute() { } instructorToUpdate.privileges.validatePrivileges(); - updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToUpdate); + logic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToUpdate); try { instructorToUpdate = logic.updateInstructor( @@ -110,33 +109,4 @@ private void updateSessionLevelPrivileges( toUpdate.privileges.updatePrivilege(sectionName, sessionName, entry.getKey(), entry.getValue()); } } - - /** - * Checks if there are any other registered instructors that can modify instructors. - * If there are none, the instructor currently being edited will be granted the privilege - * of modifying instructors automatically. - * - * @param courseId Id of the course. - * @param instructorToEdit Instructor that will be edited. - * This may be modified within the method. - */ - private void updateToEnsureValidityOfInstructorsForTheCourse(String courseId, InstructorAttributes instructorToEdit) { - List instructors = logic.getInstructorsForCourse(courseId); - int numOfInstrCanModifyInstructor = 0; - InstructorAttributes instrWithModifyInstructorPrivilege = null; - for (InstructorAttributes instructor : instructors) { - if (instructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)) { - numOfInstrCanModifyInstructor++; - instrWithModifyInstructorPrivilege = instructor; - } - } - boolean isLastRegInstructorWithPrivilege = numOfInstrCanModifyInstructor <= 1 - && instrWithModifyInstructorPrivilege != null - && (!instrWithModifyInstructorPrivilege.isRegistered() - || instrWithModifyInstructorPrivilege.googleId - .equals(instructorToEdit.googleId)); - if (isLastRegInstructorWithPrivilege) { - instructorToEdit.privileges.updatePrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR, true); - } - } } diff --git a/src/test/java/teammates/logic/core/InstructorsLogicTest.java b/src/test/java/teammates/logic/core/InstructorsLogicTest.java index 633f8b90714..7526d320b71 100644 --- a/src/test/java/teammates/logic/core/InstructorsLogicTest.java +++ b/src/test/java/teammates/logic/core/InstructorsLogicTest.java @@ -57,6 +57,7 @@ public void testAll() throws Exception { testGetCoOwnersForCourse(); testUpdateInstructorByGoogleIdCascade(); testUpdateInstructorByEmail(); + testUpdateToEnsureValidityOfInstructorsForTheCourse(); } private void testAddInstructor() throws Exception { @@ -646,4 +647,44 @@ private void testGetCoOwnersForCourse() { && generatedCoOwnersEmails.containsAll(coOwnersEmailsFromDataBundle)); } + private void testUpdateToEnsureValidityOfInstructorsForTheCourse() { + ______TS("Should not grant the currently being edited instructor the privilege of modifying instructors"); + + ______TS("The course has more than 1 instructor with modifying instructor privilege"); + String courseId = "idOfTypicalCourse1"; + InstructorAttributes instructorToUpdate = + InstructorAttributes.builder(courseId, "idOfInstructor4@gmail.com") + .withGoogleId("idOfInstructor4") + .withPrivileges( + new InstructorPrivileges( + Const.InstructorPermissionRoleNames.INSTRUCTOR_PERMISSION_ROLE_TUTOR + ) + ).build(); + instructorsLogic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToUpdate); + + assertFalse(instructorToUpdate.privileges.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); + + ______TS("The course has 1 registered instructor with modifying instructor privilege"); + courseId = "idOfArchivedCourse"; + instructorsLogic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToUpdate); + + assertFalse(instructorToUpdate.privileges.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); + + ______TS("Should grant the currently being edited instructor the privilege of modifying instructors"); + + ______TS("The course only has 1 instructor with modifying instructor privilege which is being edited"); + courseId = "idOfCourseNoEvals"; + instructorsLogic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToUpdate); + + assertTrue(instructorToUpdate.privileges.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); + + ______TS("The course only has 1 instructor with modifying instructor privilege which is not registered"); + instructorToUpdate.privileges.updatePrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR, false); + courseId = "idOfSampleCourse-demo"; + instructorsLogic.deleteInstructorCascade(courseId, "iwosc@yahoo.tmt"); + instructorsLogic.updateToEnsureValidityOfInstructorsForTheCourse(courseId, instructorToUpdate); + + assertTrue(instructorToUpdate.privileges.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); + } + } diff --git a/src/test/java/teammates/ui/webapi/UpdateInstructorActionTest.java b/src/test/java/teammates/ui/webapi/UpdateInstructorActionTest.java index 19d03a3ff25..11991a2099a 100644 --- a/src/test/java/teammates/ui/webapi/UpdateInstructorActionTest.java +++ b/src/test/java/teammates/ui/webapi/UpdateInstructorActionTest.java @@ -45,9 +45,10 @@ protected void testExecute() { String newInstructorName = "newName"; String newInstructorEmail = "newEmail@email.com"; + String newInstructorRole = Const.InstructorPermissionRoleNames.INSTRUCTOR_PERMISSION_ROLE_TUTOR; InstructorCreateRequest reqBody = new InstructorCreateRequest(instructorId, newInstructorName, - newInstructorEmail, instructorToEdit.role, + newInstructorEmail, newInstructorRole, instructorDisplayName, instructorToEdit.isDisplayedToStudents); UpdateInstructorAction updateInstructorAction = getAction(reqBody, submissionParams); @@ -61,10 +62,10 @@ protected void testExecute() { assertEquals(newInstructorName, response.getName()); assertEquals(newInstructorEmail, editedInstructor.email); assertEquals(newInstructorEmail, response.getEmail()); - assertTrue(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_COURSE)); - assertTrue(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); - assertTrue(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_SESSION)); - assertTrue(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_STUDENT)); + assertFalse(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_COURSE)); + assertFalse(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); + assertFalse(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_SESSION)); + assertFalse(editedInstructor.isAllowedForPrivilege(Const.InstructorPermissions.CAN_MODIFY_STUDENT)); ______TS("Failure case: edit failed due to invalid parameters");