mirror of
https://github.com/cpvalente/ontime.git
synced 2026-07-26 10:38:55 +00:00
feat(rundown): clear a mixed field explicitly, cover both fixes with tests
Emptying a field which holds different values across the selection does not submit, otherwise focusing it and tabbing away would wipe every entry. That left no way to clear such a field, so text fields now offer an explicit Clear action, shown only while the entries disagree. Once they agree the field can be emptied by hand and the action is withdrawn. Both of the fixes which multi edit depends on were unguarded, so the logic moves into pure helpers alongside the existing hook helper convention: - shouldSubmitValue covers an unknown initial value being submitted as an empty string - applyPatchToEvents covers the optimistic batch update, whose signature now makes the original mistake of spreading the request body unrepresentable, and pins the custom field merge Both tests were confirmed to fail against the original behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDETGBrhwgAqgTHjmrmJGJ
This commit is contained in:
+31
@@ -0,0 +1,31 @@
|
||||
import { shouldSubmitValue } from '../useReactiveTextInput';
|
||||
|
||||
describe('shouldSubmitValue()', () => {
|
||||
it('submits a value which differs from the initial value', () => {
|
||||
expect(shouldSubmitValue('new', 'old')).toBe(true);
|
||||
});
|
||||
|
||||
it('does not submit a value which has not changed', () => {
|
||||
expect(shouldSubmitValue('same', 'same')).toBe(false);
|
||||
});
|
||||
|
||||
it('submits an empty value when the initial value is known', () => {
|
||||
// clearing a field which has a value is a valid edit
|
||||
expect(shouldSubmitValue('', 'old')).toBe(true);
|
||||
});
|
||||
|
||||
it('does not submit an empty value when the initial value is unknown', () => {
|
||||
// an unknown value renders as an empty field, submitting it on blur
|
||||
// would overwrite every entry being edited with an empty value
|
||||
expect(shouldSubmitValue('', undefined)).toBe(false);
|
||||
});
|
||||
|
||||
it('submits a typed value when the initial value is unknown', () => {
|
||||
expect(shouldSubmitValue('typed', undefined)).toBe(true);
|
||||
});
|
||||
|
||||
it('submits an unchanged value when the caller opts in', () => {
|
||||
expect(shouldSubmitValue('same', 'same', true)).toBe(true);
|
||||
expect(shouldSubmitValue('', undefined, true)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,23 @@
|
||||
import { HotkeyItem, getHotkeyHandler } from '@mantine/hooks';
|
||||
import { ChangeEvent, KeyboardEvent, RefObject, useCallback, useEffect, useMemo, useRef, useState } from 'react';
|
||||
|
||||
/**
|
||||
* Whether a value should be sent to the server
|
||||
* An undefined initial value means that the value is unknown, for example when several
|
||||
* entries are being edited and they do not agree on a value
|
||||
* It is shown as an empty field, so submitting it would overwrite data the user has not edited
|
||||
*/
|
||||
export function shouldSubmitValue(
|
||||
valueToSubmit: string,
|
||||
initialText: string | undefined,
|
||||
allowSubmitSameValue?: boolean,
|
||||
): boolean {
|
||||
if (allowSubmitSameValue) {
|
||||
return true;
|
||||
}
|
||||
return valueToSubmit !== (initialText ?? '');
|
||||
}
|
||||
|
||||
interface UseReactiveTextInputReturn {
|
||||
value: string;
|
||||
onChange: (event: ChangeEvent<HTMLInputElement | HTMLTextAreaElement>) => void;
|
||||
@@ -58,9 +75,7 @@ export default function useReactiveTextInput(
|
||||
*/
|
||||
const handleSubmit = useCallback(
|
||||
(valueToSubmit: string) => {
|
||||
// No need to update if it hasn't changed
|
||||
// an undefined initial value is shown as an empty field, submitting it would overwrite the underlying data
|
||||
if (valueToSubmit === (initialText ?? '') && !options?.allowSubmitSameValue) {
|
||||
if (!shouldSubmitValue(valueToSubmit, initialText, options?.allowSubmitSameValue)) {
|
||||
options?.onCancelUpdate?.();
|
||||
} else {
|
||||
const cleanVal = valueToSubmit.trim();
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
import { OntimeEvent, OntimeGroup, RundownEntries, SupportedEntry } from 'ontime-types';
|
||||
|
||||
import { applyPatchToEvents, canPredictBatchResult } from '../entryAction.utils';
|
||||
|
||||
function makeEntries(): RundownEntries {
|
||||
return {
|
||||
'1': { type: SupportedEntry.Event, id: '1', title: 'one', custom: { lx: 'a', sound: 'x' } } as OntimeEvent,
|
||||
'2': { type: SupportedEntry.Event, id: '2', title: 'two', custom: { lx: 'b' } } as OntimeEvent,
|
||||
'3': { type: SupportedEntry.Event, id: '3', title: 'three', custom: {} } as OntimeEvent,
|
||||
group: { type: SupportedEntry.Group, id: 'group', title: 'a group', custom: {} } as OntimeGroup,
|
||||
};
|
||||
}
|
||||
|
||||
describe('applyPatchToEvents()', () => {
|
||||
it('applies the patch to the given events only', () => {
|
||||
const patched = applyPatchToEvents(makeEntries(), ['1', '2'], { title: 'patched' });
|
||||
|
||||
expect((patched['1'] as OntimeEvent).title).toBe('patched');
|
||||
expect((patched['2'] as OntimeEvent).title).toBe('patched');
|
||||
expect((patched['3'] as OntimeEvent).title).toBe('three');
|
||||
});
|
||||
|
||||
it('patches custom fields instead of replacing them', () => {
|
||||
const patched = applyPatchToEvents(makeEntries(), ['1', '2'], { custom: { lx: 'new' } });
|
||||
|
||||
// the field which was not part of the patch must survive
|
||||
expect((patched['1'] as OntimeEvent).custom).toStrictEqual({ lx: 'new', sound: 'x' });
|
||||
expect((patched['2'] as OntimeEvent).custom).toStrictEqual({ lx: 'new' });
|
||||
});
|
||||
|
||||
it('keeps the existing custom fields when the patch has none', () => {
|
||||
const patched = applyPatchToEvents(makeEntries(), ['1'], { title: 'patched' });
|
||||
|
||||
expect((patched['1'] as OntimeEvent).custom).toStrictEqual({ lx: 'a', sound: 'x' });
|
||||
});
|
||||
|
||||
it('ignores ids which are not in the rundown', () => {
|
||||
const patched = applyPatchToEvents(makeEntries(), ['1', 'does-not-exist'], { title: 'patched' });
|
||||
|
||||
expect(Object.keys(patched)).toStrictEqual(['1', '2', '3', 'group']);
|
||||
expect((patched['1'] as OntimeEvent).title).toBe('patched');
|
||||
});
|
||||
|
||||
it('ignores entries which are not events', () => {
|
||||
const patched = applyPatchToEvents(makeEntries(), ['group'], { title: 'patched' });
|
||||
|
||||
expect((patched.group as OntimeGroup).title).toBe('a group');
|
||||
});
|
||||
|
||||
it('does not mutate the given entries', () => {
|
||||
const entries = makeEntries();
|
||||
applyPatchToEvents(entries, ['1'], { title: 'patched', custom: { lx: 'new' } });
|
||||
|
||||
expect((entries['1'] as OntimeEvent).title).toBe('one');
|
||||
expect((entries['1'] as OntimeEvent).custom).toStrictEqual({ lx: 'a', sound: 'x' });
|
||||
});
|
||||
|
||||
it('handles repeated ids', () => {
|
||||
const patched = applyPatchToEvents(makeEntries(), ['1', '1'], { title: 'patched' });
|
||||
|
||||
expect((patched['1'] as OntimeEvent).title).toBe('patched');
|
||||
});
|
||||
});
|
||||
|
||||
describe('canPredictBatchResult()', () => {
|
||||
it('allows resolving patches which do not affect the schedule', () => {
|
||||
expect(canPredictBatchResult({ title: 'a title' })).toBe(true);
|
||||
expect(canPredictBatchResult({ colour: 'red', flag: true })).toBe(true);
|
||||
});
|
||||
|
||||
it('defers to the server when the duration changes', () => {
|
||||
// a duration cascades into the start and end of every linked event
|
||||
expect(canPredictBatchResult({ duration: 1000 })).toBe(false);
|
||||
expect(canPredictBatchResult({ title: 'a title', duration: 1000 })).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,39 @@
|
||||
import { EntryId, OntimeEvent, RundownEntries, isOntimeEvent } from 'ontime-types';
|
||||
|
||||
/**
|
||||
* Applies a patch to a set of events, used to optimistically resolve a batch edit
|
||||
* Entries which are missing or are not events are left untouched, mirroring the server
|
||||
* @returns a new entries object, the given entries are not mutated
|
||||
*/
|
||||
export function applyPatchToEvents(
|
||||
entries: RundownEntries,
|
||||
ids: EntryId[],
|
||||
patch: Partial<OntimeEvent>,
|
||||
): RundownEntries {
|
||||
const patchedEntries = { ...entries };
|
||||
|
||||
for (const id of new Set(ids)) {
|
||||
const entry = patchedEntries[id];
|
||||
if (!isOntimeEvent(entry)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
patchedEntries[id] = {
|
||||
...entry,
|
||||
...patch,
|
||||
// custom fields are patched, not replaced
|
||||
custom: patch.custom ? { ...entry.custom, ...patch.custom } : entry.custom,
|
||||
};
|
||||
}
|
||||
|
||||
return patchedEntries;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the result of a batch edit can be resolved without the server
|
||||
* Changing the duration cascades through the rundown, so the resulting
|
||||
* schedule is only known once the server has recalculated it
|
||||
*/
|
||||
export function canPredictBatchResult(patch: Partial<OntimeEvent>): boolean {
|
||||
return !('duration' in patch);
|
||||
}
|
||||
@@ -52,6 +52,7 @@ import {
|
||||
} from '../api/rundown';
|
||||
import { logAxiosError } from '../api/utils';
|
||||
import { useEditorSettings } from '../stores/editorSettings';
|
||||
import { applyPatchToEvents, canPredictBatchResult } from './entryAction.utils';
|
||||
|
||||
export type EventOptions = Partial<{
|
||||
// options of any new entries (event / delay / group)
|
||||
@@ -483,28 +484,9 @@ function useEntryActionsForRundown(scopedRundownId: string | undefined) {
|
||||
// Snapshot the previous value
|
||||
const previousRundown = queryClient.getQueryData<Rundown>(queryKey);
|
||||
|
||||
// changing the duration cascades through the rundown, we cannot resolve the new schedule here
|
||||
// we skip the optimistic update and wait for the recalculated rundown from the server
|
||||
const canPredictResult = !('duration' in data.data);
|
||||
|
||||
if (previousRundown && canPredictResult) {
|
||||
const { data: patch, ids } = data;
|
||||
const eventIds = new Set(ids);
|
||||
const newRundown = { ...previousRundown.entries };
|
||||
|
||||
eventIds.forEach((eventId) => {
|
||||
if (Object.hasOwn(newRundown, eventId)) {
|
||||
const event = newRundown[eventId];
|
||||
if (isOntimeEvent(event)) {
|
||||
newRundown[eventId] = {
|
||||
...event,
|
||||
...patch,
|
||||
// custom fields are patched, not replaced
|
||||
custom: patch.custom ? { ...event.custom, ...patch.custom } : event.custom,
|
||||
};
|
||||
}
|
||||
}
|
||||
});
|
||||
// when the result cannot be resolved here we wait for the recalculated rundown from the server
|
||||
if (previousRundown && canPredictBatchResult(data.data)) {
|
||||
const newRundown = applyPatchToEvents(previousRundown.entries, data.ids, data.data);
|
||||
|
||||
queryClient.setQueryData<Rundown>(queryKey, {
|
||||
id: previousRundown.id,
|
||||
|
||||
@@ -75,6 +75,26 @@
|
||||
gap: 1rem;
|
||||
}
|
||||
|
||||
// a label with an action aligned to the end of the row
|
||||
.labelRow {
|
||||
display: flex;
|
||||
align-items: baseline;
|
||||
justify-content: space-between;
|
||||
gap: 0.5rem;
|
||||
}
|
||||
|
||||
.clearAction {
|
||||
font-size: $aux-text-size;
|
||||
color: $blue-400;
|
||||
margin-bottom: $element-inner-spacing; // match the label
|
||||
text-decoration: underline;
|
||||
text-underline-offset: 2px;
|
||||
|
||||
&:hover {
|
||||
color: $blue-500;
|
||||
}
|
||||
}
|
||||
|
||||
.splitTwo {
|
||||
display: grid;
|
||||
grid-template-columns: 1fr 1fr;
|
||||
|
||||
@@ -5,6 +5,8 @@ import { AutoTextarea } from '../../../../common/components/input/auto-textarea/
|
||||
import useReactiveTextInput from '../../../../common/components/input/text-input/useReactiveTextInput';
|
||||
import { EventEditorUpdateFields } from '../EventEditor';
|
||||
|
||||
import style from '../EntryEditor.module.scss';
|
||||
|
||||
interface CountedTextAreaProps {
|
||||
className?: string;
|
||||
field: EventEditorUpdateFields;
|
||||
@@ -32,11 +34,21 @@ export default function EventTextArea({
|
||||
submitOnCtrlEnter: true,
|
||||
});
|
||||
|
||||
// an unknown value cannot be cleared by emptying the field, we offer an explicit action
|
||||
const canClear = initialValue === undefined;
|
||||
|
||||
return (
|
||||
<div>
|
||||
<Editor.Label className={className} htmlFor={field} style={givenStyles}>
|
||||
{label}
|
||||
</Editor.Label>
|
||||
<div className={style.labelRow}>
|
||||
<Editor.Label className={className} htmlFor={field} style={givenStyles}>
|
||||
{label}
|
||||
</Editor.Label>
|
||||
{canClear && (
|
||||
<button type='button' className={style.clearAction} onClick={() => submitCallback('')}>
|
||||
Clear
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
<AutoTextarea
|
||||
id={field}
|
||||
inputref={ref}
|
||||
|
||||
@@ -6,6 +6,8 @@ import useReactiveTextInput from '../../../../common/components/input/text-input
|
||||
import { EventEditorUpdateFields } from '../EventEditor';
|
||||
import { GroupEditorUpdateTextFields } from '../GroupEditor';
|
||||
|
||||
import style from '../EntryEditor.module.scss';
|
||||
|
||||
interface EntryEditorTextInputProps extends InputProps {
|
||||
field: EventEditorUpdateFields | GroupEditorUpdateTextFields;
|
||||
label: string;
|
||||
@@ -32,11 +34,21 @@ export default function EntryEditorTextInput({
|
||||
submitOnEnter: true,
|
||||
});
|
||||
|
||||
// an unknown value cannot be cleared by emptying the field, we offer an explicit action
|
||||
const canClear = initialValue === undefined;
|
||||
|
||||
return (
|
||||
<div>
|
||||
<Editor.Label className={className} htmlFor={field} style={givenStyles}>
|
||||
{label}
|
||||
</Editor.Label>
|
||||
<div className={style.labelRow}>
|
||||
<Editor.Label className={className} htmlFor={field} style={givenStyles}>
|
||||
{label}
|
||||
</Editor.Label>
|
||||
{canClear && (
|
||||
<button type='button' className={style.clearAction} onClick={() => submitCallback('')}>
|
||||
Clear
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
<Input
|
||||
id={field}
|
||||
ref={ref}
|
||||
|
||||
@@ -50,6 +50,16 @@ test('Editing multiple events', async ({ page }) => {
|
||||
await expect(page.getByTestId('entry-1').getByTestId('entry__title')).toHaveValue('first');
|
||||
await expect(page.getByTestId('entry-2').getByTestId('entry__title')).toHaveValue('second');
|
||||
|
||||
// a mixed field cannot be cleared by emptying it, so an explicit action is offered
|
||||
const clearTitle = editor.getByRole('button', { name: 'Clear', exact: true }).first();
|
||||
await expect(clearTitle).toBeVisible();
|
||||
await clearTitle.click();
|
||||
await expect(page.getByTestId('entry-1').getByTestId('entry__title')).toHaveValue('');
|
||||
await expect(page.getByTestId('entry-2').getByTestId('entry__title')).toHaveValue('');
|
||||
|
||||
// the events now agree, so the field can be cleared by hand and the action is withdrawn
|
||||
await expect(editor.getByLabel('Title', { exact: true })).not.toHaveAttribute('placeholder', 'Mixed');
|
||||
|
||||
// editing a field applies it to the whole selection
|
||||
await title.fill('shared title');
|
||||
await title.press('Enter');
|
||||
|
||||
Reference in New Issue
Block a user