Skip to content

Commit 75ae0ed

Browse files
authored
fix(widget-builder): Update table sort when field removed or modified (#82898)
When a field is removed or modified, the sort field should update accordingly. This relies on the fact that we're only going to update one step at a time (i.e. we won't update two fields through the hook in one action) and determines if: a) a field was removed b) a field was modified When a field was removed, we simply reset to the first option. When a field is modified, we look for which field that was and then update the sort to reflect that field. I had to fix a bug in the visualize component that was updating the state directly (i.e. directly modifying the memory) instead of updating it through the dispatch method. I fixed this by doing a deep clone and updating the field then calling the dispatch function.
1 parent 132dcb6 commit 75ae0ed

File tree

3 files changed

+95
-5
lines changed

3 files changed

+95
-5
lines changed

static/app/views/dashboards/widgetBuilder/components/visualize.tsx

+7-5
Original file line numberDiff line numberDiff line change
@@ -260,16 +260,18 @@ function Visualize() {
260260
: field.field
261261
}
262262
onChange={newField => {
263+
const newFields = cloneDeep(fields);
264+
const currentField = newFields[index]!;
263265
// Update the current field's aggregate with the new aggregate
264-
if (field.kind === FieldValueKind.FUNCTION) {
265-
field.function[1] = newField.value as string;
266+
if (currentField.kind === FieldValueKind.FUNCTION) {
267+
currentField.function[1] = newField.value as string;
266268
}
267-
if (field.kind === FieldValueKind.FIELD) {
268-
field.field = newField.value as string;
269+
if (currentField.kind === FieldValueKind.FIELD) {
270+
currentField.field = newField.value as string;
269271
}
270272
dispatch({
271273
type: updateAction,
272-
payload: fields,
274+
payload: newFields,
273275
});
274276
}}
275277
triggerProps={{

static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx

+49
Original file line numberDiff line numberDiff line change
@@ -827,6 +827,55 @@ describe('useWidgetBuilderState', () => {
827827
'event.type',
828828
]);
829829
});
830+
831+
it('resets the sort when the field that is being sorted is removed', () => {
832+
mockedUsedLocation.mockReturnValue(
833+
LocationFixture({
834+
query: {field: ['testField'], sort: ['-testField']},
835+
})
836+
);
837+
838+
const {result} = renderHook(() => useWidgetBuilderState(), {
839+
wrapper: WidgetBuilderProvider,
840+
});
841+
842+
expect(result.current.state.sort).toEqual([{field: 'testField', kind: 'desc'}]);
843+
844+
act(() => {
845+
result.current.dispatch({
846+
type: BuilderStateAction.SET_FIELDS,
847+
payload: [{field: 'testField2', kind: FieldValueKind.FIELD}],
848+
});
849+
});
850+
851+
expect(result.current.state.sort).toEqual([{field: 'testField2', kind: 'desc'}]);
852+
});
853+
854+
it('modifies the sort when the field that is being sorted is modified', () => {
855+
mockedUsedLocation.mockReturnValue(
856+
LocationFixture({
857+
query: {field: ['testField', 'sortField'], sort: ['-sortField']},
858+
})
859+
);
860+
861+
const {result} = renderHook(() => useWidgetBuilderState(), {
862+
wrapper: WidgetBuilderProvider,
863+
});
864+
865+
expect(result.current.state.sort).toEqual([{field: 'sortField', kind: 'desc'}]);
866+
867+
act(() => {
868+
result.current.dispatch({
869+
type: BuilderStateAction.SET_FIELDS,
870+
payload: [
871+
{field: 'testField', kind: FieldValueKind.FIELD},
872+
{field: 'newSortField', kind: FieldValueKind.FIELD},
873+
],
874+
});
875+
});
876+
877+
expect(result.current.state.sort).toEqual([{field: 'newSortField', kind: 'desc'}]);
878+
});
830879
});
831880

832881
describe('yAxis', () => {

static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx

+39
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,45 @@ function useWidgetBuilderState(): {
233233
break;
234234
case BuilderStateAction.SET_FIELDS:
235235
setFields(action.payload);
236+
const isRemoved = action.payload.length < (fields?.length ?? 0);
237+
if (
238+
displayType === DisplayType.TABLE &&
239+
action.payload.length > 0 &&
240+
!action.payload.find(
241+
field => generateFieldAsString(field) === sort?.[0]?.field
242+
)
243+
) {
244+
if (isRemoved) {
245+
setSort([
246+
{
247+
kind: 'desc',
248+
field: generateFieldAsString(action.payload[0] as QueryFieldValue),
249+
},
250+
]);
251+
} else {
252+
// Find the index of the first field that doesn't match the old fields.
253+
const changedFieldIndex = action.payload.findIndex(
254+
field =>
255+
!fields?.find(
256+
originalField =>
257+
generateFieldAsString(originalField) ===
258+
generateFieldAsString(field)
259+
)
260+
);
261+
if (changedFieldIndex !== -1) {
262+
// At this point, we can assume the fields are the same length so
263+
// using the changedFieldIndex in action.payload is safe.
264+
setSort([
265+
{
266+
kind: sort?.[0]?.kind ?? 'desc',
267+
field: generateFieldAsString(
268+
action.payload[changedFieldIndex] as QueryFieldValue
269+
),
270+
},
271+
]);
272+
}
273+
}
274+
}
236275
break;
237276
case BuilderStateAction.SET_Y_AXIS:
238277
setYAxis(action.payload);

0 commit comments

Comments
 (0)