diff --git a/client-v3/e2e/tests/06-show-config-characters.spec.ts b/client-v3/e2e/tests/06-show-config-characters.spec.ts index 94978a15..234271b1 100644 --- a/client-v3/e2e/tests/06-show-config-characters.spec.ts +++ b/client-v3/e2e/tests/06-show-config-characters.spec.ts @@ -8,6 +8,7 @@ import { waitForAppReady, waitForModal, confirmModal, + cancelModal, waitForModalClosed, confirmDialog, } from '../helpers.js'; @@ -82,6 +83,56 @@ test('deletes a character', async () => { }); }); +// ── Character Merge ──────────────────────────────────────────────────────── + +test('Merge button is visible for each character row', async () => { + const row = page.locator('tr', { has: page.locator('td:has-text("Hamlet")') }); + await expect(row.locator('button:has-text("Merge")')).toBeVisible(); +}); + +test('creates a character for merge testing', async () => { + await page.getByRole('button', { name: 'New Character', exact: true }).click(); + await waitForModal(page, 'New Character'); + await page.fill('.modal.show input[type="text"]', 'Horatio'); + await confirmModal(page); + await waitForModalClosed(page); + await expect(page.locator('td:has-text("Horatio")').first()).toBeVisible(); +}); + +test('merge modal opens with correct title', async () => { + const row = page.locator('tr', { has: page.locator('td:has-text("Horatio")') }); + await row.locator('button:has-text("Merge")').click(); + await waitForModal(page, /Merge Horatio/); + await cancelModal(page); + await waitForModalClosed(page); +}); + +test('merge OK button is disabled with no destination selected', async () => { + const row = page.locator('tr', { has: page.locator('td:has-text("Horatio")') }); + await row.locator('button:has-text("Merge")').click(); + await waitForModal(page, /Merge Horatio/); + const okBtn = page.locator('.modal.show .modal-footer button.btn-primary'); + await expect(okBtn).toBeDisabled(); + await cancelModal(page); + await waitForModalClosed(page); +}); + +test('merges a character into another', async () => { + const row = page.locator('tr', { has: page.locator('td:has-text("Horatio")') }); + await row.locator('button:has-text("Merge")').click(); + await waitForModal(page, /Merge Horatio/); + // Open the dropdown by clicking the multiselect container, then select the option. + await page.locator('.modal.show .multiselect').click(); + await page.locator('.modal.show .multiselect__option', { hasText: 'Hamlet' }).click(); + await confirmModal(page); + await waitForModalClosed(page); + // Scope to the character table to avoid matching cells in the Line Counts tab (always in DOM). + await expect(page.locator('#character-table td:has-text("Horatio")')).not.toBeVisible({ + timeout: 5_000, + }); + await expect(page.locator('#character-table td:has-text("Hamlet")')).toBeVisible(); +}); + // ── Character Groups ────────────────────────────────────────────────────── test('switches to Character Groups sub-tab', async () => { diff --git a/client-v3/src/stores/show.ts b/client-v3/src/stores/show.ts index 319e8872..20df1639 100644 --- a/client-v3/src/stores/show.ts +++ b/client-v3/src/stores/show.ts @@ -301,6 +301,22 @@ export const useShowStore = defineStore('show', { } }, + async mergeCharacter(sourceId: number, destinationId: number): Promise { + const response = await fetch(makeURL('/api/v1/show/character/merge'), { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ source_id: sourceId, destination_id: destinationId }), + }); + if (response.ok) { + // getCharacterGroupList calls getCharacterList internally + await this.getCharacterGroupList(); + toast.success('Merged character!'); + } else { + log.error('Unable to merge characters'); + toast.error('Unable to merge characters'); + } + }, + // Character Groups async getCharacterGroupList(): Promise { const response = await fetch(makeURL('/api/v1/show/character/group')); diff --git a/client-v3/src/views/show/config/ConfigCharacters.vue b/client-v3/src/views/show/config/ConfigCharacters.vue index 4a37dfd8..be36dbd0 100644 --- a/client-v3/src/views/show/config/ConfigCharacters.vue +++ b/client-v3/src/views/show/config/ConfigCharacters.vue @@ -33,14 +33,21 @@ Edit + + Merge + Delete @@ -99,6 +106,33 @@ + +

