[#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
TEAMMATES · Jul 18, 2021 · be2edf0 · be2edf0
1 parent 524f88c
commit be2edf0
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 37 deletions.
diff --git 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<InstructorAttributes> 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: <br>
      * * 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
@@ -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<InstructorAttributes> 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
@@ -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
@@ -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<InstructorAttributes> 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
@@ -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, "[email protected]")
+                        .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, "[email protected]");
+        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
@@ -45,9 +45,10 @@ protected void testExecute() {
 
         String newInstructorName = "newName";
         String newInstructorEmail = "[email protected]";
+        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");