UN-2308 delete other cost by Kariamos · Pull Request #517 · AppQuality/tryber-api

Kariamos · 2026-02-02T14:02:22Z

No description provided.

github-actions · 2026-02-02T14:03:42Z

Tests difference:

New Tests

< Authentication and Authorization - Should return 200 if logged in as admin
< Authentication and Authorization - Should return 200 if logged in as olp with access to campaign
< Authentication and Authorization - Should return 403 if user does not have access to campaign
< Authentication and Authorization - Should return 403 if user is not authenticated
< Input Validation - Should return 400 if cost_id is missing
< Input Validation - Should return 400 if cost_id is negative
< Input Validation - Should return 400 if cost_id is not a number
< Input Validation - Should return 400 if cost_id is null
< Input Validation - Should return 400 if cost_id is zero
< Not Found  - Should return 404 if cost belongs to another campaign
< Not Found  - Should return 404 if cost does not exist
< S3 Deletion - Should call deleteFromS3 once for cost with one attachment
< S3 Deletion - Should call deleteFromS3 three times for cost with three attachments
< S3 Deletion - Should not call deleteFromS3 if cost has no attachments
< S3 Deletion - Should only delete S3 files for the specified cost, not others
< Success - admin permissions - Should delete correctly only one cost item
< Success - admin permissions - Should delete cost and all its attachments
< Success - admin permissions - Should delete cost from database
< Success - admin permissions - Should delete cost without attachments
< Success - admin permissions - Should only delete attachments of the deleted cost
< Success - admin permissions - Should only delete specified cost, not others
< Success - olp permissions - Should delete correctly only one cost item
< Success - olp permissions - Should delete cost and attachments 
< Success - olp permissions - Should delete cost with olp permissions

coveralls · 2026-02-02T14:07:23Z

coverage: 79.626% (+0.03%) from 79.601%
when pulling cfa9e02 on UN-2308-delete-other-cost
into 76f0fc9 on UN-2274-other-costs.

Copilot

Pull request overview

This pull request implements a DELETE endpoint for campaign other costs (finance/otherCosts), allowing authorized users to delete cost records and their associated attachments from both the database and S3 storage.

Changes:

Added DELETE operation definition to OpenAPI schema (schema.ts and openapi.yml)
Implemented DELETE route handler with authorization, validation, and S3 cleanup logic
Added comprehensive test suite covering authentication, authorization, input validation, and deletion scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/schema.ts	Added TypeScript type definitions for the DELETE operation including parameters, request body, and response types
src/reference/openapi.yml	Added OpenAPI specification for the DELETE endpoint with request/response schemas and security requirements
src/routes/campaigns/campaignId/finance/otherCosts/_delete/index.ts	Implemented route handler with access control, validation, and logic to delete costs and S3 attachments
src/routes/campaigns/campaignId/finance/otherCosts/_delete/index.spec.ts	Added comprehensive test suite with 806 lines covering auth, validation, deletion success cases, and S3 interaction scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-02T14:44:46Z

src/routes/campaigns/campaignId/finance/otherCosts/_delete/index.spec.ts

