correctly block unauthed users from pages that require auth

previously, a page that required authorization would show the spinning modal indefinitely, instead of showing the message that informs the user they must be signed in. this is because the variable that determined whether the spinner should show, always remained true. this commit breaks out the logic to first check if there is a user. what renders on the page is based on that. checking for the presence of a session has to come after all of the hooks so we don't violate the react-hooks/rules-of-hooks rule. ref: https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level we return the loading state in two different locations because it's rendered based on separate conditions. when isLoading is true because we don't have a user, it doesn't ever become false. that's why we were previously returning the loading spinner indefinitely. using a guard clause with an early return inside the api methods also violates the react-hooks/rules-of-hooks rule.
scientist-softserv · Nov 30, 2023 · 886d285 · 886d285
1 parent d55f360
commit 886d285
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 57 deletions.
diff --git a/pages/requests/[uuid].js b/pages/requests/[uuid].js
@@ -58,25 +58,22 @@ const Request = ({ session }) => {
     }
   }, [request, isLoadingPOs, accessToken, uuid])
 
+  /**
+   * checking for the presence of a session has to come after all of the hooks so we don't violate the react-hooks/rules-of-hooks
+   * rule. ref: https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
+   * we return the loading state in two different locations because it's rendered based on separate conditions. when
+   * isLoading is true because we don't have a user, it doesn't ever become false. that's why we were previously returning
+   * the loading spinner indefinitely.
+   * using a guard clause with an early return inside the api methods also violates the react-hooks/rules-of-hooks rule.
+   */
+  if (session === undefined) return pageLoading
+  if (session === null) return unauthorizedUser
+
   const isLoading = isLoadingRequest || isLoadingSOWs || isLoadingFiles || isLoadingMessages
   const isError = isRequestError || isSOWError || isFilesError|| isMessagesError || isPOError
   const documents = (allSOWs) ? [...allSOWs, ...allPOs] : []
 
-  if (isLoading) return <Loading wrapperClass='item-page mt-5' />
-
-  if (!session) {
-    return (
-      <Notice
-        addClass='my-5'
-        alert={{
-          body: ['Please log in to view this request.'],
-          title: 'Unauthorized',
-          variant: 'info'
-        }}
-        dismissible={false}
-      />
-    )
-  }
+  if (isLoading) return pageLoading
 
   // this error is a result of creating the request
   if (createRequestError) {
@@ -219,4 +216,18 @@ const Request = ({ session }) => {
   )
 }
 
+const pageLoading = <Loading wrapperClass='item-page mt-5' />
+
+const unauthorizedUser = (
+  <Notice
+    addClass='my-5'
+    alert={{
+      body: ['Please log in to view this request.'],
+      title: 'Unauthorized',
+      variant: 'info'
+    }}
+    dismissible={false}
+  />
+)
+
 export default Request
diff --git a/pages/requests/index.js b/pages/requests/index.js
@@ -22,23 +22,17 @@ const Requests = ({ session }) => {
   const isError = isAllRequestsError || isDefaultWareError
   const isLoading = isLoadingAllRequests || isLoadingDefaultWare
 
-  // Check whether the user is authenticated first. If it does, we can return the API errors if applicable.
-
-  if (isLoading) return <Loading wrapperClass='mt-5' />
-
-  if (!session) {
-    return (
-      <Notice
-        addClass='my-5'
-        alert={{
-          body: ['Please log in to make new requests or view existing ones.'],
-          title: 'Unauthorized',
-          variant: 'info'
-        }}
-        dismissible={false}
-      />
-    )
-  }
+  /**
+   * checking for the presence of a session has to come after all of the hooks so we don't violate the react-hooks/rules-of-hooks
+   * rule. ref: https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
+   * we return the loading state in two different locations because it's rendered based on separate conditions. when
+   * isLoading is true because we don't have a user, it doesn't ever become false. that's why we were previously returning
+   * the loading spinner indefinitely.
+   * using a guard clause with an early return inside the api methods also violates the react-hooks/rules-of-hooks rule.
+   */
+  if (session === undefined) return pageLoading
+  if (session === null) return unauthorizedUser
+  if (isLoading) return pageLoading
 
   if (isError) {
     return (
@@ -72,4 +66,18 @@ const Requests = ({ session }) => {
   )
 }
 
+const pageLoading = <Loading wrapperClass='mt-5' />
+
+const unauthorizedUser = (
+  <Notice
+    addClass='my-5'
+    alert={{
+      body: ['Please log in to make new requests or view existing ones.'],
+      title: 'Unauthorized',
+      variant: 'info'
+    }}
+    dismissible={false}
+  />
+)
+
 export default Requests
diff --git a/pages/requests/new/[ware].js b/pages/requests/new/[ware].js
@@ -61,6 +61,27 @@ const NewRequest = ({ session }) => {
   const [buttonDisabled, setButtonDisabled] = useState(false)
   const [formSubmitting, setFormSubmitting] = useState(false)
 
+  useEffect(() => {
+    if (createdRequestUUID) {
+      router.push({
+        pathname: `/requests/${createdRequestUUID}`,
+        query: { createRequestError: JSON.stringify(createRequestError) },
+      }, `/requests/${createdRequestUUID}`)
+    }
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [createdRequestUUID, createRequestError])
+
+  /**
+   * checking for the presence of a session has to come after all of the hooks so we don't violate the react-hooks/rules-of-hooks
+   * rule. ref: https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
+   * we return the loading state in two different locations because it's rendered based on separate conditions. when
+   * isLoading is true because we don't have a user, it doesn't ever become false. that's why we were previously returning
+   * the loading spinner indefinitely.
+   * using a guard clause with an early return inside the api methods also violates the react-hooks/rules-of-hooks rule.
+   */
+  if (session === undefined) return pageLoading
+  if (session === null) return unauthorizedUser
+
   /**
    * @param {object} event onChange event
    * @param {string} property dot notated string representing the property in initialValue
@@ -118,32 +139,8 @@ const NewRequest = ({ session }) => {
     setCreatedRequestUUID(data.uuid)
   }
 
-  useEffect(() => {
-    if (createdRequestUUID) {
-      router.push({
-        pathname: `/requests/${createdRequestUUID}`,
-        query: { createRequestError: JSON.stringify(createRequestError) },
-      }, `/requests/${createdRequestUUID}`)
-    }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [createdRequestUUID, createRequestError])
-
   // TODO(alishaevn): use react bs placeholder component
-  if (isLoadingInitialRequest || !wareID || formSubmitting) return <Loading wrapperClass='item-page mt-5' />
-
-  if (!session) {
-    return (
-      <Notice
-        addClass='my-5'
-        alert={{
-          body: ['Please log in to make new requests.'],
-          title: 'Unauthorized',
-          variant: 'info'
-        }}
-        dismissible={false}
-      />
-    )
-  }
+  if (isLoadingInitialRequest || !wareID || formSubmitting) return pageLoading
 
   if (isInitialRequestError) {
     return (
@@ -242,4 +239,18 @@ const StandardRequestOptions = ({ buttonDisabled, defaultRequiredDate, requestFo
   )
 }
 
+const pageLoading = <Loading wrapperClass='item-page mt-5' />
+
+const unauthorizedUser = (
+  <Notice
+    addClass='my-5'
+    alert={{
+      body: ['Please log in to make new requests.'],
+      title: 'Unauthorized',
+      variant: 'info'
+    }}
+    dismissible={false}
+  />
+)
+
 export default NewRequest