[TEAMMATES#11655] Devise a better way to handle duplicate account req…

…uests (TEAMMATES#11657)
kaixin-hc · Mar 31, 2022 · cafe194 · cafe194
1 parent 489106f
commit cafe194
Show file tree

Hide file tree

Showing 39 changed files with 1,317 additions and 54 deletions.
diff --git a/src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java b/src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java
@@ -2,6 +2,7 @@
 
 import org.testng.annotations.Test;
 
+import teammates.common.datatransfer.attributes.AccountRequestAttributes;
 import teammates.common.util.AppUrl;
 import teammates.common.util.Const;
 import teammates.e2e.pageobjects.AdminHomePage;
@@ -13,7 +14,8 @@ public class AdminHomePageE2ETest extends BaseE2ETestCase {
 
     @Override
     protected void prepareTestData() {
-        // no test data used in this test
+        testData = loadDataBundle("/AdminHomePageE2ETest.json");
+        removeAndRestoreDataBundle(testData);
     }
 
     @Test
@@ -22,6 +24,8 @@ public void testAll() {
         AppUrl url = createFrontendUrl(Const.WebPageURIs.ADMIN_HOME_PAGE);
         AdminHomePage homePage = loginAdminToPage(url, AdminHomePage.class);
 
+        ______TS("Test adding instructors with both valid and invalid details");
+
         String name = "AHPUiT Instrúctör WithPlusInEmail";
         String email = "[email protected]";
         String institute = "TEAMMATES Test Institute 1";
@@ -44,6 +48,28 @@ public void testAll() {
 
         assertNotNull(BACKDOOR.getAccountRequest(email, institute));
         BACKDOOR.deleteAccountRequest(email, institute);
+
+        ______TS("Failure case: Instructor is already registered");
+        AccountRequestAttributes registeredAccountRequest = testData.accountRequests.get("AHome.instructor1OfCourse1");
+        homePage.queueInstructorForAdding(registeredAccountRequest.getName(),
+                registeredAccountRequest.getEmail(), registeredAccountRequest.getInstitute());
+
+        homePage.addAllInstructors();
+
+        failureMessage = homePage.getMessageForInstructor(2);
+        assertTrue(failureMessage.contains("Cannot create account request as instructor has already registered."));
+
+        ______TS("Success case: Reset account request");
+
+        homePage.clickMoreInfoButtonForRegisteredInstructor(2);
+        homePage.clickResetAccountRequestLink();
+
+        successMessage = homePage.getMessageForInstructor(2);
+        assertTrue(successMessage.contains(
+                "Instructor \"" + registeredAccountRequest.getName() + "\" has been successfully created"));
+
+        assertNull(BACKDOOR.getAccountRequest(
+                registeredAccountRequest.getEmail(), registeredAccountRequest.getInstitute()).getRegisteredAt());
     }
 
 }
diff --git a/src/e2e/java/teammates/e2e/cases/AdminSearchPageE2ETest.java b/src/e2e/java/teammates/e2e/cases/AdminSearchPageE2ETest.java
@@ -123,6 +123,14 @@ public void testAll() {
         ______TS("Typical case: Expand and collapse links");
         searchPage.verifyLinkExpansionButtons(student, instructor, accountRequest);
 
+        ______TS("Typical case: Reset account request successful");
+        searchContent = accountRequest.getEmail();
+        searchPage.clearSearchBox();
+        searchPage.inputSearchContent(searchContent);
+        searchPage.clickSearchButton();
+        searchPage.clickResetAccountRequestButton(accountRequest);
+        assertNull(BACKDOOR.getAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute()).getRegisteredAt());
+
         ______TS("Typical case: Delete account request successful");
         accountRequest = testData.accountRequests.get("unregisteredInstructor1");
         searchContent = accountRequest.getEmail();

diff --git a/src/e2e/java/teammates/e2e/pageobjects/AdminHomePage.java b/src/e2e/java/teammates/e2e/pageobjects/AdminHomePage.java
@@ -1,9 +1,13 @@
 package teammates.e2e.pageobjects;
 
+import java.util.List;
+
 import org.openqa.selenium.By;
 import org.openqa.selenium.WebElement;
 import org.openqa.selenium.support.FindBy;
 
+import teammates.test.ThreadHelper;
+
 /**
  * Represents the admin home page of the website.
  */
@@ -75,4 +79,20 @@ public String getMessageForInstructor(int i) {
         return element.getText();
     }
 
+    public void clickMoreInfoButtonForRegisteredInstructor(int i) {
+        By by = By.id("instructor-" + i + "-registered-info-button");
+        waitForElementVisibility(by);
+        WebElement element = browser.driver.findElement(by);
+        element.click();
+        waitForElementVisibility(By.className("modal-backdrop"));
+    }
+
+    public void clickResetAccountRequestLink() {
+        By by = By.id("reset-account-request-link");
+        WebElement element = browser.driver.findElement(by);
+        element.click();
+        ThreadHelper.waitFor(1000); // Modals are stacked, wait briefly to ensure confirmation modal is shown
+        List<WebElement> okButtons = browser.driver.findElements(By.className("modal-btn-ok"));
+        clickDismissModalButtonAndWaitForModalHidden(okButtons.get(1)); // Second modal is confirmation modal
+    }
 }
diff --git a/src/e2e/java/teammates/e2e/pageobjects/AdminSearchPage.java b/src/e2e/java/teammates/e2e/pageobjects/AdminSearchPage.java
@@ -301,6 +301,14 @@ public void clickDeleteAccountRequestButton(AccountRequestAttributes accountRequ
         waitForPageToLoad();
     }
 
+    public void clickResetAccountRequestButton(AccountRequestAttributes accountRequest) {
+        WebElement accountRequestRow = getAccountRequestRow(accountRequest);
+        WebElement deleteButton = accountRequestRow.findElement(By.cssSelector("[id^='reset-account-request-']"));
+        deleteButton.click();
+        waitForConfirmationModalAndClickOk();
+        waitForPageToLoad();
+    }
+
     public int getNumExpandedRows(WebElement row) {
         String xpath = "following-sibling::tr[1]/td/ul/li";
         return row.findElements(By.xpath(xpath)).size();

diff --git a/src/e2e/resources/data/AdminHomePageE2ETest.json b/src/e2e/resources/data/AdminHomePageE2ETest.json
@@ -0,0 +1,55 @@
+{
+  "accounts": {
+    "AHome.instructor1OfCourse1": {
+      "googleId": "tm.e2e.AHome.inst1",
+      "name": "Teammates Instr1",
+      "isInstructor": true,
+      "email": "[email protected]",
+      "institute": "TEAMMATES Test Institute 1"
+    }
+  },
+  "accountRequests": {
+    "AHome.instructor1OfCourse1": {
+      "name": "Teammates Instr1",
+      "email": "[email protected]",
+      "institute": "TEAMMATES Test Institute 1",
+      "createdAt": "2011-01-01T00:00:00Z",
+      "registeredAt": "1970-02-14T00:00:00Z"
+    }
+  },
+  "courses": {
+    "AHome.typicalCourse1": {
+      "createdAt": "2012-03-20T23:59:00Z",
+      "id": "tm.e2e.AHome.course1",
+      "name": "Typical Course 1",
+      "institute": "TEAMMATES Test Institute 1",
+      "timeZone": "Africa/Johannesburg"
+    }
+  },
+  "instructors": {
+    "AHome.instructor1OfCourse1": {
+      "googleId": "tm.e2e.AHome.inst1",
+      "courseId": "tm.e2e.AHome.course1",
+      "name": "Teammates Instr1",
+      "email": "[email protected]",
+      "isArchived": false,
+      "role": "Co-owner",
+      "isDisplayedToStudents": true,
+      "displayedName": "Instructor",
+      "privileges": {
+        "courseLevel": {
+          "canModifyCourse": true,
+          "canModifyInstructor": true,
+          "canModifySession": true,
+          "canModifyStudent": true,
+          "canViewStudentInSections": true,
+          "canViewSessionInSections": true,
+          "canSubmitSessionInSections": true,
+          "canModifySessionCommentsInSections": true
+        },
+        "sectionLevel": {},
+        "sessionLevel": {}
+      }
+    }
+  }
+}
diff --git a/src/main/java/teammates/common/datatransfer/attributes/AccountRequestAttributes.java b/src/main/java/teammates/common/datatransfer/attributes/AccountRequestAttributes.java
@@ -249,8 +249,6 @@ private abstract static class BasicBuilder<T, B extends BasicBuilder<T, B>> {
         }
 
         public B withRegisteredAt(Instant registeredAt) {
-            assert registeredAt != null;
-
             updateOptions.registeredAtOption = UpdateOption.of(registeredAt);
             return thisBuilder;
         }

diff --git a/src/main/java/teammates/common/util/Const.java b/src/main/java/teammates/common/util/Const.java
@@ -150,6 +150,7 @@ public static class ParamsNames {
         public static final String USER_CAPTCHA_RESPONSE = "captcharesponse";
 
         public static final String EMAIL_TYPE = "emailtype";
+        public static final String USER_EMAIL = "useremail";
 
         public static final String ENTITY_TYPE = "entitytype";
 
@@ -305,6 +306,8 @@ public static class ResourceURIs {
         public static final String ACCOUNT_RESET = URI_PREFIX + "/account/reset";
         public static final String ACCOUNT_DOWNGRADE = URI_PREFIX + "/account/downgrade";
         public static final String ACCOUNT_REQUEST = URI_PREFIX + "/account/request";
+        public static final String ACCOUNT_REQUEST_RESET = ACCOUNT_REQUEST + "/reset";
+        public static final String ACCOUNTS = URI_PREFIX + "/accounts";
         public static final String RESPONSE_COMMENT = URI_PREFIX + "/responsecomment";
         public static final String COURSE = URI_PREFIX + "/course";
         public static final String COURSE_ARCHIVE = URI_PREFIX + "/course/archive";

diff --git a/src/main/java/teammates/logic/api/Logic.java b/src/main/java/teammates/logic/api/Logic.java
@@ -76,6 +76,18 @@ public AccountAttributes getAccount(String googleId) {
         return accountsLogic.getAccount(googleId);
     }
 
+    /**
+     * Returns a list of accounts with email matching {@code email}.
+     *
+     * <br/> Preconditions: <br/>
+     * * All parameters are non-null.
+     */
+    public List<AccountAttributes> getAccountsForEmail(String email) {
+        assert email != null;
+
+        return accountsLogic.getAccountsForEmail(email);
+    }
+
     public String getCourseInstitute(String courseId) {
         return coursesLogic.getCourseInstitute(courseId);
     }

diff --git a/src/main/java/teammates/logic/core/AccountsLogic.java b/src/main/java/teammates/logic/core/AccountsLogic.java
@@ -62,6 +62,13 @@ public AccountAttributes getAccount(String googleId) {
         return accountsDb.getAccount(googleId);
     }
 
+    /**
+     * Returns a list of accounts with email matching {@code email}.
+     */
+    public List<AccountAttributes> getAccountsForEmail(String email) {
+        return accountsDb.getAccountsForEmail(email);
+    }
+
     /**
      * Returns true if the given account exists and is an instructor.
      */

diff --git a/src/main/java/teammates/storage/api/AccountsDb.java b/src/main/java/teammates/storage/api/AccountsDb.java
@@ -2,6 +2,8 @@
 
 import static com.googlecode.objectify.ObjectifyService.ofy;
 
+import java.util.List;
+
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.cmd.LoadType;
 
@@ -37,6 +39,17 @@ public AccountAttributes getAccount(String googleId) {
         return googleId.isEmpty() ? null : makeAttributesOrNull(getAccountEntity(googleId));
     }
 
+    /**
+     * Returns a list of accounts with email matching {@code email}.
+     */
+    public List<AccountAttributes> getAccountsForEmail(String email) {
+        assert email != null;
+
+        List<Account> accounts = load().filter("email =", email).list();
+
+        return makeAttributes(accounts);
+    }
+
     /**
      * Updates an account with {@link AccountAttributes.UpdateOptions}.
      *

diff --git a/src/main/java/teammates/ui/constants/ResourceEndpoints.java b/src/main/java/teammates/ui/constants/ResourceEndpoints.java
@@ -16,6 +16,8 @@ public enum ResourceEndpoints {
     ACCOUNT_RESET(ResourceURIs.ACCOUNT_RESET),
     ACCOUNT_DOWNGRADE(ResourceURIs.ACCOUNT_DOWNGRADE),
     ACCOUNT_REQUEST(ResourceURIs.ACCOUNT_REQUEST),
+    ACCOUNT_REQUEST_RESET(ResourceURIs.ACCOUNT_REQUEST_RESET),
+    ACCOUNTS(ResourceURIs.ACCOUNTS),
     RESPONSE_COMMENT(ResourceURIs.RESPONSE_COMMENT),
     COURSE(ResourceURIs.COURSE),
     COURSE_ARCHIVE(ResourceURIs.COURSE_ARCHIVE),

diff --git a/src/main/java/teammates/ui/output/AccountsData.java b/src/main/java/teammates/ui/output/AccountsData.java
@@ -0,0 +1,23 @@
+package teammates.ui.output;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import teammates.common.datatransfer.attributes.AccountAttributes;
+
+/**
+ * The API output format of a list of accounts.
+ */
+public class AccountsData extends ApiOutput {
+
+    private List<AccountData> accounts;
+
+    public AccountsData(List<AccountAttributes> accountAttributes) {
+        this.accounts = accountAttributes.stream().map(AccountData::new).collect(Collectors.toList());
+    }
+
+    public List<AccountData> getAccounts() {
+        return accounts;
+    }
+
+}
diff --git a/src/main/java/teammates/ui/webapi/ActionFactory.java b/src/main/java/teammates/ui/webapi/ActionFactory.java
@@ -49,6 +49,8 @@ public final class ActionFactory {
         map(ResourceURIs.ACCOUNT_REQUEST, GET, GetAccountRequestAction.class);
         map(ResourceURIs.ACCOUNT_REQUEST, POST, CreateAccountRequestAction.class);
         map(ResourceURIs.ACCOUNT_REQUEST, DELETE, DeleteAccountRequestAction.class);
+        map(ResourceURIs.ACCOUNT_REQUEST_RESET, PUT, ResetAccountRequestAction.class);
+        map(ResourceURIs.ACCOUNTS, GET, GetAccountsAction.class);
         map(ResourceURIs.COURSE, GET, GetCourseAction.class);
         map(ResourceURIs.COURSE, DELETE, DeleteCourseAction.class);
         map(ResourceURIs.COURSE, POST, CreateCourseAction.class);

diff --git a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java
@@ -28,6 +28,8 @@ public JsonResult execute() throws InvalidHttpRequestBodyException, InvalidOpera
 
         try {
             accountRequestAttributes = logic.createAccountRequest(accountRequestToCreate);
+            // only schedule for search indexing if account request created successfully
+            taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution);
         } catch (InvalidParametersException ipe) {
             throw new InvalidHttpRequestBodyException(ipe);
         } catch (EntityAlreadyExistsException eaee) {
@@ -37,14 +39,16 @@ public JsonResult execute() throws InvalidHttpRequestBodyException, InvalidOpera
 
         assert accountRequestAttributes != null;
 
+        if (accountRequestAttributes.getRegisteredAt() != null) {
+            throw new InvalidOperationException("Cannot create account request as instructor has already registered.");
+        }
+
         String joinLink = accountRequestAttributes.getRegistrationUrl();
 
         EmailWrapper email = emailGenerator.generateNewInstructorAccountJoinEmail(
                 instructorEmail, instructorName, joinLink);
         emailSender.sendEmail(email);
 
-        taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution);
-
         JoinLinkData output = new JoinLinkData(joinLink);
         return new JsonResult(output);
     }

diff --git a/src/main/java/teammates/ui/webapi/GetAccountsAction.java b/src/main/java/teammates/ui/webapi/GetAccountsAction.java
@@ -0,0 +1,25 @@
+package teammates.ui.webapi;
+
+import java.util.List;
+
+import teammates.common.datatransfer.attributes.AccountAttributes;
+import teammates.common.util.Const;
+import teammates.common.util.SanitizationHelper;
+import teammates.ui.output.AccountsData;
+
+/**
+ * Gets all accounts with the given email.
+ */
+class GetAccountsAction extends AdminOnlyAction {
+
+    @Override
+    public JsonResult execute() {
+        String email = getNonNullRequestParamValue(Const.ParamsNames.USER_EMAIL);
+        email = SanitizationHelper.sanitizeEmail(email);
+
+        List<AccountAttributes> accounts = logic.getAccountsForEmail(email);
+
+        return new JsonResult(new AccountsData(accounts));
+    }
+
+}