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

fix: Move the banner above summary, and fix the use of team hook #2859

Merged
merged 4 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const mockGetRepo = (hasUploadToken: boolean, isActive: boolean) => ({
activated: false,
oldestCommitAt: '',
active: isActive,
isFirstPullRequest: false,
},
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const mockGetRepo = {
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const mockGetRepo = {
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const mockGetRepo = {
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const mockGetRepo = {
activated: true,
oldestCommitAt: '2020-01-01T12:00:00',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const mockGetRepo = ({
activated: isRepoActivated,
oldestCommitAt: '2022-10-10T11:59:59',
active: isRepoActive,
isFirstPullRequest: false,
},
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const mockGetRepo = {
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const mockGetRepo = {
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
1 change: 1 addition & 0 deletions src/pages/RepoPage/CoverageOnboarding/NewRepoTab.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const mockGetRepo = (
activated: false,
oldestCommitAt: '',
active: hasCommits,
isFirstPullRequest: false,
},
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const mockGetRepo = {
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest: false,
},
},
}
Expand Down
15 changes: 12 additions & 3 deletions src/pages/RepoPage/CoverageTab/CoverageTab.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import { Switch, useParams } from 'react-router-dom'
import { SentryRoute } from 'sentry'

import SilentNetworkErrorWrapper from 'layouts/shared/SilentNetworkErrorWrapper'
import { useRepoSettingsTeam } from 'services/repo'
import { useRepo } from 'services/repo'
import { TierNames, useTier } from 'services/tier'
import { useFlags } from 'shared/featureFlags'
import Spinner from 'ui/Spinner'

import FirstPullRequestBanner from './FirstPullRequestBanner'
import Summary from './Summary'
import SummaryTeamPlan from './SummaryTeamPlan'
import ToggleElement from './ToggleElement'
Expand All @@ -25,11 +26,16 @@ const Loader = () => (
)

function CoverageTab() {
const { data: repoData } = useRepoSettingsTeam()
const { provider, owner, repo } = useParams()
const { data: repoData } = useRepo({
provider,
owner,
repo,
})

const { multipleTiers } = useFlags({
multipleTiers: false,
})
const { provider, owner } = useParams()
const { data: tierName } = useTier({ provider, owner })

const showTeamSummary =
Expand All @@ -39,6 +45,9 @@ function CoverageTab() {

return (
<div className="mx-4 flex flex-col gap-2 divide-y border-solid border-ds-gray-secondary sm:mx-0">
{repoData?.repository?.isFirstPullRequest ? (
<FirstPullRequestBanner />
) : null}
{showTeamSummary ? <SummaryTeamPlan /> : <Summary />}
{!showTeamSummary && (
<SentryRoute
Expand Down
58 changes: 32 additions & 26 deletions src/pages/RepoPage/CoverageTab/CoverageTab.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,6 @@ const queryClient = new QueryClient({

const server = setupServer()

const mockRepoSettings = (isPrivate = false) => ({
owner: {
repository: {
__typename: 'Repository',
activated: true,
defaultBranch: 'master',
private: isPrivate,
uploadToken: 'token',
graphToken: 'token',
yaml: 'yaml',
bot: {
username: 'test',
},
},
},
})

const wrapper =
(initialEntries = ['/gh/codecov/cool-repo/tree/main']) =>
({ children }) =>
Expand All @@ -53,13 +36,24 @@ const wrapper =
</QueryClientProvider>
)

const mockRepo = {
const mockRepo = (isPrivate = false, isFirstPullRequest = false) => ({
owner: {
isCurrentUserPartOfOrg: true,
isAdmin: null,
isCurrentUserActivated: null,
repository: {
__typename: 'Repository',
private: isPrivate,
uploadToken: '9e6a6189-20f1-482d-ab62-ecfaa2629295',
defaultBranch: 'main',
yaml: '',
activated: false,
oldestCommitAt: '',
active: true,
isFirstPullRequest,
},
},
}
})

const repoConfigMock = {
owner: {
Expand Down Expand Up @@ -208,10 +202,14 @@ afterAll(() => {

describe('Coverage Tab', () => {
function setup(
{ repoData = mockRepo, isPrivate = false, tierValue = TierNames.PRO } = {
repoData: mockRepo,
{
isPrivate = false,
tierValue = TierNames.PRO,
isFirstPullRequest = false,
} = {
isPrivate: false,
tierValue: TierNames.PRO,
isFirstPullRequest: false,
}
) {
useFlags.mockReturnValue({
Expand All @@ -220,7 +218,7 @@ describe('Coverage Tab', () => {

server.use(
graphql.query('GetRepo', (req, res, ctx) =>
res(ctx.status(200), ctx.data(repoData))
res(ctx.status(200), ctx.data(mockRepo(isPrivate, isFirstPullRequest)))
),
graphql.query('GetBranches', (req, res, ctx) =>
res(ctx.status(200), ctx.data(branchesMock))
Expand Down Expand Up @@ -268,10 +266,7 @@ describe('Coverage Tab', () => {
(req, res, ctx) => {
return res(ctx.status(200), ctx.json({ data: {} }))
}
),
graphql.query('GetRepoSettingsTeam', (req, res, ctx) => {
return res(ctx.status(200), ctx.data(mockRepoSettings(isPrivate)))
})
)
)
}

Expand Down Expand Up @@ -328,4 +323,15 @@ describe('Coverage Tab', () => {
expect(coverageChart).not.toBeInTheDocument()
})
})

it('renders first pull request banner', async () => {
setup({ isFirstPullRequest: true })

render(<CoverageTab />, { wrapper: wrapper(['/gh/test-org/repoName']) })

const firstPullRequestBanner = await screen.findByText(
/Once merged to your default branch, Codecov will show your report results on this dashboard./
)
expect(firstPullRequestBanner).toBeInTheDocument()
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { render, screen } from '@testing-library/react'
import { MemoryRouter, Route } from 'react-router-dom'

import FirstPullRequestBanner from './FirstPullRequestBanner'

const wrapper: React.FC<React.PropsWithChildren> = ({ children }) => (
<MemoryRouter initialEntries={['/gh/codecov/gazebo']}>
<Route path="/:provider/:owner/:repo">{children}</Route>
</MemoryRouter>
)

describe('FirstPullRequestBanner', () => {
it('renders content', () => {
render(<FirstPullRequestBanner />, { wrapper })
const content = screen.getByText(
'Once merged to your default branch, Codecov will show your report results on this dashboard.'
)
expect(content).toBeInTheDocument()
})

it('renders the correct link', () => {
render(<FirstPullRequestBanner />, { wrapper })
const link = screen.getByRole('link', { name: 'edit default branch' })
expect(link).toHaveAttribute('href', '/gh/codecov/gazebo/settings')
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import A from 'ui/A'
import Banner from 'ui/Banner'
import BannerContent from 'ui/Banner/BannerContent'

const FirstPullRequestBanner = () => {
return (
<Banner>
<BannerContent>
<p>
Once merged to your default branch, Codecov will show your report
results on this dashboard.{' '}
<A
to={{ pageName: 'settings' }}
hook={'repo-to-edit-branch'}
variant="semibold"
isExternal={false}
data-testid="settings-page"
>
edit default branch
</A>
</p>
</BannerContent>
</Banner>
)
}

export default FirstPullRequestBanner
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './FirstPullRequestBanner'
Original file line number Diff line number Diff line change
Expand Up @@ -356,22 +356,6 @@ describe('CodeTreeTable', () => {
})
})

describe('there is no results found', () => {
it('displays error fetching data message', async () => {
setup({ noFiles: true })
render(<CodeTreeTable />, { wrapper: wrapper() })

const message = await screen.findByText(
'Once merged to your default branch, Codecov will show your report results on this dashboard.'
)
expect(message).toBeInTheDocument()

const link = await screen.findByTestId('settings-page')
expect(link).toBeInTheDocument()
expect(link).toHaveAttribute('href', '/gh/codecov/cool-repo/settings')
})
})

describe('when head commit has no reports', () => {
it('renders no report uploaded message', async () => {
setup({ noHeadReport: true })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,24 +290,6 @@ describe('FileListTable', () => {
})
})

describe('there is no results found', () => {
it('displays error fetching data message', async () => {
setup({ noFiles: true })
render(<FileListTable />, {
wrapper: wrapper('/gh/codecov/cool-repo/tree/main/'),
})

const message = await screen.findByText(
'Once merged to your default branch, Codecov will show your report results on this dashboard.'
)
expect(message).toBeInTheDocument()

const link = await screen.findByTestId('settings-page')
expect(link).toBeInTheDocument()
expect(link).toHaveAttribute('href', '/gh/codecov/cool-repo/settings')
})
})

describe('when head commit has no reports', () => {
it('renders no report uploaded message', async () => {
setup({ noHeadReport: true })
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

m: I know you're moving the banner to the top, however I think it's still worth showing some form of an error message down here, instead of just an empty table, users "shouldn't" miss the banner at the top, but they might, so I think we should still leave some error message down in the table so they're at least aware of some issue occurring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you suggest we leave the banner there as well, or loop in Kyle for a new copy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Uhh i think new copy would probably be best imo, i would show something similar to how we currently display the other error messages ... the banner always felt out of place

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if you feel comfortable coming up with something, i think that'll be alright as well, it could be the same copy that's in the banner ... just not in the banner

Copy link
Contributor Author

@RulaKhaled RulaKhaled May 21, 2024

Choose a reason for hiding this comment

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

went with copy with no banner Screenshot 2024-05-21 at 12 36 13 PM (fyi banner would still show i just wanted to show you how it'll look)

Copy link
Contributor

Choose a reason for hiding this comment

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

different from other errors we display, as it's related to user onboarding (it's not even an error)

Yes, that is correct, it's not an error rather it's a moment in onboarding where the data isn't showing since it's not merged to default branch. Sounds like there are no blockers here – If there is a follow up for inconsistency, could we create an issue with the details to capture and resolve separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @codecovdesign

@nicholas-codecov can you create an issue for the inconsistency please? Also as it is not a blocker for this PR can we move forward?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're introducing the inconsistency in this PR in the first place, and it's only two lines. Can we revert those two lines and bring back the assignment to the copy var? We can then go through and update all of them together so we get to tackle the issue without introducing any inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicholas-codecov so delete the copy that you asked me to add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just synced with Kyle, we think having 2 exact banners in the same page is a bit weird, so let's keep this copy for now, and i really encourage you to create the issue, i'll make sure it's tackled soon

Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ const RepoContentsResult: React.FC<RepoContentsProps> = ({
head commit
`
} else {
copy = (
<p>
return (
<p className="mt-4">
Once merged to your default branch, Codecov will show your report
results on this dashboard.{' '}
<A
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe('DeactivatedRepo', () => {
activated: false,
oldestCommitAt: '',
active: false,
isFirstPullRequest: false,
},
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const mockGetRepo = ({ isAdmin = true }) => ({
activated: true,
oldestCommitAt: '2020-01-01T12:00:00',
active: true,
isFirstPullRequest: false,
},
},
})
Expand Down
1 change: 1 addition & 0 deletions src/pages/RepoPage/RepoPage.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const mockGetRepo = ({
activated: isRepoActivated,
oldestCommitAt: '',
active: isRepoActive,
isFirstPullRequest: false,
},
},
})
Expand Down
Loading
Loading