[#13066] Combine Admin Add Account Request Flow into Instructor Account Request Flow #13124

Andy-W-Developer · 2024-05-30T14:40:54Z

Outline of Solution
The "Add Instructor" and "Add Instructors" buttons now create account requests and pushes them to accountReqs for account-request-table to manage.

Removed the results table, some related functions and tests.
Changed remaining tests to reflect the changes.

Adding, removing and editing is now handled by account-request-table.

new_account_request_test.mp4

…account-requests

nusoss-bot · 2024-07-02T14:56:36Z

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

nusoss-bot · 2024-07-10T15:59:43Z

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

nusoss-bot · 2024-07-17T16:01:04Z

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

nusoss-bot · 2024-07-19T16:28:36Z

Folks, This PR seems to be stalling (no activities for the past 9 days). 🐌 😢
Hope someone can get it to move forward again soon...

nusoss-bot · 2024-07-28T03:05:34Z

Folks, This PR seems to be stalling (no activities for the past 8 days). 🐌 😢
Hope someone can get it to move forward again soon...

mingyuanc

LGTM

mingyuanc · 2024-07-30T16:17:18Z

@Andy-W-Developer is the PR done? or isit still on going. Your current changes LGTM

Andy-W-Developer · 2024-07-30T23:46:19Z

@mingyuanc Yes this PR is done, if there's anything else that could be added, let me know, thanks.

domoberzin

Sorry for getting to this so late, thanks a lot of taking this issue on, it definitely isn't a small one to handle. I think some changes need to be made, please review the comments and let us know if you need any assistance

domoberzin · 2024-07-31T15:56:48Z

src/e2e/java/teammates/e2e/cases/sql/AdminHomePageE2ETest.java

@@ -34,16 +34,6 @@ public void testAll() {

        homePage.queueInstructorForAdding(singleLineDetails);

-        homePage.addAllInstructors();


As part of our migration to SQL, please keep the E2E tests the same as its non-SQL counterpart as much as possible. The logic here does not represent the same logic in src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java

Both tests use the same logic now, the page reload was removed because fetchAccountRequests() makes it unnecessary.

domoberzin · 2024-07-31T16:00:10Z

src/web/app/pages-admin/admin-home-page/admin-home-page.component.ts

+        instructorInstitution: instructorDetailSplit[2],
+      };
+
+      this.accountService.createAccountRequest(requestData)


This batch processing needs better error handling, this implementation essentially ignores any errors. I understand that this is for the first input form. Perhaps a better way to do this is push back to the invalid lines upon error and show an error toast to indicate that the creation of certain requests. Note that requests may fail for different reasons too (duplicate email, invalid email etc.)

It now on error, pushes the line to invalidLines, rejoins invalidLines then shows an error toast.

domoberzin · 2024-07-31T16:04:22Z

src/e2e/java/teammates/e2e/pageobjects/AdminHomePage.java

    }

