From 868982f4543bddf23337006525f4b66201187670 Mon Sep 17 00:00:00 2001 From: Nicolas <25302138+NicolasCwy@users.noreply.github.com> Date: Mon, 18 Mar 2024 16:40:09 +0800 Subject: [PATCH 1/9] [#12048] V9 migration verification script optimisation - fetch ReadNotifications for account comparison (#12905) * Eager load readNotifications for accounts when verifying --------- Co-authored-by: Zhang Ziqing <69516975+ziqing26@users.noreply.github.com> --- .../scripts/sql/VerifyAccountAttributes.java | 34 +++++++++++++++++++ ...fyNonCourseEntityAttributesBaseScript.java | 32 +++++++++++++---- 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/client/java/teammates/client/scripts/sql/VerifyAccountAttributes.java b/src/client/java/teammates/client/scripts/sql/VerifyAccountAttributes.java index 358579ff66a..b2f03532968 100644 --- a/src/client/java/teammates/client/scripts/sql/VerifyAccountAttributes.java +++ b/src/client/java/teammates/client/scripts/sql/VerifyAccountAttributes.java @@ -1,11 +1,21 @@ package teammates.client.scripts.sql; +// CHECKSTYLE.OFF:ImportOrder import java.time.Instant; import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedList; import java.util.List; import java.util.Map; +import jakarta.persistence.TypedQuery; +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.JoinType; +import jakarta.persistence.criteria.Order; +import jakarta.persistence.criteria.Root; + +import teammates.common.util.HibernateUtil; import teammates.storage.entity.Account; import teammates.storage.sqlentity.ReadNotification; @@ -16,6 +26,8 @@ public class VerifyAccountAttributes extends VerifyNonCourseEntityAttributesBaseScript { + private static final String READ_NOTIFICATION_FIELD = "readNotifications"; + public VerifyAccountAttributes() { super(Account.class, teammates.storage.sqlentity.Account.class); @@ -46,6 +58,28 @@ public boolean verifyAccountFields(teammates.storage.sqlentity.Account sqlEntity } + @Override + protected List lookupSqlEntitiesByPageNumber(int pageNum) { + CriteriaBuilder cb = HibernateUtil.getCriteriaBuilder(); + CriteriaQuery pageQuery = cb.createQuery(sqlEntityClass); + + // sort by id to maintain stable order. + Root root = pageQuery.from(sqlEntityClass); + pageQuery.select(root); + List orderList = new LinkedList<>(); + orderList.add(cb.asc(root.get("id"))); + pageQuery.orderBy(orderList); + + // perform query with pagination + TypedQuery query = HibernateUtil.createQuery(pageQuery); + query.setFirstResult(calculateOffset(pageNum)); + query.setMaxResults(CONST_SQL_FETCH_BASE_SIZE); + + // Fetch read notifications eagerly with one join + root.fetch(READ_NOTIFICATION_FIELD, JoinType.LEFT); + return query.getResultList(); + } + // Used for sql data migration @Override public boolean equals(teammates.storage.sqlentity.Account sqlEntity, Account datastoreEntity) { diff --git a/src/client/java/teammates/client/scripts/sql/VerifyNonCourseEntityAttributesBaseScript.java b/src/client/java/teammates/client/scripts/sql/VerifyNonCourseEntityAttributesBaseScript.java index e4abdc63fb4..345a2248ed1 100644 --- a/src/client/java/teammates/client/scripts/sql/VerifyNonCourseEntityAttributesBaseScript.java +++ b/src/client/java/teammates/client/scripts/sql/VerifyNonCourseEntityAttributesBaseScript.java @@ -33,7 +33,10 @@ public abstract class VerifyNonCourseEntityAttributesBaseScript datastoreEntityClass; @@ -41,6 +44,8 @@ public abstract class VerifyNonCourseEntityAttributesBaseScript sqlEntityClass; + private long entitiesVerified = 0; + public VerifyNonCourseEntityAttributesBaseScript( Class datastoreEntityClass, Class sqlEntityClass) { this.datastoreEntityClass = datastoreEntityClass; @@ -77,8 +82,8 @@ protected Map lookupDataStoreEntities(List datastoreEntitiesI /** * Calculate offset. */ - private int calculateOffset(int pageNum) { - return (pageNum - 1) * constSqlFetchBaseSize; + protected int calculateOffset(int pageNum) { + return (pageNum - 1) * CONST_SQL_FETCH_BASE_SIZE; } /** @@ -89,17 +94,22 @@ private int getNumPages() { CriteriaQuery countQuery = cb.createQuery(Long.class); countQuery.select(cb.count(countQuery.from(sqlEntityClass))); long countResults = HibernateUtil.createQuery(countQuery).getSingleResult().longValue(); - int numPages = (int) (Math.ceil((double) countResults / (double) constSqlFetchBaseSize)); + int numPages = (int) (Math.ceil((double) countResults / (double) CONST_SQL_FETCH_BASE_SIZE)); log(String.format("Has %d entities with %d pages", countResults, numPages)); return numPages; } - private List lookupSqlEntitiesByPageNumber(int pageNum) { + /** + * Sort SQL entities by id in ascending order and return entities on page. + * @param pageNum page in a sorted entities tables + * @return list of SQL entities on page num + */ + protected List lookupSqlEntitiesByPageNumber(int pageNum) { CriteriaBuilder cb = HibernateUtil.getCriteriaBuilder(); CriteriaQuery pageQuery = cb.createQuery(sqlEntityClass); - // sort by createdAt to maintain stable order. + // sort by id to maintain stable order. Root root = pageQuery.from(sqlEntityClass); pageQuery.select(root); List orderList = new LinkedList<>(); @@ -109,7 +119,7 @@ private List lookupSqlEntitiesByPageNumber(int pageNum) { // perform query with pagination TypedQuery query = HibernateUtil.createQuery(pageQuery); query.setFirstResult(calculateOffset(pageNum)); - query.setMaxResults(constSqlFetchBaseSize); + query.setMaxResults(CONST_SQL_FETCH_BASE_SIZE); return query.getResultList(); } @@ -149,15 +159,18 @@ protected List> checkAllEntitiesForFailures() { + (endTimeForDatastore - startTimeForDatastore) + " milliseconds"); long startTimeForEquals = System.currentTimeMillis(); + entitiesVerified += sqlEntities.size(); for (T sqlEntity : sqlEntities) { E datastoreEntity = datastoreEntities.get(generateID(sqlEntity)); if (datastoreEntity == null) { + entitiesVerified -= 1; failures.add(new AbstractMap.SimpleEntry(sqlEntity, null)); continue; } boolean isEqual = equals(sqlEntity, datastoreEntity); if (!isEqual) { + entitiesVerified -= 1; failures.add(new AbstractMap.SimpleEntry(sqlEntity, datastoreEntity)); continue; } @@ -175,6 +188,7 @@ protected List> checkAllEntitiesForFailures() { protected void runCheckAllEntities(Class sqlEntityClass, Class datastoreEntityClass) { HibernateUtil.beginTransaction(); + long checkStartTime = System.currentTimeMillis(); List> failedEntities = checkAllEntitiesForFailures(); System.out.println("========================================"); @@ -186,6 +200,10 @@ protected void runCheckAllEntities(Class sqlEntityClass, } else { log("No errors detected"); } + + long checkEndTime = System.currentTimeMillis(); + log("Entity took " + (checkEndTime - checkStartTime) + " milliseconds to verify"); + log("Verified " + entitiesVerified + " SQL entities successfully"); HibernateUtil.commitTransaction(); } From 2eedf1d3169dbf6c663f58b949307f5d8295a7fd Mon Sep 17 00:00:00 2001 From: Zhang Ziqing <69516975+ziqing26@users.noreply.github.com> Date: Mon, 18 Mar 2024 22:41:28 +0800 Subject: [PATCH 2/9] [#12048] Patch Usage Statistics Migration (#12889) * Patch Usage Statistics * Add fix * Remove static * Fix bug for createdAt --------- Co-authored-by: FergusMok --- .../DataMigrationEntitiesBaseScriptSql.java | 7 +++ .../DataMigrationForAccountRequestSql.java | 8 +++ .../sql/DataMigrationForNotificationSql.java | 8 +++ .../DataMigrationForUsageStatisticsSql.java | 54 +++++++++++++++++-- 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/client/java/teammates/client/scripts/sql/DataMigrationEntitiesBaseScriptSql.java b/src/client/java/teammates/client/scripts/sql/DataMigrationEntitiesBaseScriptSql.java index 88cd680f97f..c509558a1ca 100644 --- a/src/client/java/teammates/client/scripts/sql/DataMigrationEntitiesBaseScriptSql.java +++ b/src/client/java/teammates/client/scripts/sql/DataMigrationEntitiesBaseScriptSql.java @@ -91,6 +91,12 @@ public DataMigrationEntitiesBaseScriptSql() { */ protected abstract boolean isPreview(); + /** + * Sets the criterias used in {@link #isMigrationNeeded(E entity)} on whether migration is needed. + * Ran during initialization. + */ + protected abstract void setMigrationCriteria(); + /** * Checks whether data migration is needed. * @@ -181,6 +187,7 @@ private void migrateWithTrx(Key entityKey) { protected void doOperation() { log("Running " + getClass().getSimpleName() + "..."); log("Preview: " + isPreview()); + setMigrationCriteria(); Cursor cursor = readPositionOfCursorFromFile().orElse(null); if (cursor == null) { diff --git a/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java b/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java index a06d79ab72a..b42be34734e 100644 --- a/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java +++ b/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java @@ -28,6 +28,14 @@ protected boolean isPreview() { return false; } + /* + * Sets the migration criteria used in isMigrationNeeded. + */ + @Override + protected void setMigrationCriteria() { + // No migration criteria currently needed. + } + /** * Always returns true, as the migration is needed for all entities from * Datastore to CloudSQL. diff --git a/src/client/java/teammates/client/scripts/sql/DataMigrationForNotificationSql.java b/src/client/java/teammates/client/scripts/sql/DataMigrationForNotificationSql.java index 812e24d5891..87a6479c3cf 100644 --- a/src/client/java/teammates/client/scripts/sql/DataMigrationForNotificationSql.java +++ b/src/client/java/teammates/client/scripts/sql/DataMigrationForNotificationSql.java @@ -28,6 +28,14 @@ protected boolean isPreview() { return false; } + /* + * Sets the migration criteria used in isMigrationNeeded. + */ + @Override + protected void setMigrationCriteria() { + // No migration criteria currently needed. + } + @Override protected boolean isMigrationNeeded(Notification entity) { HibernateUtil.beginTransaction(); diff --git a/src/client/java/teammates/client/scripts/sql/DataMigrationForUsageStatisticsSql.java b/src/client/java/teammates/client/scripts/sql/DataMigrationForUsageStatisticsSql.java index a8868616382..91af9ebc76d 100644 --- a/src/client/java/teammates/client/scripts/sql/DataMigrationForUsageStatisticsSql.java +++ b/src/client/java/teammates/client/scripts/sql/DataMigrationForUsageStatisticsSql.java @@ -1,18 +1,30 @@ package teammates.client.scripts.sql; +// CHECKSTYLE.OFF:ImportOrder +import java.time.Instant; import java.util.UUID; import com.googlecode.objectify.cmd.Query; +import teammates.common.util.HibernateUtil; + +import jakarta.persistence.criteria.CriteriaBuilder; +import jakarta.persistence.criteria.CriteriaQuery; +import jakarta.persistence.criteria.Root; import teammates.storage.sqlentity.UsageStatistics; +// CHECKSTYLE.ON:ImportOrder /** * Data migration class for usage statistics. */ +@SuppressWarnings("PMD") public class DataMigrationForUsageStatisticsSql extends - DataMigrationEntitiesBaseScriptSql< - teammates.storage.entity.UsageStatistics, - UsageStatistics> { + DataMigrationEntitiesBaseScriptSql { + + // Runs the migration only for newly-created SQL entities since the initial migration. + private static final boolean IS_PATCHING_MIGRATION = true; + + private Instant patchingStartTime; public static void main(String[] args) { new DataMigrationForUsageStatisticsSql().doOperationRemotely(); @@ -33,11 +45,43 @@ protected boolean isPreview() { } /** - * Always returns true, as the migration is needed for all entities from Datastore to CloudSQL . + * Queries for the latest SQL entity created, so that patching will only migrate newly created Datastore entities. */ @Override + protected void setMigrationCriteria() { + if (!IS_PATCHING_MIGRATION) { + return; + } + + HibernateUtil.beginTransaction(); + CriteriaBuilder cb = HibernateUtil.getCriteriaBuilder(); + CriteriaQuery cq = cb.createQuery(Instant.class); + Root root = cq.from(UsageStatistics.class); + cq.select(cb.greatest(root.get("startTime"))); + + // If no entity found, Hibernate will return null for Instant instead of throwing NoResultException. + patchingStartTime = HibernateUtil.createQuery(cq).getSingleResult(); + HibernateUtil.commitTransaction(); + + if (patchingStartTime == null) { + System.out.println(this.getClass().getSimpleName() + " Patching enabled, but unable to find SQL entity"); + System.exit(1); + } + + System.out.println(this.getClass().getSimpleName() + " Patching migration, with time " + patchingStartTime); + } + + /** + * Always returns true, as the migration is needed for all entities from + * Datastore to CloudSQL. + */ + @SuppressWarnings("unused") + @Override protected boolean isMigrationNeeded(teammates.storage.entity.UsageStatistics entity) { - return true; + if (patchingStartTime == null) { + return true; + } + return entity.getStartTime().isAfter(patchingStartTime); } @Override From ddd6f0902efd583a922555708bc8d839905767d6 Mon Sep 17 00:00:00 2001 From: Dominic Lim <46486515+domlimm@users.noreply.github.com> Date: Tue, 19 Mar 2024 01:21:19 +0800 Subject: [PATCH 3/9] [#12048] Add deep comparison for entities in `verifyEquals` for E2E (#12892) * Add base for verifyEquals for all entities * Remove branches with missing EntityData class * Add comparison for response details JSON string * Fix lint errors * Fix lint errors * Update method to compare feedbackquestion and response details by JSON string --- .../BaseTestCaseWithSqlDatabaseAccess.java | 131 ++++++++++++++++-- 1 file changed, 122 insertions(+), 9 deletions(-) diff --git a/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java b/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java index cdde3eb8f12..0568f17aeae 100644 --- a/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java +++ b/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java @@ -3,12 +3,30 @@ import teammates.common.datatransfer.SqlDataBundle; import teammates.common.datatransfer.questions.FeedbackQuestionDetails; import teammates.common.datatransfer.questions.FeedbackResponseDetails; +import teammates.storage.sqlentity.Account; import teammates.storage.sqlentity.BaseEntity; +import teammates.storage.sqlentity.Course; +import teammates.storage.sqlentity.DeadlineExtension; import teammates.storage.sqlentity.FeedbackQuestion; import teammates.storage.sqlentity.FeedbackResponse; +import teammates.storage.sqlentity.FeedbackResponseComment; +import teammates.storage.sqlentity.FeedbackSession; +import teammates.storage.sqlentity.Instructor; +import teammates.storage.sqlentity.Notification; +import teammates.storage.sqlentity.Student; +import teammates.storage.sqlentity.UsageStatistics; +import teammates.ui.output.AccountData; import teammates.ui.output.ApiOutput; +import teammates.ui.output.CourseData; +import teammates.ui.output.DeadlineExtensionData; import teammates.ui.output.FeedbackQuestionData; +import teammates.ui.output.FeedbackResponseCommentData; import teammates.ui.output.FeedbackResponseData; +import teammates.ui.output.FeedbackSessionData; +import teammates.ui.output.InstructorData; +import teammates.ui.output.NotificationData; +import teammates.ui.output.StudentData; +import teammates.ui.output.UsageStatisticsData; /** * Base class for all test cases which are allowed to access the database. @@ -46,21 +64,116 @@ protected void verifyEquals(BaseEntity expected, ApiOutput actual) { FeedbackQuestion expectedQuestion = (FeedbackQuestion) expected; FeedbackQuestionDetails expectedQuestionDetails = expectedQuestion.getQuestionDetailsCopy(); FeedbackQuestionData actualQuestion = (FeedbackQuestionData) actual; + FeedbackQuestionDetails actualQuestionDetails = actualQuestion.getQuestionDetails(); assertEquals(expectedQuestion.getQuestionNumber(), (Integer) actualQuestion.getQuestionNumber()); - assertEquals(expectedQuestionDetails.getQuestionText(), actualQuestion.getQuestionBrief()); assertEquals(expectedQuestion.getDescription(), actualQuestion.getQuestionDescription()); - assertEquals(expectedQuestionDetails.getQuestionType(), actualQuestion.getQuestionType()); assertEquals(expectedQuestion.getGiverType(), actualQuestion.getGiverType()); assertEquals(expectedQuestion.getRecipientType(), actualQuestion.getRecipientType()); - // TODO: compare the rest of the attributes D: + assertEquals(expectedQuestion.getNumOfEntitiesToGiveFeedbackTo(), + actualQuestion.getCustomNumberOfEntitiesToGiveFeedbackTo()); + assertEquals(expectedQuestionDetails.getJsonString(), actualQuestionDetails.getJsonString()); } else if (expected instanceof FeedbackResponse) { - FeedbackResponse expectedResponse = (FeedbackResponse) expected; - FeedbackResponseDetails expectedResponseDetails = expectedResponse.getFeedbackResponseDetailsCopy(); + FeedbackResponse expectedFeedbackResponse = (FeedbackResponse) expected; + FeedbackResponseDetails expectedResponseDetails = + expectedFeedbackResponse.getFeedbackResponseDetailsCopy(); FeedbackResponseData actualResponse = (FeedbackResponseData) actual; - assertEquals(expectedResponse.getGiver(), actualResponse.getGiverIdentifier()); - assertEquals(expectedResponse.getRecipient(), actualResponse.getRecipientIdentifier()); - assertEquals(expectedResponseDetails.getAnswerString(), actualResponse.getResponseDetails().getAnswerString()); - // TODO: compare the rest of the attributes D: + FeedbackResponseDetails actualResponseDetails = actualResponse.getResponseDetails(); + assertEquals(expectedFeedbackResponse.getGiver(), actualResponse.getGiverIdentifier()); + assertEquals(expectedFeedbackResponse.getRecipient(), actualResponse.getRecipientIdentifier()); + assertEquals(expectedResponseDetails.getAnswerString(), + actualResponse.getResponseDetails().getAnswerString()); + assertEquals(expectedResponseDetails.getQuestionType(), + actualResponse.getResponseDetails().getQuestionType()); + assertEquals(expectedResponseDetails.getJsonString(), actualResponseDetails.getJsonString()); + } else if (expected instanceof Account) { + Account expectedAccount = (Account) expected; + AccountData actualAccount = (AccountData) actual; + assertEquals(expectedAccount.getGoogleId(), actualAccount.getGoogleId()); + assertEquals(expectedAccount.getName(), actualAccount.getName()); + assertEquals(expectedAccount.getEmail(), actualAccount.getEmail()); + } else if (expected instanceof Course) { + Course expectedCourse = (Course) expected; + CourseData actualCourse = (CourseData) actual; + assertEquals(expectedCourse.getName(), actualCourse.getCourseName()); + assertEquals(expectedCourse.getTimeZone(), actualCourse.getTimeZone()); + assertEquals(expectedCourse.getInstitute(), actualCourse.getInstitute()); + } else if (expected instanceof DeadlineExtension) { + DeadlineExtension expectedDeadlineExtension = (DeadlineExtension) expected; + DeadlineExtensionData actualDeadlineExtension = (DeadlineExtensionData) actual; + assertEquals(expectedDeadlineExtension.getEndTime().toEpochMilli(), actualDeadlineExtension.getEndTime()); + assertEquals(expectedDeadlineExtension.isClosingSoonEmailSent(), + actualDeadlineExtension.getSentClosingEmail()); + } else if (expected instanceof FeedbackResponseComment) { + FeedbackResponseComment expectedFeedbackResponseComment = (FeedbackResponseComment) expected; + FeedbackResponseCommentData actualComment = (FeedbackResponseCommentData) actual; + assertEquals(expectedFeedbackResponseComment.getGiver(), actualComment.getCommentGiver()); + assertEquals(expectedFeedbackResponseComment.getCommentText(), actualComment.getCommentText()); + assertEquals(expectedFeedbackResponseComment.getIsVisibilityFollowingFeedbackQuestion(), + actualComment.isVisibilityFollowingFeedbackQuestion()); + assertEquals(expectedFeedbackResponseComment.getLastEditorEmail(), actualComment.getLastEditorEmail()); + } else if (expected instanceof FeedbackSession) { + FeedbackSession expectedFeedbackSession = (FeedbackSession) expected; + FeedbackSessionData actualFeedbackSession = (FeedbackSessionData) actual; + assertEquals(expectedFeedbackSession.getName(), actualFeedbackSession.getFeedbackSessionName()); + assertEquals(expectedFeedbackSession.getInstructions(), actualFeedbackSession.getInstructions()); + assertEquals(expectedFeedbackSession.getStartTime().toEpochMilli(), + actualFeedbackSession.getSubmissionStartTimestamp()); + assertEquals(expectedFeedbackSession.getEndTime().toEpochMilli(), + actualFeedbackSession.getSubmissionEndTimestamp()); + assertEquals(expectedFeedbackSession.getSessionVisibleFromTime().toEpochMilli(), + actualFeedbackSession.getSessionVisibleFromTimestamp().longValue()); + assertEquals(expectedFeedbackSession.getResultsVisibleFromTime().toEpochMilli(), + actualFeedbackSession.getResultVisibleFromTimestamp().longValue()); + assertEquals(expectedFeedbackSession.getGracePeriod().toMinutes(), + actualFeedbackSession.getGracePeriod().longValue()); + assertEquals(expectedFeedbackSession.isClosingEmailEnabled(), + actualFeedbackSession.getIsClosingEmailEnabled()); + assertEquals(expectedFeedbackSession.isPublishedEmailEnabled(), + actualFeedbackSession.getIsPublishedEmailEnabled()); + } else if (expected instanceof Instructor) { + Instructor expectedInstructor = (Instructor) expected; + InstructorData actualInstructor = (InstructorData) actual; + assertEquals(expectedInstructor.getCourseId(), actualInstructor.getCourseId()); + assertEquals(expectedInstructor.getName(), actualInstructor.getName()); + assertEquals(expectedInstructor.getEmail(), actualInstructor.getEmail()); + assertEquals(expectedInstructor.getRegKey(), actualInstructor.getKey()); + assertEquals(expectedInstructor.isDisplayedToStudents(), actualInstructor.getIsDisplayedToStudents()); + assertEquals(expectedInstructor.getDisplayName(), actualInstructor.getDisplayedToStudentsAs()); + assertEquals(expectedInstructor.getRole(), actualInstructor.getRole()); + } else if (expected instanceof Notification) { + Notification expectedNotification = (Notification) expected; + NotificationData actualNotification = (NotificationData) actual; + assertEquals(expectedNotification.getStartTime().toEpochMilli(), actualNotification.getStartTimestamp()); + assertEquals(expectedNotification.getEndTime().toEpochMilli(), actualNotification.getEndTimestamp()); + assertEquals(expectedNotification.getStyle(), actualNotification.getStyle()); + assertEquals(expectedNotification.getTargetUser(), actualNotification.getTargetUser()); + assertEquals(expectedNotification.getTitle(), actualNotification.getTitle()); + assertEquals(expectedNotification.getMessage(), actualNotification.getMessage()); + assertEquals(expectedNotification.isShown(), actualNotification.isShown()); + } else if (expected instanceof Student) { + Student expectedStudent = (Student) expected; + StudentData actualStudent = (StudentData) actual; + assertEquals(expectedStudent.getCourseId(), actualStudent.getCourseId()); + assertEquals(expectedStudent.getName(), actualStudent.getName()); + assertEquals(expectedStudent.getEmail(), actualStudent.getEmail()); + assertEquals(expectedStudent.getRegKey(), actualStudent.getKey()); + assertEquals(expectedStudent.getComments(), actualStudent.getComments()); + // TODO: A student might not have a team or section. + // assertEquals(expectedStudent.getTeamName(), actualStudent.getTeamName()); + // assertEquals(expectedStudent.getSectionName(), actualStudent.getSectionName()); + } else if (expected instanceof UsageStatistics) { + UsageStatistics expectedUsageStatistics = (UsageStatistics) expected; + UsageStatisticsData actualUsageStatistics = (UsageStatisticsData) actual; + assertEquals(expectedUsageStatistics.getStartTime().toEpochMilli(), actualUsageStatistics.getStartTime()); + assertEquals(expectedUsageStatistics.getTimePeriod(), actualUsageStatistics.getTimePeriod()); + assertEquals(expectedUsageStatistics.getNumResponses(), actualUsageStatistics.getNumResponses()); + assertEquals(expectedUsageStatistics.getNumCourses(), actualUsageStatistics.getNumCourses()); + assertEquals(expectedUsageStatistics.getNumStudents(), actualUsageStatistics.getNumStudents()); + assertEquals(expectedUsageStatistics.getNumInstructors(), actualUsageStatistics.getNumInstructors()); + assertEquals(expectedUsageStatistics.getNumAccountRequests(), + actualUsageStatistics.getNumAccountRequests()); + assertEquals(expectedUsageStatistics.getNumEmails(), actualUsageStatistics.getNumEmails()); + assertEquals(expectedUsageStatistics.getNumSubmissions(), actualUsageStatistics.getNumSubmissions()); } else { fail("Unknown entity"); } From a34c3c57b22456de526d5af9a0317d502f44907a Mon Sep 17 00:00:00 2001 From: yuanxi1 <52706394+yuanxi1@users.noreply.github.com> Date: Tue, 19 Mar 2024 02:06:04 +0800 Subject: [PATCH 4/9] [#12048] Migrate InstructorNotificationsPageE2E (#12906) * Migrate InstructorNotificationsPageE2E * Fix unsaved transient entity error * Fix lint * Fix lint * Update testng xml --------- Co-authored-by: YX Z Co-authored-by: YX Z Co-authored-by: Wei Qing <48304907+weiquu@users.noreply.github.com> Co-authored-by: Cedric Ong <67156011+cedricongjh@users.noreply.github.com> --- .../InstructorNotificationsPageE2ETest.java | 78 +++++++++++ ...InstructorNotificationsPageE2ESqlTest.json | 111 ++++++++++++++++ src/e2e/resources/testng-e2e-sql.xml | 1 + .../sqllogic/core/DataBundleLogic.java | 124 ++++++++++++++++++ 4 files changed, 314 insertions(+) create mode 100644 src/e2e/java/teammates/e2e/cases/sql/InstructorNotificationsPageE2ETest.java create mode 100644 src/e2e/resources/data/InstructorNotificationsPageE2ESqlTest.json diff --git a/src/e2e/java/teammates/e2e/cases/sql/InstructorNotificationsPageE2ETest.java b/src/e2e/java/teammates/e2e/cases/sql/InstructorNotificationsPageE2ETest.java new file mode 100644 index 00000000000..9a8d84989c7 --- /dev/null +++ b/src/e2e/java/teammates/e2e/cases/sql/InstructorNotificationsPageE2ETest.java @@ -0,0 +1,78 @@ +package teammates.e2e.cases.sql; + +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.testng.annotations.AfterClass; +import org.testng.annotations.Test; + +import teammates.common.util.AppUrl; +import teammates.common.util.Const; +import teammates.e2e.pageobjects.InstructorNotificationsPage; +import teammates.storage.sqlentity.Account; +import teammates.storage.sqlentity.Notification; +import teammates.ui.output.AccountData; + +/** + * SUT: {@link Const.WebPageURIs#INSTRUCTOR_NOTIFICATIONS_PAGE}. + */ +public class InstructorNotificationsPageE2ETest extends BaseE2ETestCase { + + @Override + protected void prepareTestData() { + testData = loadSqlDataBundle("/InstructorNotificationsPageE2ESqlTest.json"); + removeAndRestoreDataBundle(testData); + } + + @Test + @Override + protected void testAll() { + Account account = testData.accounts.get("INotifs.instr"); + AppUrl notificationsPageUrl = createFrontendUrl(Const.WebPageURIs.INSTRUCTOR_NOTIFICATIONS_PAGE); + InstructorNotificationsPage notificationsPage = loginToPage(notificationsPageUrl, InstructorNotificationsPage.class, + account.getGoogleId()); + + ______TS("verify that only active notifications with correct target user are shown"); + Notification[] notShownNotifications = { + testData.notifications.get("notification2"), + testData.notifications.get("expiredNotification1"), + }; + Notification[] shownNotifications = { + testData.notifications.get("notification1"), + testData.notifications.get("notification3"), + testData.notifications.get("notification4"), + }; + + Notification[] readNotifications = { + testData.notifications.get("notification4"), + }; + + Set readNotificationsIds = Stream.of(readNotifications) + .map(readNotification -> readNotification.getId().toString()) + .collect(Collectors.toSet()); + + notificationsPage.verifyNotShownNotifications(notShownNotifications); + notificationsPage.verifyShownNotifications(shownNotifications, readNotificationsIds); + + ______TS("mark notification as read"); + Notification notificationToMarkAsRead = testData.notifications.get("notification3"); + notificationsPage.markNotificationAsRead(notificationToMarkAsRead); + notificationsPage.verifyStatusMessage("Notification marked as read."); + + // Verify that account's readNotifications attribute is updated + AccountData accountFromDb = BACKDOOR.getAccountData(account.getGoogleId()); + assertTrue(accountFromDb.getReadNotifications().containsKey(notificationToMarkAsRead.getId().toString())); + + ______TS("notification banner is not visible"); + assertFalse(notificationsPage.isBannerVisible()); + } + + @AfterClass + public void classTeardown() { + for (Notification notification : testData.notifications.values()) { + BACKDOOR.deleteNotification(notification.getId()); + } + } + +} diff --git a/src/e2e/resources/data/InstructorNotificationsPageE2ESqlTest.json b/src/e2e/resources/data/InstructorNotificationsPageE2ESqlTest.json new file mode 100644 index 00000000000..52521c06d2d --- /dev/null +++ b/src/e2e/resources/data/InstructorNotificationsPageE2ESqlTest.json @@ -0,0 +1,111 @@ +{ + "accounts": { + "INotifs.instr": { + "id": "00000000-0000-4000-8000-000000000001", + "googleId": "tm.e2e.INotifs.instr", + "name": "Teammates Test", + "email": "INotifs.instr@gmail.tmt" + } + }, + "courses": { + "typicalCourse1": { + "id": "tm.e2e.INotifs.course1", + "name": "Typical Course 1", + "institute": "TEAMMATES Test Institute 1", + "timeZone": "Africa/Johannesburg" + } + }, + "instructors": { + "INotifs.instr": { + "id": "00000000-0000-4000-8000-000000100001", + "name": "Teammates Instr1", + "email": "INotifs.instr@gmail.tmt", + "role": "INSTRUCTOR_PERMISSION_ROLE_COOWNER", + "isDisplayedToStudents": true, + "displayName": "Instructor", + "privileges": { + "courseLevel": { + "canModifyCourse": true, + "canModifyInstructor": true, + "canModifySession": true, + "canModifyStudent": true, + "canViewStudentInSections": true, + "canViewSessionInSections": true, + "canSubmitSessionInSections": true, + "canModifySessionCommentsInSections": true + }, + "sectionLevel": {}, + "sessionLevel": {} + }, + "course": { + "id": "tm.e2e.INotifs.course1" + }, + "account": { + "id": "00000000-0000-4000-8000-000000000001" + } + } + }, + "notifications": { + "notification1": { + "id": "00000000-0000-4000-8000-000000001101", + "startTime": "2011-01-01T00:00:00Z", + "endTime": "2099-01-01T00:00:00Z", + "style": "DANGER", + "targetUser": "GENERAL", + "title": "INotifs.notification1", + "message": "

This notification is shown to general users

", + "shown": false + }, + "notification2": { + "id": "00000000-0000-4000-8000-000000001102", + "startTime": "2011-02-02T00:00:00Z", + "endTime": "2099-02-02T00:00:00Z", + "style": "SUCCESS", + "targetUser": "STUDENT", + "title": "INotifs.notification2", + "message": "

This notification is shown to students only

", + "shown": false + }, + "notification3": { + "id": "00000000-0000-4000-8000-000000001103", + "startTime": "2011-03-03T00:00:00Z", + "endTime": "2099-03-03T00:00:00Z", + "style": "SUCCESS", + "targetUser": "INSTRUCTOR", + "title": "INotifs.notification3", + "message": "

This notification is shown to instructors only

", + "shown": false + }, + "notification4": { + "id": "00000000-0000-4000-8000-000000001104", + "startTime": "2011-04-04T00:00:00Z", + "endTime": "2099-04-04T00:00:00Z", + "style": "WARNING", + "targetUser": "GENERAL", + "title": "INotifs.notification4", + "message": "

This notification has been read by the user

", + "shown": false + }, + "expiredNotification1": { + "id": "00000000-0000-4000-8000-000000001105", + "startTime": "2011-01-01T00:00:00Z", + "endTime": "2011-02-02T00:00:00Z", + "style": "DANGER", + "targetUser": "GENERAL", + "title": "INotifs.expiredNotification1", + "message": "

This notification has expired

", + "shown": false + } + }, + "readNotifications": { + "notification4INotifs.instr": { + "id": "00000000-0000-4000-8000-000000002100", + "account": { + "id": "00000000-0000-4000-8000-000000000001" + }, + "notification": { + "id": "00000000-0000-4000-8000-000000001104" + } + } + } + } diff --git a/src/e2e/resources/testng-e2e-sql.xml b/src/e2e/resources/testng-e2e-sql.xml index 049897961b9..ed13cbf44f1 100644 --- a/src/e2e/resources/testng-e2e-sql.xml +++ b/src/e2e/resources/testng-e2e-sql.xml @@ -8,6 +8,7 @@ + diff --git a/src/main/java/teammates/sqllogic/core/DataBundleLogic.java b/src/main/java/teammates/sqllogic/core/DataBundleLogic.java index 6b7376a7e9b..8ab165d31b7 100644 --- a/src/main/java/teammates/sqllogic/core/DataBundleLogic.java +++ b/src/main/java/teammates/sqllogic/core/DataBundleLogic.java @@ -254,6 +254,8 @@ public SqlDataBundle persistDataBundle(SqlDataBundle dataBundle) throw new InvalidParametersException("Null data bundle"); } + linkEntities(dataBundle); + Collection accounts = dataBundle.accounts.values(); Collection accountRequests = dataBundle.accountRequests.values(); Collection courses = dataBundle.courses.values(); @@ -338,6 +340,7 @@ public void removeDataBundle(SqlDataBundle dataBundle) throws InvalidParametersE throw new InvalidParametersException("Data bundle is null"); } + linkEntities(dataBundle); dataBundle.courses.values().forEach(course -> { coursesLogic.deleteCourseCascade(course.getId()); }); @@ -372,4 +375,125 @@ public void putDocuments(SqlDataBundle dataBundle) throws SearchServiceException } } + private static void linkEntities(SqlDataBundle dataBundle) { + Collection accounts = dataBundle.accounts.values(); + Collection courses = dataBundle.courses.values(); + Collection
sections = dataBundle.sections.values(); + Collection teams = dataBundle.teams.values(); + Collection instructors = dataBundle.instructors.values(); + Collection students = dataBundle.students.values(); + Collection sessions = dataBundle.feedbackSessions.values(); + Collection questions = dataBundle.feedbackQuestions.values(); + Collection responses = dataBundle.feedbackResponses.values(); + Collection responseComments = dataBundle.feedbackResponseComments.values(); + Collection deadlineExtensions = dataBundle.deadlineExtensions.values(); + Collection notifications = dataBundle.notifications.values(); + Collection readNotifications = dataBundle.readNotifications.values(); + + // Mapping of IDs or placeholder IDs to actual entity + Map coursesMap = new HashMap<>(); + Map sectionsMap = new HashMap<>(); + Map teamsMap = new HashMap<>(); + Map sessionsMap = new HashMap<>(); + Map questionMap = new HashMap<>(); + Map responseMap = new HashMap<>(); + Map accountsMap = new HashMap<>(); + Map usersMap = new HashMap<>(); + Map notificationsMap = new HashMap<>(); + + for (Course course : courses) { + coursesMap.put(course.getId(), course); + } + + for (Section section : sections) { + sectionsMap.put(section.getId(), section); + Course course = coursesMap.get(section.getCourse().getId()); + section.setCourse(course); + } + + for (Team team : teams) { + teamsMap.put(team.getId(), team); + Section section = sectionsMap.get(team.getSection().getId()); + team.setSection(section); + } + + for (FeedbackSession session : sessions) { + sessionsMap.put(session.getId(), session); + Course course = coursesMap.get(session.getCourse().getId()); + session.setCourse(course); + } + + for (FeedbackQuestion question : questions) { + questionMap.put(question.getId(), question); + FeedbackSession fs = sessionsMap.get(question.getFeedbackSession().getId()); + question.setFeedbackSession(fs); + } + + for (FeedbackResponse response : responses) { + UUID placeholderId = response.getId(); + responseMap.put(placeholderId, response); + FeedbackQuestion fq = questionMap.get(response.getFeedbackQuestion().getId()); + Section giverSection = sectionsMap.get(response.getGiverSection().getId()); + Section recipientSection = response.getRecipientSection() != null + ? sectionsMap.get(response.getRecipientSection().getId()) : null; + response.setFeedbackQuestion(fq); + response.setGiverSection(giverSection); + response.setRecipientSection(recipientSection); + } + + for (FeedbackResponseComment responseComment : responseComments) { + FeedbackResponse fr = responseMap.get(responseComment.getFeedbackResponse().getId()); + Section giverSection = sectionsMap.get(responseComment.getGiverSection().getId()); + Section recipientSection = sectionsMap.get(responseComment.getRecipientSection().getId()); + responseComment.setFeedbackResponse(fr); + responseComment.setGiverSection(giverSection); + responseComment.setRecipientSection(recipientSection); + } + + for (Account account : accounts) { + accountsMap.put(account.getId(), account); + } + + for (Instructor instructor : instructors) { + usersMap.put(instructor.getId(), instructor); + Course course = coursesMap.get(instructor.getCourse().getId()); + instructor.setCourse(course); + if (instructor.getAccount() != null) { + Account account = accountsMap.get(instructor.getAccount().getId()); + instructor.setAccount(account); + } + instructor.generateNewRegistrationKey(); + } + + for (Student student : students) { + usersMap.put(student.getId(), student); + Course course = coursesMap.get(student.getCourse().getId()); + student.setCourse(course); + Team team = teamsMap.get(student.getTeam().getId()); + student.setTeam(team); + if (student.getAccount() != null) { + Account account = accountsMap.get(student.getAccount().getId()); + student.setAccount(account); + } + student.generateNewRegistrationKey(); + } + + for (Notification notification : notifications) { + notificationsMap.put(notification.getId(), notification); + } + + for (ReadNotification readNotification : readNotifications) { + Account account = accountsMap.get(readNotification.getAccount().getId()); + readNotification.setAccount(account); + Notification notification = notificationsMap.get(readNotification.getNotification().getId()); + readNotification.setNotification(notification); + } + + for (DeadlineExtension deadlineExtension : deadlineExtensions) { + FeedbackSession session = sessionsMap.get(deadlineExtension.getFeedbackSession().getId()); + deadlineExtension.setFeedbackSession(session); + User user = usersMap.get(deadlineExtension.getUser().getId()); + deadlineExtension.setUser(user); + } + } } From 4dc0c6deff8108eba7be7d85ccdd2f770d2b1a8e Mon Sep 17 00:00:00 2001 From: DS Date: Tue, 19 Mar 2024 15:51:16 +0800 Subject: [PATCH 5/9] [#12048] Migrate FeedbackMsqQuestionE2ETest (#12904) * Migrate test * Update test * Update sql json * fix lint * Fix lint * Update json and add test to xml * Revert "Fix lint" This reverts commit f767b5233dc280290b31bc6e669b173d0befd26d. * Revert "Update sql json" This reverts commit 45744750bb262c3537d5628e9beef1289e163fb6. * Fix xml * Change to use makeDeepCopy * sort questions * fix verifyEquals method --------- Co-authored-by: Cedric Ong <67156011+cedricongjh@users.noreply.github.com> Co-authored-by: Cedric Ong --- .../cases/sql/FeedbackMsqQuestionE2ETest.java | 150 ++++++++ .../e2e/pageobjects/FeedbackSubmitPage.java | 44 +++ .../InstructorFeedbackEditPage.java | 9 + .../data/FeedbackMsqQuestionE2ESqlTest.json | 333 ++++++++++++++++++ src/e2e/resources/testng-e2e-sql.xml | 1 + .../sqllogic/core/FeedbackQuestionsLogic.java | 2 +- .../questions/FeedbackMsqQuestion.java | 2 +- .../BaseTestCaseWithSqlDatabaseAccess.java | 14 +- 8 files changed, 551 insertions(+), 4 deletions(-) create mode 100644 src/e2e/java/teammates/e2e/cases/sql/FeedbackMsqQuestionE2ETest.java create mode 100644 src/e2e/resources/data/FeedbackMsqQuestionE2ESqlTest.json diff --git a/src/e2e/java/teammates/e2e/cases/sql/FeedbackMsqQuestionE2ETest.java b/src/e2e/java/teammates/e2e/cases/sql/FeedbackMsqQuestionE2ETest.java new file mode 100644 index 00000000000..713563cdab3 --- /dev/null +++ b/src/e2e/java/teammates/e2e/cases/sql/FeedbackMsqQuestionE2ETest.java @@ -0,0 +1,150 @@ +package teammates.e2e.cases.sql; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +import org.testng.annotations.Test; + +import teammates.common.datatransfer.questions.FeedbackMsqQuestionDetails; +import teammates.common.datatransfer.questions.FeedbackMsqResponseDetails; +import teammates.common.util.Const; +import teammates.e2e.pageobjects.FeedbackSubmitPage; +import teammates.e2e.pageobjects.InstructorFeedbackEditPage; +import teammates.storage.sqlentity.FeedbackQuestion; +import teammates.storage.sqlentity.FeedbackResponse; +import teammates.storage.sqlentity.Student; + +/** + * SUT: {@link Const.WebPageURIs#INSTRUCTOR_SESSION_EDIT_PAGE}, + * {@link Const.WebPageURIs#SESSION_SUBMISSION_PAGE} + * specifically for msq questions. + */ +public class FeedbackMsqQuestionE2ETest extends BaseFeedbackQuestionE2ETest { + @Override + protected void prepareTestData() { + testData = removeAndRestoreDataBundle(loadSqlDataBundle("/FeedbackMsqQuestionE2ESqlTest.json")); + + instructor = testData.instructors.get("instructor"); + course = testData.courses.get("course"); + feedbackSession = testData.feedbackSessions.get("openSession"); + student = testData.students.get("alice.tmms@FMsqQn.CS2104"); + } + + @Test + @Override + public void testAll() { + testEditPage(); + logout(); + testSubmitPage(); + } + + @Override + protected void testEditPage() { + InstructorFeedbackEditPage feedbackEditPage = loginToFeedbackEditPage(); + + ______TS("verify loaded question"); + FeedbackQuestion loadedQuestion = testData.feedbackQuestions.get("qn1ForFirstSession") + .makeDeepCopy(feedbackSession); + FeedbackMsqQuestionDetails questionDetails = (FeedbackMsqQuestionDetails) loadedQuestion + .getQuestionDetailsCopy(); + feedbackEditPage.verifyMsqQuestionDetails(1, questionDetails); + + ______TS("add new question"); + // add new question exactly like loaded question + loadedQuestion.setQuestionNumber(2); + feedbackEditPage.addMsqQuestion(loadedQuestion); + + feedbackEditPage.verifyMsqQuestionDetails(2, questionDetails); + verifyPresentInDatabase(loadedQuestion); + + ______TS("copy question"); + FeedbackQuestion copiedQuestion = testData.feedbackQuestions.get("qn1ForSecondSession"); + questionDetails = (FeedbackMsqQuestionDetails) copiedQuestion.getQuestionDetailsCopy(); + feedbackEditPage.copyQuestion(copiedQuestion.getCourseId(), + copiedQuestion.getQuestionDetailsCopy().getQuestionText()); + copiedQuestion.setFeedbackSession(feedbackSession); + copiedQuestion.setQuestionNumber(3); + + feedbackEditPage.verifyMsqQuestionDetails(3, questionDetails); + verifyPresentInDatabase(copiedQuestion); + + ______TS("edit question"); + questionDetails = (FeedbackMsqQuestionDetails) loadedQuestion.getQuestionDetailsCopy(); + questionDetails.setHasAssignedWeights(false); + questionDetails.setMsqWeights(new ArrayList<>()); + questionDetails.setOtherEnabled(false); + questionDetails.setMsqOtherWeight(0); + questionDetails.setMaxSelectableChoices(Const.POINTS_NO_VALUE); + List choices = questionDetails.getMsqChoices(); + choices.add("Edited choice"); + questionDetails.setMsqChoices(choices); + loadedQuestion.setQuestionDetails(questionDetails); + feedbackEditPage.editMsqQuestion(2, questionDetails); + feedbackEditPage.waitForPageToLoad(); + + feedbackEditPage.verifyMsqQuestionDetails(2, questionDetails); + verifyPresentInDatabase(loadedQuestion); + } + + @Override + protected void testSubmitPage() { + FeedbackSubmitPage feedbackSubmitPage = loginToFeedbackSubmitPage(); + + ______TS("verify loaded question"); + FeedbackQuestion question = testData.feedbackQuestions.get("qn1ForFirstSession"); + Student receiver = testData.students.get("benny.tmms@FMsqQn.CS2104"); + feedbackSubmitPage.verifyMsqQuestion(1, receiver.getName(), + (FeedbackMsqQuestionDetails) question.getQuestionDetailsCopy()); + + ______TS("verify loaded question with generated options"); + FeedbackQuestion generatedQn = testData.feedbackQuestions.get("qn1ForSecondSession"); + feedbackSubmitPage.verifyGeneratedMsqQuestion(3, "", + (FeedbackMsqQuestionDetails) generatedQn.getQuestionDetailsCopy(), getGeneratedTeams()); + + ______TS("submit response"); + List answers = Arrays.asList("Leadership", "This is the other response."); + FeedbackResponse response = getResponse(question, receiver, answers.get(answers.size() - 1), answers); + feedbackSubmitPage.fillMsqResponse(1, receiver.getName(), response); + feedbackSubmitPage.clickSubmitQuestionButton(1); + + // TODO: uncomment when SubmitFeedbackResponse is working + // verifyPresentInDatabase(response); + + // ______TS("check previous response"); + // feedbackSubmitPage = getFeedbackSubmitPage(); + // feedbackSubmitPage.verifyMsqResponse(1, receiver.getName(), response); + + // ______TS("edit response"); + // answers = Arrays.asList(""); + // response = getResponse(question, receiver, "", answers); + // feedbackSubmitPage.fillMsqResponse(1, receiver.getName(), response); + // feedbackSubmitPage.clickSubmitQuestionButton(1); + + // feedbackSubmitPage = getFeedbackSubmitPage(); + // feedbackSubmitPage.verifyMsqResponse(1, receiver.getName(), response); + // verifyPresentInDatabase(response); + } + + private List getGeneratedTeams() { + return testData.students.values().stream() + .filter(s -> s.getCourse().equals(student.getCourse())) + .map(s -> s.getTeam().getName()) + .distinct() + .collect(Collectors.toList()); + } + + private FeedbackResponse getResponse(FeedbackQuestion feedbackQuestion, Student receiver, String other, + List answers) { + FeedbackMsqResponseDetails details = new FeedbackMsqResponseDetails(); + if (!other.isEmpty()) { + details.setOther(true); + details.setOtherFieldContent(other); + } + details.setAnswers(answers); + + return FeedbackResponse.makeResponse(feedbackQuestion, student.getEmail(), student.getSection(), + receiver.getEmail(), receiver.getSection(), details); + } +} diff --git a/src/e2e/java/teammates/e2e/pageobjects/FeedbackSubmitPage.java b/src/e2e/java/teammates/e2e/pageobjects/FeedbackSubmitPage.java index 33f3215d638..21810e41525 100644 --- a/src/e2e/java/teammates/e2e/pageobjects/FeedbackSubmitPage.java +++ b/src/e2e/java/teammates/e2e/pageobjects/FeedbackSubmitPage.java @@ -290,6 +290,27 @@ public void fillMsqResponse(int qnNumber, String recipient, FeedbackResponseAttr } } + public void fillMsqResponse(int qnNumber, String recipient, FeedbackResponse response) { + FeedbackMsqResponseDetails responseDetails = (FeedbackMsqResponseDetails) response.getFeedbackResponseDetailsCopy(); + List answers = responseDetails.getAnswers(); + if (answers.get(0).isEmpty()) { + answers.add("None of the above"); + } + List optionTexts = getMsqOptions(qnNumber, recipient); + List checkboxes = getMsqCheckboxes(qnNumber, recipient); + for (int i = 0; i < optionTexts.size(); i++) { + if (answers.contains(optionTexts.get(i).getText())) { + markOptionAsSelected(checkboxes.get(i)); + } else { + markOptionAsUnselected(checkboxes.get(i)); + } + } + if (responseDetails.isOther()) { + markOptionAsSelected(getMsqOtherOptionCheckbox(qnNumber, recipient)); + fillTextBox(getMsqOtherOptionTextbox(qnNumber, recipient), responseDetails.getOtherFieldContent()); + } + } + public void verifyMsqResponse(int qnNumber, String recipient, FeedbackResponseAttributes response) { FeedbackMsqResponseDetails responseDetails = (FeedbackMsqResponseDetails) response.getResponseDetailsCopy(); List answers = responseDetails.getAnswers(); @@ -313,6 +334,29 @@ public void verifyMsqResponse(int qnNumber, String recipient, FeedbackResponseAt } } + public void verifyMsqResponse(int qnNumber, String recipient, FeedbackResponse response) { + FeedbackMsqResponseDetails responseDetails = (FeedbackMsqResponseDetails) response.getFeedbackResponseDetailsCopy(); + List answers = responseDetails.getAnswers(); + if (answers.get(0).isEmpty()) { + answers.add("None of the above"); + } + List optionTexts = getMsqOptions(qnNumber, recipient); + List checkboxes = getMsqCheckboxes(qnNumber, recipient); + for (int i = 0; i < optionTexts.size(); i++) { + if (answers.contains(optionTexts.get(i).getText())) { + assertTrue(checkboxes.get(i).isSelected()); + } else if ("Other".equals(optionTexts.get(i).getText())) { + assertEquals(checkboxes.get(i).isSelected(), responseDetails.isOther()); + } else { + assertFalse(checkboxes.get(i).isSelected()); + } + } + if (responseDetails.isOther()) { + assertEquals(getMsqOtherOptionTextbox(qnNumber, recipient).getAttribute("value"), + responseDetails.getOtherFieldContent()); + } + } + public void verifyNumScaleQuestion(int qnNumber, String recipient, FeedbackNumericalScaleQuestionDetails questionDetails) { double step = questionDetails.getStep(); diff --git a/src/e2e/java/teammates/e2e/pageobjects/InstructorFeedbackEditPage.java b/src/e2e/java/teammates/e2e/pageobjects/InstructorFeedbackEditPage.java index 0b9c3b02b37..8bcbe5a898f 100644 --- a/src/e2e/java/teammates/e2e/pageobjects/InstructorFeedbackEditPage.java +++ b/src/e2e/java/teammates/e2e/pageobjects/InstructorFeedbackEditPage.java @@ -631,6 +631,15 @@ public void addMsqQuestion(FeedbackQuestionAttributes feedbackQuestion) { clickSaveNewQuestionButton(); } + public void addMsqQuestion(FeedbackQuestion feedbackQuestion) { + addNewQuestion(4); + int questionNum = getNumQuestions(); + inputQuestionDetails(questionNum, feedbackQuestion); + FeedbackMsqQuestionDetails questionDetails = (FeedbackMsqQuestionDetails) feedbackQuestion.getQuestionDetailsCopy(); + inputMsqDetails(questionNum, questionDetails); + clickSaveNewQuestionButton(); + } + public void editMsqQuestion(int questionNum, FeedbackMsqQuestionDetails msqQuestionDetails) { clickEditQuestionButton(questionNum); inputMsqDetails(questionNum, msqQuestionDetails); diff --git a/src/e2e/resources/data/FeedbackMsqQuestionE2ESqlTest.json b/src/e2e/resources/data/FeedbackMsqQuestionE2ESqlTest.json new file mode 100644 index 00000000000..0194bd1697f --- /dev/null +++ b/src/e2e/resources/data/FeedbackMsqQuestionE2ESqlTest.json @@ -0,0 +1,333 @@ +{ + "accounts": { + "instructorWithSessions": { + "id": "00000000-0000-4000-8000-000000000001", + "googleId": "tm.e2e.FMsqQn.instructor", + "name": "Teammates Test", + "email": "tmms.test@gmail.tmt" + }, + "tm.e2e.FMsqQn.alice.tmms": { + "id": "00000000-0000-4000-8000-000000000002", + "googleId": "tm.e2e.FMsqQn.alice.tmms", + "name": "Alice Betsy", + "email": "alice.b.tmms@gmail.tmt" + } + }, + "accountRequests": {}, + "courses": { + "course": { + "id": "tm.e2e.FMsqQn.CS2104", + "name": "Programming Language Concepts", + "institute": "TEAMMATES Test Institute 1", + "timeZone": "Africa/Johannesburg" + }, + "course2": { + "id": "tm.e2e.FMsqQn.CS1101", + "name": "Programming Methodology", + "institute": "TEAMMATES Test Institute 1", + "timeZone": "Africa/Johannesburg" + } + }, + "instructors": { + "instructor": { + "id": "00000000-0000-4000-8000-000000000501", + "account": { + "id": "00000000-0000-4000-8000-000000000001" + }, + "name": "Teammates Test", + "email": "tmms.test@gmail.tmt", + "role": "INSTRUCTOR_PERMISSION_ROLE_COOWNER", + "isDisplayedToStudents": true, + "privileges": { + "courseLevel": { + "canViewStudentInSections": true, + "canSubmitSessionInSections": true, + "canModifySessionCommentsInSections": true, + "canModifyCourse": true, + "canViewSessionInSections": true, + "canModifySession": true, + "canModifyStudent": true, + "canModifyInstructor": true + }, + "sectionLevel": {}, + "sessionLevel": {} + }, + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "displayName": "Co-owner" + }, + "instructor2": { + "id": "00000000-0000-4000-8000-000000000502", + "name": "Teammates Test 2", + "email": "tmms.test2@gmail.tmt", + "role": "INSTRUCTOR_PERMISSION_ROLE_COOWNER", + "isDisplayedToStudents": true, + "privileges": { + "courseLevel": { + "canViewStudentInSections": true, + "canSubmitSessionInSections": true, + "canModifySessionCommentsInSections": true, + "canModifyCourse": true, + "canViewSessionInSections": true, + "canModifySession": true, + "canModifyStudent": true, + "canModifyInstructor": true + }, + "sectionLevel": {}, + "sessionLevel": {} + }, + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "displayName": "Co-owner" + }, + "instructor3": { + "account": { + "id": "00000000-0000-4000-8000-000000000001" + }, + "name": "Teammates Test", + "email": "tmms.test@gmail.tmt", + "role": "INSTRUCTOR_PERMISSION_ROLE_COOWNER", + "isDisplayedToStudents": true, + "privileges": { + "courseLevel": { + "canViewStudentInSections": true, + "canSubmitSessionInSections": true, + "canModifySessionCommentsInSections": true, + "canModifyCourse": true, + "canViewSessionInSections": true, + "canModifySession": true, + "canModifyStudent": true, + "canModifyInstructor": true + }, + "sectionLevel": {}, + "sessionLevel": {} + }, + "id": "00000000-0000-4000-8000-000000000503", + "course": { + "id": "tm.e2e.FMsqQn.CS1101" + }, + "displayName": "Co-owner" + } + }, + "sections": { + "ProgrammingLanguageConceptsNone": { + "id": "00000000-0000-4000-8000-000000000101", + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "name": "None" + }, + "ProgrammingMethodologysNone": { + "id": "00000000-0000-4000-8000-000000000102", + "course": { + "id": "tm.e2e.FMsqQn.CS1101" + }, + "name": "None" + } + }, + "teams": { + "ProgrammingLanguageConceptsTeam1": { + "id": "00000000-0000-4000-8000-000000000201", + "section": { + "id": "00000000-0000-4000-8000-000000000101" + }, + "name": "Team 1" + }, + "ProgrammingLanguageConceptsTeam2": { + "id": "00000000-0000-4000-8000-000000000202", + "section": { + "id": "00000000-0000-4000-8000-000000000101" + }, + "name": "Team 2" + }, + "ProgrammingMethodologyTeam1": { + "id": "00000000-0000-4000-8000-000000000203", + "section": { + "id": "00000000-0000-4000-8000-000000000102" + }, + "name": "Team 1" + } + }, + "feedbackSessions": { + "openSession": { + "creatorEmail": "tmms.test@gmail.tmt", + "instructions": "

Instructions for first session

", + "createdTime": "2012-04-01T23:59:00Z", + "startTime": "2012-04-01T22:00:00Z", + "endTime": "2026-04-30T22:00:00Z", + "sessionVisibleFromTime": "2012-04-01T22:00:00Z", + "resultsVisibleFromTime": "2026-05-01T22:00:00Z", + "timeZone": "Africa/Johannesburg", + "gracePeriod": 10, + "sentOpenEmail": false, + "sentClosingEmail": false, + "sentClosedEmail": false, + "sentPublishedEmail": false, + "isOpeningEmailEnabled": true, + "isClosingEmailEnabled": true, + "isPublishedEmailEnabled": true, + "studentDeadlines": {}, + "instructorDeadlines": {}, + "id": "00000000-0000-4000-8000-000000000701", + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "name": "First Session" + }, + "openSession2": { + "creatorEmail": "tmms.test@gmail.tmt", + "instructions": "

Instructions for Second session

", + "createdTime": "2012-04-01T23:59:00Z", + "startTime": "2012-04-01T22:00:00Z", + "endTime": "2026-04-30T22:00:00Z", + "sessionVisibleFromTime": "2012-04-01T22:00:00Z", + "resultsVisibleFromTime": "2026-05-01T22:00:00Z", + "timeZone": "Africa/Johannesburg", + "gracePeriod": 10, + "sentOpenEmail": false, + "sentClosingEmail": false, + "sentClosedEmail": false, + "sentPublishedEmail": false, + "isOpeningEmailEnabled": true, + "isClosingEmailEnabled": true, + "isPublishedEmailEnabled": true, + "studentDeadlines": {}, + "instructorDeadlines": {}, + "id": "00000000-0000-4000-8000-000000000702", + "course": { + "id": "tm.e2e.FMsqQn.CS1101" + }, + "name": "Second Session" + } + }, + "feedbackQuestions": { + "qn1ForFirstSession": { + "id": "00000000-0000-4000-8000-000000000801", + "feedbackSession": { + "id": "00000000-0000-4000-8000-000000000701" + }, + "questionDetails": { + "questionType": "MSQ", + "questionText": "Which area did this student work well in?", + "numOfMsqChoices": 3, + "msqChoices": [ + "Teamwork", + "Leadership", + "Creativity" + ], + "otherEnabled": true, + "hasAssignedWeights": true, + "msqWeights": [ + 30.61, + 29.4, + 20 + ], + "msqOtherWeight": 60, + "maxSelectableChoices": 2 + }, + "description": "

Testing description for first session

", + "questionNumber": 1, + "giverType": "STUDENTS", + "recipientType": "STUDENTS_EXCLUDING_SELF", + "numOfEntitiesToGiveFeedbackTo": 1, + "showResponsesTo": [ + "INSTRUCTORS", + "RECEIVER" + ], + "showGiverNameTo": [ + "INSTRUCTORS" + ], + "showRecipientNameTo": [ + "INSTRUCTORS", + "RECEIVER" + ] + }, + "qn1ForSecondSession": { + "id": "00000000-0000-4000-8000-000000000802", + "feedbackSession": { + "id": "00000000-0000-4000-8000-000000000702" + }, + "questionDetails": { + "numOfMsqChoices": 0, + "msqChoices": [], + "questionText": "Which teams did well in your opinion?", + "questionType": "MSQ", + "generateOptionsFor": "TEAMS", + "otherEnabled": false, + "minSelectableChoices": 2 + }, + "description": "

Testing description for second session

", + "questionNumber": 1, + "giverType": "STUDENTS", + "recipientType": "NONE", + "numOfEntitiesToGiveFeedbackTo": -100, + "showResponsesTo": [ + "INSTRUCTORS" + ], + "showGiverNameTo": [ + "INSTRUCTORS" + ], + "showRecipientNameTo": [ + "INSTRUCTORS" + ] + } + }, + "notifications": {}, + "readNotifications": {}, + "feedbackResponseComments": {}, + "students": { + "alice.tmms@FMsqQn.CS2104": { + "id": "00000000-0000-4000-8000-000000000601", + "account": { + "id": "00000000-0000-4000-8000-000000000002" + }, + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "team": { + "id": "00000000-0000-4000-8000-000000000201" + }, + "email": "alice.b.tmm1s@gmail.tmt", + "name": "Alice Betsy", + "comments": "This student's name is Alice Betsy" + }, + "benny.tmms@FMsqQn.CS2104": { + "id": "00000000-0000-4000-8000-000000000602", + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "team": { + "id": "00000000-0000-4000-8000-000000000201" + }, + "email": "benny.tmms@gmail.tmt", + "name": "Benny Charles", + "comments": "This student's name is Benny Charles" + }, + "charlie.tmms@FMsqQn.CS2104": { + "id": "00000000-0000-4000-8000-000000000603", + "course": { + "id": "tm.e2e.FMsqQn.CS2104" + }, + "team": { + "id": "00000000-0000-4000-8000-000000000202" + }, + "email": "charlie.tmms@gmail.tmt", + "name": "Charlie Davis", + "comments": "This student's name is Charlie Davis" + }, + "danny.tmms@FMsqQn.CS1101": { + "id": "00000000-0000-4000-8000-000000000603", + "course": { + "id": "tm.e2e.FMsqQn.CS1101" + }, + "team": { + "id": "00000000-0000-4000-8000-000000000203" + }, + "email": "danny.tmms@gmail.tmt", + "name": "Danny Engrid", + "comments": "This student's name is Danny Engrid" + } + } +} diff --git a/src/e2e/resources/testng-e2e-sql.xml b/src/e2e/resources/testng-e2e-sql.xml index ed13cbf44f1..9f114503b43 100644 --- a/src/e2e/resources/testng-e2e-sql.xml +++ b/src/e2e/resources/testng-e2e-sql.xml @@ -6,6 +6,7 @@ + diff --git a/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java b/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java index 8fbeec0fc36..e502e2d2189 100644 --- a/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java +++ b/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java @@ -157,7 +157,7 @@ public List getFeedbackQuestionsForStudents(FeedbackSession fe questions.addAll(fqDb.getFeedbackQuestionsForGiverType(feedbackSession, FeedbackParticipantType.STUDENTS)); questions.addAll(fqDb.getFeedbackQuestionsForGiverType(feedbackSession, FeedbackParticipantType.SELF)); - + questions.sort(null); return questions; } diff --git a/src/main/java/teammates/storage/sqlentity/questions/FeedbackMsqQuestion.java b/src/main/java/teammates/storage/sqlentity/questions/FeedbackMsqQuestion.java index 2d94f145ae2..329e194474e 100644 --- a/src/main/java/teammates/storage/sqlentity/questions/FeedbackMsqQuestion.java +++ b/src/main/java/teammates/storage/sqlentity/questions/FeedbackMsqQuestion.java @@ -51,7 +51,7 @@ public FeedbackMsqQuestion makeDeepCopy(FeedbackSession newFeedbackSession) { newFeedbackSession, this.getQuestionNumber(), this.getDescription(), this.getGiverType(), this.getRecipientType(), this.getNumOfEntitiesToGiveFeedbackTo(), new ArrayList<>(this.getShowResponsesTo()), new ArrayList<>(this.getShowGiverNameTo()), new ArrayList<>(this.getShowRecipientNameTo()), - new FeedbackMsqQuestionDetails(this.questionDetails.getQuestionText()) + this.questionDetails.getDeepCopy() ); } diff --git a/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java b/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java index 0568f17aeae..831d34a7e67 100644 --- a/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java +++ b/src/test/java/teammates/test/BaseTestCaseWithSqlDatabaseAccess.java @@ -3,6 +3,7 @@ import teammates.common.datatransfer.SqlDataBundle; import teammates.common.datatransfer.questions.FeedbackQuestionDetails; import teammates.common.datatransfer.questions.FeedbackResponseDetails; +import teammates.common.util.Const; import teammates.storage.sqlentity.Account; import teammates.storage.sqlentity.BaseEntity; import teammates.storage.sqlentity.Course; @@ -25,6 +26,7 @@ import teammates.ui.output.FeedbackSessionData; import teammates.ui.output.InstructorData; import teammates.ui.output.NotificationData; +import teammates.ui.output.NumberOfEntitiesToGiveFeedbackToSetting; import teammates.ui.output.StudentData; import teammates.ui.output.UsageStatisticsData; @@ -69,8 +71,16 @@ protected void verifyEquals(BaseEntity expected, ApiOutput actual) { assertEquals(expectedQuestion.getDescription(), actualQuestion.getQuestionDescription()); assertEquals(expectedQuestion.getGiverType(), actualQuestion.getGiverType()); assertEquals(expectedQuestion.getRecipientType(), actualQuestion.getRecipientType()); - assertEquals(expectedQuestion.getNumOfEntitiesToGiveFeedbackTo(), - actualQuestion.getCustomNumberOfEntitiesToGiveFeedbackTo()); + if (expectedQuestion.getNumOfEntitiesToGiveFeedbackTo() == Const.MAX_POSSIBLE_RECIPIENTS) { + assertEquals(actualQuestion.getNumberOfEntitiesToGiveFeedbackToSetting(), + NumberOfEntitiesToGiveFeedbackToSetting.UNLIMITED); + assertNull(actualQuestion.getCustomNumberOfEntitiesToGiveFeedbackTo()); + } else { + assertEquals(actualQuestion.getNumberOfEntitiesToGiveFeedbackToSetting(), + NumberOfEntitiesToGiveFeedbackToSetting.CUSTOM); + assertEquals(expectedQuestion.getNumOfEntitiesToGiveFeedbackTo(), + actualQuestion.getCustomNumberOfEntitiesToGiveFeedbackTo()); + } assertEquals(expectedQuestionDetails.getJsonString(), actualQuestionDetails.getJsonString()); } else if (expected instanceof FeedbackResponse) { FeedbackResponse expectedFeedbackResponse = (FeedbackResponse) expected; From d4ebcda838e1898aac2b0d2b2e943793f1c059e7 Mon Sep 17 00:00:00 2001 From: Jay Aljelo Ting <65202977+jayasting98@users.noreply.github.com> Date: Fri, 22 Mar 2024 10:56:34 +0800 Subject: [PATCH 6/9] Add account request search indexing (#12923) --- .../webapi/CreateAccountRequestActionIT.java | 60 +++++++++++++++++++ .../ui/webapi/CreateAccountRequestAction.java | 3 +- 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java diff --git a/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java b/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java new file mode 100644 index 00000000000..70aa4ecc521 --- /dev/null +++ b/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java @@ -0,0 +1,60 @@ +package teammates.it.ui.webapi; + +import org.testng.annotations.Test; + +import teammates.common.util.Const; +import teammates.common.util.EmailType; +import teammates.common.util.EmailWrapper; +import teammates.storage.sqlentity.AccountRequest; +import teammates.ui.output.JoinLinkData; +import teammates.ui.request.AccountCreateRequest; +import teammates.ui.webapi.CreateAccountRequestAction; +import teammates.ui.webapi.JsonResult; + +/** + * SUT: {@link CreateAccountRequestAction}. + */ +public class CreateAccountRequestActionIT extends BaseActionIT { + + @Override + String getActionUri() { + return Const.ResourceURIs.ACCOUNT_REQUEST; + } + + @Override + String getRequestMethod() { + return POST; + } + + @Override + @Test + protected void testExecute() throws Exception { + // This is a minimal test; other cases are not tested due to upcoming changes in behaviour. + AccountCreateRequest request = new AccountCreateRequest(); + request.setInstructorEmail("ring-bearer@fellowship.net"); + request.setInstructorName("Frodo Baggins"); + request.setInstructorInstitution("The Fellowship of the Ring"); + CreateAccountRequestAction action = getAction(request); + JsonResult result = getJsonResult(action); + JoinLinkData output = (JoinLinkData) result.getOutput(); + AccountRequest accountRequest = logic.getAccountRequest("ring-bearer@fellowship.net", "The Fellowship of the Ring"); + assertEquals("ring-bearer@fellowship.net", accountRequest.getEmail()); + assertEquals("Frodo Baggins", accountRequest.getName()); + assertEquals("The Fellowship of the Ring", accountRequest.getInstitute()); + assertNull(accountRequest.getRegisteredAt()); + assertEquals(accountRequest.getRegistrationUrl(), output.getJoinLink()); + verifyNumberOfEmailsSent(1); + verifySpecifiedTasksAdded(Const.TaskQueue.SEARCH_INDEXING_QUEUE_NAME, 1); + EmailWrapper emailSent = mockEmailSender.getEmailsSent().get(0); + assertEquals(String.format(EmailType.NEW_INSTRUCTOR_ACCOUNT.getSubject(), "Frodo Baggins"), + emailSent.getSubject()); + assertEquals("ring-bearer@fellowship.net", emailSent.getRecipient()); + assertTrue(emailSent.getContent().contains(output.getJoinLink())); + } + + @Override + protected void testAccessControl() throws Exception { + // This is not tested due to upcoming changes in behaviour. + } + +} diff --git a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java index 0022e2ff26a..27de0e8437b 100644 --- a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java +++ b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java @@ -11,7 +11,7 @@ /** * Creates a new account request. */ -class CreateAccountRequestAction extends AdminOnlyAction { +public class CreateAccountRequestAction extends AdminOnlyAction { @Override public JsonResult execute() @@ -26,6 +26,7 @@ public JsonResult execute() try { accountRequest = sqlLogic.createAccountRequest(instructorName, instructorEmail, instructorInstitution); + taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution); } catch (InvalidParametersException ipe) { throw new InvalidHttpRequestBodyException(ipe); } catch (EntityAlreadyExistsException eaee) { From 186a97a2bc8efd5d5f350eacdb9288cad2dafc64 Mon Sep 17 00:00:00 2001 From: Zhang Ziqing Date: Tue, 12 Mar 2024 09:56:35 +0800 Subject: [PATCH 7/9] Enable CI on account request form branch --- .github/workflows/axe.yml | 2 ++ .github/workflows/component.yml | 2 ++ .github/workflows/e2e-sql.yml | 2 ++ .github/workflows/e2e.yml | 2 ++ .github/workflows/jdk17.yml | 1 + .github/workflows/lnp.yml | 1 + .github/workflows/pr.yml | 1 + 7 files changed, 11 insertions(+) diff --git a/.github/workflows/axe.yml b/.github/workflows/axe.yml index 771dd8a3edc..bee66fbcfc3 100644 --- a/.github/workflows/axe.yml +++ b/.github/workflows/axe.yml @@ -5,10 +5,12 @@ on: branches: - master - release + - account-request-form pull_request: branches: - master - release + - account-request-form jobs: axe-testing: runs-on: ubuntu-latest diff --git a/.github/workflows/component.yml b/.github/workflows/component.yml index 11ecfb7357f..bb814a02c0e 100644 --- a/.github/workflows/component.yml +++ b/.github/workflows/component.yml @@ -5,10 +5,12 @@ on: branches: - master - release + - account-request-form pull_request: branches: - master - release + - account-request-form schedule: - cron: "0 0 * * *" #end of every day jobs: diff --git a/.github/workflows/e2e-sql.yml b/.github/workflows/e2e-sql.yml index 0a67cbac52d..267cd0d219f 100644 --- a/.github/workflows/e2e-sql.yml +++ b/.github/workflows/e2e-sql.yml @@ -5,10 +5,12 @@ on: branches: - master - release + - account-request-form pull_request: branches: - master - release + - account-request-form schedule: - cron: "0 0 * * *" #end of every day jobs: diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 52ab76019ed..71cfa72bd24 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -5,10 +5,12 @@ on: branches: - master - release + - account-request-form pull_request: branches: - master - release + - account-request-form schedule: - cron: "0 0 * * *" #end of every day jobs: diff --git a/.github/workflows/jdk17.yml b/.github/workflows/jdk17.yml index 29ab59bb561..f0d68910de5 100644 --- a/.github/workflows/jdk17.yml +++ b/.github/workflows/jdk17.yml @@ -4,6 +4,7 @@ on: push: branches: - master + - account-request-form jobs: lint: diff --git a/.github/workflows/lnp.yml b/.github/workflows/lnp.yml index 2ff33dda920..4389bac81ff 100644 --- a/.github/workflows/lnp.yml +++ b/.github/workflows/lnp.yml @@ -5,6 +5,7 @@ on: branches: - master - release + - account-request-form schedule: - cron: "0 0 * * *" # end of every day jobs: diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 03f11d9b6d3..f55fd672fe0 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -6,6 +6,7 @@ on: - reopened branches: - master + - account-request-form jobs: check-pr: From 395bdd7bcdffdbdbbdf474eefa96fa09270541cd Mon Sep 17 00:00:00 2001 From: Jay Ting <65202977+jayasting98@users.noreply.github.com> Date: Tue, 19 Mar 2024 02:19:52 +0800 Subject: [PATCH 8/9] [#11878] Remove AccountRequest unique constraint (#12899) * Remove AccountRequest unique constraint * Remove EntityAlreadyExistsException from the throws clause * Remove unused import of EntityAlreadyExistsException * Fix failing checks * Remove EntityAlreadyExistsException in dependents * Remove assertion that is now incorrect * Remove mysterious trailing whitespaces that appeared out of nowhere * Remove parts in E2E test that are no longer relevant * Remove unused import * Improve clarity of test case Co-authored-by: EuniceSim142 <77243938+EuniceSim142@users.noreply.github.com> --------- Co-authored-by: EuniceSim142 <77243938+EuniceSim142@users.noreply.github.com> --- .../e2e/cases/AdminHomePageE2ETest.java | 23 ------------------- .../storage/sqlapi/AccountRequestsDbIT.java | 11 +++++---- .../java/teammates/sqllogic/api/Logic.java | 2 +- .../sqllogic/core/AccountRequestsLogic.java | 6 ++--- .../storage/sqlapi/AccountRequestsDb.java | 12 +--------- .../storage/sqlentity/AccountRequest.java | 1 - .../ui/webapi/CreateAccountRequestAction.java | 4 ---- .../storage/sqlapi/AccountRequestsDbTest.java | 15 ++++-------- 8 files changed, 15 insertions(+), 59 deletions(-) diff --git a/src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java b/src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java index 6cfb6b478e0..56953689329 100644 --- a/src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java +++ b/src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java @@ -5,7 +5,6 @@ import teammates.common.util.AppUrl; import teammates.common.util.Const; import teammates.e2e.pageobjects.AdminHomePage; -import teammates.storage.sqlentity.AccountRequest; /** * SUT: {@link Const.WebPageURIs#ADMIN_HOME_PAGE}. @@ -50,28 +49,6 @@ public void testAll() { assertNotNull(BACKDOOR.getAccountRequest(email, institute)); BACKDOOR.deleteAccountRequest(email, institute); - - ______TS("Failure case: Instructor is already registered"); - AccountRequest registeredAccountRequest = sqlTestData.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/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java b/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java index 8af4c8065df..c2aa8aef078 100644 --- a/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java +++ b/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java @@ -4,7 +4,6 @@ import org.testng.annotations.Test; -import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.it.test.BaseTestCaseWithSqlDatabaseAccess; import teammates.storage.sqlapi.AccountRequestsDb; @@ -51,21 +50,23 @@ public void testCreateReadDeleteAccountRequest() throws Exception { accountRequest.getCreatedAt().minusMillis(2000)); assertEquals(0, actualAccReqCreatedAtOutside.size()); - ______TS("Create acccount request, already exists, execption thrown"); + ______TS("Create account request, same email address and institute already exist, creates successfully"); AccountRequest identicalAccountRequest = new AccountRequest("test@gmail.com", "name", "institute"); assertNotSame(accountRequest, identicalAccountRequest); - assertThrows(EntityAlreadyExistsException.class, - () -> accountRequestDb.createAccountRequest(identicalAccountRequest)); + accountRequestDb.createAccountRequest(identicalAccountRequest); + AccountRequest actualIdenticalAccountRequest = + accountRequestDb.getAccountRequestByRegistrationKey(identicalAccountRequest.getRegistrationKey()); + verifyEquals(identicalAccountRequest, actualIdenticalAccountRequest); ______TS("Delete account request that was created"); accountRequestDb.deleteAccountRequest(accountRequest); AccountRequest actualAccountRequest = - accountRequestDb.getAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute()); + accountRequestDb.getAccountRequestByRegistrationKey(accountRequest.getRegistrationKey()); assertNull(actualAccountRequest); } diff --git a/src/main/java/teammates/sqllogic/api/Logic.java b/src/main/java/teammates/sqllogic/api/Logic.java index a10fe1bae21..121257a6601 100644 --- a/src/main/java/teammates/sqllogic/api/Logic.java +++ b/src/main/java/teammates/sqllogic/api/Logic.java @@ -89,7 +89,7 @@ public static Logic inst() { * @throws EntityAlreadyExistsException if the account request already exists. */ public AccountRequest createAccountRequest(String name, String email, String institute) - throws InvalidParametersException, EntityAlreadyExistsException { + throws InvalidParametersException { return accountRequestLogic.createAccountRequest(name, email, institute); } diff --git a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java index fd7742b3a73..0465e061519 100644 --- a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java +++ b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java @@ -2,7 +2,6 @@ import java.util.List; -import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; import teammates.common.exception.SearchServiceException; @@ -51,8 +50,7 @@ public void putDocument(AccountRequest accountRequest) throws SearchServiceExcep /** * Creates an account request. */ - public AccountRequest createAccountRequest(AccountRequest accountRequest) - throws InvalidParametersException, EntityAlreadyExistsException { + public AccountRequest createAccountRequest(AccountRequest accountRequest) throws InvalidParametersException { return accountRequestDb.createAccountRequest(accountRequest); } @@ -60,7 +58,7 @@ public AccountRequest createAccountRequest(AccountRequest accountRequest) * Creates an account request. */ public AccountRequest createAccountRequest(String name, String email, String institute) - throws InvalidParametersException, EntityAlreadyExistsException { + throws InvalidParametersException { AccountRequest toCreate = new AccountRequest(email, name, institute); return accountRequestDb.createAccountRequest(toCreate); diff --git a/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java b/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java index a315cb18484..068ed3ed253 100644 --- a/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java +++ b/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java @@ -1,6 +1,5 @@ package teammates.storage.sqlapi; -import static teammates.common.util.Const.ERROR_CREATE_ENTITY_ALREADY_EXISTS; import static teammates.common.util.Const.ERROR_UPDATE_NON_EXISTENT; import java.time.Instant; @@ -9,7 +8,6 @@ import java.util.List; import java.util.UUID; -import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; import teammates.common.exception.SearchServiceException; @@ -46,20 +44,12 @@ public AccountRequestSearchManager getSearchManager() { /** * Creates an AccountRequest in the database. */ - public AccountRequest createAccountRequest(AccountRequest accountRequest) - throws InvalidParametersException, EntityAlreadyExistsException { + public AccountRequest createAccountRequest(AccountRequest accountRequest) throws InvalidParametersException { assert accountRequest != null; if (!accountRequest.isValid()) { throw new InvalidParametersException(accountRequest.getInvalidityInfo()); } - - // don't need to check registrationKey for uniqueness since it is generated using email + institute - if (getAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute()) != null) { - throw new EntityAlreadyExistsException( - String.format(ERROR_CREATE_ENTITY_ALREADY_EXISTS, accountRequest.toString())); - } - persist(accountRequest); return accountRequest; } diff --git a/src/main/java/teammates/storage/sqlentity/AccountRequest.java b/src/main/java/teammates/storage/sqlentity/AccountRequest.java index 2389fbf352d..70d74e8b910 100644 --- a/src/main/java/teammates/storage/sqlentity/AccountRequest.java +++ b/src/main/java/teammates/storage/sqlentity/AccountRequest.java @@ -27,7 +27,6 @@ @Table(name = "AccountRequests", uniqueConstraints = { @UniqueConstraint(name = "Unique registration key", columnNames = "registrationKey"), - @UniqueConstraint(name = "Unique name and institute", columnNames = {"email", "institute"}) }) public class AccountRequest extends BaseEntity { @Id diff --git a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java index 27de0e8437b..5f13178f426 100644 --- a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java +++ b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java @@ -1,6 +1,5 @@ package teammates.ui.webapi; -import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.InvalidParametersException; import teammates.common.util.EmailWrapper; import teammates.storage.sqlentity.AccountRequest; @@ -29,9 +28,6 @@ public JsonResult execute() taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution); } catch (InvalidParametersException ipe) { throw new InvalidHttpRequestBodyException(ipe); - } catch (EntityAlreadyExistsException eaee) { - // Use existing account request - accountRequest = sqlLogic.getAccountRequest(instructorEmail, instructorInstitution); } assert accountRequest != null; diff --git a/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java b/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java index ef31306aef9..9d150133689 100644 --- a/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java @@ -16,7 +16,6 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; -import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; import teammates.common.exception.SearchServiceException; @@ -49,8 +48,7 @@ public void teardownMethod() { } @Test - public void testCreateAccountRequest_accountRequestDoesNotExist_success() - throws InvalidParametersException, EntityAlreadyExistsException { + public void testCreateAccountRequest_accountRequestDoesNotExist_success() throws InvalidParametersException { AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); doReturn(null).when(accountRequestDb).getAccountRequest(anyString(), anyString()); @@ -60,16 +58,13 @@ public void testCreateAccountRequest_accountRequestDoesNotExist_success() } @Test - public void testCreateAccountRequest_accountRequestAlreadyExists_throwsEntityAlreadyExistsException() { + public void testCreateAccountRequest_accountRequestAlreadyExists_createsSuccessfully() + throws InvalidParametersException { AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); doReturn(new AccountRequest("test@gmail.com", "name", "institute")) .when(accountRequestDb).getAccountRequest(anyString(), anyString()); - - EntityAlreadyExistsException ex = assertThrows(EntityAlreadyExistsException.class, - () -> accountRequestDb.createAccountRequest(accountRequest)); - - assertEquals(ex.getMessage(), "Trying to create an entity that exists: " + accountRequest.toString()); - mockHibernateUtil.verify(() -> HibernateUtil.persist(accountRequest), never()); + accountRequestDb.createAccountRequest(accountRequest); + mockHibernateUtil.verify(() -> HibernateUtil.persist(accountRequest)); } @Test From fc1342faf82484868b839c249155658e978c81a1 Mon Sep 17 00:00:00 2001 From: Jay Ting <65202977+jayasting98@users.noreply.github.com> Date: Tue, 19 Mar 2024 04:04:04 +0800 Subject: [PATCH 9/9] [#11878] Add status and comments to AccountRequest (#12898) * Add AccountRequestStatus * Add AccountRequest status attribute * Add status to AccountRequest constructor * Add AccountRequest comments attribute * Add comments to AccountRequest constructor * Wrap lines * Remove mysterious unnecessary imports that appeared out of nowhere * Use non-null placeholder * Use literal placeholder --- .../DataMigrationForAccountRequestSql.java | 5 ++- .../sql/VerifyDataMigrationConnection.java | 5 ++- .../sqllogic/core/AccountRequestsLogicIT.java | 5 ++- .../it/sqllogic/core/DataBundleLogicIT.java | 3 +- .../storage/sqlapi/AccountRequestsDbIT.java | 36 ++++++++++++------- src/it/resources/data/DataBundleLogicIT.json | 2 ++ .../datatransfer/AccountRequestStatus.java | 27 ++++++++++++++ .../java/teammates/sqllogic/api/Logic.java | 7 ++-- .../sqllogic/core/AccountRequestsLogic.java | 7 ++-- .../storage/sqlentity/AccountRequest.java | 34 ++++++++++++++++-- .../ui/webapi/CreateAccountRequestAction.java | 6 +++- .../storage/sqlapi/AccountRequestsDbTest.java | 21 +++++++---- 12 files changed, 125 insertions(+), 33 deletions(-) create mode 100644 src/main/java/teammates/common/datatransfer/AccountRequestStatus.java diff --git a/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java b/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java index b42be34734e..dbe37b9760e 100644 --- a/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java +++ b/src/client/java/teammates/client/scripts/sql/DataMigrationForAccountRequestSql.java @@ -2,6 +2,7 @@ import com.googlecode.objectify.cmd.Query; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.storage.sqlentity.AccountRequest; /** @@ -50,7 +51,9 @@ protected void migrateEntity(teammates.storage.entity.AccountRequest oldEntity) AccountRequest newEntity = new AccountRequest( oldEntity.getEmail(), oldEntity.getName(), - oldEntity.getInstitute()); + oldEntity.getInstitute(), + AccountRequestStatus.APPROVED, + null); // set registration key to the old value if exists if (oldEntity.getRegistrationKey() != null) { diff --git a/src/client/java/teammates/client/scripts/sql/VerifyDataMigrationConnection.java b/src/client/java/teammates/client/scripts/sql/VerifyDataMigrationConnection.java index 909c8ac649e..c26147b4016 100644 --- a/src/client/java/teammates/client/scripts/sql/VerifyDataMigrationConnection.java +++ b/src/client/java/teammates/client/scripts/sql/VerifyDataMigrationConnection.java @@ -7,6 +7,7 @@ import teammates.client.connector.DatastoreClient; import teammates.client.util.ClientProperties; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.util.HibernateUtil; import teammates.storage.entity.UsageStatistics; import teammates.storage.sqlentity.Notification; @@ -43,7 +44,9 @@ protected void verifySqlConnection() { teammates.storage.sqlentity.AccountRequest newEntity = new teammates.storage.sqlentity.AccountRequest( "dummy-teammates-account-request-email@gmail.com", "dummy-teammates-account-request", - "dummy-teammates-institute"); + "dummy-teammates-institute", + AccountRequestStatus.PENDING, + "dummy-comments"); HibernateUtil.beginTransaction(); HibernateUtil.persist(newEntity); HibernateUtil.commitTransaction(); diff --git a/src/it/java/teammates/it/sqllogic/core/AccountRequestsLogicIT.java b/src/it/java/teammates/it/sqllogic/core/AccountRequestsLogicIT.java index c449f3358a0..f95a6cfa682 100644 --- a/src/it/java/teammates/it/sqllogic/core/AccountRequestsLogicIT.java +++ b/src/it/java/teammates/it/sqllogic/core/AccountRequestsLogicIT.java @@ -4,6 +4,7 @@ import org.testng.annotations.Test; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; @@ -28,8 +29,10 @@ public void testResetAccountRequest() String name = "name lee"; String email = "email@gmail.com"; String institute = "institute"; + AccountRequestStatus status = AccountRequestStatus.PENDING; + String comments = "comments"; - AccountRequest toReset = accountRequestsLogic.createAccountRequest(name, email, institute); + AccountRequest toReset = accountRequestsLogic.createAccountRequest(name, email, institute, status, comments); AccountRequestsDb accountRequestsDb = AccountRequestsDb.inst(); toReset.setRegisteredAt(Instant.now()); diff --git a/src/it/java/teammates/it/sqllogic/core/DataBundleLogicIT.java b/src/it/java/teammates/it/sqllogic/core/DataBundleLogicIT.java index f8571037632..079f95b13e7 100644 --- a/src/it/java/teammates/it/sqllogic/core/DataBundleLogicIT.java +++ b/src/it/java/teammates/it/sqllogic/core/DataBundleLogicIT.java @@ -8,6 +8,7 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.datatransfer.FeedbackParticipantType; import teammates.common.datatransfer.InstructorPermissionRole; import teammates.common.datatransfer.InstructorPrivileges; @@ -62,7 +63,7 @@ public void testCreateDataBundle_typicalValues_createdCorrectly() throws Excepti AccountRequest actualAccountRequest = dataBundle.accountRequests.get("instructor1"); AccountRequest expectedAccountRequest = new AccountRequest("instr1@teammates.tmt", "Instructor 1", - "TEAMMATES Test Institute 1"); + "TEAMMATES Test Institute 1", AccountRequestStatus.REGISTERED, "These are some comments."); expectedAccountRequest.setId(actualAccountRequest.getId()); expectedAccountRequest.setRegisteredAt(Instant.parse("2015-02-14T00:00:00Z")); expectedAccountRequest.setRegistrationKey(actualAccountRequest.getRegistrationKey()); diff --git a/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java b/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java index c2aa8aef078..a4c25763e40 100644 --- a/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java +++ b/src/it/java/teammates/it/storage/sqlapi/AccountRequestsDbIT.java @@ -4,6 +4,7 @@ import org.testng.annotations.Test; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.exception.EntityDoesNotExistException; import teammates.it.test.BaseTestCaseWithSqlDatabaseAccess; import teammates.storage.sqlapi.AccountRequestsDb; @@ -20,7 +21,8 @@ public class AccountRequestsDbIT extends BaseTestCaseWithSqlDatabaseAccess { public void testCreateReadDeleteAccountRequest() throws Exception { ______TS("Create account request, does not exists, succeeds"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.createAccountRequest(accountRequest); ______TS("Read account request using the given email and institute"); @@ -53,7 +55,7 @@ public void testCreateReadDeleteAccountRequest() throws Exception { ______TS("Create account request, same email address and institute already exist, creates successfully"); AccountRequest identicalAccountRequest = - new AccountRequest("test@gmail.com", "name", "institute"); + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); assertNotSame(accountRequest, identicalAccountRequest); accountRequestDb.createAccountRequest(identicalAccountRequest); @@ -74,7 +76,8 @@ public void testCreateReadDeleteAccountRequest() throws Exception { public void testUpdateAccountRequest() throws Exception { ______TS("Update account request, does not exists, exception thrown"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); assertThrows(EntityDoesNotExistException.class, () -> accountRequestDb.updateAccountRequest(accountRequest)); @@ -96,7 +99,8 @@ public void testSqlInjectionInCreateAccountRequestEmailField() throws Exception // Attempt to use SQL commands in email field String email = "email'/**/OR/**/1=1/**/@gmail.com"; - AccountRequest accountRequest = new AccountRequest(email, "name", "institute"); + AccountRequest accountRequest = + new AccountRequest(email, "name", "institute", AccountRequestStatus.PENDING, "comments"); // The system should treat the input as a plain text string accountRequestDb.createAccountRequest(accountRequest); @@ -110,7 +114,8 @@ public void testSqlInjectionInCreateAccountRequestNameField() throws Exception { // Attempt to use SQL commands in name field String name = "name'; SELECT * FROM account_requests; --"; - AccountRequest accountRequest = new AccountRequest("test@gmail.com", name, "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", name, "institute", AccountRequestStatus.PENDING, "comments"); // The system should treat the input as a plain text string accountRequestDb.createAccountRequest(accountRequest); @@ -124,7 +129,8 @@ public void testSqlInjectionInCreateAccountRequestInstituteField() throws Except // Attempt to use SQL commands in institute field String institute = "institute'; DROP TABLE account_requests; --"; - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", institute); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", institute, AccountRequestStatus.PENDING, "comments"); // The system should treat the input as a plain text string accountRequestDb.createAccountRequest(accountRequest); @@ -136,7 +142,8 @@ public void testSqlInjectionInCreateAccountRequestInstituteField() throws Except public void testSqlInjectionInGetAccountRequest() throws Exception { ______TS("SQL Injection test in getAccountRequest"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.createAccountRequest(accountRequest); String instituteInjection = "institute'; DROP TABLE account_requests; --"; @@ -151,7 +158,8 @@ public void testSqlInjectionInGetAccountRequest() throws Exception { public void testSqlInjectionInGetAccountRequestByRegistrationKey() throws Exception { ______TS("SQL Injection test in getAccountRequestByRegistrationKey"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.createAccountRequest(accountRequest); String regKeyInjection = "regKey'; DROP TABLE account_requests; --"; @@ -166,7 +174,8 @@ public void testSqlInjectionInGetAccountRequestByRegistrationKey() throws Except public void testSqlInjectionInUpdateAccountRequest() throws Exception { ______TS("SQL Injection test in updateAccountRequest"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.createAccountRequest(accountRequest); String nameInjection = "newName'; DROP TABLE account_requests; --"; @@ -181,13 +190,15 @@ public void testSqlInjectionInUpdateAccountRequest() throws Exception { public void testSqlInjectionInDeleteAccountRequest() throws Exception { ______TS("SQL Injection test in deleteAccountRequest"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.createAccountRequest(accountRequest); String emailInjection = "email'/**/OR/**/1=1/**/@gmail.com"; String nameInjection = "name'; DROP TABLE account_requests; --"; String instituteInjection = "institute'; DROP TABLE account_requests; --"; - AccountRequest accountRequestInjection = new AccountRequest(emailInjection, nameInjection, instituteInjection); + AccountRequest accountRequestInjection = new AccountRequest(emailInjection, nameInjection, instituteInjection, + AccountRequestStatus.PENDING, "comments"); accountRequestDb.deleteAccountRequest(accountRequestInjection); AccountRequest actual = accountRequestDb.getAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute()); @@ -198,7 +209,8 @@ public void testSqlInjectionInDeleteAccountRequest() throws Exception { public void testSqlInjectionSearchAccountRequestsInWholeSystem() throws Exception { ______TS("SQL Injection test in searchAccountRequestsInWholeSystem"); - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.createAccountRequest(accountRequest); String searchInjection = "institute'; DROP TABLE account_requests; --"; diff --git a/src/it/resources/data/DataBundleLogicIT.json b/src/it/resources/data/DataBundleLogicIT.json index 49e7cdec993..371a87c963b 100644 --- a/src/it/resources/data/DataBundleLogicIT.json +++ b/src/it/resources/data/DataBundleLogicIT.json @@ -19,6 +19,8 @@ "name": "Instructor 1", "email": "instr1@teammates.tmt", "institute": "TEAMMATES Test Institute 1", + "status": "REGISTERED", + "comments": "These are some comments.", "registeredAt": "2015-02-14T00:00:00Z" } }, diff --git a/src/main/java/teammates/common/datatransfer/AccountRequestStatus.java b/src/main/java/teammates/common/datatransfer/AccountRequestStatus.java new file mode 100644 index 00000000000..db80e7cb830 --- /dev/null +++ b/src/main/java/teammates/common/datatransfer/AccountRequestStatus.java @@ -0,0 +1,27 @@ +package teammates.common.datatransfer; + +/** + * The status of an account request. + */ +public enum AccountRequestStatus { + + /** + * The account request has not yet been processed by the admin. + */ + PENDING, + + /** + * The account request has been rejected by the admin. + */ + REJECTED, + + /** + * The account request has been approved by the admin but the instructor has not created an account yet. + */ + APPROVED, + + /** + * The account request has been approved by the admin and the instructor has created an account. + */ + REGISTERED +} diff --git a/src/main/java/teammates/sqllogic/api/Logic.java b/src/main/java/teammates/sqllogic/api/Logic.java index 121257a6601..3ffa0458042 100644 --- a/src/main/java/teammates/sqllogic/api/Logic.java +++ b/src/main/java/teammates/sqllogic/api/Logic.java @@ -8,6 +8,7 @@ import javax.annotation.Nullable; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.datatransfer.FeedbackQuestionRecipient; import teammates.common.datatransfer.FeedbackResultFetchType; import teammates.common.datatransfer.NotificationStyle; @@ -88,10 +89,10 @@ public static Logic inst() { * @throws InvalidParametersException if the account request details are invalid. * @throws EntityAlreadyExistsException if the account request already exists. */ - public AccountRequest createAccountRequest(String name, String email, String institute) - throws InvalidParametersException { + public AccountRequest createAccountRequest(String name, String email, String institute, AccountRequestStatus status, + String comments) throws InvalidParametersException { - return accountRequestLogic.createAccountRequest(name, email, institute); + return accountRequestLogic.createAccountRequest(name, email, institute, status, comments); } /** diff --git a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java index 0465e061519..5766fa6c224 100644 --- a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java +++ b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java @@ -2,6 +2,7 @@ import java.util.List; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; import teammates.common.exception.SearchServiceException; @@ -57,9 +58,9 @@ public AccountRequest createAccountRequest(AccountRequest accountRequest) throws /** * Creates an account request. */ - public AccountRequest createAccountRequest(String name, String email, String institute) - throws InvalidParametersException { - AccountRequest toCreate = new AccountRequest(email, name, institute); + public AccountRequest createAccountRequest(String name, String email, String institute, AccountRequestStatus status, + String comments) throws InvalidParametersException { + AccountRequest toCreate = new AccountRequest(email, name, institute, status, comments); return accountRequestDb.createAccountRequest(toCreate); } diff --git a/src/main/java/teammates/storage/sqlentity/AccountRequest.java b/src/main/java/teammates/storage/sqlentity/AccountRequest.java index 70d74e8b910..97a65e6b468 100644 --- a/src/main/java/teammates/storage/sqlentity/AccountRequest.java +++ b/src/main/java/teammates/storage/sqlentity/AccountRequest.java @@ -9,13 +9,17 @@ import org.hibernate.annotations.UpdateTimestamp; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.util.Config; import teammates.common.util.Const; import teammates.common.util.FieldValidator; import teammates.common.util.SanitizationHelper; import teammates.common.util.StringHelper; +import jakarta.persistence.Column; import jakarta.persistence.Entity; +import jakarta.persistence.EnumType; +import jakarta.persistence.Enumerated; import jakarta.persistence.Id; import jakarta.persistence.Table; import jakarta.persistence.UniqueConstraint; @@ -40,6 +44,12 @@ public class AccountRequest extends BaseEntity { private String institute; + @Enumerated(EnumType.STRING) + private AccountRequestStatus status; + + @Column(columnDefinition = "TEXT") + private String comments; + private Instant registeredAt; @UpdateTimestamp @@ -49,11 +59,13 @@ protected AccountRequest() { // required by Hibernate } - public AccountRequest(String email, String name, String institute) { + public AccountRequest(String email, String name, String institute, AccountRequestStatus status, String comments) { this.setId(UUID.randomUUID()); this.setEmail(email); this.setName(name); this.setInstitute(institute); + this.setStatus(status); + this.setComments(comments); this.generateNewRegistrationKey(); this.setCreatedAt(Instant.now()); this.setRegisteredAt(null); @@ -128,6 +140,22 @@ public void setInstitute(String institute) { this.institute = SanitizationHelper.sanitizeTitle(institute); } + public AccountRequestStatus getStatus() { + return this.status; + } + + public void setStatus(AccountRequestStatus status) { + this.status = status; + } + + public String getComments() { + return this.comments; + } + + public void setComments(String comments) { + this.comments = comments; + } + public Instant getRegisteredAt() { return this.registeredAt; } @@ -166,8 +194,8 @@ public int hashCode() { @Override public String toString() { return "AccountRequest [id=" + id + ", registrationKey=" + registrationKey + ", name=" + name + ", email=" - + email + ", institute=" + institute + ", registeredAt=" + registeredAt + ", createdAt=" + getCreatedAt() - + ", updatedAt=" + updatedAt + "]"; + + email + ", institute=" + institute + ", status=" + status + ", comments=" + comments + + ", registeredAt=" + registeredAt + ", createdAt=" + getCreatedAt() + ", updatedAt=" + updatedAt + "]"; } public String getRegistrationUrl() { diff --git a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java index 5f13178f426..30a4e9da8a7 100644 --- a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java +++ b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java @@ -1,5 +1,6 @@ package teammates.ui.webapi; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.exception.InvalidParametersException; import teammates.common.util.EmailWrapper; import teammates.storage.sqlentity.AccountRequest; @@ -20,11 +21,14 @@ public JsonResult execute() String instructorName = createRequest.getInstructorName().trim(); String instructorEmail = createRequest.getInstructorEmail().trim(); String instructorInstitution = createRequest.getInstructorInstitution().trim(); + // TODO: This is a placeholder. It should be obtained from AccountCreateRequest, in a separate PR. + String comments = "PLACEHOLDER"; AccountRequest accountRequest; try { - accountRequest = sqlLogic.createAccountRequest(instructorName, instructorEmail, instructorInstitution); + accountRequest = sqlLogic.createAccountRequest(instructorName, instructorEmail, instructorInstitution, + AccountRequestStatus.PENDING, comments); taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution); } catch (InvalidParametersException ipe) { throw new InvalidHttpRequestBodyException(ipe); diff --git a/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java b/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java index 9d150133689..61f9f8e0d24 100644 --- a/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java @@ -16,6 +16,7 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import teammates.common.datatransfer.AccountRequestStatus; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; import teammates.common.exception.SearchServiceException; @@ -49,7 +50,8 @@ public void teardownMethod() { @Test public void testCreateAccountRequest_accountRequestDoesNotExist_success() throws InvalidParametersException { - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); doReturn(null).when(accountRequestDb).getAccountRequest(anyString(), anyString()); accountRequestDb.createAccountRequest(accountRequest); @@ -60,8 +62,9 @@ public void testCreateAccountRequest_accountRequestDoesNotExist_success() throws @Test public void testCreateAccountRequest_accountRequestAlreadyExists_createsSuccessfully() throws InvalidParametersException { - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); - doReturn(new AccountRequest("test@gmail.com", "name", "institute")) + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); + doReturn(new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments")) .when(accountRequestDb).getAccountRequest(anyString(), anyString()); accountRequestDb.createAccountRequest(accountRequest); mockHibernateUtil.verify(() -> HibernateUtil.persist(accountRequest)); @@ -69,7 +72,8 @@ public void testCreateAccountRequest_accountRequestAlreadyExists_createsSuccessf @Test public void testUpdateAccountRequest_invalidEmail_throwsInvalidParametersException() { - AccountRequest accountRequestWithInvalidEmail = new AccountRequest("testgmail.com", "name", "institute"); + AccountRequest accountRequestWithInvalidEmail = + new AccountRequest("testgmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); assertThrows(InvalidParametersException.class, () -> accountRequestDb.updateAccountRequest(accountRequestWithInvalidEmail)); @@ -79,7 +83,8 @@ public void testUpdateAccountRequest_invalidEmail_throwsInvalidParametersExcepti @Test public void testUpdateAccountRequest_accountRequestDoesNotExist_throwsEntityDoesNotExistException() { - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); doReturn(null).when(accountRequestDb).getAccountRequest(anyString(), anyString()); assertThrows(EntityDoesNotExistException.class, @@ -90,7 +95,8 @@ public void testUpdateAccountRequest_accountRequestDoesNotExist_throwsEntityDoes @Test public void testUpdateAccountRequest_success() throws InvalidParametersException, EntityDoesNotExistException { - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); doReturn(accountRequest).when(accountRequestDb).getAccountRequest(anyString(), anyString()); accountRequestDb.updateAccountRequest(accountRequest); @@ -100,7 +106,8 @@ public void testUpdateAccountRequest_success() throws InvalidParametersException @Test public void testDeleteAccountRequest_success() { - AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute"); + AccountRequest accountRequest = + new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); accountRequestDb.deleteAccountRequest(accountRequest);