+  describe("S3 Deletion", () => {
+    it("Should not call deleteFromS3 if cost has no attachments", async () => {
+      await tryber.tables.WpAppqCampaignOtherCosts.do().insert({
+        id: 1,
+        campaign_id: 1,
+        description: "Cost without attachments",
+        cost: 100.0,
+        type_id: 1,
+        supplier_id: 1,
+      });
+
+      const response = await request(app)
+        .delete("/campaigns/1/finance/otherCosts")
+        .send({ cost_id: 1 })
+        .set("Authorization", "Bearer admin");
+      expect(response.status).toBe(200);
+      expect(deleteFromS3).toBeCalledTimes(0);
+    });
+
+    it("Should call deleteFromS3 once for cost with one attachment", async () => {
+      await tryber.tables.WpAppqCampaignOtherCosts.do().insert({
+        id: 1,
+        campaign_id: 1,
+        description: "Cost with one attachment",
+        cost: 100.0,
+        type_id: 1,
+        supplier_id: 1,
+      });
+
+      await tryber.tables.WpAppqCampaignOtherCostsAttachment.do().insert({
+        id: 1,
+        cost_id: 1,
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/file1.pdf",
+        mime_type: "application/pdf",
+      });
+
+      const response = await request(app)
+        .delete("/campaigns/1/finance/otherCosts")
+        .send({ cost_id: 1 })
+        .set("Authorization", "Bearer admin");
+      expect(response.status).toBe(200);
+      expect(deleteFromS3).toBeCalledTimes(1);
+      expect(deleteFromS3).toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/file1.pdf",
+      });
+    });
+
+    it("Should call deleteFromS3 three times for cost with three attachments", async () => {
+      await tryber.tables.WpAppqCampaignOtherCosts.do().insert({
+        id: 1,
+        campaign_id: 1,
+        description: "Cost with multiple attachments",
+        cost: 100.0,
+        type_id: 1,
+        supplier_id: 1,
+      });
+
+      await tryber.tables.WpAppqCampaignOtherCostsAttachment.do().insert([
+        {
+          id: 1,
+          cost_id: 1,
+          url: "https://s3.eu-west-1.amazonaws.com/bucket/file1.pdf",
+          mime_type: "application/pdf",
+        },
+        {
+          id: 2,
+          cost_id: 1,
+          url: "https://s3.eu-west-1.amazonaws.com/bucket/file2.jpg",
+          mime_type: "image/jpeg",
+        },
+        {
+          id: 3,
+          cost_id: 1,
+          url: "https://s3.eu-west-1.amazonaws.com/bucket/file3.png",
+          mime_type: "image/png",
+        },
+      ]);
+
+      const response = await request(app)
+        .delete("/campaigns/1/finance/otherCosts")
+        .send({ cost_id: 1 })
+        .set("Authorization", "Bearer admin");
+      expect(response.status).toBe(200);
+      expect(deleteFromS3).toBeCalledTimes(3);
+      expect(deleteFromS3).toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/file1.pdf",
+      });
+      expect(deleteFromS3).toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/file2.jpg",
+      });
+      expect(deleteFromS3).toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/file3.png",
+      });
+    });
+
+    it("Should only delete S3 files for the specified cost, not others", async () => {
+      await tryber.tables.WpAppqCampaignOtherCosts.do().insert([
+        {
+          id: 1,
+          campaign_id: 1,
+          description: "Cost to delete",
+          cost: 100.0,
+          type_id: 1,
+          supplier_id: 1,
+        },
+        {
+          id: 2,
+          campaign_id: 1,
+          description: "Cost to keep",
+          cost: 200.0,
+          type_id: 2,
+          supplier_id: 2,
+        },
+      ]);
+
+      await tryber.tables.WpAppqCampaignOtherCostsAttachment.do().insert([
+        {
+          id: 1,
+          cost_id: 1,
+          url: "https://s3.eu-west-1.amazonaws.com/bucket/delete1.pdf",
+          mime_type: "application/pdf",
+        },
+        {
+          id: 2,
+          cost_id: 1,
+          url: "https://s3.eu-west-1.amazonaws.com/bucket/delete2.jpg",
+          mime_type: "image/jpeg",
+        },
+        {
+          id: 3,
+          cost_id: 2,
+          url: "https://s3.eu-west-1.amazonaws.com/bucket/keep.png",
+          mime_type: "image/png",
+        },
+      ]);
+
+      const response = await request(app)
+        .delete("/campaigns/1/finance/otherCosts")
+        .send({ cost_id: 1 })
+        .set("Authorization", "Bearer admin");
+      expect(response.status).toBe(200);
+      expect(deleteFromS3).toBeCalledTimes(2);
+      expect(deleteFromS3).toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/delete1.pdf",
+      });
+      expect(deleteFromS3).toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/delete2.jpg",
+      });
+      expect(deleteFromS3).not.toHaveBeenCalledWith({
+        url: "https://s3.eu-west-1.amazonaws.com/bucket/keep.png",
+      });
+    });
+  });


