Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Homepage accessible regardless of login status #1049 #1050

Merged
merged 7 commits into from
May 5, 2022

Conversation

sam-glendenning
Copy link
Contributor

Description

The user's login state now has no effect on access to the homepage or display of the navigation drawer. This means that, like ISIS, DLS now allows access to the homepage at the web root and the navigation drawer shows all web app routes (but no access allowed while not logged in). This is in comparison to how things used to work on DLS, whereby all nav drawer routes including the homepage were inaccessible until the user logged in.

This PR also includes a fix to a problem where the user would not be redirected to their original request following a successful login, instead being redirected to the homepage. This took a lot of trial and error but I came to the conclusion that this was because the current method detailed below no longer works:

  • User tries to navigate to /protected/domain
  • Router redirects to /login while storing /protected/domain in router state
  • User successfully logs in
  • A router location change is triggered to /logout, clearing the router state in the process (still not sure why this location change happens)
  • Router state is gone at the crucial stage, therefore default redirect to web root occurs

The state is cleared between page refreshes/redirects because this is what react-router-dom does. But I'm unsure as to why a router REPLACE to /logout happens.

To get around this, I made a new method whereby the referral URL is stored in localStorage instead. It is then fetched, changed and deleted as required.

Testing instructions

Run this with at least one other plugin loaded so that the navigation drawer can be populated. Set the "authUrl" in settings.json to be "https://datagateway-preprod.diamond.ac.uk/api" to mimic DLS DG environment (no auto login).
When navigating to the web root, you should be directed to the homepage instead of /login. Also, the navigation bar should be accessible but you should be redirected to /login upon trying to navigate to any of the routes.

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

Closes #1049

Sam Glendenning added 4 commits April 28, 2022 11:52
…n not logged in #1049

The user's login state now has no effect on access to the homepage or display of the navigation drawer. This means that DLS now allows access to the homepage at the web root and the navigation drawer shows all web app routes (access to these is determined elsewhere). This is in comparison to how things used to work on DLS, whereby all nav drawer routes including the homepage were inaccessible until the user logged in.
Since this PR is related to SciGateway login changes, I thought I'd lump in this additional fix. This addresses the problem whereby the user would try to navigate to a protected resource, be redirected to login, login successfully and then be mistakenly directed to the homepage instead of their original page. This was previously working by storing the referrer in the login redirect state but this state is cleared on a page refresh, meaning we defaulted back to the homepage. We now store the referrer in localStorage and clear it when we don't need it.
@sam-glendenning sam-glendenning added enhancement New feature or request MVP needed for minimum viable product labels Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #1050 (df71427) into develop (f98bd43) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1050      +/-   ##
===========================================
- Coverage    97.94%   97.94%   -0.01%     
===========================================
  Files           42       42              
  Lines         1557     1555       -2     
  Branches       411      421      +10     
===========================================
- Hits          1525     1523       -2     
  Misses          31       31              
  Partials         1        1              
Impacted Files Coverage Δ
src/mainAppBar/mainAppBar.component.tsx 100.00% <100.00%> (ø)
src/routing/authorisedRoute.component.tsx 100.00% <100.00%> (ø)
src/state/actions/scigateway.actions.tsx 98.14% <100.00%> (-0.03%) ⬇️
src/state/reducers/scigateway.reducer.tsx 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f98bd43...df71427. Read the comment docs.

Comment on lines 490 to -494
dispatch(authorised());

// redirect the user to the original page they were trying to get to
// the referrer is added by the redirect in routing.component.tsx
const previousRouteState = getState().router.location.state;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you moved the reading of previousRouteState before dispatching the authorised() action then the router referrer would be accurate. The redirect to /logout happens because if a user tries to access the login page when logged in they should instead see the logout page. If you grab what's in the referrer before the user is logged in, then this should be before the redirect to /logout.

Though tbh, I don't mind doing it via localStorage, just thought you'd want to know what was causing the problem :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I get it. I'll give it another go

Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well - on page load you see the homepage, and upon clicking any of the links on the homepage or in the sidebar you are redirected to the login page and once you login you are taken back to the link you clicked on and the sidebar opens as a default.

I'll wait to see if you manage to get the old redirect working though

The reason the referrer was being wiped from state was because the user was logging in, which meant the login page becomes inaccessible and is thus replaced with logout. This removes the referrer from state. I am now changing this so we grab the referrer earlier in the process before redirecting. This removes the need to use localStorage
@sam-glendenning
Copy link
Contributor Author

@louise-davies fixed with your solution! You were right, I simply needed to grab the referrer from state a little earlier. Thanks for the help

Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well with the new fix - glad I could help out and that it worked :)

@sam-glendenning sam-glendenning merged commit a2f17e4 into develop May 5, 2022
@sam-glendenning sam-glendenning deleted the feature/dls-homepage-by-default-#1049 branch May 5, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request MVP needed for minimum viable product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homepage / login options for DLS
2 participants