-
Notifications
You must be signed in to change notification settings - Fork 28
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: respect settings for showing also in overview table #2861
base: chore/user-config-handling-and-performance-fixes
Are you sure you want to change the base?
Changes from all commits
3cd7899
7b9d4d4
fa3605e
eb8308d
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 |
---|---|---|
|
@@ -7,16 +7,19 @@ import MuiTable from '@mui/material/Table'; | |
import MuiTableBody from '@mui/material/TableBody'; | ||
import MuiTableContainer from '@mui/material/TableContainer'; | ||
import { orderBy, upperFirst } from 'lodash'; | ||
import { useMemo, useState } from 'react'; | ||
import { useCallback, useMemo, useState } from 'react'; | ||
|
||
import { text } from '../../../shared/text'; | ||
import { useShowClassifications } from '../../state/variables/use-show-classifications'; | ||
import { useShowCriticality } from '../../state/variables/use-show-criticality'; | ||
import { | ||
LicenseCounts, | ||
LicenseNamesWithClassification, | ||
LicenseNamesWithCriticality, | ||
} from '../../types/types'; | ||
import { Order } from '../TableCellWithSorting/TableCellWithSorting'; | ||
import { | ||
Column, | ||
ColumnConfig, | ||
orderLicenseNames, | ||
SingleColumn, | ||
|
@@ -32,12 +35,16 @@ const classes = { | |
}, | ||
}; | ||
|
||
interface AttributionCountPerSourcePerLicenseTableProps { | ||
export interface AttributionCountPerSourcePerLicenseTableProps { | ||
licenseCounts: LicenseCounts; | ||
licenseNamesWithCriticality: LicenseNamesWithCriticality; | ||
licenseNamesWithClassification: LicenseNamesWithClassification; | ||
} | ||
|
||
const defaultOrdering: TableOrdering = { | ||
orderDirection: 'asc', | ||
orderedColumn: SingleColumn.NAME, | ||
}; | ||
export const AttributionCountPerSourcePerLicenseTable: React.FC< | ||
AttributionCountPerSourcePerLicenseTableProps | ||
> = (props) => { | ||
|
@@ -47,6 +54,39 @@ export const AttributionCountPerSourcePerLicenseTable: React.FC< | |
props.licenseCounts.totalAttributionsPerSource, | ||
); | ||
|
||
const showCriticality = useShowCriticality(); | ||
const showClassifications = useShowClassifications(); | ||
|
||
const getCriticalityColumn = useCallback((): Array<Column> => { | ||
if (showCriticality) { | ||
return [ | ||
{ | ||
columnName: componentText.columns.criticality.title, | ||
columnType: SingleColumn.CRITICALITY, | ||
columnId: SingleColumn.CRITICALITY, | ||
align: 'center', | ||
defaultOrder: 'desc', | ||
}, | ||
]; | ||
} | ||
return []; | ||
}, [componentText.columns.criticality.title, showCriticality]); | ||
|
||
const getClassificationColumn = useCallback((): Array<Column> => { | ||
if (showClassifications) { | ||
return [ | ||
{ | ||
columnName: componentText.columns.classification, | ||
columnType: SingleColumn.CLASSIFICATION, | ||
columnId: SingleColumn.CLASSIFICATION, | ||
align: 'center', | ||
defaultOrder: 'desc', | ||
}, | ||
]; | ||
} | ||
return []; | ||
}, [componentText.columns.classification, showClassifications]); | ||
|
||
const columnConfig: ColumnConfig = useMemo( | ||
() => | ||
new ColumnConfig([ | ||
|
@@ -60,20 +100,8 @@ export const AttributionCountPerSourcePerLicenseTable: React.FC< | |
align: 'left', | ||
defaultOrder: 'asc', | ||
}, | ||
{ | ||
columnName: componentText.columns.criticality.title, | ||
columnType: SingleColumn.CRITICALITY, | ||
columnId: SingleColumn.CRITICALITY, | ||
align: 'center', | ||
defaultOrder: 'desc', | ||
}, | ||
{ | ||
columnName: componentText.columns.classification, | ||
columnType: SingleColumn.CLASSIFICATION, | ||
columnId: SingleColumn.CLASSIFICATION, | ||
align: 'center', | ||
defaultOrder: 'desc', | ||
}, | ||
...getCriticalityColumn(), | ||
...getClassificationColumn(), | ||
], | ||
}, | ||
{ | ||
|
@@ -96,16 +124,33 @@ export const AttributionCountPerSourcePerLicenseTable: React.FC< | |
], | ||
}, | ||
]), | ||
[sourceNames, componentText], | ||
[ | ||
componentText.columns.licenseInfo, | ||
componentText.columns.licenseName, | ||
componentText.columns.signalCountPerSource, | ||
componentText.columns.totalSources, | ||
getCriticalityColumn, | ||
getClassificationColumn, | ||
sourceNames, | ||
], | ||
); | ||
|
||
const [ordering, setOrdering] = useState<TableOrdering>({ | ||
orderDirection: 'asc', | ||
orderedColumn: SingleColumn.NAME, | ||
}); | ||
const [ordering, setOrdering] = useState<TableOrdering>(defaultOrdering); | ||
const effectiveOrdering = columnConfig.getColumnById(ordering.orderedColumn) | ||
? ordering | ||
: defaultOrdering; | ||
|
||
const handleRequestSort = (columnId: string, defaultOrder: Order) => { | ||
if (ordering.orderedColumn === columnId) { | ||
if ( | ||
effectiveOrdering !== ordering && | ||
effectiveOrdering.orderedColumn === columnId | ||
) { | ||
setOrdering({ | ||
...effectiveOrdering, | ||
orderDirection: | ||
effectiveOrdering.orderDirection === 'asc' ? 'desc' : 'asc', | ||
}); | ||
} else if (ordering.orderedColumn === columnId) { | ||
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. Can we not just go into the first case regardless of whether effectiveOrdering equals ordering? |
||
setOrdering((currentOrdering) => ({ | ||
...currentOrdering, | ||
orderDirection: | ||
|
@@ -121,30 +166,31 @@ export const AttributionCountPerSourcePerLicenseTable: React.FC< | |
|
||
const orderedLicenseNames = useMemo(() => { | ||
const orderedColumnType = columnConfig.getColumnById( | ||
ordering.orderedColumn, | ||
effectiveOrdering.orderedColumn, | ||
)?.columnType; | ||
|
||
if (orderedColumnType === undefined) { | ||
return orderBy( | ||
Object.keys(props.licenseNamesWithCriticality), | ||
(licenseName) => licenseName.toLowerCase(), | ||
ordering.orderDirection, | ||
effectiveOrdering.orderDirection, | ||
); | ||
} | ||
|
||
return orderLicenseNames( | ||
props.licenseNamesWithCriticality, | ||
props.licenseNamesWithClassification, | ||
props.licenseCounts, | ||
ordering.orderDirection, | ||
effectiveOrdering.orderDirection, | ||
orderedColumnType, | ||
); | ||
}, [ | ||
columnConfig, | ||
effectiveOrdering.orderedColumn, | ||
effectiveOrdering.orderDirection, | ||
props.licenseNamesWithCriticality, | ||
props.licenseNamesWithClassification, | ||
props.licenseCounts, | ||
columnConfig, | ||
ordering, | ||
]); | ||
|
||
return ( | ||
|
@@ -153,7 +199,7 @@ export const AttributionCountPerSourcePerLicenseTable: React.FC< | |
<MuiTable size="small" stickyHeader> | ||
<AttributionCountPerSourcePerLicenseTableHead | ||
columnConfig={columnConfig} | ||
tableOrdering={ordering} | ||
tableOrdering={effectiveOrdering} | ||
onRequestSort={handleRequestSort} | ||
/> | ||
<MuiTableBody> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
// SPDX-FileCopyrightText: Meta Platforms, Inc. and its affiliates | ||
// SPDX-FileCopyrightText: TNG Technology Consulting GmbH <https://www.tngtech.com> | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
import { Screen } from '@testing-library/dom/types/screen'; | ||
import { act, fireEvent, screen } from '@testing-library/react'; | ||
|
||
import { Criticality } from '../../../../shared/shared-types'; | ||
import { setUserSetting } from '../../../state/actions/user-settings-actions/user-settings-actions'; | ||
import { renderComponent } from '../../../test-helpers/render'; | ||
import { LicenseCounts } from '../../../types/types'; | ||
import { | ||
AttributionCountPerSourcePerLicenseTable, | ||
AttributionCountPerSourcePerLicenseTableProps, | ||
} from '../AttributionCountPerSourcePerLicenseTable'; | ||
|
||
const licenseCounts: LicenseCounts = { | ||
attributionCountPerSourcePerLicense: { | ||
licenseA: { | ||
sourceA: 1, | ||
sourceB: 3, | ||
}, | ||
licenseB: { | ||
sourceB: 2, | ||
}, | ||
}, | ||
totalAttributionsPerLicense: { | ||
licenseA: 4, | ||
licenseB: 2, | ||
}, | ||
totalAttributionsPerSource: { | ||
sourceA: 1, | ||
sourceB: 5, | ||
}, | ||
}; | ||
const props: AttributionCountPerSourcePerLicenseTableProps = { | ||
licenseCounts, | ||
licenseNamesWithCriticality: { | ||
licenseA: Criticality.High, | ||
licenseB: Criticality.Medium, | ||
}, | ||
licenseNamesWithClassification: { | ||
licenseA: 2, | ||
licenseB: 3, | ||
}, | ||
}; | ||
|
||
function expectHeaderTextsToEqual( | ||
screen: Screen, | ||
expectedHeaderTexts: Array<string>, | ||
) { | ||
const headerTexts = screen | ||
.getAllByTestId('table-cell-with-sorting') | ||
.map((node) => node.textContent); | ||
|
||
expect(headerTexts).toEqual(expectedHeaderTexts); | ||
} | ||
|
||
describe('Attribution count per source per license table', () => { | ||
it('shows by default criticality and classification columns', () => { | ||
renderComponent(<AttributionCountPerSourcePerLicenseTable {...props} />); | ||
|
||
expectHeaderTextsToEqual(screen, [ | ||
'Namesorted ascending', //correct, the sorted, ascending is for a11y | ||
'Criticality', | ||
'Classification', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
}); | ||
Comment on lines
+59
to
+71
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. Is there a way to get rid of the linter warnings that these tests have no assertions? Maybe it's better to move the expect statement out of the function back into the test cases themselves? |
||
|
||
it('does not show criticality if disabled', () => { | ||
renderComponent(<AttributionCountPerSourcePerLicenseTable {...props} />, { | ||
actions: [setUserSetting({ showCriticality: false })], | ||
}); | ||
|
||
expectHeaderTextsToEqual(screen, [ | ||
'Namesorted ascending', //correct, the sorted, ascending is for a11y | ||
'Classification', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
}); | ||
|
||
it('does not show classification if disabled', () => { | ||
renderComponent(<AttributionCountPerSourcePerLicenseTable {...props} />, { | ||
actions: [setUserSetting({ showClassifications: false })], | ||
}); | ||
|
||
expectHeaderTextsToEqual(screen, [ | ||
'Namesorted ascending', //correct, the sorted, ascending is for a11y | ||
'Criticality', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
}); | ||
|
||
it('switches back to default sorting if the sorted by column is dropped', () => { | ||
const { store } = renderComponent( | ||
<AttributionCountPerSourcePerLicenseTable {...props} />, | ||
); | ||
expectHeaderTextsToEqual(screen, [ | ||
'Namesorted ascending', //correct, the sorted, ascending is for a11y | ||
'Criticality', | ||
'Classification', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
|
||
fireEvent.click(screen.getByText('Criticality')); | ||
|
||
expectHeaderTextsToEqual(screen, [ | ||
'Name', | ||
'Criticalitysorted descending', //correct, the sorted, descending is for a11y | ||
'Classification', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
|
||
act(() => { | ||
store.dispatch(setUserSetting({ showCriticality: false })); | ||
}); | ||
|
||
expectHeaderTextsToEqual(screen, [ | ||
'Namesorted ascending', //correct, the sorted, ascending is for a11y | ||
'Classification', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
|
||
fireEvent.click(screen.getByText('Name')); | ||
|
||
expectHeaderTextsToEqual(screen, [ | ||
'Namesorted descending', //correct, the sorted, descending is for a11y | ||
'Classification', | ||
'SourceA', | ||
'SourceB', | ||
'Total', | ||
]); | ||
}); | ||
}); |
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.
Did you commit this on purpose?