refactor: clean up ProtectedRoute and LoginContext #431
Conversation
create 3 exports: LoginContext itself, LoginProvider, LoginConsumer
Required for jest to find the test files
There was a problem hiding this comment.
The PR looks good to me, the routing is a lot easier to make sense of now. Thanks for taking on this refactor! Left some minor comments to be addressed.
The app routing tests also make sense to me. Do we want to add more scenarios for routing in this PR (such as accessing another component, such as a particular repo's workspace, when logged in and logged out) or should we add that in a separate PR?
Replied to your comments above, thanks for the quick review! W.r.t. the tests, I was thinking of this test suite as a unit test suite that addresses the question: I think navigating from page to page is a different concept and so it warrants a different PR. And to be honest, I haven't figured out the best way to test that just yet (should we test by clicking on the UI components? should we run tests on a specified test repo that exists on GitHub?), so it'll have to come later. |
|
My bad, I don't think I sufficiently explained my comment on test coverage, I didn't mean to suggest we should do testing on the routing from page to page - I agree that the purpose of our route selector tests are just to check we render the right layout given a particular route param. I meant more of checking different route params with the route selector (as you have done). Originally I was thinking simply checking that any one repo-specific route would be sufficient, but I think it makes more sense to check every major route. |
| const resp = await axios.get(`${BACKEND_URL}/auth/whoami`) | ||
| const { userId } = resp.data | ||
| setUserId(userId) | ||
| localStorage.setItem(LOCAL_STORAGE_USER_ID_KEY, userId) |
There was a problem hiding this comment.
The logic in lines 17, 23 for retrieving and storing userId into localStorage seems redundant. With this refactor, would it make sense to now, instead of storing the userId in localStorage, just access the userId from the LoginContext as with all our other components, e.g.
const { userId } = useContext(LoginContext)
Another question, since the context is stored only when the app is rendered, this means that users will no longer be able to persist logged in sessions if they exit the page, is this right? It'd redirect them to the homepage to log in with Github, ping the /whoami endpoint, before users are able to access the site?
There was a problem hiding this comment.
Re: the first point, if we don't store it in localStorage, we initialize userId in LoginProvider as null and that would cause the ProtectedRoute to send us to the login page if we close a tab and open a new one / on refresh (i.e. the login state does not get persisted). You can try this by setting the initialization value of the useState to be null or '' instead of localStorage.getItem(LOCAL_STORAGE_USER_ID_KEY).
There was a problem hiding this comment.
That's a valid point. I was thinking about how sinceverifyLoginAndSetLocalStorage will run anyway on refresh or on load, retrieving it from localStorage in line 17 is just a workaround from having the page flicker from / to ProtectedRoute on refresh. But I think the page flicker is not a good UI for users anyway, so happy to keep it in local storage.
There was a problem hiding this comment.
With the current PR implementation, if we didn't retrieve the userId from local storage, there wouldn't just be a page flicker - instead, the user will always be routed the following way (assuming that they land on a protected route, e.g. a workspace page):
- The user refreshes the browser window on a workspace page;
userIdwould always benullsince we don't store it in the browser local storage - Since
verifyLoginAndSetLocalStorageis an asynchronous network call, it takes a while to complete running. At this moment, the<ProtectedRoute>component is unable to finduserIdimmediately and redirects the user to the/path verifyLoginAndSetLocalStoragecompletes successfully, anduserIdis stored in local storage.- Since we are on
/,<RedirectIfLoggedInRoute>kicks in and redirects the user to the/sitespath
The user would always be redirected to the / path and land on the /sites path even though they started off on a different protected path.
* refactor: clean up ProtectedRoute and LoginContext * refactor: make LoginContext testable create 3 exports: LoginContext itself, LoginProvider, LoginConsumer * refactor: make route selector testable in App.jsx * refactor: group routing components * feat: add basic routing tests * refactor: move __tests__ folder to correct place Required for jest to find the test files * style: delete unnecessary div * refactor: make exports more obvious for LoginContext.js * refactor: delete duplicate route * chore: add rest of routing tests * style: remove unused declarations
* Add linting and formatting tools (#378) * fix: outdated packages with vulnerabilities * feat: install eslint and initiate config * feat: install prettier and set prettier options * feat: install eslint-config-prettier * feat: install eslint-plugin-prettier * chore: reformat eslint config * feat: add @trivago/prettier-plugin-sort-imports, define preferred import order * fix: css-loader file resolution bug introduced by CRA v4 In recent commits, we upgraded our react-scripts version from 3.4.4 to 4.0.3. This is because CRA (create-react-app) v3 uses an outdated version of eslint (facebook/create-react-app#8849). This introduced a bug related to the css-loader library, which can no longer resolve assets in the public folder: - facebook/create-react-app#9870 (comment) - webpack-contrib/css-loader#1136 (comment) This commit fixes this bug by moving the referenced image to the relevant sub-directory in the src directory. * chore: temporarily disable eslint * chore: add more files and folders to .prettierignore * chore: upgrade prettier-plugin-sort-imports to 2.0.2 fixes trivago/prettier-plugin-sort-imports#22 * chore: temporarily disable prettier * chore: remove prettier config temporarily * chore: remove jsx-a11y references temporarily * temporarily remove import/prefer-default-export reference Co-authored-by: jiehao <jiehao@open.gov.sg> Co-authored-by: Preston Lim <prestonlimlianjie@gmail.com> * Fix/resource color (#430) * fix: resource page header changed to bg-secondary * fix: using isResourcePage to determine page header * Fix/rearrange layout (#427) * fix: simplify directoryFile utils for retrieve and update * fix: update methods using directoryFile utils * fix: introduce FolderReorderingModal * fix: refactors FolderContent * fix: update params for FolderContentItem * fix: fix breadcrumb display * fix: add propTypes for FolderReorderingModal * fix: add Cancel button to FolderReorderingModal * fix: updates draggable-id for React dnd * fix: updates dropdown button behavior for reordering * fix: updates copy text * fix: updates copy text * fix: updates variable naming for directory file output * refactor: clean up ProtectedRoute and LoginContext (#431) * refactor: clean up ProtectedRoute and LoginContext * refactor: make LoginContext testable create 3 exports: LoginContext itself, LoginProvider, LoginConsumer * refactor: make route selector testable in App.jsx * refactor: group routing components * feat: add basic routing tests * refactor: move __tests__ folder to correct place Required for jest to find the test files * style: delete unnecessary div * refactor: make exports more obvious for LoginContext.js * refactor: delete duplicate route * chore: add rest of routing tests * style: remove unused declarations * Fix/fine-tune react-query settings (#389) * fix: turn off refetching on window focus for useQuery This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above. * fix: invalidate queries after mutation This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations * feat: standardize output of GET API calls to return resp.data This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed. * fix: load yaml content when reading directory file * feat: disable useQuery if component tracks local state As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes. * fix: rebase errors * fix: invocation of LoginContext * Fix: rebase errors * Fix: update resource category and resource page get calls to return data directly * Refactor: use invalidateQueries instead of passing refetch function for CollectionPagesSection * Fix: error when retrieving page settings * Fix: rebase errors * Fix: invalidate query instead of reload when changing page settings * Feat: add success toast when changing settings * Fix: change folder naming to use invalidation instead of reload * Fix: invalidate correct resource folder key * Fix: update success toast messages * Fix: remove log statement * Fix: add successtoast Co-authored-by: Alexander Lee <alexander@open.gov.sg> * fix: pass parameters to wrapped components (#439) * fix: fixes toast popup on item select, folder deletion modal (#440) * fix: fixes toast popup on item select, folder deletion modal * fix: add condition for folder deletion Co-authored-by: kwajiehao <31984694+kwajiehao@users.noreply.github.com> Co-authored-by: jiehao <jiehao@open.gov.sg> Co-authored-by: Preston Lim <prestonlimlianjie@gmail.com> Co-authored-by: Alexander Lee <alexander@open.gov.sg> Co-authored-by: Alexander Lee <alexleegs@gmail.com>
This PR introduces a major refactor of the routing flow, the way
LoginContextis used, and it adds basic routing tests. To be reviewed in conjunction with isomerpages/isomercms-backend#165.Closes #425
Refactor
Created a new
routingfolder to hold all components that deal with the router<ProtectedRoute>such that it does only one thing logically - if loggedIn ? renderWrappedComponent: redirect to '/'<Route>swithin the massive<Switch>statement inApp.jsxand created a new<RouteSelector>component<RedirectIfLoggedInRoute>routing component that does the following logically - if loggedIn ? redirect to '/sites' : render 'WrappedComponent'Moved all
LoginContextcode into theLoginContext.jsxfileLoginContext,LoginProvider,LoginConsumerAdded basic routing tests for all routes
Dependencies
Dev dependencies