-    public String getMessageForInstructor(int i) {
-        By by = By.id("message-instructor-" + i);
+    public String getToastTextContent() {


There should be a method (i think its named verifyStatusMessage) in the superclass AppPage to verify the toast contents. I'd suggest using that method as it has a retry mechanism

Switched over to verifyStatusMessage, thanks.

domoberzin · 2024-07-31T16:06:22Z

src/web/app/pages-admin/admin-home-page/admin-home-page.component.ts

+      this.accountService.createAccountRequest(requestData)
+      .subscribe({
+        next: () => {
+          this.fetchAccountRequests();


This makes an API call for each account request, I think you can just call this once at the end of the entire loop after all requests are processed, since this is a batch creation feature anyway

I ran into an issue where the account requests are not updated if the fetch is moved outside of the subscribe, I'm assuming because the API call is not instant - logging seemed to show that was happening.
What is the recommended way of waiting for the requests to finish before moving onto the next statement?

Moved createAccountRequest() into a promise and push to an array of promises

Moved fetchAccountRequests() into an allSettled thenable.

domoberzin · 2024-07-31T16:10:44Z

src/e2e/java/teammates/e2e/cases/AdminHomePageE2ETest.java

        assertTrue(successMessage.contains(
-                "Instructor \"AHPUiT Instrúctör WithPlusInEmail\" has been successfully created"));
-
-        String failureMessage = homePage.getMessageForInstructor(1);


Try not to remove this negative test case

I've added back an error toast for when a single line request has an invalid email under validateAndAddInstructorDetails, it works when I add a request manually but when running the e2e test, no toast is shown.
A toast made by the approve account button - under the account request table still works, any ideas why on this is happening?
https://github.com/user-attachments/assets/c985d4dd-5a51-4ca0-90e1-67ee7e48197a
I've tried rerunning the e2e tests after deleting all the postgres data and remaking all docker containers.

The issue was user error, I forgot to build the front-end files after editing.
I've added back in the negative test case with a updated error message.

Andy-W-Developer · 2024-08-06T04:35:14Z

@domoberzin Mostly ready for review.

I need help with the accountCreateRequest loop, I'm don't know how to call fetchAccountRequests and push to invalidLines only after all request have been processed.

I've tried looking into finalize but don't think it'll work unless createAccountRequest or an alternative supports a list of account requests.

A less than ideal way could be to move the account requests into a separate list and for loop then have an if statement inside createAccountRequests to check if the loop index is equal to the list length and run fetchAccountRequests there.

Let me know your thoughts, thanks.

mingyuanc · 2024-08-07T12:30:12Z

@Andy-W-Developer could you look into why the SQL tests are failing first before we review?

nusoss-bot · 2024-08-14T15:46:38Z

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

Andy-W-Developer · 2024-08-15T15:52:06Z

Sorry for the late update.

The previously failing test in InstructorNotificationsPageE2ETest might be unrelated.

I rewrote validateAndAddInstructorDetails() to use promises so that fetchAccountRequests() is only called once.

The component tests are failing because changes to instructorDetails are made inside the subscribe, and createAccountRequest() isn't mocked yet.

I'm looking into why, after mocking the observables, the code inside the subscribe doesn't seem to run or only partially run.

nusoss-bot · 2024-08-23T13:53:54Z

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

…void>

Andy-W-Developer · 2024-08-28T13:38:57Z

@mingyuanc Ready for review.
The failing test - UUID string too large under SearchAccountRequestsActionTest line 959 seems unrelated, the same tests fail under the current master branch.

nusoss-bot · 2024-09-02T03:08:57Z

Folks, This PR seems to be stalling (no activities for the past 9 days). 🐌 😢
Hope someone can get it to move forward again soon...

Andy-W-Developer added 10 commits May 30, 2024 11:56

Change admin add account flow to use instructor request flow

0b29350

Remove old flow results table

13f7add

Remove old flow functions

4f24c5e

Remove old flow variables

880278f

Remove old flow tests

86ecbaf

Change tests to new instructor request flow

b97b637

Fix formatting inconsistencies

105cb50

Add missing expects

1132790

Add test for fetchAccountRequests

63fb244

Change sample strings to shorter strings

b81a48f

weiquu added the s.Ongoing The PR is being worked on by the author(s) label Jun 10, 2024

Andy-W-Developer added 4 commits June 18, 2024 14:51

Merge branch 'master' of github.com:TEAMMATES/teammates into combine-…

0223006

…account-requests

Update AdminHomePageE2ETest

49f4ce1

Update SQL AdminHomePageE2ETest

0de6816

Remove deprecated E2E test functions

c481da4

Merge branch 'master' into combine-account-requests

35471f2

Merge branch 'master' into combine-account-requests

3e6daf8

mingyuanc approved these changes Jul 30, 2024

View reviewed changes

mingyuanc removed the s.Ongoing The PR is being worked on by the author(s) label Jul 30, 2024

domoberzin requested changes Jul 31, 2024

View reviewed changes

Andy-W-Developer added 3 commits August 3, 2024 15:15

Merge branch 'master' into combine-account-requests

1030c7e

Replace getToastTextContent with verifyStatusMessage

2dc547f

Fix inconsistent whitespacing

9e15d2f

Andy-W-Developer added 4 commits August 4, 2024 21:58

Add error handling to batch requests

66d4a0e

Add negative test case for invalid single line email

7862935

Fix mismatching logic between sql and non sql test

28e5c97

Fix linting

7a954f7

Fix api being called multiple times

3cbf5a0

Andy-W-Developer added 5 commits August 28, 2024 21:49

Fix loop array being changed inside loop

7d5feca

Change validateAndAddInstructorDetails() return from void to Promise<…

0fa111b

…void>

Add string checks before request creation

a2f3b4f

Fix break used instead of continue in for loop

266a37e

Add mock implementation for createAccountRequest

3351df8

Andy-W-Developer requested review from domoberzin and mingyuanc September 4, 2024 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#13066] Combine Admin Add Account Request Flow into Instructor Account Request Flow #13124

[#13066] Combine Admin Add Account Request Flow into Instructor Account Request Flow #13124

Andy-W-Developer commented May 30, 2024 •

edited

Loading

nusoss-bot commented Jul 2, 2024

nusoss-bot commented Jul 10, 2024

nusoss-bot commented Jul 17, 2024

nusoss-bot commented Jul 19, 2024

nusoss-bot commented Jul 28, 2024

mingyuanc left a comment

mingyuanc commented Jul 30, 2024

Andy-W-Developer commented Jul 30, 2024

domoberzin left a comment

domoberzin Jul 31, 2024

Andy-W-Developer Aug 6, 2024

domoberzin Jul 31, 2024

Andy-W-Developer Aug 6, 2024

domoberzin Jul 31, 2024

Andy-W-Developer Aug 4, 2024

domoberzin Jul 31, 2024

Andy-W-Developer Aug 4, 2024

Andy-W-Developer Aug 15, 2024

domoberzin Jul 31, 2024

Andy-W-Developer Aug 4, 2024

Andy-W-Developer Aug 6, 2024

Andy-W-Developer commented Aug 6, 2024

mingyuanc commented Aug 7, 2024

nusoss-bot commented Aug 14, 2024

Andy-W-Developer commented Aug 15, 2024

nusoss-bot commented Aug 23, 2024

Andy-W-Developer commented Aug 28, 2024

nusoss-bot commented Sep 2, 2024

		@@ -34,16 +34,6 @@ public void testAll() {

		homePage.queueInstructorForAdding(singleLineDetails);

		homePage.addAllInstructors();

[#13066] Combine Admin Add Account Request Flow into Instructor Account Request Flow #13124

Are you sure you want to change the base?

[#13066] Combine Admin Add Account Request Flow into Instructor Account Request Flow #13124

Conversation

Andy-W-Developer commented May 30, 2024 • edited Loading

nusoss-bot commented Jul 2, 2024

nusoss-bot commented Jul 10, 2024

nusoss-bot commented Jul 17, 2024

nusoss-bot commented Jul 19, 2024

nusoss-bot commented Jul 28, 2024

mingyuanc left a comment

Choose a reason for hiding this comment

mingyuanc commented Jul 30, 2024

Andy-W-Developer commented Jul 30, 2024

domoberzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andy-W-Developer commented Aug 6, 2024

mingyuanc commented Aug 7, 2024

nusoss-bot commented Aug 14, 2024

Andy-W-Developer commented Aug 15, 2024

nusoss-bot commented Aug 23, 2024

Andy-W-Developer commented Aug 28, 2024

nusoss-bot commented Sep 2, 2024

Andy-W-Developer commented May 30, 2024 •

edited

Loading