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(recordings): render labels in 'key=value' form #1313

Merged
merged 16 commits into from
Jul 30, 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
7 changes: 4 additions & 3 deletions src/app/RecordingMetadata/ClickableLabel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { KeyValue } from '@app/Shared/Services/api.types';
import { KeyValue, keyValueToString } from '@app/Shared/Services/api.types';
import { Label } from '@patternfly/react-core';
import * as React from 'react';

Expand Down Expand Up @@ -47,16 +47,17 @@ export const ClickableLabel: React.FC<ClickableLabelCellProps> = ({ label, isSel
return (
<>
<Label
aria-label={`${label.key}: ${label.value}`}
aria-label={keyValueToString(label)}
style={style}
isTruncated
onMouseEnter={handleHoveredOrFocused}
onMouseLeave={handleNonHoveredOrFocused}
onFocus={handleHoveredOrFocused}
onClick={handleLabelClicked}
key={label.key}
color={labelColor}
>
{`${label.key}: ${label.value}`}
{keyValueToString(label)}
</Label>
</>
);
Expand Down
11 changes: 5 additions & 6 deletions src/app/RecordingMetadata/LabelCell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@
*/

import { UpdateFilterOptions } from '@app/Shared/Redux/Filters/Common';
import { KeyValue } from '@app/Shared/Services/api.types';
import { KeyValue, keyValueToString } from '@app/Shared/Services/api.types';
import { Label, Text } from '@patternfly/react-core';
import * as React from 'react';
import { ClickableLabel } from './ClickableLabel';
import { getLabelDisplay } from './utils';

export interface LabelCellProps {
target: string;
Expand All @@ -36,7 +35,7 @@ export const LabelCell: React.FC<LabelCellProps> = ({ target, labels, clickableO
(label: KeyValue) => {
if (clickableOptions) {
const labelFilterSet = new Set(clickableOptions.labelFilters);
return labelFilterSet.has(getLabelDisplay(label));
return labelFilterSet.has(keyValueToString(label));
}
return false;
},
Expand All @@ -52,7 +51,7 @@ export const LabelCell: React.FC<LabelCellProps> = ({ target, labels, clickableO
if (clickableOptions) {
clickableOptions.updateFilters(target, {
filterKey: 'Label',
filterValue: getLabelDisplay(clickedLabel),
filterValue: keyValueToString(clickedLabel),
deleted: isLabelSelected(clickedLabel),
});
}
Expand All @@ -72,8 +71,8 @@ export const LabelCell: React.FC<LabelCellProps> = ({ target, labels, clickableO
onLabelClick={onLabelSelectToggle}
/>
) : (
<Label aria-label={`${label.key}: ${label.value}`} key={label.key} color={getLabelColor(label)}>
{`${label.key}: ${label.value}`}
<Label aria-label={keyValueToString(label)} key={label.key} color={getLabelColor(label)}>
{keyValueToString(label)}
</Label>
),
)
Expand Down
2 changes: 0 additions & 2 deletions src/app/RecordingMetadata/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ export const parseLabelsFromFile = (file: File): Observable<KeyValue[]> => {
);
};

export const getLabelDisplay = (label: KeyValue) => `${label.key}:${label.value}`;

export const LabelPattern = /^\S+$/;

export const getValidatedOption = (isValid: boolean) => {
Expand Down
30 changes: 18 additions & 12 deletions src/app/Recordings/ActiveRecordingsTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ import {
ActiveRecording,
AnalysisResult,
CategorizedRuleEvaluations,
KeyValue,
NotificationCategory,
RecordingState,
Target,
keyValueToString,
} from '@app/Shared/Services/api.types';
import { ServiceContext } from '@app/Shared/Services/Services';
import { useDayjs } from '@app/utils/hooks/useDayjs';
Expand Down Expand Up @@ -902,6 +904,18 @@ export const ActiveRecordingRow: React.FC<ActiveRecordingRowProps> = ({
handleLoadAnalysis();
}, [isExpanded, handleLoadAnalysis]);

const recordingOptions = (recording: ActiveRecording): KeyValue[] => {
const options: KeyValue[] = [];
options.push({ key: 'toDisk', value: String(recording.toDisk) });
if (recording.maxAge) {
options.push({ key: 'maxAge', value: `${recording.maxAge / 1000}s` });
}
if (recording.maxSize) {
options.push({ key: 'maxSize', value: formatBytes(recording.maxAge) });
}
return options;
};

const parentRow = React.useMemo(() => {
const RecordingDuration = (props: { duration: number }) => {
const str = React.useMemo(
Expand Down Expand Up @@ -952,19 +966,11 @@ export const ActiveRecordingRow: React.FC<ActiveRecordingRowProps> = ({
</Td>
<Td key={`active-table-row-${index}_6`} dataLabel={tableColumns[4].title}>
<LabelGroup isVertical style={{ padding: '0.2em' }}>
<Label color="blue" key="toDisk">
toDisk: {String(recording.toDisk)}
</Label>
{recording.maxAge ? (
<Label color="blue" key="maxAge">
maxAge: {recording.maxAge / 1000}s
{recordingOptions(recording).map((options) => (
<Label color="blue" key={options.key} isTruncated>
{keyValueToString(options)}
</Label>
) : undefined}
{recording.maxSize ? (
<Label color="blue" key="maxSize">
maxSize: {formatBytes(recording.maxSize)}
</Label>
) : undefined}
))}
</LabelGroup>
</Td>
<Td key={`active-table-row-${index}_7`} dataLabel={tableColumns[5].title}>
Expand Down
5 changes: 2 additions & 3 deletions src/app/Recordings/Filters/LabelFilter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
* limitations under the License.
*/

import { getLabelDisplay } from '@app/RecordingMetadata/utils';
import { Recording } from '@app/Shared/Services/api.types';
import { Recording, keyValueToString } from '@app/Shared/Services/api.types';
import { Label, Select, SelectOption, SelectVariant } from '@patternfly/react-core';
import * as React from 'react';

Expand All @@ -42,7 +41,7 @@ export const LabelFilter: React.FC<LabelFilterProps> = ({ recordings, filteredLa
const labels = new Set<string>();
recordings.forEach((r) => {
if (!r || !r.metadata || !r.metadata.labels) return;
r.metadata.labels.map((label) => labels.add(getLabelDisplay(label)));
r.metadata.labels.map((label) => labels.add(keyValueToString(label)));
});
return Array.from(labels)
.filter((l) => !filteredLabels.includes(l))
Expand Down
4 changes: 2 additions & 2 deletions src/app/Recordings/RecordingFilters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
allowedArchivedRecordingFilters,
} from '@app/Shared/Redux/Filters/RecordingFilterSlice';
import { recordingUpdateCategoryIntent, RootState, StateDispatch } from '@app/Shared/Redux/ReduxStore';
import { Recording, RecordingState } from '@app/Shared/Services/api.types';
import { KeyValue, Recording, RecordingState, keyValueToString } from '@app/Shared/Services/api.types';
import { useDayjs } from '@app/utils/hooks/useDayjs';
import dayjs from '@i18n/datetime';
import {
Expand Down Expand Up @@ -294,7 +294,7 @@ export const filterRecordings = (recordings: any[], filters: RecordingFiltersCat
}
if (filters.Label.length) {
filtered = filtered.filter((recording) => {
const recordingLabels = recording.metadata.labels.map((label) => `${label.key}:${label.value}`);
const recordingLabels = recording.metadata.labels.map((label: KeyValue) => keyValueToString(label));
return filters.Label.some((filterLabel) => recordingLabels.includes(filterLabel));
});
}
Expand Down
16 changes: 14 additions & 2 deletions src/app/Topology/Toolbar/TopologyFilters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,13 @@ export const TopologyFilter: React.FC<{ isDisabled?: boolean }> = ({ isDisabled,
},
}}
>
{isLabelOrAnnotation(cat) ? <Label color="grey">{opt}</Label> : opt}
{isLabelOrAnnotation(cat) ? (
<Label color="grey" isTruncated>
{opt}
</Label>
) : (
opt
)}
</SelectOption>
))}
</SelectGroup>
Expand Down Expand Up @@ -285,7 +291,13 @@ export const TopologyFilter: React.FC<{ isDisabled?: boolean }> = ({ isDisabled,
},
}}
>
{isLabelOrAnnotation(cat) ? <Label color="grey">{opt}</Label> : opt}
{isLabelOrAnnotation(cat) ? (
<Label color="grey" isTruncated>
{opt}
</Label>
) : (
opt
)}
</SelectOption>
);
});
Expand Down
10 changes: 5 additions & 5 deletions src/test/RecordingMetadata/BulkEditLabels.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ describe('<BulkEditLabels />', () => {
},
});

const label = screen.getByLabelText('someLabel: someValue');
const label = screen.getByLabelText('someLabel=someValue');
expect(label).toBeInTheDocument();
expect(label).toBeVisible();
expect(label.onclick).toBeNull();
Expand Down Expand Up @@ -262,11 +262,11 @@ describe('<BulkEditLabels />', () => {
},
});

const newLabel = screen.getByLabelText('someNewLabel: someNewValue');
const newLabel = screen.getByLabelText('someNewLabel=someNewValue');
expect(newLabel).toBeInTheDocument();
expect(newLabel).toBeVisible();

const oldLabel = screen.getByLabelText('someLabel: someValue');
const oldLabel = screen.getByLabelText('someLabel=someValue');
expect(oldLabel).toBeInTheDocument();
expect(oldLabel).toBeVisible();
});
Expand All @@ -283,11 +283,11 @@ describe('<BulkEditLabels />', () => {
},
});

const newLabel = screen.getByLabelText('someNewLabel: someNewValue');
const newLabel = screen.getByLabelText('someNewLabel=someNewValue');
expect(newLabel).toBeInTheDocument();
expect(newLabel).toBeVisible();

const oldLabel = screen.getByLabelText('someLabel: someValue');
const oldLabel = screen.getByLabelText('someLabel=someValue');
expect(oldLabel).toBeInTheDocument();
expect(oldLabel).toBeVisible();
});
Expand Down
2 changes: 1 addition & 1 deletion src/test/RecordingMetadata/ClickableLabel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const mockLabel = {
key: 'someLabel',
value: 'someValue',
} as KeyValue;
const mockLabelAsString = 'someLabel: someValue';
const mockLabelAsString = 'someLabel=someValue';

