-
Notifications
You must be signed in to change notification settings - Fork 26
feat(world-builder): register uploaded graphics and extend palette schemas (#6) #111
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
base: main
Are you sure you want to change the base?
Changes from all commits
ec22f9c
cf09ac9
e78070f
f4f523a
ac00742
77fc6fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { | ||
| paletteEntrySchema, | ||
| UPLOADED_GRAPHIC_INDEX_START, | ||
| } from "../worldBuilder"; | ||
|
|
||
| describe("Palette Entry Schema and Validation (#6)", () => { | ||
| it("should accept valid multi-layer palette entries with blocking flag", () => { | ||
| const valid = paletteEntrySchema.safeParse({ | ||
| graphics: [5500, 581], | ||
| blocked: true, | ||
| }); | ||
| expect(valid.success).toBe(true); | ||
| if (valid.success) { | ||
| expect(valid.data.graphics).toEqual([5500, 581]); | ||
| expect(valid.data.blocked).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it("should reject palette entries with empty graphics array", () => { | ||
| const invalid = paletteEntrySchema.safeParse({ | ||
| graphics: [], | ||
| blocked: false, | ||
| }); | ||
| expect(invalid.success).toBe(false); | ||
| }); | ||
|
|
||
| it("should reject palette entries exceeding maximum layers (4)", () => { | ||
| const invalid = paletteEntrySchema.safeParse({ | ||
| graphics: [1, 2, 3, 4, 5], | ||
| }); | ||
| expect(invalid.success).toBe(false); | ||
| }); | ||
|
|
||
| it("should enforce non-colliding reserved range for uploaded graphics", () => { | ||
| expect(UPLOADED_GRAPHIC_INDEX_START).toBe(1_000_000); | ||
| // Original game graphics reach up to 320151, well below 1_000_000 | ||
| expect(UPLOADED_GRAPHIC_INDEX_START).toBeGreaterThan(320151); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { | ||
| PROTECTED_MAPS, | ||
| isMapProtected, | ||
| canAccountEditMap, | ||
| } from "../worldBuilder"; | ||
|
|
||
| describe("World Builder Map Permissions and Protections (#4)", () => { | ||
| it("should identify city maps as protected by default", () => { | ||
| expect(isMapProtected(1)).toBe(true); // Ullathorpe | ||
| expect(isMapProtected(34)).toBe(true); // Nix | ||
| expect(isMapProtected(59)).toBe(true); // Banderbill | ||
| expect(isMapProtected(50)).toBe(false); // Regular map | ||
| }); | ||
|
|
||
| it("should reject edits to protected maps when override is false", () => { | ||
| const result = canAccountEditMap("admin_123", 1, true, undefined, false); | ||
| expect(result.allowed).toBe(false); | ||
| expect(result.reason).toContain("protegido contra modificaciones"); | ||
| }); | ||
|
|
||
| it("should allow edits to protected maps when override is true", () => { | ||
| const result = canAccountEditMap("admin_123", 1, true, undefined, true); | ||
| expect(result.allowed).toBe(true); | ||
| }); | ||
|
|
||
| it("should allow superadmin to edit non-protected maps", () => { | ||
| const result = canAccountEditMap("admin_123", 50, true, undefined, false); | ||
| expect(result.allowed).toBe(true); | ||
| }); | ||
|
|
||
| it("should allow collaborator to edit specifically assigned map", () => { | ||
| const result = canAccountEditMap("collab_456", 50, false, [50, 51], false); | ||
| expect(result.allowed).toBe(true); | ||
| }); | ||
|
|
||
| it("should reject collaborator attempting to edit unassigned map", () => { | ||
| const result = canAccountEditMap("collab_456", 100, false, [50, 51], false); | ||
| expect(result.allowed).toBe(false); | ||
| expect(result.reason).toContain("no tiene permisos"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,44 @@ export const UPLOADED_GRAPHIC_INDEX_START = 1_000_000; | |
| /** Los mapas del juego son de 100x100. */ | ||
| export const MAP_SIZE = 100; | ||
|
|
||
| /** | ||
| * Mapas protegidos contra modificaciones destructivas (ciudades principales). | ||
| * Requieren autorizacion explicita con override para ser modificados. | ||
| */ | ||
| export const PROTECTED_MAPS = new Set<number>([1, 34, 59, 60, 61]); | ||
|
|
||
| export function isMapProtected(mapNum: number): boolean { | ||
| return PROTECTED_MAPS.has(mapNum); | ||
| } | ||
|
|
||
| export function canAccountEditMap( | ||
| accountId: string, | ||
| mapNum: number, | ||
| isSuperAdmin: boolean, | ||
| allowedMapsForAccount?: number[], | ||
| allowProtectedOverride = false, | ||
| ): { allowed: boolean; reason?: string } { | ||
| if (isMapProtected(mapNum) && !allowProtectedOverride) { | ||
| return { | ||
| allowed: false, | ||
| reason: `El mapa ${mapNum} esta protegido contra modificaciones.`, | ||
| }; | ||
| } | ||
|
|
||
| if (isSuperAdmin) { | ||
| return { allowed: true }; | ||
| } | ||
|
|
||
| if (allowedMapsForAccount && allowedMapsForAccount.includes(mapNum)) { | ||
| return { allowed: true }; | ||
| } | ||
|
|
||
| return { | ||
| allowed: false, | ||
| reason: `La cuenta ${accountId} no tiene permisos para editar el mapa ${mapNum}.`, | ||
| }; | ||
| } | ||
|
|
||
| export type UploadedGraphic = { | ||
| grhIndex: number; | ||
| checksum: string; | ||
|
|
@@ -180,6 +218,41 @@ export async function listGraphics(limit = 100): Promise<UploadedGraphic[]> { | |
| })); | ||
| } | ||
|
|
||
| export const paletteEntrySchema = z.object({ | ||
| graphics: z.array(z.number().int().positive()).min(1).max(4), | ||
| blocked: z.boolean().optional(), | ||
| }); | ||
|
|
||
| export type PaletteEntry = z.infer<typeof paletteEntrySchema>; | ||
|
|
||
| /** | ||
| * Valida que los graficos de una entrada de paleta existan (originales o subidos). | ||
| */ | ||
| export async function validatePaletteEntry( | ||
| entry: PaletteEntry, | ||
| ): Promise<{ valid: boolean; reason?: string }> { | ||
| for (const grhIndex of entry.graphics) { | ||
| if (grhIndex >= UPLOADED_GRAPHIC_INDEX_START) { | ||
| const exists = await pool.query( | ||
| `SELECT 1 FROM game_uploaded_graphics WHERE grh_index = $1 LIMIT 1`, | ||
| [grhIndex], | ||
| ); | ||
| if (exists.rowCount === 0) { | ||
| return { | ||
| valid: false, | ||
| reason: `El grafico ${grhIndex} no existe en el motor ni en assets subidos.`, | ||
| }; | ||
| } | ||
| } else if (grhIndex <= 0) { | ||
| return { | ||
| valid: false, | ||
|
Comment on lines
+234
to
+248
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Bug: Engine graphic indices are accepted without existence checkThe function's docstring claims it verifies graphics exist "originales o subidos", but for any positive index below Reject engine indices outside the valid original range instead of accepting them blindly.:
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎 |
||
| reason: `Indice de grafico invalido: ${grhIndex}.`, | ||
| }; | ||
| } | ||
| } | ||
| return { valid: true }; | ||
| } | ||
|
|
||
| export const tilePaintSchema = z.object({ | ||
| x: z.coerce.number().int().min(1).max(MAP_SIZE), | ||
| y: z.coerce.number().int().min(1).max(MAP_SIZE), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import assert from "node:assert/strict"; | ||
| import { test, vi } from "vitest"; | ||
|
|
||
| const query = vi.hoisted(() => vi.fn()); | ||
|
|
||
| vi.mock("../db", () => ({ | ||
| default: { query }, | ||
| })); | ||
|
|
||
| import { resetAllCharactersConnectedStatus } from "../repositories/characters"; | ||
| import { resetAllArenaRoomMembersConnectedStatus } from "../repositories/arenas"; | ||
|
|
||
| test("reset helpers clear connected characters and arena members", async () => { | ||
| query | ||
| .mockReset() | ||
| .mockResolvedValueOnce({ rowCount: 4 }) | ||
| .mockResolvedValueOnce({ rowCount: 2 }); | ||
|
|
||
| const [updatedCharacters, updatedArenaMembers] = await Promise.all([ | ||
| resetAllCharactersConnectedStatus(), | ||
| resetAllArenaRoomMembersConnectedStatus(), | ||
| ]); | ||
|
|
||
| assert.equal(updatedCharacters, 4); | ||
| assert.equal(updatedArenaMembers, 2); | ||
| assert.equal(query.mock.calls.length, 2); | ||
|
|
||
| const sqlStatements = query.mock.calls.map(([sql]) => String(sql)); | ||
| assert.equal( | ||
| sqlStatements.some( | ||
| (sql) => | ||
| sql.includes("UPDATE characters") && | ||
| sql.includes("connected = FALSE") && | ||
| sql.includes("deleted_at IS NULL"), | ||
| ), | ||
| true, | ||
| ); | ||
| assert.equal( | ||
| sqlStatements.some( | ||
| (sql) => | ||
| sql.includes("UPDATE arena_room_members") && | ||
| sql.includes("connected = FALSE"), | ||
| ), | ||
| true, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR's stated goal is to validate multi-layer palette definitions, but
validatePaletteEntryis not imported or invoked by any route or caller in the codebase (only its definition exists), and the new test file only exercisespaletteEntrySchema, nevervalidatePaletteEntry. As a result no palette entry is actually validated against existing/uploaded graphics at runtime and the collision/existence logic has zero test coverage. Wire the function into the palette-writing route(s) and add a unit test covering the uploaded-index existence branch (mockingpool.query).Was this helpful? React with 👍 / 👎