mirror of
https://github.com/cpvalente/ontime.git
synced 2026-09-10 16:49:41 +00:00
fix(automation): make deleting a trigger idempotent
Review pointed out that reconciling triggers breaks if one of them is deleted by someone else first. The form deletes sequentially and the server rejected a delete for a trigger that was already gone, so the whole save failed — and failed again on every retry, because the trigger the form still wants removed can never be removed. Two operators with the settings panel open, or a project import that replaces the automation block, is enough to reach it. Fixing that in the form would have meant recognising "already gone" from an error message, since the refusal comes back as a 400 like any other. The server is the better place: deleteAutomation fifty lines below already returns early for an id it cannot find, for exactly this reason, so the two delete functions now agree with each other and with what DELETE is supposed to mean. Nothing depended on the throw — the controller was its only caller and no test covered it. Verified end to end: with the automation form open and its lifecycle unticked, another client deletes that trigger, and the save now completes and closes rather than failing with an error the user cannot clear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AfDKsy6PE3Rbyt32Fg4YKf
This commit is contained in:
@@ -116,6 +116,14 @@ describe('deleteTrigger()', () => {
|
|||||||
expect(removed.length).toEqual(1);
|
expect(removed.length).toEqual(1);
|
||||||
expect(removed[0].title).not.toEqual('test-osc');
|
expect(removed[0].title).not.toEqual('test-osc');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('ignores a trigger that is already gone', async () => {
|
||||||
|
// a client reconciling several triggers must not be stuck because another client
|
||||||
|
// removed one of them first: the end state it asked for is the one it gets
|
||||||
|
const before = getAutomationTriggers();
|
||||||
|
await expect(deleteTrigger('never-existed')).resolves.toBeUndefined();
|
||||||
|
expect(getAutomationTriggers()).toEqual(before);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('addAutomation()', () => {
|
describe('addAutomation()', () => {
|
||||||
|
|||||||
@@ -84,15 +84,17 @@ export async function editTrigger(id: string, newTrigger: TriggerDTO): Promise<T
|
|||||||
* Deletes an automation trigger given its ID
|
* Deletes an automation trigger given its ID
|
||||||
*/
|
*/
|
||||||
export async function deleteTrigger(id: string): Promise<void> {
|
export async function deleteTrigger(id: string): Promise<void> {
|
||||||
let triggers = getAutomationTriggers();
|
const triggers = getAutomationTriggers();
|
||||||
const index = triggers.findIndex((trigger) => trigger.id === id);
|
const index = triggers.findIndex((trigger) => trigger.id === id);
|
||||||
|
|
||||||
|
// ignore request if the trigger does not exist, as deleteAutomation does for the same reason:
|
||||||
|
// the caller asked for it to be gone and it is, and failing here makes a client that is
|
||||||
|
// reconciling several triggers unable to finish once another client removed one of them
|
||||||
if (index === -1) {
|
if (index === -1) {
|
||||||
throw new Error(`Automation with id ${id} not found`);
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
triggers = deleteAtIndex(index, triggers);
|
await saveChanges({ triggers: deleteAtIndex(index, triggers) });
|
||||||
await saveChanges({ triggers });
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
Reference in New Issue
Block a user