-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement nav bar for internal users #478
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-4155 So, pass through authentication should be all sorted. We can now read the cookie passed in my the legacy UI, confirm the user is authenticated and authorised to access a page. We pass through auth, user and scope details in the global context so all view templates have access to the information. We also have our first use of this; deciding whether to display the header links to **Change password** and **Sign out**. The final piece that all our pages should handle is the nav bar. This is where internal users can switch between **Search**, **Bill runs** and **Manage** assuming they have the right permissions. This change implements the nav bar.
All users get to see the search nav bar option. But the 'Bill runs' and 'Manage' options are determined by what roles they have. We don't want to pollute our Nunjucks templates with anything more than if/else kind of logic. So, somewhere in the app we need to work out which options a user has access to. We could do it in the controllers, and pass the result to the view via the context there. But that means repeating the same logic lots of times (or at least calling the same logic from lots of places). This is something we'll need to know for most requests. So, we could determine the permissions in our ViewsPlugin global context builder. But it seems inconsistent that pull most details about the user from the auth object in the request but then work out this final piece in the views plugin. So, we came to the conclusion that this should sit in the `AuthService` and be returned as part of the `credentials` object that gets added to all requests by Hapi.
Now all templates will have access to this information.
The html for the layout and the SCSS all come from the [prototype](https://github.com/defra-design/wrls-prototype). We assume they are right! One feature to note is the UI expects to highlight one of the nav bar options when on a page. The legacy service had it's own `ViewContext` plugin that created the global context from a variety of sources. Which nav bar item to highlight was one of them and they chose to set this in the config options of a route, along with the page title. We think it will be clearer if data relevant to a page is route dependent, _all_ the data should come from the controller. For example ```javascript return h.view('info.njk', { pageTitle: 'Info', activeNavBar: 'bill-runs', ...pageData }) ``` That way we know there are only 2 sources; the global context and the context we define in a controller.
Confirmed we need to include Digitise! in the nav-bar like the existing service does (wasn't in the prototype). To support this we need to add another top level permission check.
Cruikshanks
added a commit
that referenced
this pull request
Oct 23, 2023
https://eaflood.atlassian.net/browse/WATER-4155 We thought the [nav bar for internal users](#478) would be the final piece. But we have since confirmed with our designer that until we know otherwise, we should also be including the [phase banner](https://design-system.service.gov.uk/components/phase-banner/) even though this is an internal app. So, this change adds the banner to our layout. It will be displayed always, no matter if someone is authenticated or not.
Learnt whilst working on implementing the actual bill page (WATER-4155) that the nav bar needs sit in this block if it is also too play nice with the phase banner.
Beckyrose200
approved these changes
Oct 25, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jozzey
approved these changes
Oct 25, 2023
Cruikshanks
added a commit
that referenced
this pull request
Oct 25, 2023
https://eaflood.atlassian.net/browse/WATER-4155 We thought the [nav bar for internal users](#478) would be the final piece. But we have since confirmed with our designer that until we know otherwise, we should also be including the [phase banner](https://design-system.service.gov.uk/components/phase-banner/) even though this is an internal app. So, this change adds the banner to our layout. It will be displayed always, no matter if someone is authenticated or not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4155
So, pass-through authentication should be all sorted. We can now read the cookie passed in by the legacy UI, and confirm the user is authenticated and authorised to access a page. We pass auth, user and scope details to the global context so all view templates have access to this information. We also have our first use of this; deciding whether to display the header links to Change password and Sign out.
The final piece that all our pages should handle is the nav bar. This is where internal users can switch between Search, Bill runs and Manage assuming they have the right permissions.
This change implements the nav bar.
Notes
Add new permission object to auth credentials
All users get to see the search nav bar option. But the 'Bill runs', 'Digitize' and 'Manage' options are determined by what roles they have. We don't want to pollute our Nunjucks templates with anything more than if/else. So, somewhere in the app we need to work out which options a user has access to.
We could have done it in the controllers and passed the result to the view via the context there. But that means repeating the same logic lots of times (or at least calling the same logic from lots of places).
We'll need to know this for most requests. So, we could determine the permissions in our
ViewsPlugin
global context builder. But it seems inconsistent to pull most details about the user from the auth object in the request but then work out this final piece in the views plugin.So, we came to the conclusion that this should sit in the
AuthService
and be returned as part of thecredentials
object that gets added to all requests by Hapi.Mark option as active
One feature to note is the UI expects to highlight one of the nav bar options when on a page. The legacy service had its own
ViewContext
plugin that created the global context from a variety of sources. Which nav bar item to highlight was one of them and they chose to set this in the config options of a route, along with the page title.We think it's clearer if data relevant to a page is route-dependent, all the data should come from the controller. For example
That way we know there are only 2 sources; the global context and the context we define in a controller.
Screenshot of nav bar