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

feat: Update files explorer tab with components selector #2450

Merged
merged 2 commits into from
Dec 12, 2023
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
17 changes: 11 additions & 6 deletions src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

import { useRepoPullContentsTable } from './hooks'

import ComponentsSelector from '../ComponentsSelector'
Copy link

Choose a reason for hiding this comment

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

Ensure that 'ComponentsSelector' is properly exported from its module, as it is being imported here.


const Loader = () => (
<div className="flex flex-1 justify-center">
<Spinner size={60} />
Expand Down Expand Up @@ -43,12 +45,15 @@
<DisplayTypeButton />
<Breadcrumb paths={treePaths} />
</div>
<SearchField
dataMarketing="pull-files-search"
placeholder="Search for files"
searchValue={params?.search}
setSearchValue={(search) => updateParams({ search })}
/>
Copy link

Choose a reason for hiding this comment

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

Keep an eye on CSS to make sure that the combination of the 'ComponentsSelector' and the 'SearchField' within the flex container looks good in all main browsers.

<div className="flex gap-2">
Copy link

Choose a reason for hiding this comment

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

You have added a new component (ComponentsSelector) but did not provide any prop. Please make sure to add necessary props and validate them using propTypes in the 'ComponentsSelector' component. This will increase type safety and avoid possible issues in the future.

<ComponentsSelector />
<SearchField
dataMarketing="pull-files-search"
placeholder="Search for files"
searchValue={params?.search}
setSearchValue={(search) => updateParams({ search })}

Check warning on line 54 in src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx

View check run for this annotation

Codecov - QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx#L54

Added line #L54 was not covered by tests

Check warning on line 54 in src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx

View check run for this annotation

Codecov Public QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx#L54

Added line #L54 was not covered by tests

Check warning on line 54 in src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx

View check run for this annotation

Codecov / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx#L54

Added line #L54 was not covered by tests
/>
</div>
</ContentsTableHeader>
<Table
data={data}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { MemoryRouter, Route } from 'react-router-dom'

import FileExplorer from './FileExplorer'

jest.mock('../ComponentsSelector', () => () => 'ComponentsSelector')

const queryClient = new QueryClient({
defaultOptions: {
queries: {
Expand Down Expand Up @@ -194,6 +196,16 @@ describe('FileExplorer', () => {
})
})

Copy link

Choose a reason for hiding this comment

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

Use a more explicit test for 'renders ComponentsSelector' to ensure that it contributes to the component and its behaviors as expected.

describe('when rendered', () => {
it('renders ComponentsSelector', async () => {
setup()
render(<FileExplorer />, { wrapper: wrapper() })
Copy link

Choose a reason for hiding this comment

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

Ensure that you have handled potential exceptions and erroneous cases by writing negative test cases as well, not just happy path scenarios. This will greatly boost the confidence in the correctness of the code.


const selector = await screen.findByText('ComponentsSelector')
expect(selector).toBeInTheDocument()
})
})

describe('table is displaying file tree', () => {
describe('default sort is set', () => {
it('sets default sort to name asc', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@
lines: 'LINES',
})

const getQueryFilters = ({ params, sortBy, flags }) => {
const getQueryFilters = ({ params, sortBy, flags, components }) => {

Check warning on line 156 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov - QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L156

Added line #L156 was not covered by tests

Check warning on line 156 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov Public QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L156

Added line #L156 was not covered by tests

Check warning on line 156 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L156

Added line #L156 was not covered by tests
Copy link

@codecov codecov bot Dec 12, 2023

Choose a reason for hiding this comment

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

Check that the 'components' parameter is incorporated correctly into the 'getQueryFilters' function.

return {
...(params?.search && { searchValue: params.search }),
...(params?.displayType && {
Expand All @@ -166,6 +166,7 @@
},
}),
...(flags ? { flags } : {}),
...(components ? { components } : {}),

Check warning on line 169 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov - QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L169

Added line #L169 was not covered by tests

Check warning on line 169 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov Public QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L169

Added line #L169 was not covered by tests

Check warning on line 169 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L169

Added line #L169 was not covered by tests
}
}

Expand All @@ -177,6 +178,8 @@
depth: 1,
})
Copy link

Choose a reason for hiding this comment

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

Query parameters should be validated before use. Improper or lack of validation could lead to potential security vulnerabilities such as Injections, and Denial of Service(DoS) if the input is not what's expected.

const flags = queryParams?.flags
const components = queryParams?.components

Check warning on line 181 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov - QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L181

Added line #L181 was not covered by tests

Check warning on line 181 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov Public QA / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L181

Added line #L181 was not covered by tests

Check warning on line 181 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js

View check run for this annotation

Codecov / codecov/patch

src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L181

Added line #L181 was not covered by tests

const { params } = useLocationParams(defaultQueryParams)
const { treePaths } = usePullTreePaths()
const [sortBy, setSortBy] = useTableDefaultSort()
Expand All @@ -187,7 +190,7 @@
repo,
Copy link

Choose a reason for hiding this comment

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

Update the code according to the new data structure introduced on line 75, considering the 'components' filter as well.

pullId,
path: urlPath || '',
filters: getQueryFilters({ params, sortBy: sortBy[0], flags }),
filters: getQueryFilters({ params, sortBy: sortBy[0], flags, components }),
opts: {
suspense: false,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,63 @@ describe('useRepoPullContentsTable', () => {
})
})

describe('when called with a selected components', () => {
it('makes a gql request with the list param', async () => {
Copy link

Choose a reason for hiding this comment

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

Validate that new unit tests are working properly that handle additional parameter 'components'.

const { variablesPassed } = setup()
const { result } = renderHook(() => useRepoPullContentsTable(), {
wrapper: wrapper([
'/gh/test-org/test-repo/pull/123/tree?components=a,b',
]),
})

await waitFor(() => result.current.isLoading)
await waitFor(() => !result.current.isLoading)

expect(variablesPassed).toHaveBeenCalledWith({
pullId: 123,
filters: {
components: 'a,b',
ordering: {
direction: 'ASC',
parameter: 'NAME',
},
},
owner: 'test-org',
repo: 'test-repo',
path: '',
})
})
Copy link

Choose a reason for hiding this comment

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

Please check if you have any memory leaks in your hooks especially those containing listeners or subscriptions.

})

describe('when called with a selected flags and components', () => {
Copy link

Choose a reason for hiding this comment

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

Validate that new unit tests are working properly that handle both parameters 'flags' and 'components'.

it('makes a gql request with the list param', async () => {
const { variablesPassed } = setup()
const { result } = renderHook(() => useRepoPullContentsTable(), {
wrapper: wrapper([
'/gh/test-org/test-repo/pull/123/tree?flags=a&components=b',
]),
})

await waitFor(() => result.current.isLoading)
await waitFor(() => !result.current.isLoading)

expect(variablesPassed).toHaveBeenCalledWith({
pullId: 123,
filters: {
flags: 'a',
components: 'b',
ordering: {
direction: 'ASC',
parameter: 'NAME',
},
},
owner: 'test-org',
repo: 'test-repo',
path: '',
})
})
})

describe('when handleSort is triggered', () => {
it('makes a gql request with the updated params', async () => {
const { variablesPassed } = setup()
Expand Down
Loading