+ Select a destination character. All script lines and group memberships from + {{ mergeSourceCharacter?.name }} will be transferred to the selected + character, and {{ mergeSourceCharacter?.name }} will be deleted. +

+ + + + + >(); const editCharacterModal = ref>(); +const mergeCharacterModal = ref>(); + +const mergingCharacter = ref(false); +const mergeSourceCharacter = ref(null); +const mergeDestinationObject = ref(null); const characterFields = [ 'name', @@ -194,6 +235,12 @@ const editRules = { editFormState: { name: { required } } }; const newV$ = useVuelidate(newRules, { newFormState }); const editV$ = useVuelidate(editRules, { editFormState }); +const mergeDestinationOptions = computed(() => + showStore.characterList.filter( + (c) => mergeSourceCharacter.value === null || c.id !== mergeSourceCharacter.value.id + ) +); + const castOptions = computed(() => [ { value: null, text: 'Please select an option', disabled: true }, ...showStore.castList.map((c) => ({ @@ -274,6 +321,36 @@ async function onSubmitEdit(event: Event): Promise { } } +function openMergeForm(character: Character): void { + mergeSourceCharacter.value = character; + mergeDestinationObject.value = null; + mergeCharacterModal.value?.show(); +} + +function resetMergeForm(): void { + mergeSourceCharacter.value = null; + mergeDestinationObject.value = null; + mergingCharacter.value = false; +} + +async function onSubmitMerge(event: Event): Promise { + if (!mergeDestinationObject.value || mergingCharacter.value) { + event.preventDefault(); + return; + } + mergingCharacter.value = true; + try { + await showStore.mergeCharacter(mergeSourceCharacter.value!.id, mergeDestinationObject.value.id); + mergeCharacterModal.value?.hide(); + resetMergeForm(); + } catch (error) { + log.error('Error merging character:', error); + event.preventDefault(); + } finally { + mergingCharacter.value = false; + } +} + async function deleteCharacter(character: Character): Promise { if (deletingCharacter.value) return; const ok = await confirm(`Are you sure you want to delete ${character.name}?`); diff --git a/client/src/store/modules/show.ts b/client/src/store/modules/show.ts index 462b1a2a..7c38f310 100644 --- a/client/src/store/modules/show.ts +++ b/client/src/store/modules/show.ts @@ -212,6 +212,23 @@ const module: Module = { VueToast.$toast.error('Unable to delete character'); } }, + async MERGE_CHARACTER( + context, + { source_id, destination_id }: { source_id: number; destination_id: number } + ) { + const response = await fetch(makeURL('/api/v1/show/character/merge'), { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ source_id, destination_id }), + }); + if (response.ok) { + context.dispatch('GET_CHARACTER_GROUP_LIST'); + VueToast.$toast.success('Merged character!'); + } else { + log.error('Unable to merge characters'); + VueToast.$toast.error('Unable to merge characters'); + } + }, async UPDATE_CHARACTER(context, character: Partial) { const response = await fetch(`${makeURL('/api/v1/show/character')}`, { method: 'PATCH', diff --git a/client/src/views/show/config/ConfigCharacters.vue b/client/src/views/show/config/ConfigCharacters.vue index 8d57b93b..4be8756a 100644 --- a/client/src/views/show/config/ConfigCharacters.vue +++ b/client/src/views/show/config/ConfigCharacters.vue @@ -29,14 +29,21 @@ Edit + + Merge + Delete @@ -62,6 +69,32 @@ + +

+ Select a destination character. All script lines and group memberships from + {{ mergeSourceCharacter ? mergeSourceCharacter.name : '' }} + will be transferred to the selected character, and it will be deleted. +

+ + + + + !(this as any).mergeSourceCharacter || c.id !== (this as any).mergeSourceCharacter.id + ); + }, }, async mounted(): Promise { await Promise.all([(this as any).GET_CHARACTER_LIST(), (this as any).GET_CAST_LIST()]); @@ -308,12 +350,43 @@ export default defineComponent({ } } }, + openMergeForm(character: any): void { + this.mergeSourceCharacter = character.item; + this.mergeDestinationCharacter = null; + (this as any).$bvModal.show('merge-character'); + }, + resetMergeForm(): void { + this.mergeSourceCharacter = null; + this.mergeDestinationCharacter = null; + this.mergingCharacter = false; + }, + async onSubmitMerge(event: Event): Promise { + if (!this.mergeDestinationCharacter || this.mergingCharacter) { + event.preventDefault(); + return; + } + this.mergingCharacter = true; + try { + await (this as any).MERGE_CHARACTER({ + source_id: this.mergeSourceCharacter.id, + destination_id: this.mergeDestinationCharacter.id, + }); + (this as any).$bvModal.hide('merge-character'); + this.resetMergeForm(); + } catch (error) { + log.error('Error merging character:', error); + event.preventDefault(); + } finally { + this.mergingCharacter = false; + } + }, ...mapActions([ 'GET_CHARACTER_LIST', 'GET_CAST_LIST', 'ADD_CHARACTER', 'UPDATE_CHARACTER', 'DELETE_CHARACTER', + 'MERGE_CHARACTER', ]), }, }); diff --git a/server/controllers/api/constants.py b/server/controllers/api/constants.py index b487758e..c171cc87 100644 --- a/server/controllers/api/constants.py +++ b/server/controllers/api/constants.py @@ -111,3 +111,6 @@ ERROR_NAME_ALREADY_TAKEN = "Name already taken" ERROR_TAG_NAME_EXISTS = "Tag name already exists (case-insensitive)" +ERROR_CANNOT_MERGE_SAME_CHARACTER = ( + "Source and destination characters must be different" +) diff --git a/server/controllers/api/show/characters.py b/server/controllers/api/show/characters.py index 67258218..d9b78188 100644 --- a/server/controllers/api/show/characters.py +++ b/server/controllers/api/show/characters.py @@ -1,9 +1,10 @@ from collections import defaultdict -from sqlalchemy import select +from sqlalchemy import select, update from tornado import escape from controllers.api.constants import ( + ERROR_CANNOT_MERGE_SAME_CHARACTER, ERROR_CAST_MEMBER_NOT_FOUND, ERROR_CHARACTER_GROUP_NOT_FOUND, ERROR_CHARACTER_NOT_FOUND, @@ -12,7 +13,13 @@ ERROR_NAME_MISSING, ERROR_SHOW_NOT_FOUND, ) -from models.script import Script, ScriptLine, ScriptLineType, ScriptRevision +from models.script import ( + Script, + ScriptLine, + ScriptLinePart, + ScriptLineType, + ScriptRevision, +) from models.show import Cast, Character, CharacterGroup, Show from rbac.role import Role from schemas.schemas import CharacterGroupSchema, CharacterSchema @@ -186,6 +193,94 @@ async def delete(self): await self.finish({"message": ERROR_SHOW_NOT_FOUND}) +@ApiRoute("show/character/merge", ApiVersion.V1) +class CharacterMergeController(BaseAPIController): + @requires_show + @no_live_session + async def post(self): + """Merge a source character into a destination character. + + Reassigns all ScriptLinePart rows referencing the source character to + the destination character across all revisions, then adds the destination + to every CharacterGroup that contained the source (if not already a + member), and finally deletes the source character (which cascades away + mic allocations and secondary-table group memberships automatically). + + :raises HTTPError 400: If source_id or destination_id are missing, or + if the source and destination are the same character. + :raises HTTPError 404: If either character cannot be found, or if + either character does not belong to the current show. + """ + current_show = self.get_current_show() + show_id = current_show["id"] + + with self.make_session() as session: + show: Show = session.get(Show, show_id) + if not show: + self.set_status(404) + await self.finish({"message": ERROR_SHOW_NOT_FOUND}) + return + + self.requires_role(show, Role.WRITE) + data = escape.json_decode(self.request.body) + + source_id = data.get("source_id", None) + destination_id = data.get("destination_id", None) + + if not source_id or not destination_id: + self.set_status(400) + await self.finish({"message": ERROR_ID_MISSING}) + return + + if source_id == destination_id: + self.set_status(400) + await self.finish({"message": ERROR_CANNOT_MERGE_SAME_CHARACTER}) + return + + source: Character = session.get(Character, source_id) + if not source or source.show_id != show_id: + self.set_status(404) + await self.finish({"message": ERROR_CHARACTER_NOT_FOUND}) + return + + destination: Character = session.get(Character, destination_id) + if not destination or destination.show_id != show_id: + self.set_status(404) + await self.finish({"message": ERROR_CHARACTER_NOT_FOUND}) + return + + # Step 1: Bulk-update all ScriptLinePart rows across all revisions. + # synchronize_session=False is safe: we do not re-read these objects + # after the update within this transaction. + session.execute( + update(ScriptLinePart) + .where(ScriptLinePart.character_id == source_id) + .values(character_id=destination_id) + .execution_options(synchronize_session=False) + ) + + # Step 2: Add destination to every group the source belongs to, + # skipping groups where destination is already a member to avoid + # a composite-PK collision on the association table. + for group in source.character_groups: + if destination not in group.characters: + group.characters.append(destination) + + # Step 3: Delete source. SQLAlchemy cascades: + # - mic_allocations: via cascade="all, delete-orphan" + # - character_group_association rows: via secondary= M2M management + session.delete(source) + session.commit() + + self.set_status(200) + await self.finish({"message": "Successfully merged character"}) + + await self.application.ws_send_to_all("NOOP", "GET_CHARACTER_LIST", {}) + await self.application.ws_send_to_all( + "NOOP", "GET_CHARACTER_GROUP_LIST", {} + ) + + @ApiRoute("show/character/stats", ApiVersion.V1) class CharacterStatsController(BaseAPIController): async def get(self): diff --git a/server/test/controllers/api/show/test_characters.py b/server/test/controllers/api/show/test_characters.py index 0b2d213c..d4674d25 100644 --- a/server/test/controllers/api/show/test_characters.py +++ b/server/test/controllers/api/show/test_characters.py @@ -1,7 +1,15 @@ import tornado.escape +from sqlalchemy import select -from models.script import Script, ScriptRevision -from models.show import Show, ShowScriptType +from models.mics import Microphone, MicrophoneAllocation +from models.script import ( + Script, + ScriptLine, + ScriptLinePart, + ScriptLineType, + ScriptRevision, +) +from models.show import Act, Character, CharacterGroup, Scene, Show, ShowScriptType from test.conftest import DigiScriptTestCase @@ -59,3 +67,212 @@ def test_get_character_stats_no_script(self): response = self.fetch("/api/v1/show/character/stats") # Should get an error because there's no script self.assertNotEqual(200, response.code) + + +class TestCharacterMergeController(DigiScriptTestCase): + """Test suite for POST /api/v1/show/character/merge endpoint.""" + + def setUp(self): + super().setUp() + self.token = self._create_and_login_admin() + + with self._app.get_db().sessionmaker() as session: + show = Show(name="Test Show", script_mode=ShowScriptType.FULL) + session.add(show) + session.flush() + self.show_id = show.id + + source = Character(show_id=show.id, name="Source") + destination = Character(show_id=show.id, name="Destination") + session.add(source) + session.add(destination) + session.flush() + self.source_id = source.id + self.destination_id = destination.id + session.commit() + + self._app.digi_settings.settings["current_show"].set_value(self.show_id) + + def _merge(self, body: dict) -> object: + return self.fetch( + "/api/v1/show/character/merge", + method="POST", + body=tornado.escape.json_encode(body), + headers={"Authorization": f"Bearer {self.token}"}, + ) + + def test_merge_missing_source_id(self): + """POST with no source_id returns 400.""" + response = self._merge({"destination_id": self.destination_id}) + self.assertEqual(400, response.code) + + def test_merge_missing_destination_id(self): + """POST with no destination_id returns 400.""" + response = self._merge({"source_id": self.source_id}) + self.assertEqual(400, response.code) + + def test_merge_same_character(self): + """POST with identical source and destination returns 400.""" + response = self._merge( + {"source_id": self.source_id, "destination_id": self.source_id} + ) + self.assertEqual(400, response.code) + + def test_merge_source_not_found(self): + """POST with a nonexistent source_id returns 404.""" + response = self._merge( + {"source_id": 99999, "destination_id": self.destination_id} + ) + self.assertEqual(404, response.code) + + def test_merge_destination_not_found(self): + """POST with a nonexistent destination_id returns 404.""" + response = self._merge({"source_id": self.source_id, "destination_id": 99999}) + self.assertEqual(404, response.code) + + def test_merge_cross_show_source(self): + """POST with a source character from a different show returns 404.""" + with self._app.get_db().sessionmaker() as session: + other_show = Show(name="Other Show", script_mode=ShowScriptType.FULL) + session.add(other_show) + session.flush() + foreign_char = Character(show_id=other_show.id, name="Foreign") + session.add(foreign_char) + session.flush() + foreign_id = foreign_char.id + session.commit() + + response = self._merge( + {"source_id": foreign_id, "destination_id": self.destination_id} + ) + self.assertEqual(404, response.code) + + def test_merge_deletes_source(self): + """Successful merge removes the source character.""" + response = self._merge( + {"source_id": self.source_id, "destination_id": self.destination_id} + ) + self.assertEqual(200, response.code) + + with self._app.get_db().sessionmaker() as session: + remaining = session.scalars( + select(Character).where(Character.show_id == self.show_id) + ).all() + names = [c.name for c in remaining] + self.assertNotIn("Source", names) + self.assertIn("Destination", names) + + def test_merge_transfers_script_line_parts(self): + """Merge updates all ScriptLinePart rows for the source to the destination.""" + with self._app.get_db().sessionmaker() as session: + act = Act(show_id=self.show_id, name="Act 1") + session.add(act) + session.flush() + scene = Scene(show_id=self.show_id, act_id=act.id, name="Scene 1") + session.add(scene) + session.flush() + line = ScriptLine( + act_id=act.id, + scene_id=scene.id, + page=1, + line_type=ScriptLineType.DIALOGUE, + ) + session.add(line) + session.flush() + part = ScriptLinePart( + line_id=line.id, + part_index=0, + character_id=self.source_id, + ) + session.add(part) + session.flush() + part_id = part.id + session.commit() + + response = self._merge( + {"source_id": self.source_id, "destination_id": self.destination_id} + ) + self.assertEqual(200, response.code) + + with self._app.get_db().sessionmaker() as session: + updated_part = session.get(ScriptLinePart, part_id) + self.assertEqual(self.destination_id, updated_part.character_id) + + def test_merge_updates_character_groups(self): + """Merge adds destination to groups that contained the source.""" + with self._app.get_db().sessionmaker() as session: + source = session.get(Character, self.source_id) + group = CharacterGroup(show_id=self.show_id, name="Ensemble") + group.characters.append(source) + session.add(group) + session.flush() + group_id = group.id + session.commit() + + response = self._merge( + {"source_id": self.source_id, "destination_id": self.destination_id} + ) + self.assertEqual(200, response.code) + + with self._app.get_db().sessionmaker() as session: + group = session.get(CharacterGroup, group_id) + member_ids = [c.id for c in group.characters] + self.assertIn(self.destination_id, member_ids) + self.assertNotIn(self.source_id, member_ids) + + def test_merge_group_deduplication(self): + """Merge does not create a duplicate when destination is already in the group.""" + with self._app.get_db().sessionmaker() as session: + source = session.get(Character, self.source_id) + destination = session.get(Character, self.destination_id) + group = CharacterGroup(show_id=self.show_id, name="Royalty") + group.characters.append(source) + group.characters.append(destination) + session.add(group) + session.flush() + group_id = group.id + session.commit() + + response = self._merge( + {"source_id": self.source_id, "destination_id": self.destination_id} + ) + self.assertEqual(200, response.code) + + with self._app.get_db().sessionmaker() as session: + group = session.get(CharacterGroup, group_id) + member_ids = [c.id for c in group.characters] + self.assertEqual(1, len(member_ids)) + self.assertIn(self.destination_id, member_ids) + + def test_merge_deletes_mic_allocations(self): + """Merge deletes all mic allocations belonging to the source character.""" + with self._app.get_db().sessionmaker() as session: + mic = Microphone(show_id=self.show_id, name="Radio Mic 1") + session.add(mic) + session.flush() + act = Act(show_id=self.show_id, name="Act 1") + session.add(act) + session.flush() + scene = Scene(show_id=self.show_id, act_id=act.id, name="Scene 1") + session.add(scene) + session.flush() + alloc = MicrophoneAllocation( + mic_id=mic.id, + scene_id=scene.id, + character_id=self.source_id, + ) + session.add(alloc) + session.commit() + + response = self._merge( + {"source_id": self.source_id, "destination_id": self.destination_id} + ) + self.assertEqual(200, response.code) + + with self._app.get_db().sessionmaker() as session: + remaining = session.scalars( + select(MicrophoneAllocation).where( + MicrophoneAllocation.character_id == self.source_id + ) + ).all() + self.assertEqual(0, len(remaining))