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.

Sign up for GitHub

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

fix: Resolve Sentry issue fetching in MissingDesignatedAdmins Without a Provider #3612

Merged
merged 4 commits into from
Dec 30, 2024
Merged

fix: Resolve Sentry issue fetching in MissingDesignatedAdmins Without a Provider #3612

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e86520
add small wrapper to conditionally render component
nicholas-codecov Dec 27, 2024
93a4601
tidy up test to remove query v4 stuff
nicholas-codecov Dec 27, 2024
16f49f6
provider may be optional with global banners
nicholas-codecov Dec 27, 2024
a24f4e3
remove conditional as it's always truthy
nicholas-codecov Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions src/shared/GlobalBanners/MissingDesignatedAdmins/MissingDesignatedAdmins.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import {
QueryClientProvider as QueryClientProviderV5,
QueryClient as QueryClientV5,
Expand All @@ -19,9 +18,6 @@ const mockApiSelfHostedSetUpCorrectly = { config: { hasAdmins: true } }
const mockApiSelfHostedSetUpIncorrectly = { config: { hasAdmins: false } }
const mockApiCloud = { config: undefined }

const queryClient = new QueryClient({
defaultOptions: { queries: { retry: false } },
})
const queryClientV5 = new QueryClientV5({
defaultOptions: { queries: { retry: false } },
})
Expand All @@ -30,13 +26,11 @@ const wrapper =
(initialEntries = ['/gh/test-org/test-repo/pull/12']) =>
({ children }) => (
<QueryClientProviderV5 client={queryClientV5}>
<QueryClientProvider client={queryClient}>
<MemoryRouter initialEntries={initialEntries}>
<Route path="/:provider/:owner/:repo/pull/:pullId">
<Suspense fallback={<div>Loading</div>}>{children}</Suspense>
</Route>
</MemoryRouter>
</QueryClientProvider>
<MemoryRouter initialEntries={initialEntries}>
<Route path="/:provider/:owner/:repo/pull/:pullId">
<Suspense fallback={<div>Loading</div>}>{children}</Suspense>
</Route>
</MemoryRouter>
</QueryClientProviderV5>
)

Expand All @@ -46,7 +40,6 @@ beforeAll(() => {
})

afterEach(() => {
queryClient.clear()
queryClientV5.clear()
server.resetHandlers()
})
Expand Down
50 changes: 19 additions & 31 deletions src/shared/GlobalBanners/MissingDesignatedAdmins/MissingDesignatedAdmins.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,18 @@ import { Provider } from 'shared/api/helpers'
import A from 'ui/A'
import Banner from 'ui/Banner'

interface Props {
interface MissingDesignatedAdminsProps {
provider: Provider
hasAdmins?: boolean | null
isFetching?: boolean
isSelfHosted?: boolean
}

const useHideBanner = ({
const MissingDesignatedAdmins: React.FC<MissingDesignatedAdminsProps> = ({
provider,
hasAdmins,
isFetching,
isSelfHosted,
}: Props) => {
if (!isSelfHosted || !provider || hasAdmins || isFetching) {
return true
}

return false
}

interface URLParams {
provider: Provider
}

const MissingDesignatedAdmins = () => {
const { provider } = useParams<URLParams>()
}) => {
const { data: hasAdmins, isFetching } = useSuspenseQueryV5(
SelfHostedHasAdminsQueryOpts({ provider })
)
// This hook is purely side stepping the complexity rule here.
const hideBanner = useHideBanner({
provider,
hasAdmins,
isFetching,
isSelfHosted: !!config.IS_SELF_HOSTED,
})

if (hideBanner) {
if (!config.IS_SELF_HOSTED || !provider || hasAdmins || isFetching) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we no longer need this !provider check either right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, yea we can get rid of it, just a left over from a copy paste 🫣

return null
}

Expand All @@ -70,4 +44,18 @@ const MissingDesignatedAdmins = () => {
)
}

export default MissingDesignatedAdmins
interface URLParams {
provider?: Provider
}

const MissingDesignatedAdminsWrapper = () => {
const { provider } = useParams<URLParams>()

if (provider) {
return <MissingDesignatedAdmins provider={provider} />
}

return null
}

export default MissingDesignatedAdminsWrapper
Loading