const onLabelClick = jest.fn((_label: KeyValue) => {
/**Do nothing. Used for checking renders */
Expand Down
12 changes: 5 additions & 7 deletions src/test/RecordingMetadata/LabelCell.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,10 @@ const mockAnotherLabel = { key: 'anotherLabel', value: 'anotherValue' } as KeyVa
const mockLabelList = [mockLabel, mockAnotherLabel];

// For display
const mockLabelStringDisplayList = [
`${mockLabel.key}: ${mockLabel.value}`,
`${mockAnotherLabel.key}: ${mockAnotherLabel.value}`,
const mockLabelStringList = [
`${mockLabel.key}=${mockLabel.value}`,
`${mockAnotherLabel.key}=${mockAnotherLabel.value}`,
];
// For filters and labeling elements
const mockLabelStringList = mockLabelStringDisplayList.map((s: string) => s.replace(' ', ''));

describe('<LabelCell />', () => {
let onUpdateLabels: (target: string, updateFilterOptions: UpdateFilterOptions) => void;
Expand Down Expand Up @@ -84,7 +82,7 @@ describe('<LabelCell />', () => {
},
});

for (const labelAsString of mockLabelStringDisplayList) {
for (const labelAsString of mockLabelStringList) {
const displayedLabel = screen.getByLabelText(labelAsString);

expect(displayedLabel).toBeInTheDocument();
Expand Down Expand Up @@ -117,7 +115,7 @@ describe('<LabelCell />', () => {

let count = 0;
let index = 0;
for (const labelAsString of mockLabelStringDisplayList) {
for (const labelAsString of mockLabelStringList) {
const displayedLabel = screen.getByLabelText(labelAsString);

expect(displayedLabel).toBeInTheDocument();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ exports[`<BulkEditLabels /> renders correctly 1`] = `
className="pf-l-stack__item"
>
<span
aria-label="someLabel: someValue"
aria-label="someLabel=someValue"
className="pf-c-label"
>
<span
className="pf-c-label__content"
>
someLabel: someValue
someLabel=someValue
</span>
</span>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

exports[`<ClickableLabel /> renders correctly 1`] = `
<span
aria-label="someLabel: someValue"
aria-label="someLabel=someValue"
className="pf-c-label pf-m-blue"
onClick={[Function]}
onFocus={[Function]}
Expand All @@ -13,7 +13,11 @@ exports[`<ClickableLabel /> renders correctly 1`] = `
<span
className="pf-c-label__content"
>
someLabel: someValue
<span
className="pf-c-label__text"
>
someLabel=someValue
</span>
</span>
</span>
`;
16 changes: 12 additions & 4 deletions src/test/RecordingMetadata/__snapshots__/LabelCell.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`<LabelCell /> renders correctly 1`] = `
Array [
<span
aria-label="someLabel: someValue"
aria-label="someLabel=someValue"
className="pf-c-label"
onClick={[Function]}
onFocus={[Function]}
Expand All @@ -14,11 +14,15 @@ Array [
<span
className="pf-c-label__content"
>
someLabel: someValue
<span
className="pf-c-label__text"
>
someLabel=someValue
</span>
</span>
</span>,
<span
aria-label="anotherLabel: anotherValue"
aria-label="anotherLabel=anotherValue"
className="pf-c-label"
onClick={[Function]}
onFocus={[Function]}
Expand All @@ -29,7 +33,11 @@ Array [
<span
className="pf-c-label__content"
>
anotherLabel: anotherValue
<span
className="pf-c-label__text"
>
anotherLabel=anotherValue
</span>
</span>
</span>,
]
Expand Down
6 changes: 3 additions & 3 deletions src/test/Recordings/ActiveRecordingsTable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ describe('<ActiveRecordingsTable />', () => {
expect(state).toBeVisible();

mockRecordingLabels.forEach((entry) => {
const label = screen.getByText(`${entry.key}: ${entry.value}`);
const label = screen.getByText(`${entry.key}=${entry.value}`);
expect(label).toBeInTheDocument();
expect(label).toBeVisible();
});
Expand Down Expand Up @@ -285,8 +285,8 @@ describe('<ActiveRecordingsTable />', () => {
preloadedState: preloadedState,
});

expect(screen.getByText('someLabel: someUpdatedValue')).toBeInTheDocument();
expect(screen.queryByText('someLabel: someValue')).not.toBeInTheDocument();
expect(screen.getByText('someLabel=someUpdatedValue')).toBeInTheDocument();
expect(screen.queryByText('someLabel=someValue')).not.toBeInTheDocument();
});

it('stops a Recording after receiving a notification', async () => {
Expand Down
6 changes: 3 additions & 3 deletions src/test/Recordings/ArchivedRecordingsTable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ describe('<ArchivedRecordingsTable />', () => {
}); */

for (const entry of mockRecordingLabels) {
const label = await screen.findByText(`${entry.key}: ${entry.value}`);
const label = await screen.findByText(`${entry.key}=${entry.value}`);
expect(label).toBeInTheDocument();
expect(label).toBeVisible();
}
Expand Down Expand Up @@ -333,8 +333,8 @@ describe('<ArchivedRecordingsTable />', () => {
},
preloadedState: preloadedState,
});
expect(screen.getByText('someLabel: someUpdatedValue')).toBeInTheDocument();
expect(screen.queryByText('someLabel: someValue')).not.toBeInTheDocument();
expect(screen.getByText('someLabel=someUpdatedValue')).toBeInTheDocument();
expect(screen.queryByText('someLabel=someValue')).not.toBeInTheDocument();
});

it('removes a Recording after receiving a notification', async () => {
Expand Down
Loading
Loading