[#11389] Refactor user join page operation order (#11421)

TEAMMATES · Oct 10, 2021 · 97f48df · 97f48df
1 parent f0f28a1
commit 97f48df
Show file tree

Hide file tree

Showing 13 changed files with 87 additions and 92 deletions.
diff --git a/src/e2e/java/teammates/e2e/cases/InstructorCourseJoinConfirmationPageE2ETest.java b/src/e2e/java/teammates/e2e/cases/InstructorCourseJoinConfirmationPageE2ETest.java
@@ -6,7 +6,6 @@
 import teammates.common.util.AppUrl;
 import teammates.common.util.Const;
 import teammates.e2e.pageobjects.CourseJoinConfirmationPage;
-import teammates.e2e.pageobjects.ErrorReportingModal;
 import teammates.e2e.pageobjects.InstructorHomePage;
 
 /**
@@ -32,17 +31,19 @@ public void testAll() {
         AppUrl joinLink = createUrl(Const.WebPageURIs.JOIN_PAGE)
                 .withRegistrationKey(invalidKey)
                 .withEntityType(Const.EntityType.INSTRUCTOR);
-        ErrorReportingModal errorPage = loginToPage(joinLink, ErrorReportingModal.class, newInstructor.getGoogleId());
+        CourseJoinConfirmationPage confirmationPage = loginToPage(
+                joinLink, CourseJoinConfirmationPage.class, newInstructor.getGoogleId());
 
-        errorPage.verifyErrorMessage("No instructor with given registration key: " + invalidKey);
+        confirmationPage.verifyDisplayedMessage("The course join link is invalid. You may have "
+                + "entered the URL incorrectly or the URL may correspond to a/an instructor that does not exist.");
 
         ______TS("Click join link: valid key");
         String courseId = testData.courses.get("ICJoinConf.CS1101").getId();
         String instructorEmail = newInstructor.getEmail();
         joinLink = createUrl(Const.WebPageURIs.JOIN_PAGE)
                 .withRegistrationKey(getKeyForInstructor(courseId, instructorEmail))
                 .withEntityType(Const.EntityType.INSTRUCTOR);
-        CourseJoinConfirmationPage confirmationPage = getNewPageInstance(joinLink, CourseJoinConfirmationPage.class);
+        confirmationPage = getNewPageInstance(joinLink, CourseJoinConfirmationPage.class);
 
         confirmationPage.verifyJoiningUser(newInstructor.getGoogleId());
         confirmationPage.confirmJoinCourse(InstructorHomePage.class);

diff --git a/src/e2e/java/teammates/e2e/cases/StudentCourseJoinConfirmationPageE2ETest.java b/src/e2e/java/teammates/e2e/cases/StudentCourseJoinConfirmationPageE2ETest.java
@@ -6,7 +6,6 @@
 import teammates.common.util.AppUrl;
 import teammates.common.util.Const;
 import teammates.e2e.pageobjects.CourseJoinConfirmationPage;
-import teammates.e2e.pageobjects.ErrorReportingModal;
 import teammates.e2e.pageobjects.StudentHomePage;
 
 /**
@@ -34,16 +33,18 @@ public void testAll() {
                 .withRegistrationKey(invalidKey)
                 .withCourseId(courseId)
                 .withEntityType(Const.EntityType.STUDENT);
-        ErrorReportingModal errorPage = loginToPage(joinLink, ErrorReportingModal.class, newStudent.getGoogleId());
+        CourseJoinConfirmationPage confirmationPage = loginToPage(
+                joinLink, CourseJoinConfirmationPage.class, newStudent.getGoogleId());
 
-        errorPage.verifyErrorMessage("No student with given registration key: " + invalidKey);
+        confirmationPage.verifyDisplayedMessage("The course join link is invalid. You may have "
+                + "entered the URL incorrectly or the URL may correspond to a/an student that does not exist.");
 
         ______TS("Click join link: valid key");
         joinLink = createUrl(Const.WebPageURIs.JOIN_PAGE)
                 .withRegistrationKey(getKeyForStudent(newStudent))
                 .withCourseId(courseId)
                 .withEntityType(Const.EntityType.STUDENT);
-        CourseJoinConfirmationPage confirmationPage = getNewPageInstance(joinLink, CourseJoinConfirmationPage.class);
+        confirmationPage = getNewPageInstance(joinLink, CourseJoinConfirmationPage.class);
 
         confirmationPage.verifyJoiningUser(newStudent.getGoogleId());
         confirmationPage.confirmJoinCourse(StudentHomePage.class);

diff --git a/src/e2e/java/teammates/e2e/pageobjects/CourseJoinConfirmationPage.java b/src/e2e/java/teammates/e2e/pageobjects/CourseJoinConfirmationPage.java
@@ -19,7 +19,8 @@ public CourseJoinConfirmationPage(Browser browser) {
 
     @Override
     public boolean containsExpectedPageContents() {
-        return waitForElementPresence(By.tagName("h3")).getText().contains("Confirm your Google account");
+        // This page has no unique indicator as the content depends on whether it follows the happy path or not
+        return true;
     }
 
     public void verifyJoiningUser(String googleId) {
@@ -31,4 +32,8 @@ public <T extends AppPage> T confirmJoinCourse(Class<T> typeOfPage) {
         waitForPageToLoad();
         return changePageType(typeOfPage);
     }
+
+    public void verifyDisplayedMessage(String message) {
+        assertEquals(browser.driver.findElement(By.className("card-body")).getText(), message);
+    }
 }
diff --git a/src/main/java/teammates/ui/output/JoinStatus.java b/src/main/java/teammates/ui/output/JoinStatus.java
@@ -6,19 +6,13 @@
 public class JoinStatus extends ApiOutput {
 
     private final boolean hasJoined;
-    private final String userId;
 
-    public JoinStatus(boolean hasJoined, String userId) {
+    public JoinStatus(boolean hasJoined) {
         this.hasJoined = hasJoined;
-        this.userId = userId;
     }
 
     public boolean getHasJoined() {
         return hasJoined;
     }
 
-    public String getUserId() {
-        return userId;
-    }
-
 }
diff --git a/src/main/java/teammates/ui/webapi/GetCourseJoinStatusAction.java b/src/main/java/teammates/ui/webapi/GetCourseJoinStatusAction.java
@@ -51,7 +51,7 @@ private JsonResult getInstructorJoinStatus(String regkey) {
     }
 
     private JsonResult getJoinStatusResult(boolean hasJoined) {
-        JoinStatus result = new JoinStatus(hasJoined, userInfo.id);
+        JoinStatus result = new JoinStatus(hasJoined);
         return new JsonResult(result);
     }
 }
diff --git a/src/test/java/teammates/ui/webapi/GetCourseJoinStatusActionTest.java b/src/test/java/teammates/ui/webapi/GetCourseJoinStatusActionTest.java
@@ -51,7 +51,6 @@ protected void testExecute() {
 
         JoinStatus output = (JoinStatus) result.getOutput();
         assertTrue(output.getHasJoined());
-        assertEquals("unreg.user", output.getUserId());
 
         ______TS("Normal case: student is not registered");
 
@@ -68,7 +67,6 @@ protected void testExecute() {
 
         output = (JoinStatus) result.getOutput();
         assertFalse(output.getHasJoined());
-        assertEquals("unreg.user", output.getUserId());
 
         ______TS("Failure case: regkey is not valid for student");
 
@@ -94,7 +92,6 @@ protected void testExecute() {
 
         output = (JoinStatus) result.getOutput();
         assertTrue(output.getHasJoined());
-        assertEquals("unreg.user", output.getUserId());
 
         ______TS("Normal case: instructor is not registered");
 
@@ -111,7 +108,6 @@ protected void testExecute() {
 
         output = (JoinStatus) result.getOutput();
         assertFalse(output.getHasJoined());
-        assertEquals("unreg.user", output.getUserId());
 
         ______TS("Failure case: regkey is not valid for instructor");
 

diff --git a/src/web/app/__snapshots__/user-join-page.component.spec.ts.snap b/src/web/app/__snapshots__/user-join-page.component.spec.ts.snap
@@ -7,10 +7,8 @@ exports[`UserJoinPageComponent should snap if user is not logged in and has a va
   courseService={[Function CourseService]}
   entityType={[Function String]}
   hasJoined="false"
-  institute={[Function String]}
   isLoading="false"
   key={[Function String]}
-  mac={[Function String]}
   navigationService={[Function NavigationService]}
   ngbModal={[Function NgbModal]}
   route={[Function Object]}
@@ -52,10 +50,8 @@ exports[`UserJoinPageComponent should snap with default fields 1`] = `
   courseService={[Function CourseService]}
   entityType={[Function String]}
   hasJoined="false"
-  institute={[Function String]}
   isLoading={[Function Boolean]}
   key={[Function String]}
-  mac={[Function String]}
   navigationService={[Function NavigationService]}
   ngbModal={[Function NgbModal]}
   route={[Function Object]}
@@ -74,15 +70,13 @@ exports[`UserJoinPageComponent should snap with invalid course join link 1`] = `
   courseService={[Function CourseService]}
   entityType={[Function String]}
   hasJoined="false"
-  institute={[Function String]}
   isLoading="false"
   key={[Function String]}
-  mac={[Function String]}
   navigationService={[Function NavigationService]}
   ngbModal={[Function NgbModal]}
   route={[Function Object]}
   router={[Function Router]}
-  userId=""
+  userId={[Function String]}
   validUrl="false"
 >
   <div>
@@ -105,7 +99,7 @@ exports[`UserJoinPageComponent should snap with invalid course join link 1`] = `
       <div
         class="card-body"
       >
-         Please check that you copied the entire join link URL, or have it resent to you if this message persists. 
+         The course join link is invalid. You may have entered the URL incorrectly or the URL may correspond to a/an student that does not exist. 
       </div>
       
     </div>
@@ -121,15 +115,13 @@ exports[`UserJoinPageComponent should snap with valid course join link that has
   courseService={[Function CourseService]}
   entityType={[Function String]}
   hasJoined={[Function Boolean]}
-  institute={[Function String]}
   isLoading="false"
   key={[Function String]}
-  mac={[Function String]}
   navigationService={[Function NavigationService]}
   ngbModal={[Function NgbModal]}
   route={[Function Object]}
   router={[Function Router]}
-  userId=""
+  userId={[Function String]}
   validUrl={[Function Boolean]}
 >
   <div>
@@ -168,10 +160,8 @@ exports[`UserJoinPageComponent should snap with valid course join link that has
   courseService={[Function CourseService]}
   entityType={[Function String]}
   hasJoined="false"
-  institute={[Function String]}
   isLoading="false"
   key={[Function String]}
-  mac={[Function String]}
   navigationService={[Function NavigationService]}
   ngbModal={[Function NgbModal]}
   route={[Function Object]}

diff --git a/src/web/app/pages-admin/admin-page.component.ts b/src/web/app/pages-admin/admin-page.component.ts
@@ -60,8 +60,6 @@ export class AdminPageComponent implements OnInit {
         this.isMaintainer = res.user.isMaintainer;
         if (!this.isAdmin) {
           // User is not a valid admin; redirect to home page.
-          // This should not happen in production server as the /web/admin/* routing is protected,
-          // and a 403 error page will be shown instead.
           this.navigationService.navigateWithErrorMessage(this.router, '/web',
               'You are not authorized to view the page.');
         }

diff --git a/src/web/app/user-join-page.component.html b/src/web/app/user-join-page.component.html
@@ -7,12 +7,13 @@ <h3 style="margin-bottom: 0;">Not logged in</h3>
       You are not logged in. Please log in first.
     </div>
   </div>
-  <div class="card bg-light mx-auto" style="max-width: 800px;" *ngIf="!validUrl || hasJoined">
+  <div class="card bg-light mx-auto" style="max-width: 800px;" *ngIf="userId && (!validUrl || hasJoined)">
     <div class="card-header bg-warning">
       <h3 style="margin-bottom: 0;">Invalid course join link</h3>
     </div>
     <div class="card-body" *ngIf="!validUrl">
-      Please check that you copied the entire join link URL, or have it resent to you if this message persists.
+      The course join link is invalid. You may have entered the URL incorrectly or the URL may correspond
+      to a/an {{ entityType }} that does not exist.
     </div>
     <div class="card-body" *ngIf="hasJoined && validUrl">
       The course join link has been used and is no longer valid.

diff --git a/src/web/app/user-join-page.component.spec.ts b/src/web/app/user-join-page.component.spec.ts
@@ -33,8 +33,6 @@ describe('UserJoinPageComponent', () => {
             queryParams: of({
               entitytype: 'student',
               key: 'key',
-              instructorinstitution: 'nus',
-              mac: 'mac',
             }),
           },
         },
@@ -72,6 +70,7 @@ describe('UserJoinPageComponent', () => {
   });
 
   it('should snap with invalid course join link', () => {
+    component.userId = 'user';
     component.validUrl = false;
     component.isLoading = false;
 
@@ -81,6 +80,7 @@ describe('UserJoinPageComponent', () => {
   });
 
   it('should snap with valid course join link that has been used', () => {
+    component.userId = 'user';
     component.validUrl = true;
     component.hasJoined = true;
     component.isLoading = false;
@@ -102,14 +102,12 @@ describe('UserJoinPageComponent', () => {
   });
 
   it('should join course when join course button is clicked on', () => {
-    const params: string[] = ['key', 'student', 'NUS', 'mac'];
+    const params: string[] = ['key', 'student'];
     component.isLoading = false;
     component.hasJoined = false;
     component.userId = 'user';
     component.key = params[0];
     component.entityType = params[1];
-    component.institute = params[2];
-    component.mac = params[3];
     component.validUrl = true;
 
     const courseSpy: Spy = spyOn(courseService, 'joinCourse').and.returnValue(of({}));
@@ -126,10 +124,19 @@ describe('UserJoinPageComponent', () => {
     expect(navSpy.calls.mostRecent().args[1]).toEqual(`/web/${params[1]}`);
   });
 
-  it('should redirect user to home page if user is logged in', () => {
+  it('should redirect user to home page if user is logged in and join URL has been used', () => {
+    spyOn(authService, 'getAuthUser').and.returnValue(of({
+      user: {
+        id: 'user',
+        isAdmin: false,
+        isInstructor: false,
+        isStudent: false,
+        isMaintainer: false,
+      },
+      masquerade: false,
+    }));
     spyOn(courseService, 'getJoinCourseStatus').and.returnValue(of({
       hasJoined: true,
-      userId: 'user',
     }));
     const navSpy: Spy = spyOn(navService, 'navigateByURL');
 
@@ -141,28 +148,37 @@ describe('UserJoinPageComponent', () => {
     expect(navSpy.calls.mostRecent().args[1]).toEqual('/web/student/home');
   });
 
-  it('should stop loading if user is not logged in', () => {
-    spyOn(courseService, 'getJoinCourseStatus').and.returnValue(of({
-      hasJoined: true,
-      userId: '',
+  it('should stop loading and show error message if 404 is returned', () => {
+    spyOn(authService, 'getAuthUser').and.returnValue(of({
+      user: {
+        id: 'user',
+        isAdmin: false,
+        isInstructor: false,
+        isStudent: false,
+        isMaintainer: false,
+      },
+      masquerade: false,
+    }));
+    spyOn(courseService, 'getJoinCourseStatus').and.returnValue(throwError({
+      status: 404,
     }));
 
     component.ngOnInit();
 
     expect(component.isLoading).toBeFalsy();
+    expect(component.validUrl).toBeFalsy();
   });
 
-  it('should fetch user auth information on error', () => {
-    const navParams: any[] = [undefined, window.location.pathname + window.location.search];
-    spyOn(courseService, 'getJoinCourseStatus').and.returnValue(throwError({
-      status: 403,
+  it('should stop loading and redirect if user is not logged in', () => {
+    spyOn(authService, 'getAuthUser').and.returnValue(of({
+      masquerade: false,
+    }));
+    spyOn(courseService, 'getJoinCourseStatus').and.returnValue(of({
+      hasJoined: true,
     }));
-    const authSpy: Spy = spyOn(authService, 'getAuthUser');
 
     component.ngOnInit();
 
     expect(component.isLoading).toBeFalsy();
-    expect(authSpy.calls.count()).toEqual(1);
-    expect(authSpy.calls.mostRecent().args).toEqual(navParams);
   });
 });