diff --git a/hasura/metadata/actions.graphql b/hasura/metadata/actions.graphql index 2ed281f7..9aaf536e 100644 --- a/hasura/metadata/actions.graphql +++ b/hasura/metadata/actions.graphql @@ -899,7 +899,7 @@ input ClipSpecInput { segments: [ClipSegmentInput!]! overlays: [ClipOverlayInput!] audio: ClipAudioInput - output: ClipOutputInput! + output: ClipOutputInput destination: String! title: String } diff --git a/hasura/migrations/default/1889000000400_admin_only_clip_settings/down.sql b/hasura/migrations/default/1889000000400_admin_only_clip_settings/down.sql new file mode 100644 index 00000000..8c1e79b5 --- /dev/null +++ b/hasura/migrations/default/1889000000400_admin_only_clip_settings/down.sql @@ -0,0 +1,8 @@ +INSERT INTO public.settings (name, value) +SELECT 'public.' || name, value + FROM public.settings + WHERE name IN ('clip_fps', 'clip_resolution') +ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value; + +DELETE FROM public.settings + WHERE name IN ('clip_fps', 'clip_resolution'); diff --git a/hasura/migrations/default/1889000000400_admin_only_clip_settings/up.sql b/hasura/migrations/default/1889000000400_admin_only_clip_settings/up.sql new file mode 100644 index 00000000..e3f46862 --- /dev/null +++ b/hasura/migrations/default/1889000000400_admin_only_clip_settings/up.sql @@ -0,0 +1,9 @@ +-- The `public.` row wins a clash: it is the value the api applied since 1889000000300. +INSERT INTO public.settings (name, value) +SELECT substring(name FROM length('public.') + 1), value + FROM public.settings + WHERE name IN ('public.clip_fps', 'public.clip_resolution') +ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value; + +DELETE FROM public.settings + WHERE name IN ('public.clip_fps', 'public.clip_resolution'); diff --git a/src/hasura/hasura.service.ts b/src/hasura/hasura.service.ts index a751b9ca..935b3177 100644 --- a/src/hasura/hasura.service.ts +++ b/src/hasura/hasura.service.ts @@ -254,10 +254,10 @@ export class HasuraService { "delete from settings where name = 'public.utility_practice_daily_limit'", ); - // Renamed to `public.` by migration 1889000000300; an admin still on the - // old web app writes the unprefixed names again, which the api ignores. + // Admin-only settings: a web app from before migration 1889000000400 still + // saves them under `public.`, which every role can read. await this.postgresService.query( - "delete from settings where name in ('clip_fps', 'clip_resolution')", + "delete from settings where name in ('public.clip_fps', 'public.clip_resolution')", ); // The light-mode branding palette was retired; only `public.color_dark_*` diff --git a/src/matches/game-streamer/game-streamer.service.ts b/src/matches/game-streamer/game-streamer.service.ts index 26314770..8b3d842b 100644 --- a/src/matches/game-streamer/game-streamer.service.ts +++ b/src/matches/game-streamer/game-streamer.service.ts @@ -386,16 +386,12 @@ export class GameStreamerService { return value === "720p" ? "720p" : "1080p"; } - // The render dialogs offer a per-clip resolution but no fps choice, so the - // operator's fps is final whatever the client sends. - public async resolveClipOutput( - resolution?: string, - ): Promise<{ resolution: "720p" | "1080p"; fps: 30 | 60 }> { + public async resolveClipOutput(): Promise<{ + resolution: "720p" | "1080p"; + fps: 30 | 60; + }> { return { - resolution: - resolution === "720p" || resolution === "1080p" - ? resolution - : await this.resolveClipResolution(), + resolution: await this.resolveClipResolution(), fps: await this.resolveClipFps(), }; } diff --git a/src/matches/matches.controller.spec.ts b/src/matches/matches.controller.spec.ts index 77b20666..33ee1279 100644 --- a/src/matches/matches.controller.spec.ts +++ b/src/matches/matches.controller.spec.ts @@ -274,10 +274,7 @@ describe("MatchesController", () => { const streamer = { role: "streamer", steam_id: "76561198000000001" }; const admin = { role: "administrator", steam_id: "76561198000000002" }; - const operatorSettings: Record = { - "public.clip_fps": "30", - "public.clip_resolution": "720p", - }; + let operatorSettings: Record; const preset = (extra: Record = {}) => controller.createClipFromPreset({ @@ -291,6 +288,8 @@ describe("MatchesController", () => { const presetOutput = () => clips.buildPresetSpec.mock.calls[0][3]; beforeEach(() => { + operatorSettings = { clip_fps: "30", clip_resolution: "720p" }; + const hasura = { query: jest.fn( async (query: { settings_by_pk?: { __args: { name: string } } }) => { @@ -327,25 +326,32 @@ describe("MatchesController", () => { expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 }); }); - it("keeps the operator's fps when the client sends its own", async () => { - await preset({ fps: 60 }); + it("ignores clip settings saved under the `public.` names", async () => { + operatorSettings = { + "public.clip_fps": "30", + "public.clip_resolution": "720p", + }; - expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 }); + await preset(); + + expect(presetOutput()).toEqual({ resolution: "1080p", fps: 60 }); }); - it("honours a resolution picked in the render dialog", async () => { + it("ignores the fps and resolution a client sends with a preset", async () => { await preset({ resolution: "1080p", fps: 60 }); - expect(presetOutput()).toEqual({ resolution: "1080p", fps: 30 }); + expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 }); }); - it("falls back to the operator's resolution for one the dialog does not offer", async () => { - await preset({ resolution: "4k" }); + it("keeps the operator's 1080p/60 over a client's 720p/30", async () => { + operatorSettings = { clip_fps: "60", clip_resolution: "1080p" }; - expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 }); + await preset({ resolution: "720p", fps: 30 }); + + expect(presetOutput()).toEqual({ resolution: "1080p", fps: 60 }); }); - it("queues a highlight at the operator's fps", async () => { + it("queues a highlight at the operator's settings whatever the client sends", async () => { await controller.queueClipFromPreset({ match_map_id: "map-1", target_steam_id: "76561198000000009", @@ -356,25 +362,40 @@ describe("MatchesController", () => { } as any); expect(clips.queueClipFromPreset.mock.calls[0][1].output).toEqual({ - resolution: "1080p", + resolution: "720p", fps: 30, }); }); - it("renders an edited clip at the operator's fps", async () => { - await controller.createClipRender({ + const renderEdited = (extra: Record = {}) => + controller.createClipRender({ spec: { match_map_id: "map-1", segments: [{ start_tick: 1, end_tick: 2 }], - output: { format: "mp4", resolution: "1080p", fps: 60 }, destination: "library", + ...extra, }, user: streamer as any, }); + it("replaces the output a client sends with an edited clip", async () => { + await renderEdited({ + output: { format: "mp4", resolution: "1080p", fps: 60 }, + }); + expect(clips.createClipRender.mock.calls[0][1].output).toEqual({ format: "mp4", - resolution: "1080p", + resolution: "720p", + fps: 30, + }); + }); + + it("renders an edited clip sent without an output at the operator's settings", async () => { + await renderEdited(); + + expect(clips.createClipRender.mock.calls[0][1].output).toEqual({ + format: "mp4", + resolution: "720p", fps: 30, }); }); diff --git a/src/matches/matches.controller.ts b/src/matches/matches.controller.ts index a6301843..159bf400 100644 --- a/src/matches/matches.controller.ts +++ b/src/matches/matches.controller.ts @@ -1945,7 +1945,10 @@ export class MatchesController { } @HasuraAction() - public async createClipRender(data: { spec: ClipSpec; user: User }) { + public async createClipRender(data: { + spec: Omit; + user: User; + }) { const { spec, user } = data; if (!isRoleAbove(user.role, "streamer")) { throw Error("clip rendering requires the streamer role or above"); @@ -1953,10 +1956,13 @@ export class MatchesController { if (!spec || !spec.match_map_id) { throw Error("invalid clip spec"); } - if (spec.output) { - spec.output.fps = await this.gameStreamer.resolveClipFps(); - } - const { jobId } = await this.clips.createClipRender(user.steam_id, spec); + const { jobId } = await this.clips.createClipRender(user.steam_id, { + ...spec, + output: { + format: "mp4", + ...(await this.gameStreamer.resolveClipOutput()), + }, + }); return { success: true, job_id: jobId, @@ -2081,7 +2087,6 @@ export class MatchesController { match_map_id: string; target_steam_id: string; preset: "knife" | "multikills" | "best_round" | "recap"; - resolution?: string; title?: string; target_name?: string; user: User; @@ -2094,7 +2099,7 @@ export class MatchesController { data.match_map_id, data.target_steam_id, data.preset, - await this.gameStreamer.resolveClipOutput(data.resolution), + await this.gameStreamer.resolveClipOutput(), data.title, data.target_name, ); @@ -2107,7 +2112,6 @@ export class MatchesController { match_map_id: string; target_steam_id: string; preset: "knife" | "multikills" | "best_round" | "recap"; - resolution?: string; title?: string; target_name?: string; user: User; @@ -2120,7 +2124,7 @@ export class MatchesController { matchMapId: data.match_map_id, targetSteamId: data.target_steam_id, preset: data.preset, - output: await this.gameStreamer.resolveClipOutput(data.resolution), + output: await this.gameStreamer.resolveClipOutput(), title: data.title, targetName: data.target_name, }); diff --git a/src/system/enums/SystemSettingName.ts b/src/system/enums/SystemSettingName.ts index ab0fc906..6f17aa33 100644 --- a/src/system/enums/SystemSettingName.ts +++ b/src/system/enums/SystemSettingName.ts @@ -72,8 +72,8 @@ export enum SystemSettingName { GamePluginRegistryUrl = "game_plugin_registry_url", DefaultBroadcastHud = "public.default_broadcast_hud", DefaultHudMode = "default_hud_mode", - ClipFps = "public.clip_fps", - ClipResolution = "public.clip_resolution", + ClipFps = "clip_fps", + ClipResolution = "clip_resolution", // VAPID identifies this panel to the browser push services. The keypair is // self-generated -- there is no vendor to register with -- so it is stored // here rather than demanding an env var of every operator. The private half diff --git a/test/public-clip-settings.spec.ts b/test/admin-only-clip-settings.spec.ts similarity index 56% rename from test/public-clip-settings.spec.ts rename to test/admin-only-clip-settings.spec.ts index f2a39804..634ef0f2 100644 --- a/test/public-clip-settings.spec.ts +++ b/test/admin-only-clip-settings.spec.ts @@ -2,15 +2,16 @@ import { readFileSync } from "fs"; import { join } from "path"; import { bootMigratedDb, SqlTestDb } from "./utils/sql-test-db"; -describe("public clip settings migration", () => { +describe("admin-only clip settings", () => { let db: SqlTestDb; - const migration = join( - __dirname, - "../hasura/migrations/default/1889000000300_public_clip_settings", - ); - const up = readFileSync(join(migration, "up.sql"), "utf8"); - const down = readFileSync(join(migration, "down.sql"), "utf8"); + const migrations = join(__dirname, "../hasura/migrations/default"); + const sql = (migration: string, file: "up" | "down") => + readFileSync(join(migrations, migration, `${file}.sql`), "utf8"); + + const publicUp = sql("1889000000300_public_clip_settings", "up"); + const up = sql("1889000000400_admin_only_clip_settings", "up"); + const down = sql("1889000000400_admin_only_clip_settings", "down"); const NAMES = [ "clip_fps", @@ -36,7 +37,7 @@ describe("public clip settings migration", () => { ); beforeAll(async () => { - db = await bootMigratedDb("PublicClipSettingsTest"); + db = await bootMigratedDb("AdminOnlyClipSettingsTest"); }, 600_000); afterAll(async () => { @@ -50,33 +51,58 @@ describe("public clip settings migration", () => { ); }); - it("moves the operator's values to the public names", async () => { - await set("clip_fps", "30"); - await set("clip_resolution", "720p"); + it("moves the operator's values back to the admin-only names", async () => { + await set("public.clip_fps", "30"); + await set("public.clip_resolution", "720p"); await db.postgres.query(up); expect(await rows()).toEqual({ - "public.clip_fps": "30", - "public.clip_resolution": "720p", + clip_fps: "30", + clip_resolution: "720p", }); }); - it("keeps the operator's value over form defaults saved under the new name", async () => { - await set("clip_fps", "30"); - await set("public.clip_fps", "60"); + it("keeps the value the admin page saved under the public name on a clash", async () => { + await set("public.clip_fps", "30"); + await set("clip_fps", "60"); await db.postgres.query(up); - expect(await rows()).toEqual({ "public.clip_fps": "30" }); + expect(await rows()).toEqual({ clip_fps: "30" }); }); it("changes nothing when run again", async () => { + await set("public.clip_fps", "30"); + await set("public.clip_resolution", "720p"); + + await db.postgres.query(up); + await db.postgres.query(up); + + expect(await rows()).toEqual({ + clip_fps: "30", + clip_resolution: "720p", + }); + }); + + it("keeps an install's value when it upgrades past both migrations at once", async () => { await set("clip_fps", "30"); await set("clip_resolution", "720p"); + await db.postgres.query(publicUp); await db.postgres.query(up); - await db.postgres.query(up); + + expect(await rows()).toEqual({ + clip_fps: "30", + clip_resolution: "720p", + }); + }); + + it("restores the public names on down", async () => { + await set("clip_fps", "30"); + await set("clip_resolution", "720p"); + + await db.postgres.query(down); expect(await rows()).toEqual({ "public.clip_fps": "30", @@ -84,27 +110,24 @@ describe("public clip settings migration", () => { }); }); - it("drops unprefixed rows an old web app writes after the rename", async () => { - await set("public.clip_fps", "30"); - await set("clip_fps", "60"); - await set("clip_resolution", "1080p"); + it("keeps the admin-only value on a clash when run down", async () => { + await set("clip_fps", "30"); + await set("public.clip_fps", "60"); - await ( - db.hasura as unknown as { updateSettings(): Promise } - ).updateSettings(); + await db.postgres.query(down); expect(await rows()).toEqual({ "public.clip_fps": "30" }); }); - it("restores the old names on down", async () => { - await set("public.clip_fps", "30"); - await set("public.clip_resolution", "720p"); + it("drops public rows an old web app writes after the rename", async () => { + await set("clip_fps", "30"); + await set("public.clip_fps", "60"); + await set("public.clip_resolution", "1080p"); - await db.postgres.query(down); + await ( + db.hasura as unknown as { updateSettings(): Promise } + ).updateSettings(); - expect(await rows()).toEqual({ - clip_fps: "30", - clip_resolution: "720p", - }); + expect(await rows()).toEqual({ clip_fps: "30" }); }); });