Merge pull request #326 from scientist-softserv/325-unauthed-experience

325-unauthed-experience

pages/_app.js

@@ -19,6 +19,9 @@ import {
 import '../utils/theme/globals.scss'
 
 const WebStore = ({ Component }) => {
+  /**
+   * the session will return undefined if it's still loading, null if the user is not logged in, or an object if the user is logged in.
+   */
   const { data: session } = useSession()
   const router = useRouter()
 

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

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

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