Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PER-9883 [back-end] Delete feature flag endpoint #140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/api/docs/present/paths/feature_flags.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,22 @@ feature-flags/{id}:
$ref: "../../shared/errors.yaml#/400"
"500":
$ref: "../../shared/errors.yaml#/500"
delete:
parameters:
- name: id
in: path
required: true
schema:
type: string
format: uuid
summary: Delete a feature flag
security:
- bearerHttpAuthentication: []
operationId: delete-feature-flags
responses:
"204":
description: Empty response if delete was successful
"400":
$ref: "../../shared/errors.yaml#/400"
"500":
$ref: "../../shared/errors.yaml#/500"
85 changes: 84 additions & 1 deletion packages/api/src/feature_flag/controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
extractUserIsAdminFromAuthToken,
verifyAdminAuthentication,
} from "../middleware";
import type { CreateFeatureFlagRequest, FeatureFlagRequest } from "./models";
import type {
CreateFeatureFlagRequest,
DeleteFeatureFlagRequest,
FeatureFlagRequest,
} from "./models";

jest.mock("../database");
jest.mock("../middleware");
Expand Down Expand Up @@ -283,3 +287,82 @@ describe("POST /feature-flag", () => {
expect(logger.error).toHaveBeenCalled();
});
});

describe("DELETE /feature-flag/1bdf2da6-026b-4e8e-9b57-a86b1817be5d", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend adding a test for the 404 case

const agent = request(app);
beforeEach(async () => {
(verifyAdminAuthentication as jest.Mock).mockImplementation(
(req: Request, __, next: NextFunction) => {
(req.body as DeleteFeatureFlagRequest).emailFromAuthToken =
"test@permanent.org";
(req.body as DeleteFeatureFlagRequest).adminSubjectFromAuthToken =
"6b640c73-4963-47de-a096-4a05ff8dc5f5";
next();
}
);
jest.restoreAllMocks();
jest.clearAllMocks();
await clearDatabase();
await loadFixtures();
});

afterEach(async () => {
jest.restoreAllMocks();
jest.clearAllMocks();
await clearDatabase();
});

test("should respond with a 200 status code", async () => {
await agent
.delete("/api/v2/feature-flags/1bdf2da6-026b-4e8e-9b57-a86b1817be5d")
.send({})
.expect(204);
});

test("should respond with 401 status code if lacking admin authentication", async () => {
(verifyAdminAuthentication as jest.Mock).mockImplementation(
(_: Request, __, next: NextFunction) => {
next(new createError.Unauthorized("You aren't logged in"));
}
);
await agent
.delete("/api/v2/feature-flags/1bdf2da6-026b-4e8e-9b57-a86b1817be5d")
.expect(401);
});

test("should delete the feature flag in the database", async () => {
await agent
.delete("/api/v2/feature-flags/1bdf2da6-026b-4e8e-9b57-a86b1817be5d")
.send({})
.expect(204);
const result = await db.query(
`SELECT
name
FROM
feature_flag
WHERE
id = '1bdf2da6-026b-4e8e-9b57-a86b1817be5d'`
);
expect(result.rows.length).toBe(0);
});

test("should respond with 500 if the database call fails", async () => {
jest.spyOn(db, "sql").mockImplementation(() => {
throw new Error("SQL error");
});
await agent
.delete("/api/v2/feature-flags/1bdf2da6-026b-4e8e-9b57-a86b1817be5d")
.send({})
.expect(500);
});

test("should log the error if the database call fails", async () => {
const testError = new Error("SQL error");
jest.spyOn(db, "sql").mockRejectedValueOnce(testError);
await agent
.delete("/api/v2/feature-flags/1bdf2da6-026b-4e8e-9b57-a86b1817be5d")
.send({})
.expect(500);
expect(logger.error).toHaveBeenCalled();
});
});
24 changes: 23 additions & 1 deletion packages/api/src/feature_flag/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ import type { Request, Response, NextFunction } from "express";
import { logger } from "@stela/logger";
import { featureService } from "./service";
import { createFeatureService } from "./service/create";
import {
validateCreateFeatureFlagRequest,
validateDeleteFeatureFlagParams,
} from "./validators";
import { deleteFeatureService } from "./service/delete";
import {
extractUserIsAdminFromAuthToken,
verifyAdminAuthentication,
} from "../middleware";
import { validateCreateFeatureFlagRequest } from "./validators";
import { validateIsAdminFromAuthentication } from "../validators/shared";
import { isValidationError } from "../validators/validator_util";

Expand Down Expand Up @@ -52,3 +56,21 @@ featureController.post(
}
}
);

featureController.delete(
"/:featureId",
verifyAdminAuthentication,
async (req: Request, res: Response, next: NextFunction) => {
try {
validateDeleteFeatureFlagParams(req.params);
await deleteFeatureService.deleteFeatureFlag(req.params.featureId);
res.status(204).send();
} catch (err) {
if (isValidationError(err)) {
res.status(400).json({ error: err.message });
return;
}
next(err);
}
}
);
5 changes: 5 additions & 0 deletions packages/api/src/feature_flag/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,8 @@ export interface CreateFeatureFlagRequest {
export interface FeatureFlagRequest {
admin: boolean;
}

export interface DeleteFeatureFlagRequest {
emailFromAuthToken: string;
adminSubjectFromAuthToken: string;
}
6 changes: 6 additions & 0 deletions packages/api/src/feature_flag/queries/delete_feature_flag.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
DELETE FROM
feature_flag
WHERE
id = :featureFlagId
RETURNING
id AS "featureFlagId";
28 changes: 28 additions & 0 deletions packages/api/src/feature_flag/service/delete.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { logger } from "@stela/logger";
import createError from "http-errors";
import { db } from "../../database";

export const deleteFeatureFlag = async (
featureFlagId: string
): Promise<string> => {
const result = await db
.sql("feature_flag.queries.delete_feature_flag", {
featureFlagId,
})
.catch((err) => {
logger.error(err);
throw new createError.InternalServerError(
"Failed to delete feature flag"
);
});

if (result.rows[0] === undefined) {
throw new createError.NotFound("Feature Flag not found");
}

return featureFlagId;
};

export const deleteFeatureService = {
deleteFeatureFlag,
};
13 changes: 13 additions & 0 deletions packages/api/src/feature_flag/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,16 @@ export function validateCreateFeatureFlagRequest(
throw validation.error;
}
}

export function validateDeleteFeatureFlagParams(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially the same as the param validator for update; perhaps they should be one function?

data: unknown
): asserts data is { featureId: string } {
const validation = Joi.object()
.keys({
featureId: Joi.string().uuid().required(),
})
.validate(data);
if (validation.error) {
throw validation.error;
}
}
Loading