-
Notifications
You must be signed in to change notification settings - Fork 1
If runner feature disabled, don't show 'runner' navlink or pages #262
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
Changes from all commits
03239a0
d123fc8
cfe611a
6f9f351
48d4b57
91034a8
23c56dc
06c93d7
c06e525
51c839b
730fd77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Before submitting a PR or set of changes to a PR, if any changes have been made to the front end, ensure linting and formatting are clean, e.g. with `npm run lint:fix --prefix=app` or `npm run format:write --prefix=app`. Don't make these pass by using comments to ignore linter warnings: actually fix the highlighted issues. No exceptions. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { SidebarItem } from "@lib/types/SidebarItem"; | |||||
| import { useUser } from "../../providers/UserProvider"; | ||||||
| import { Sidebar } from "../common/Sidebar"; | ||||||
| import { Unauthorized } from "../common/Unauthorized"; | ||||||
| import { useRunnerConfig } from "../../providers/RunnerConfigProvider"; | ||||||
|
|
||||||
| const sidebarItems: SidebarItem[] = [ | ||||||
| { | ||||||
|
|
@@ -18,7 +19,9 @@ const sidebarItems: SidebarItem[] = [ | |||||
|
|
||||||
| export const PacketRunnerLayout = () => { | ||||||
| const { authorities } = useUser(); | ||||||
| if (!hasPacketRunPermission(authorities)) { | ||||||
| const isRunnerEnabled = useRunnerConfig(); | ||||||
|
|
||||||
| if (!hasPacketRunPermission(authorities) || isRunnerEnabled === false) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
should probs not show if cant get this config as well. Also this is pretty static so we can probably just fetch once and store in local storage like we do with other instance related stuff. (i think auth type)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refer to app/src/app/components/providers/hooks/useGetAuthConfig.ts, app/src/app/components/providers/AuthConfigProvider.tsx, app/src/lib/localStorageManager.ts. Branch off this PR to make the fetch to the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually not convinced this is the best idea, because what if the instance decides they want to change the setting from enabled to disabled or vice versa? The setting would be cached in localStorage, unable to change. Maybe sessionStorage would be a better place than localStorage. You'd do one fetch per session rather than once per browser.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in d123fc8. Created a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This has since been changed to use sessionStorage instead of localStorage.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea |
||||||
| return <Unauthorized />; | ||||||
| } | ||||||
| return ( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import useSWR from "swr"; | ||
| import appConfig from "@config/appConfig"; | ||
| import { fetcher } from "@lib/fetch"; | ||
|
|
||
| export const useGetRunnerEnabled = (runnerEnabled: boolean | null) => { | ||
| const { data, error, isLoading } = useSWR<boolean>( | ||
| runnerEnabled === null ? `${appConfig.apiUrl()}/runner/enabled` : null, | ||
| (url: string) => fetcher({ url, noAuth: true }), | ||
| { revalidateOnFocus: false } | ||
| ); | ||
|
|
||
| return { | ||
| isRunnerEnabled: data, | ||
| isLoading, | ||
| error | ||
| }; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { ReactNode, createContext, useContext, useEffect, useState } from "react"; | ||
| import { getRunnerConfigFromSessionStorage, setRunnerConfigInSessionStorage } from "@lib/storageManager"; | ||
| import { ErrorComponent } from "../contents/common/ErrorComponent"; | ||
| import { useGetRunnerEnabled } from "../header/hooks/useGetRunnerEnabled"; | ||
|
|
||
| const RunnerConfigContext = createContext<boolean | null>(null); | ||
|
|
||
| export const useRunnerConfig = () => useContext(RunnerConfigContext); | ||
|
|
||
| interface RunnerConfigProviderProps { | ||
| children: ReactNode; | ||
| } | ||
|
|
||
| export const RunnerConfigProvider = ({ children }: RunnerConfigProviderProps) => { | ||
| const [runnerEnabled, setRunnerEnabled] = useState<boolean | null>(() => getRunnerConfigFromSessionStorage()); | ||
| const { isRunnerEnabled, isLoading, error } = useGetRunnerEnabled(runnerEnabled); | ||
|
|
||
| useEffect(() => { | ||
| if (isRunnerEnabled !== undefined) { | ||
| setRunnerEnabled(isRunnerEnabled); | ||
| setRunnerConfigInSessionStorage(isRunnerEnabled); | ||
| } | ||
| }, [isRunnerEnabled]); | ||
|
|
||
| if (error) return <ErrorComponent message="failed to load runner config" error={error} />; | ||
|
|
||
| return ( | ||
| <RunnerConfigContext.Provider value={isLoading ? null : runnerEnabled}>{children}</RunnerConfigContext.Provider> | ||
| ); | ||
| }; | ||
|
david-mears-2 marked this conversation as resolved.
|
||
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.
should we not be getting from appconfig? and have this set in application.properties?
Uh oh!
There was an error while loading. Please reload this page.
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.
It's set in application.properties already, and I think this line is what reads it (setting config), though it bypasses AppConfig.kt itself. Then this line checks if config was set, returns RunnerService if it is, and if not it returns the DisabledRunnerService.
My changes just add a property to the RunnerService and DisabledRunnerService which is static (enabled -> true for RunnerService, enabled -> false for DisabledRunnerService)
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.
ohh thanks for the clarification. i got a bit confused by the setup of it all