There are no tests covering S3 deletion failure scenarios. If deleteFromS3 fails or throws an error, the code throws a generic error (line 71) and the transaction fails (line 45), but this behavior is not tested. Add test cases that mock deleteFromS3 to reject/throw errors to verify the endpoint properly handles S3 failures and returns a 500 error without leaving the database in an inconsistent state.

Copilot · 2026-02-02T14:44:47Z

src/routes/campaigns/campaignId/finance/otherCosts/_delete/index.spec.ts

+    });
+  });
+
+  describe("Not Found ", () => {


Extra space after "Not Found" in the describe block name. Remove the trailing space for consistency.

Suggested change

describe("Not Found ", () => {

describe("Not Found", () => {

Copilot · 2026-02-02T14:44:47Z

src/routes/campaigns/campaignId/finance/otherCosts/_delete/index.spec.ts

+      expect(getResponse.body.items).toHaveLength(0);
+    });
+
+    it("Should delete cost and attachments ", async () => {


Extra space before closing quote in the test description. Remove the trailing space for consistency.

Suggested change

it("Should delete cost and attachments ", async () => {

it("Should delete cost and attachments", async () => {

Copilot · 2026-02-02T14:44:47Z

src/reference/openapi.yml

+      - schema:
+          type: string
+        name: campaign
+        in: path
+        required: true


The campaign parameter is defined twice - once inline and once through a reference. This creates a duplicate parameter definition. Remove the inline parameter definition (lines 13449-13453) and keep only the reference on line 13454, or vice versa, but not both.

Suggested change

- schema:

type: string

name: campaign

in: path

required: true

Copilot · 2026-02-02T14:44:48Z

src/routes/campaigns/campaignId/finance/otherCosts/_delete/index.ts

+    if (attachments.length > 0) {
+      for (const attachment of attachments) {
+        try {
+          await deleteFromS3({ url: attachment.url });
+        } catch (e) {
+          console.error(
+            `Error deleting attachment from S3: ${attachment.url}`,
+            e
+          );
+          throw new Error("Error deleting attachment from S3");
+        }
+        await tryber.tables.WpAppqCampaignOtherCostsAttachment.do()
+          .where({ id: attachment.id })
+          .delete();
+      }


The deletion logic processes attachments one at a time in a loop: delete from S3, then delete from database. If multiple attachments exist and S3 deletion fails for any attachment after the first, the database will be in an inconsistent state (some attachments deleted from both S3 and DB, others still in DB). Consider collecting all S3 URLs first, deleting all from S3, and only then deleting all database records if all S3 deletions succeed. This ensures atomicity - either all attachments are deleted or none are.

it@app-quality.com and others added 4 commits February 2, 2026 14:20

Modified src/reference/openapi.yml

4eca463

feat: add DELETE endpoint for campaign finance other costs

b396767

feat: implement DELETE endpoint for campaign finance other costs

b89e62c

feat: implement S3 deletion for campaign finance other costs

86ca664

fix: ensure attachments are deleted from the database after S3 deletion

cfa9e02

Kariamos requested a review from Copilot February 2, 2026 14:39

Copilot started reviewing on behalf of Kariamos February 2, 2026 14:40 View session

iDome89 merged commit e114d33 into UN-2274-other-costs Feb 2, 2026
8 checks passed

iDome89 deleted the UN-2308-delete-other-cost branch February 2, 2026 14:43

Copilot AI reviewed Feb 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UN-2308 delete other cost#517

UN-2308 delete other cost#517
iDome89 merged 5 commits intoUN-2274-other-costsfrom
UN-2308-delete-other-cost

Kariamos commented Feb 2, 2026

Uh oh!

github-actions bot commented Feb 2, 2026

Uh oh!

coveralls commented Feb 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	it("Should delete cost and attachments ", async () => {
	it("Should delete cost and attachments", async () => {

Conversation

Kariamos commented Feb 2, 2026

Uh oh!

github-actions bot commented Feb 2, 2026

Tests difference:

Uh oh!

coveralls commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Feb 2, 2026 •

edited

Loading