diff --git a/api/src/controllers/information-sharing-agreements/revert-to-draft-controller.ts b/api/src/controllers/information-sharing-agreements/revert-to-draft-controller.ts index 889dd0a5..3fdf447d 100644 --- a/api/src/controllers/information-sharing-agreements/revert-to-draft-controller.ts +++ b/api/src/controllers/information-sharing-agreements/revert-to-draft-controller.ts @@ -45,7 +45,7 @@ export class RevertToDraftController extends BaseController { return InformationSharingAgreement.findByPk(this.params.informationSharingAgreementId, { - include: ["accessGrants", "informationSharingAgreementArchiveItems"], + include: ["accessGrants"], }) } diff --git a/api/src/policies/information-sharing-agreements/revert-to-draft-policy.ts b/api/src/policies/information-sharing-agreements/revert-to-draft-policy.ts index d465a69e..15208292 100644 --- a/api/src/policies/information-sharing-agreements/revert-to-draft-policy.ts +++ b/api/src/policies/information-sharing-agreements/revert-to-draft-policy.ts @@ -1,12 +1,9 @@ -import { isUndefined } from "lodash" - import { InformationSharingAgreement } from "@/models" import { PolicyFactory } from "@/policies/base-policy" export class RevertToDraftPolicy extends PolicyFactory(InformationSharingAgreement) { create(): boolean { if (!this.record.isSigned()) return false - if (this.hasArchiveItems()) return false if (this.user.id === this.record.creatorId) return true if (this.user.isSystemAdmin) return true @@ -14,15 +11,4 @@ export class RevertToDraftPolicy extends PolicyFactory(InformationSharingAgreeme return false } - - private hasArchiveItems(): boolean { - const { informationSharingAgreementArchiveItems } = this.record - if (isUndefined(informationSharingAgreementArchiveItems)) { - throw new Error( - "Expected informationSharingAgreementArchiveItems association to be pre-loaded." - ) - } - - return informationSharingAgreementArchiveItems.length > 0 - } } diff --git a/api/src/services/information-sharing-agreements/revert-to-draft-service.ts b/api/src/services/information-sharing-agreements/revert-to-draft-service.ts index fcbb412f..a1d6ff8b 100644 --- a/api/src/services/information-sharing-agreements/revert-to-draft-service.ts +++ b/api/src/services/information-sharing-agreements/revert-to-draft-service.ts @@ -1,14 +1,6 @@ -import { isUndefined } from "lodash" - -import db, { - Attachment, - InformationSharingAgreement, - User, - InformationSharingAgreementArchiveItem, -} from "@/models" -import { AttachmentTargetTypes } from "@/models/attachment" +import db, { InformationSharingAgreement, User } from "@/models" import BaseService from "@/services/base-service" -import { Attachments, InformationSharingAgreements } from "@/services" +import { InformationSharingAgreements } from "@/services" export class RevertToDraftService extends BaseService { constructor( @@ -23,30 +15,11 @@ export class RevertToDraftService extends BaseService { throw new Error("Only signed agreements can be reverted to draft.") } - const { informationSharingAgreementArchiveItems } = this.informationSharingAgreement - if (isUndefined(informationSharingAgreementArchiveItems)) { - throw new Error( - "Expected informationSharingAgreementArchiveItems association to be pre-loaded." - ) - } - this.assertNoArchiveItemsLinked(informationSharingAgreementArchiveItems) - return db.transaction(async () => { + // The groups (and their access grants) are removed on revert, but signed documents and + // any linked knowledge items are intentionally kept so that amending a signed agreement + // does not lose them. See TK-32. await this.destroyGroups(this.informationSharingAgreement, this.currentUser) - // Destroyed one at a time through the service so each removal notifies the - // designated contacts, as a bulk destroy would not. See TK-6. - await Attachment.findEach( - { - where: { - targetId: this.informationSharingAgreement.id, - targetType: AttachmentTargetTypes.InformationSharingAgreement, - associationName: "signedConfidentialityAcknowledgement", - }, - }, - async (attachment) => { - await Attachments.DestroyService.perform(attachment, this.currentUser) - } - ) await this.informationSharingAgreement.update({ status: InformationSharingAgreement.Status.DRAFT, signedById: null, @@ -62,14 +35,6 @@ export class RevertToDraftService extends BaseService { }) } - private assertNoArchiveItemsLinked( - informationSharingAgreementArchiveItems: InformationSharingAgreementArchiveItem[] - ): void { - if (informationSharingAgreementArchiveItems.length > 0) { - throw new Error("Cannot revert to draft because archive items are linked to this agreement.") - } - } - private async destroyGroups( informationSharingAgreement: InformationSharingAgreement, currentUser: User diff --git a/api/tests/policies/information-sharing-agreements/revert-to-draft-policy.test.ts b/api/tests/policies/information-sharing-agreements/revert-to-draft-policy.test.ts new file mode 100644 index 00000000..39fc3c46 --- /dev/null +++ b/api/tests/policies/information-sharing-agreements/revert-to-draft-policy.test.ts @@ -0,0 +1,60 @@ +import { InformationSharingAgreement } from "@/models" + +import { + archiveItemFactory, + informationSharingAgreementArchiveItemFactory, + informationSharingAgreementFactory, + userFactory, +} from "@/tests/factories" + +import { RevertToDraftPolicy } from "@/policies/information-sharing-agreements" + +describe("api/src/policies/information-sharing-agreements/revert-to-draft-policy.ts", () => { + describe("RevertToDraftPolicy", () => { + describe("#create", () => { + test("allows the creator to revert a signed agreement even when knowledge items are linked", async () => { + const creator = await userFactory.create() + const informationSharingAgreement = await informationSharingAgreementFactory.create({ + status: InformationSharingAgreement.Status.SIGNED, + creatorId: creator.id, + }) + const archiveItem = await archiveItemFactory.create({ userId: creator.id }) + await informationSharingAgreementArchiveItemFactory.create({ + informationSharingAgreementId: informationSharingAgreement.id, + archiveItemId: archiveItem.id, + creatorId: creator.id, + }) + + const policy = new RevertToDraftPolicy(creator, informationSharingAgreement) + + expect(policy.create()).toBe(true) + }) + + test("does not allow reverting an agreement that is not signed", async () => { + const creator = await userFactory.create() + const informationSharingAgreement = await informationSharingAgreementFactory.create({ + status: InformationSharingAgreement.Status.DRAFT, + creatorId: creator.id, + }) + + const policy = new RevertToDraftPolicy(creator, informationSharingAgreement) + + expect(policy.create()).toBe(false) + }) + + test("does not allow an unrelated user to revert", async () => { + const creator = await userFactory.create() + const unrelatedUser = await userFactory.create() + const informationSharingAgreement = await informationSharingAgreementFactory.create({ + status: InformationSharingAgreement.Status.SIGNED, + creatorId: creator.id, + }) + await informationSharingAgreement.reload({ include: ["accessGrants"] }) + + const policy = new RevertToDraftPolicy(unrelatedUser, informationSharingAgreement) + + expect(policy.create()).toBe(false) + }) + }) + }) +}) diff --git a/api/tests/services/information-sharing-agreements/revert-to-draft-service.test.ts b/api/tests/services/information-sharing-agreements/revert-to-draft-service.test.ts new file mode 100644 index 00000000..7ad912b8 --- /dev/null +++ b/api/tests/services/information-sharing-agreements/revert-to-draft-service.test.ts @@ -0,0 +1,113 @@ +import { + Attachment, + Group, + InformationSharingAgreement, + InformationSharingAgreementArchiveItem, +} from "@/models" +import { AttachmentTargetTypes } from "@/models/attachment" + +import { + archiveItemFactory, + attachmentFactory, + groupFactory, + informationSharingAgreementArchiveItemFactory, + informationSharingAgreementFactory, + userFactory, +} from "@/tests/factories" + +import RevertToDraftService from "@/services/information-sharing-agreements/revert-to-draft-service" + +// Group removal fans out notifications that are irrelevant to reverting; silence them. +vi.mock("@/mailers/groups/notify-user-of-removal-mailer", () => { + const NotifyUserOfRemovalMailerMock = { perform: vi.fn() } + return { NotifyUserOfRemovalMailer: NotifyUserOfRemovalMailerMock, default: NotifyUserOfRemovalMailerMock } +}) +vi.mock("@/mailers/groups/notify-admins-of-removed-user-mailer", () => { + const NotifyAdminsOfRemovedUserMailerMock = { perform: vi.fn() } + return { NotifyAdminsOfRemovedUserMailer: NotifyAdminsOfRemovedUserMailerMock, default: NotifyAdminsOfRemovedUserMailerMock } +}) +vi.mock("@/services/notifications/groups/notify-user-of-removal-service", () => { + const NotifyUserOfRemovalServiceMock = { perform: vi.fn() } + return { NotifyUserOfRemovalService: NotifyUserOfRemovalServiceMock, default: NotifyUserOfRemovalServiceMock } +}) +vi.mock("@/services/notifications/groups/notify-admins-of-removed-user-service", () => { + const NotifyAdminsOfRemovedUserServiceMock = { perform: vi.fn() } + return { NotifyAdminsOfRemovedUserService: NotifyAdminsOfRemovedUserServiceMock, default: NotifyAdminsOfRemovedUserServiceMock } +}) + +describe("api/src/services/information-sharing-agreements/revert-to-draft-service.ts", () => { + describe("RevertToDraftService", () => { + describe("#perform", () => { + async function buildSignedAgreement() { + const currentUser = await userFactory.create() + const internalGroup = await groupFactory.create({ isExternal: false }) + const externalGroup = await groupFactory.create({ isExternal: true }) + const informationSharingAgreement = await informationSharingAgreementFactory.create({ + status: InformationSharingAgreement.Status.SIGNED, + internalGroupId: internalGroup.id, + externalGroupId: externalGroup.id, + signedById: currentUser.id, + signedAt: new Date(), + }) + return { currentUser, informationSharingAgreement } + } + + test("reverts to draft and clears the signing metadata", async () => { + const { currentUser, informationSharingAgreement } = await buildSignedAgreement() + + const reverted = await RevertToDraftService.perform(informationSharingAgreement, currentUser) + + expect(reverted.status).toBe(InformationSharingAgreement.Status.DRAFT) + expect(reverted.signedById).toBeNull() + expect(reverted.signedAt).toBeNull() + }) + + test("removes the agreement's groups", async () => { + const { currentUser, informationSharingAgreement } = await buildSignedAgreement() + + await RevertToDraftService.perform(informationSharingAgreement, currentUser) + + expect(await Group.count()).toBe(0) + }) + + test("keeps the signed documents attached", async () => { + const { currentUser, informationSharingAgreement } = await buildSignedAgreement() + const attachment = await attachmentFactory.create({ + targetId: informationSharingAgreement.id, + targetType: AttachmentTargetTypes.InformationSharingAgreement, + associationName: "signedConfidentialityAcknowledgement", + }) + + await RevertToDraftService.perform(informationSharingAgreement, currentUser) + + expect(await Attachment.findByPk(attachment.id)).not.toBeNull() + }) + + test("reverts even when knowledge items are linked, keeping the links", async () => { + const { currentUser, informationSharingAgreement } = await buildSignedAgreement() + const archiveItem = await archiveItemFactory.create({ userId: currentUser.id }) + const link = await informationSharingAgreementArchiveItemFactory.create({ + informationSharingAgreementId: informationSharingAgreement.id, + archiveItemId: archiveItem.id, + creatorId: currentUser.id, + }) + + const reverted = await RevertToDraftService.perform(informationSharingAgreement, currentUser) + + expect(reverted.status).toBe(InformationSharingAgreement.Status.DRAFT) + expect(await InformationSharingAgreementArchiveItem.findByPk(link.id)).not.toBeNull() + }) + + test("throws when the agreement is not signed", async () => { + const currentUser = await userFactory.create() + const informationSharingAgreement = await informationSharingAgreementFactory.create({ + status: InformationSharingAgreement.Status.DRAFT, + }) + + await expect( + RevertToDraftService.perform(informationSharingAgreement, currentUser) + ).rejects.toThrow("Only signed agreements can be reverted to draft.") + }) + }) + }) +}) diff --git a/web/src/components/archive-items/ArchiveItemAttachmentsCard.vue b/web/src/components/archive-items/ArchiveItemAttachmentsCard.vue index 68f9ba70..06a4ed53 100644 --- a/web/src/components/archive-items/ArchiveItemAttachmentsCard.vue +++ b/web/src/components/archive-items/ArchiveItemAttachmentsCard.vue @@ -55,6 +55,7 @@ import { ref, toRefs } from "vue" import archiveItemsApi from "@/api/archive-items-api" import useArchiveItem from "@/use/use-archive-item" +import useSnack from "@/use/use-snack" import ArchiveItemFileCard from "@/components/archive-item-files/ArchiveItemFileCard.vue" @@ -68,6 +69,7 @@ const emit = defineEmits<{ const { archiveItemId } = toRefs(props) const { archiveItem, policy, refresh } = useArchiveItem(archiveItemId) +const snack = useSnack() const filesToUpload = ref([]) const isUploading = ref(false) @@ -81,6 +83,9 @@ async function uploadFiles(files: File | File[]) { await archiveItemsApi.createFiles(archiveItemId.value, filesAsArray) await refresh() filesToUpload.value = [] + } catch (error) { + console.error("Failed to upload attachment:", error) + snack.error("Failed to upload attachment") } finally { isUploading.value = false } diff --git a/web/src/components/common/AttachmentAttributesRow.vue b/web/src/components/common/AttachmentAttributesRow.vue index 23a5ed78..3e1b12b4 100644 --- a/web/src/components/common/AttachmentAttributesRow.vue +++ b/web/src/components/common/AttachmentAttributesRow.vue @@ -27,7 +27,7 @@ import { computed } from "vue" import { type AttachmentAsReference } from "@/api/attachments-api" -import { formatBytes, formatDate } from "@/utils/formatters" +import { formatBytes, formatDateTime } from "@/utils/formatters" const props = defineProps<{ attachment: AttachmentAsReference @@ -65,6 +65,6 @@ const icon = computed(() => { return "mdi-file" }) -const formattedDate = computed(() => formatDate(props.attachment.createdAt)) +const formattedDate = computed(() => formatDateTime(props.attachment.createdAt)) const formattedSize = computed(() => formatBytes(props.attachment.size)) diff --git a/web/src/components/information-sharing-agreements/InformationSharingAgreementRevertToDraftDialog.vue b/web/src/components/information-sharing-agreements/InformationSharingAgreementRevertToDraftDialog.vue index c6b0b740..4c476fcf 100644 --- a/web/src/components/information-sharing-agreements/InformationSharingAgreementRevertToDraftDialog.vue +++ b/web/src/components/information-sharing-agreements/InformationSharingAgreementRevertToDraftDialog.vue @@ -10,8 +10,8 @@

- Are you sure you want to revert this agreement to draft? Any signed documents will be - removed. + Are you sure you want to revert this agreement to draft? Its groups and access will be + removed, but any signed documents and linked knowledge items will be kept.

diff --git a/web/src/components/information-sharing-agreements/signed/InformationSharingAgreementSignedActionsMenu.vue b/web/src/components/information-sharing-agreements/signed/InformationSharingAgreementSignedActionsMenu.vue index 80ff1fcf..745ca27e 100644 --- a/web/src/components/information-sharing-agreements/signed/InformationSharingAgreementSignedActionsMenu.vue +++ b/web/src/components/information-sharing-agreements/signed/InformationSharingAgreementSignedActionsMenu.vue @@ -49,7 +49,7 @@ /> Revert to Draft