-
Notifications
You must be signed in to change notification settings - Fork 23
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
Changes from all commits
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 |
---|---|---|
|
@@ -13,6 +13,8 @@ | |
|
||
import { useRepoPullContentsTable } from './hooks' | ||
|
||
import ComponentsSelector from '../ComponentsSelector' | ||
|
||
const Loader = () => ( | ||
<div className="flex flex-1 justify-center"> | ||
<Spinner size={60} /> | ||
|
@@ -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 })} | ||
/> | ||
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. 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"> | ||
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. You have added a new component ( |
||
<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 Codecov - QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx#L54
Check warning on line 54 in src/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx Codecov Public QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/FileExplorer.jsx#L54
|
||
/> | ||
</div> | ||
</ContentsTableHeader> | ||
<Table | ||
data={data} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: { | ||
|
@@ -194,6 +196,16 @@ describe('FileExplorer', () => { | |
}) | ||
}) | ||
|
||
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. 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() }) | ||
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. 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 () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 Codecov - QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L156
Check warning on line 156 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js Codecov Public QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L156
|
||
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. Check that the 'components' parameter is incorporated correctly into the 'getQueryFilters' function. |
||
return { | ||
...(params?.search && { searchValue: params.search }), | ||
...(params?.displayType && { | ||
|
@@ -166,6 +166,7 @@ | |
}, | ||
}), | ||
...(flags ? { flags } : {}), | ||
...(components ? { components } : {}), | ||
Check warning on line 169 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js Codecov - QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L169
Check warning on line 169 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js Codecov Public QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L169
|
||
} | ||
} | ||
|
||
|
@@ -177,6 +178,8 @@ | |
depth: 1, | ||
}) | ||
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. 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 Codecov - QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L181
Check warning on line 181 in src/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js Codecov Public QA / codecov/patchsrc/pages/PullRequestPage/subroute/FileExplorer/hooks/useRepoPullContentsTable.js#L181
|
||
|
||
const { params } = useLocationParams(defaultQueryParams) | ||
const { treePaths } = usePullTreePaths() | ||
const [sortBy, setSortBy] = useTableDefaultSort() | ||
|
@@ -187,7 +190,7 @@ | |
repo, | ||
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. 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, | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -258,6 +258,63 @@ describe('useRepoPullContentsTable', () => { | |
}) | ||
}) | ||
|
||
describe('when called with a selected components', () => { | ||
it('makes a gql request with the list param', async () => { | ||
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. 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: '', | ||
}) | ||
}) | ||
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. 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', () => { | ||
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. 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() | ||
|
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.
Ensure that 'ComponentsSelector' is properly exported from its module, as it is being imported here.