diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a0f3c57..5e96c3c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ Lines marked **Upgrade step** need you to act after updating. - On screens narrower than 1280 px, a form's editor bar now sits in two rows under the app header: the form with Dashboard and Preview, then Editor, Settings, Integrations and Submissions as tabs. Nothing in it scrolls sideways any more, and the header keeps the logo and your team name. On every screen, keyboard focus in the editor bar and the account menu now shows a blue ring. (#257) - A form's pages have one top bar instead of two: the form's name with its menu, a Draft or Live label, the pages, Preview, Publish and your account. Publish first checks for questions without text, rules that point to a deleted question and a missing privacy link. Once the form is live, Share has its link, Open form and Unpublish. On a phone the pages sit in a tab bar at the bottom. (#265) - A logic rule with a missing field now tells you in plain words what to fill in, like "Pick a question for condition 1." (#278) +- Deleting a question, a group, an answer choice, a consent policy or a logic rule in the form editor now asks first in the app's own dialog. It names the question, how many answers will be deleted with it, and the questions whose rules will stop working. A choice nobody picked, or a policy nobody agreed to, is still deleted at once. (#269) ### Fixed diff --git a/resources/js/components/Factory/Main/Interactions/ClickInteraction.vue b/resources/js/components/Factory/Main/Interactions/ClickInteraction.vue index ba678267..13bc9d20 100644 --- a/resources/js/components/Factory/Main/Interactions/ClickInteraction.vue +++ b/resources/js/components/Factory/Main/Interactions/ClickInteraction.vue @@ -37,7 +37,13 @@ - + + @@ -45,6 +51,8 @@ import { D9Input } from "@deck9/ui"; import InteractionHoverActions from "@/components/Factory/Main/InteractionHoverActions.vue"; import IndexItem from "@/components/Factory/Shared/IndexItem.vue"; +import DeleteChoiceDialog from "@/components/Factory/Shared/DeleteChoiceDialog.vue"; +import { useDeleteChoice } from "@/components/Factory/Shared/useDeleteBlock"; import { useWorkbench } from "@/stores"; import { Ref, ref, watch } from "vue"; @@ -97,8 +105,7 @@ const keyboardCommands = async (event: KeyboardEvent) => { case "Backspace": if (event.metaKey) { - emit("onDelete", props.index); - await workbench.deleteInteraction(props.item); + await askDelete(); } break; @@ -110,6 +117,13 @@ const keyboardCommands = async (event: KeyboardEvent) => { } }; +const { + asking, + answers, + ask: askDelete, + confirm: deleteNow, +} = useDeleteChoice(props.item, () => emit("onDelete", props.index)); + const focus = () => { labelElement.value?.focus(); }; diff --git a/resources/js/components/Factory/Main/Interactions/ConsentInteraction.vue b/resources/js/components/Factory/Main/Interactions/ConsentInteraction.vue index fd8151cb..961503ab 100644 --- a/resources/js/components/Factory/Main/Interactions/ConsentInteraction.vue +++ b/resources/js/components/Factory/Main/Interactions/ConsentInteraction.vue @@ -45,7 +45,14 @@ - + + @@ -53,6 +60,8 @@ import { D9Input, D9Textarea, D9Switch } from "@deck9/ui"; import InteractionHoverActions from "@/components/Factory/Main/InteractionHoverActions.vue"; import IndexItem from "@/components/Factory/Shared/IndexItem.vue"; +import DeleteChoiceDialog from "@/components/Factory/Shared/DeleteChoiceDialog.vue"; +import { useDeleteChoice } from "@/components/Factory/Shared/useDeleteBlock"; import { useWorkbench } from "@/stores"; import { Ref, ref, watch } from "vue"; @@ -88,6 +97,13 @@ watch([label, message, isRequired], (newValues) => { workbench.updateInteraction(update); }); +const { + asking, + answers, + ask: askDelete, + confirm: deleteNow, +} = useDeleteChoice(props.item); + const focus = () => { labelElement.value.focus(); }; diff --git a/resources/js/components/Factory/Shared/DeleteBlockDialog.vue b/resources/js/components/Factory/Shared/DeleteBlockDialog.vue new file mode 100644 index 00000000..309a671c --- /dev/null +++ b/resources/js/components/Factory/Shared/DeleteBlockDialog.vue @@ -0,0 +1,104 @@ + + + diff --git a/resources/js/components/Factory/Shared/DeleteChoiceDialog.vue b/resources/js/components/Factory/Shared/DeleteChoiceDialog.vue new file mode 100644 index 00000000..16b197db --- /dev/null +++ b/resources/js/components/Factory/Shared/DeleteChoiceDialog.vue @@ -0,0 +1,51 @@ + + + diff --git a/resources/js/components/Factory/Shared/DeleteDialog.vue b/resources/js/components/Factory/Shared/DeleteDialog.vue new file mode 100644 index 00000000..93e872c9 --- /dev/null +++ b/resources/js/components/Factory/Shared/DeleteDialog.vue @@ -0,0 +1,63 @@ + + + diff --git a/resources/js/components/Factory/Shared/DeleteDialogs.test.ts b/resources/js/components/Factory/Shared/DeleteDialogs.test.ts new file mode 100644 index 00000000..561360c5 --- /dev/null +++ b/resources/js/components/Factory/Shared/DeleteDialogs.test.ts @@ -0,0 +1,481 @@ +import { flushPromises, mount } from "@vue/test-utils"; +import { AxiosResponse } from "axios"; +import { createPinia, setActivePinia } from "pinia"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { defineComponent } from "vue"; +import DeleteDialog from "./DeleteDialog.vue"; +import DeleteBlockDialog from "./DeleteBlockDialog.vue"; +import DeleteChoiceDialog from "./DeleteChoiceDialog.vue"; +import { checkDelete, useDeleteBlock } from "./useDeleteBlock"; +import ClickInteraction from "@/components/Factory/Main/Interactions/ClickInteraction.vue"; +import ConsentInteraction from "@/components/Factory/Main/Interactions/ConsentInteraction.vue"; +import InteractionHoverActions from "@/components/Factory/Main/InteractionHoverActions.vue"; +import BlockLogicRule from "@/components/Factory/Sidebar/BlockLogicRule.vue"; +import { callGetFormBlocks } from "@/api/blocks"; +import { useForm, useLogic, useWorkbench } from "@/stores"; + +vi.mock("@/api/blocks", async (importOriginal) => ({ + ...(await importOriginal()), + callGetFormBlocks: vi.fn(), +})); + +const use = (id: number, block: string): FormBlockRuleUse => ({ + id, + name: `Rule ${id}`, + block, + as: ["source"], +}); + +// the meetup form as the server sends it with includeSubmissions=true +const blocks = (): FormBlockModel[] => + [ + { + id: 1, + uuid: "q1", + type: "radio", + message: "

Will you come to the meetup?

", + session_count: 40, + used_by: [use(1, "q1"), use(2, "q3"), use(6, "g")], + }, + { + id: 2, + uuid: "g", + type: "group", + title: "Food and drinks", + used_by: [{ ...use(3, "q1"), as: ["target"] }], + }, + { + id: 3, + uuid: "q2", + type: "input-short", + parent_block: "g", + message: "

Any allergies?

", + session_count: 0, + used_by: [use(4, "q3"), use(5, "q2")], + }, + { + id: 4, + uuid: "q3", + type: "radio", + message: "", + session_count: 12, + used_by: [], + interactions: [ + { id: 10, label: "No", responses_count: 12 }, + { id: 11, label: "Yes", responses_count: 0 }, + ], + }, + { + id: 5, + uuid: "c1", + type: "consent", + message: "", + session_count: 3, + used_by: [], + interactions: [ + { id: 20, label: "Privacy policy", responses_count: 3 }, + { id: 21, label: "Newsletter", responses_count: 0 }, + ], + }, + ] as unknown as FormBlockModel[]; + +const block = (uuid: string) => + useForm().blocks!.find((item) => item.uuid === uuid)!; + +const stubs = { teleport: true }; + +describe("asking before a delete", () => { + let pinia: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + // real actions, not the stubs of the global testing pinia + pinia = createPinia(); + setActivePinia(pinia); + useForm().form = { uuid: "form1" } as FormModel; + useForm().blocks = blocks(); + vi.mocked(callGetFormBlocks).mockResolvedValue({ + data: blocks(), + } as AxiosResponse); + }); + + describe("checkDelete", () => { + it("counts a question's answers and the rules on other questions that use it", async () => { + const check = await checkDelete(block("q1")); + + expect(callGetFormBlocks).toHaveBeenCalledWith("form1", true); + expect(check.answers).toBe(40); + expect(check.usedBy.map((rule) => rule.id)).toEqual([2, 6]); + }); + + it("counts a group's questions and the rules outside it that use the group or its questions", async () => { + const check = await checkDelete(block("g")); + + expect(check.questions).toBe(1); + expect(check.usedBy.map((rule) => rule.id)).toEqual([3, 4]); + }); + + it("counts the answers per choice", async () => { + const check = await checkDelete(block("q3")); + + expect(check.choices).toEqual({ 10: 12, 11: 0 }); + }); + + it("still names the rules when the counts can't load", async () => { + vi.mocked(callGetFormBlocks).mockRejectedValue( + new Error("offline"), + ); + // the editor loads blocks without counts + delete block("q1").session_count; + + const check = await checkDelete(block("q1")); + + expect(check.answers).toBeUndefined(); + expect(check.usedBy.map((rule) => rule.id)).toEqual([2, 6]); + }); + }); + + describe("DeleteBlockDialog", () => { + const open = async (uuid: string) => + mount(DeleteBlockDialog, { + props: { + block: block(uuid), + check: await checkDelete(block(uuid)), + }, + global: { plugins: [pinia], stubs }, + }); + + it("names the question, its answers and the questions whose rules stop working", async () => { + const wrapper = await open("q1"); + + expect(wrapper.find("[role=alertdialog]").text()).toContain( + "Delete question?", + ); + expect(wrapper.text()).toContain( + "“Will you come to the meetup?” and its 40 answers will be deleted. This can't be undone.", + ); + expect(wrapper.text()).toContain( + "2 rules use this question and will stop working: question 3, group “Food and drinks”.", + ); + expect(wrapper.find("button.bg-red-600").text()).toBe( + "Delete question", + ); + }); + + it("leaves out answers a question doesn't have", async () => { + const wrapper = await open("q2"); + + expect(wrapper.text()).toContain( + "“Any allergies?” will be deleted. This can't be undone.", + ); + expect(wrapper.text()).toContain( + "1 rule uses this question and will stop working: question 3.", + ); + }); + + it("leaves out rules when none use the question", () => { + const wrapper = mount(DeleteBlockDialog, { + props: { + block: block("q1"), + check: { + answers: 1, + questions: 0, + choices: {}, + usedBy: [], + }, + }, + global: { plugins: [pinia], stubs }, + }); + + expect(wrapper.text()).toContain( + "“Will you come to the meetup?” and its 1 answer will be deleted.", + ); + expect(wrapper.text()).not.toContain("rule"); + }); + + it("says the answers go too when it can't count them", () => { + const wrapper = mount(DeleteBlockDialog, { + props: { + block: block("q1"), + check: { questions: 0, choices: {}, usedBy: [] }, + }, + global: { plugins: [pinia], stubs }, + }); + + expect(wrapper.text()).toContain( + "“Will you come to the meetup?” and its answers will be deleted. This can't be undone.", + ); + }); + + it("names a group and how many questions go with it", async () => { + const wrapper = await open("g"); + + expect(wrapper.text()).toContain("Delete group “Food and drinks”?"); + expect(wrapper.text()).toContain( + "Its 1 question and its answers will be deleted too. This can't be undone.", + ); + expect(wrapper.text()).toContain( + "2 rules use this group and will stop working: question 1, question 3.", + ); + expect(wrapper.find("button.bg-red-600").text()).toBe( + "Delete group", + ); + }); + + it("emits confirm and close from its buttons", async () => { + const wrapper = await open("q1"); + + await wrapper.find("button.bg-red-600").trigger("click"); + await wrapper.findAll("button")[0].trigger("click"); + + expect(wrapper.emitted("confirm")).toHaveLength(1); + expect(wrapper.emitted("close")).toHaveLength(1); + }); + }); + + describe("useDeleteBlock", () => { + it("deletes only after the owner confirms, and clears the workbench", async () => { + const form = useForm(); + const workbench = useWorkbench(); + const remove = vi + .spyOn(form, "deleteFormBlock") + .mockResolvedValue(); + const clear = vi.spyOn(workbench, "clearWorkbench"); + workbench.block = block("q1"); + + const deletion = mount( + defineComponent({ + setup: () => useDeleteBlock(block("q1")), + template: "
", + }), + { global: { plugins: [pinia] } }, + ).vm; + + await deletion.ask(); + expect(deletion.check?.answers).toBe(40); + expect(remove).not.toHaveBeenCalled(); + + deletion.confirm(); + expect(remove).toHaveBeenCalledWith(block("q1")); + expect(clear).toHaveBeenCalled(); + expect(deletion.check).toBeNull(); + }); + }); + + describe("deleting a choice", () => { + const choice = (id: number, index: number) => { + const workbench = useWorkbench(); + workbench.block = block("q3"); + const remove = vi + .spyOn(workbench, "deleteInteraction") + .mockResolvedValue(); + const wrapper = mount(ClickInteraction, { + props: { + index, + item: block("q3").interactions!.find( + (item) => item.id === id, + )!, + }, + global: { plugins: [pinia], stubs }, + }); + + return { wrapper, remove }; + }; + + const pressCmdBackspace = (wrapper: ReturnType) => + wrapper + .find("input[name=label]") + .trigger("keydown", { key: "Backspace", metaKey: true }); + + it("deletes a choice nobody picked at once", async () => { + const { wrapper, remove } = choice(11, 1); + + await pressCmdBackspace(wrapper); + await flushPromises(); + + expect(remove).toHaveBeenCalledWith( + expect.objectContaining({ id: 11 }), + ); + expect(wrapper.find("[role=alertdialog]").exists()).toBe(false); + }); + + it("asks first when people picked the choice", async () => { + const { wrapper, remove } = choice(10, 0); + + await pressCmdBackspace(wrapper); + await flushPromises(); + + expect(remove).not.toHaveBeenCalled(); + expect(wrapper.text()).toContain("Delete “No”?"); + expect(wrapper.text()).toContain( + "12 people picked it. Their answers to it will be deleted.", + ); + + await wrapper.find("button.bg-red-600").trigger("click"); + await flushPromises(); + + expect(remove).toHaveBeenCalledWith( + expect.objectContaining({ id: 10 }), + ); + }); + + it("says 1 person for a single answer", () => { + const wrapper = mount(DeleteChoiceDialog, { + props: { choice: { id: 10, label: "No" }, answers: 1 }, + global: { plugins: [pinia], stubs }, + }); + + expect(wrapper.text()).toContain("1 person picked it."); + expect(wrapper.find("button.bg-red-600").text()).toBe( + "Delete choice", + ); + }); + }); + + describe("deleting a consent policy", () => { + const policy = (id: number) => { + const workbench = useWorkbench(); + workbench.block = block("c1"); + const remove = vi + .spyOn(workbench, "deleteInteraction") + .mockResolvedValue(); + const wrapper = mount(ConsentInteraction, { + props: { + index: 0, + item: block("c1").interactions!.find( + (item) => item.id === id, + )!, + }, + global: { plugins: [pinia], stubs }, + }); + const pressTrash = async () => { + await wrapper + .findComponent(InteractionHoverActions) + .find("button") + .trigger("click"); + await flushPromises(); + }; + + return { wrapper, remove, pressTrash }; + }; + + it("deletes a policy nobody agreed to at once", async () => { + const { wrapper, remove, pressTrash } = policy(21); + + await pressTrash(); + + expect(remove).toHaveBeenCalledWith( + expect.objectContaining({ id: 21 }), + ); + expect(wrapper.find("[role=alertdialog]").exists()).toBe(false); + }); + + it("asks first when people agreed to the policy", async () => { + const { wrapper, remove, pressTrash } = policy(20); + + await pressTrash(); + + expect(remove).not.toHaveBeenCalled(); + expect(wrapper.text()).toContain("Delete policy “Privacy policy”?"); + expect(wrapper.text()).toContain( + "3 people agreed to it. Their answers to it will be deleted.", + ); + expect(wrapper.find("button.bg-red-600").text()).toBe( + "Delete policy", + ); + + await wrapper.find("button.bg-red-600").trigger("click"); + await flushPromises(); + + expect(remove).toHaveBeenCalledWith( + expect.objectContaining({ id: 20 }), + ); + }); + }); + + describe("DeleteChoiceDialog when it can't count the answers", () => { + it("says a choice's answers go if people picked it", () => { + const wrapper = mount(DeleteChoiceDialog, { + props: { choice: { id: 10, label: "No" } }, + global: { plugins: [pinia], stubs }, + }); + + expect(wrapper.text()).toContain( + "If people picked it, their answers to it will be deleted.", + ); + }); + + it("says a policy's answers go if people agreed to it", () => { + const wrapper = mount(DeleteChoiceDialog, { + props: { + choice: { id: 20, label: "Privacy policy" }, + policy: true, + }, + global: { plugins: [pinia], stubs }, + }); + + expect(wrapper.text()).toContain( + "If people agreed to it, their answers to it will be deleted.", + ); + }); + }); + + describe("DeleteDialog", () => { + it("closes only itself on Escape, not the logic panel under it", () => { + const panel = vi.fn(); + window.addEventListener("keydown", panel); + const wrapper = mount(DeleteDialog, { + props: { + show: true, + title: "Delete?", + text: "", + confirmLabel: "Delete", + }, + global: { stubs }, + }); + + document.body.dispatchEvent( + new KeyboardEvent("keydown", { key: "Escape", bubbles: true }), + ); + + expect(wrapper.emitted("close")).toHaveLength(1); + expect(panel).not.toHaveBeenCalled(); + window.removeEventListener("keydown", panel); + wrapper.unmount(); + }); + }); + + describe("deleting a rule", () => { + it("asks in the app's dialog, not the browser's", async () => { + const confirm = vi.spyOn(window, "confirm"); + const remove = vi + .spyOn(useLogic(), "removeRule") + .mockResolvedValue(); + const wrapper = mount(BlockLogicRule, { + props: { + index: 2, + rule: { + name: "Skip", + evaluate: "after", + action: "goto", + action_payload: "q1", + conditions: [], + } as unknown as FormBlockLogic, + }, + global: { plugins: [pinia], stubs }, + }); + + await wrapper + .find("button[aria-label='Delete rule']") + .trigger("click"); + + expect(confirm).not.toHaveBeenCalled(); + expect(wrapper.text()).toContain("Delete rule “Skip”?"); + expect(remove).not.toHaveBeenCalled(); + + await wrapper.find("button.bg-red-600").trigger("click"); + + expect(remove).toHaveBeenCalledWith(2); + }); + }); +}); diff --git a/resources/js/components/Factory/Shared/useDeleteBlock.ts b/resources/js/components/Factory/Shared/useDeleteBlock.ts new file mode 100644 index 00000000..6ecc276a --- /dev/null +++ b/resources/js/components/Factory/Shared/useDeleteBlock.ts @@ -0,0 +1,107 @@ +import { ref } from "vue"; +import { callGetFormBlocks } from "@/api/blocks"; +import { useForm, useWorkbench } from "@/stores"; + +export interface DeleteCheck { + // answers to the block, missing when the server couldn't say + answers?: number; + // questions that go with a group + questions: number; + // answers per choice id + choices: Record; + // rules on other blocks that stop working + usedBy: FormBlockRuleUse[]; +} + +// What a delete takes with it, asked right before: answer counts need +// includeSubmissions, and the editor's used_by can be older than its rules. +export async function checkDelete(block: FormBlockModel): Promise { + const store = useForm(); + let blocks = store.blocks ?? []; + + try { + blocks = (await callGetFormBlocks(store.form!.uuid, true)).data; + } catch { + // ask anyway, with what the editor has + } + + const gone = blocks.filter( + (item) => item.uuid === block.uuid || item.parent_block === block.uuid, + ); + const goneUuids = gone.map((item) => item.uuid); + const fresh = gone.find((item) => item.uuid === block.uuid); + + return { + answers: fresh?.session_count, + questions: gone.length - 1, + choices: Object.fromEntries( + (fresh?.interactions ?? []) + .filter((item) => item.responses_count !== undefined) + .map((item) => [item.id, item.responses_count]), + ), + usedBy: gone + .flatMap((item) => item.used_by ?? []) + .filter((rule) => !goneUuids.includes(rule.block)), + }; +} + +// asks before a question or group is deleted +export function useDeleteBlock(block: FormBlockModel) { + const check = ref(null); + + const ask = async () => { + check.value = await checkDelete(block); + }; + + const cancel = () => { + check.value = null; + }; + + const confirm = () => { + const workbench = useWorkbench(); + check.value = null; + useForm().deleteFormBlock(block); + + if (workbench.block?.id === block.id) { + workbench.clearWorkbench(); + } + }; + + return { check, ask, cancel, confirm }; +} + +// asks before a choice is deleted; one nobody picked goes at once +export function useDeleteChoice( + item: FormBlockInteractionModel, + onDelete?: () => void, +) { + const workbench = useWorkbench(); + // open while the owner decides + const asking = ref(null); + const answers = ref(); + let isChecking = false; + + const confirm = async () => { + asking.value = null; + onDelete?.(); + await workbench.deleteInteraction(item); + }; + + const ask = async () => { + if (isChecking || !workbench.block) { + return; + } + + isChecking = true; + answers.value = (await checkDelete(workbench.block)).choices[item.id]; + isChecking = false; + + if (answers.value === 0) { + return confirm(); + } + + asking.value = item; + }; + + return { asking, answers, ask, confirm }; +} diff --git a/resources/js/components/Factory/Sidebar/Block.vue b/resources/js/components/Factory/Sidebar/Block.vue index d9be24dd..b5f97014 100644 --- a/resources/js/components/Factory/Sidebar/Block.vue +++ b/resources/js/components/Factory/Sidebar/Block.vue @@ -30,7 +30,7 @@ as="button" class="block w-full text-left" label="Delete" - @click.stop="deleteBlock" + @click.stop="ask" />
@@ -62,6 +62,12 @@ + + @@ -73,6 +79,8 @@ import BlockInteraction from "./BlockInteraction.vue"; import BlockFooter from "./BlockFooter.vue"; import BlockLogicVisualizer from "./BlockLogicVisualizer.vue"; import InsertAfterButton from "./InsertAfterButton.vue"; +import DeleteBlockDialog from "@/components/Factory/Shared/DeleteBlockDialog.vue"; +import { useDeleteBlock } from "@/components/Factory/Shared/useDeleteBlock"; import { useWorkbench, useForm } from "@/stores"; import { D9Menu, D9MenuLink } from "@deck9/ui"; import copy from "copy-text-to-clipboard"; @@ -101,17 +109,7 @@ const isActive = computed((): boolean => { const { cardStyle } = useActiveCard(isActive); -const deleteBlock = () => { - const result = window.confirm("Do you really want to delete this block?"); - - if (result) { - store.deleteFormBlock(props.block); - - if (workbench.block?.id === props.block.id) { - workbench.clearWorkbench(); - } - } -}; +const { check, ask, cancel, confirm } = useDeleteBlock(props.block); const copyId = () => { copy(props.block.uuid); diff --git a/resources/js/components/Factory/Sidebar/BlockLogicRule.vue b/resources/js/components/Factory/Sidebar/BlockLogicRule.vue index dbc94627..9d3f0061 100644 --- a/resources/js/components/Factory/Sidebar/BlockLogicRule.vue +++ b/resources/js/components/Factory/Sidebar/BlockLogicRule.vue @@ -6,15 +6,20 @@ {{ rule.name }} - + +
@@ -61,12 +61,20 @@
+ +