Mall staff workflow: object checker, inspection and JSON export - #422
Open
DJAscendance wants to merge 19 commits into
Open
Mall staff workflow: object checker, inspection and JSON export#422DJAscendance wants to merge 19 commits into
DJAscendance wants to merge 19 commits into
Conversation
Adds staff-facing tooling for the Mall moderation workflow: - Shared MallObjectRow / ObjectViewer components and a mall-actions mixin, replacing the duplicated action wiring across the five staff pages (pending, search, soldout, stocked, warehouse). - Object checker page with X_ITE-backed preview and technical facts pane. - VRML libs: tokenizer, scene scanner and WorldInfo comparison. - Mall inspection service producing structured per-object findings. - Mall export service streaming a JSON export of Mall objects, with a cheap (derived=0) mode that performs no source reads. - Object source service for reading stored object files. - Repository/controller/route wiring for the three new staff endpoints. No schema migration. Export schema documented in docs/mall-export-schema.md.
Remediation of the review findings on PR #13, plus restoration of the uploader rejection notice that the historical Mall had and CTR had lost. Export - Stream settles on drain/close/error and removes its listeners on every path, so a client that disconnects while backpressured no longer leaves export() pending forever. - All global queries moved into a preflight that runs before any header is sent, so a failure there is a clean 500 instead of a truncated body. - Failures emit a stable public error code; raw Error.message could carry absolute asset paths into a document that gets passed around. - Truncation reports the last emitted object id rather than a row count. - A catalogue of exactly MAX_OBJECTS reports complete; only a genuine overflow is truncated. Covered at MAX-1, MAX and MAX+1. - Scope is now Pending objects only (schemaVersion 2.0.0). The document is the submission queue the Mall Checker publishes to the Mall's own site, not the CTR catalogue; it says so in schema.scope. Stores stay as reference data. The export control is offered only on the Pending list. - Placement comes from the keyed store map, which already collapses to one row per object, so it cannot fan the export page out. Inspection and source - realpath containment on both the configured root and the candidate, so a symlink inside the assets root can no longer read outside it. A missing target is reported as missing rather than as an escape, and a legitimately symlinked ASSETS_DIR keeps working. - Textures referenced through a subdirectory are now checked and reported: uploads are stored flat, so such a reference can never resolve. - Findings carry a severity of info / warning / needs_staff_review, derived from the finding code in one place. needs_staff_review means the page could not establish the facts below it, not "worst"; an unrecognised code defaults there rather than being quietly downgraded. - Decompression moved off the event loop. The async form enforces maxOutputLength identically on the deployed node 14.21.3. VRML - WorldInfo declared inside a PROTO body no longer satisfies the scene-level requirement or drive comparisons. - A relative url that climbs out of the object directory is reported as an external reference; an in-directory subpath still is not. - Field prefixes must actually end at the label, so "Pricey:" is no longer read as "Price". Longer prefixes still win over shorter ones. - Numeric fields are anchored, so "USD 75" and "not 25" no longer parse. - A field declared twice with different values is UNPARSED with a note rather than silently resolving to whichever came first. Staff UI - The viewer releases the previous LoadSensor before installing the next and disposes the browser on teardown, so a long review session stops accumulating sensors watching the same Inline. - Inspection and raw-source responses are discarded if staff have already moved on, so a slow reply cannot render under another object's id. - A failed raw-source fetch is no longer cached as if it had succeeded. - The decompressed .wrl download goes through the authenticated client; a bare <a href> cannot carry the apitoken header and was rejected with 400. - Queue extension subtracts consumed objects from the offset, so acting on the last row of a page no longer skips the row it exposes. - Rows without a stored thumbnail render a placeholder instead of requesting /assets/object/undefined/undefined. - Search restores its term, limit and offset; Out of Stock clamps the page after the list shrinks; the Warehouse store lookup reports its failures. - Staff edits set isProcessing, which their buttons already bound. - The export download takes the server's timestamped filename and no longer re-serialises an already-parsed payload. Housekeeping in the files this feature touches: dead declarations removed, missing semicolons added, the `Object` model aliased so it stops shadowing the global built-in, and mall-object.repository.ts normalised to LF (it was CRLF, which was 60 linebreak-style errors on its own). No schema migration. No production state was read or written.
The checker is opened from Warehouse, Stocked, Out of Stock and Search as well as from Pending, but its action bar rendered Accept and Reject for every object. Both endpoints mutate status without regard to the current one, so rejecting a stocked object would delete and refund it -- the server's only short-circuit is for an object that is already deleted. Gated on the object's own status rather than the list it was reached from, so a stale `from` in the url cannot re-enable them. Edit Name and Update Limit stay available everywhere, and a line explains why the two buttons are absent rather than leaving a gap.
The schema doc still described version 1.0.0 exporting every object regardless of status, and described ctrViews as overlapping in a document where five of the six lists are now empty by construction. Both are stated accurately, along with the second-precision download filename and the note that stores remains the full list as reference data.
Both from the re-review of the remediation commit. The refund now runs before the status change. `createObjectUploadRefundTransaction` already performs the wallet credit and the transaction row inside one knex transaction, so the refund is all-or-nothing by itself; doing it first means a refund failure leaves the object still pending, the 400 staff see is honest, and their retry re-runs the whole rejection correctly. The previous order was worse than it looked. A refund that threw after the status was already set left an object marked deleted whose uploader was never paid, and the already-rejected guard added in 5952288 then turned every retry into a no-op success -- putting the money permanently out of reach through the API. That is a regression the guard introduced, and reordering removes it. The residual window is the reverse case: a refund that lands followed by a failing status update, where a retry refunds twice. It is a single-row update by primary key rather than a multi-statement transaction, it is visible in the member's transaction history, and it errs towards the uploader. Closing it entirely needs one transaction spanning both writes, which means threading a trx through the object repository. Two regression tests pin the ordering and the recoverability. Separately, both download helpers revoked their blob url in the same task as the click, which some browsers treat as cancelling the download. Deferred by a tick in the checker's source download and in the export save.
Atomicity --------- Rejection now runs as one transaction: the object row is read `FOR UPDATE`, its status re-checked inside that transaction, then the wallet credit, the ledger row and the status change all commit together or not at all. The refund's own transaction is reused rather than nested, via an optional trx on `createObjectUploadRefundTransaction`. This closes the window the previous ordering left. Refunding first and rejecting second meant a failed status update left an uploader paid for an object that was still pending, and the retry paid them again. The row lock closes the concurrent case too: two staff rejecting the same object at the same moment used to both read STATUS_PENDING and both credit the wallet; the second now blocks until the first commits and sees STATUS_DELETED. The uploader's notification stays outside the transaction, after the commit. A mail failure must not roll back a completed refund, so it is still reported as `notified: false` on an otherwise successful rejection. Server-side state authority --------------------------- Reject and Accept both decide from the status read under the lock, not from what the browser sent. A crafted or stale request against a stocked object is refused rather than refunding it. Hiding the buttons in the SPA was never enough. Approval also awaited --------------------- `updateStatusApproved` fired `addToMallObjects` without awaiting it, so the status could commit -- and the request report success -- before the object was placed in the Mall. `approvePendingObject` awaits it inside the transaction; the mall repository takes an optional trx because that insert locks the same object row, and on a separate connection it would wait for a transaction waiting on it. Tests ----- `object.service.atomic.spec.ts` proves these against a real MySQL, because mocks cannot: rollback of a wallet credit when a later write fails, exactly one refund across a failed attempt and a retry, refusal of already-rejected and non-pending objects, and one refund when two rejections race. Removing `.forUpdate()` makes the race test fail, which is what makes it a proof rather than a description. It registers as skipped, never as passing, when no database is configured. Lint ---- Every file this branch touches now leaves with zero errors and zero warnings. The `any` returns are replaced with real row and document types rather than suppressed, which surfaced several things they had been hiding: - `ObjectRepository.removeAccount` ended with `return object;`, referencing a binding that does not exist -- a ReferenceError on every account removal. - `MemberService.getMemberId` was annotated `Promise<number>` while returning rows; every caller already read `[0].id`. - `RoleAssignmentService.countByAssigned` was annotated `RoleAssignment[]` for a count query. - The `Object` model shadowed the global built-in wherever it was imported unaliased, in a codebase that also calls `Object.values`. - `object.description` was missing from the model despite existing on the table. - Rows that services decorate after a query (`instances`, `store`, `username`, and friends) now say so in their types instead of being untyped. Two dead branches are left deliberately inert and annotated: `AdminController.addDonor` and `MemberController.getOnlineUsers` compare an access-level list to a string, so they have never run. Turning them into `.includes(...)` would newly enable an access-gated path, which is not a change to make while fixing types. Raised separately for a decision.
Four findings from the review of 3d5e3e7. The wallet credit was a read-modify-write, and that loses money. The object-row lock only serialises rejections of the SAME object; two different objects belonging to one uploader are not serialised by it, so both transactions could read the same balance and the second would overwrite the first. Both ledger rows committed and one refund vanished. Reproduced before fixing -- two concurrent rejections of one uploader's objects credited 50 instead of 100 -- and the credit is now `balance = balance + ?` in SQL. A wallet that does not exist now raises rather than silently crediting nothing, because the caller is mid-refund and must not commit a ledger row for money never paid. The same read-modify-write shape remains in the other credit helpers on this repository (daily credit, unsold-object refund, purchase). They are outside this feature and untouched; raised separately. The export's time budget was checked once per 200-row page. In derived mode each object is read, decompressed, hashed and scanned, so a page could run long past the deadline before anything noticed. The deadline is now checked per row and the document truncates mid-page, with the cursor still pointing at the last object actually emitted. The export dialog still described a whole-catalogue download -- "every object, every store" -- which stopped being true when the scope became pending-only. It now says what the file contains, and that `stores` is reference data rather than an index of what is inside. `checker.vue` resolved its object id with `Number.parseInt`, which takes a numeric prefix, so `/mall/checker/3339-not-an-id` inspected object 3339. The API already refuses ids like that; the checker no longer sends them.
The staff pages rendered as a detached full-window application with their own left sidebar, which is not what the Mall's tools are: they are part of Cybertown, and staff use them alongside the chat and the 3D world rather than instead of them. The staff routes now render in the site's normal content region and put their navigation in the historical right-hand control panel, through the `tools` named router-view every other Cybertown page already uses. The left sidebar is gone rather than duplicated. - A staff-only MALL CHECK control sits between MY UPLOADS and UPDATE on the Mall's own control panel, gated on the server-authoritative `/mall/can_admin` rather than on a client-side role flag. - Warehouse launches as a popup, the same `window.open` mechanism Inbox and the message boards already use, so a dropper announcing a drop in Mall chat keeps the main window in the Mall while placing items. Its route stays bare for that reason; every other staff route gains the normal chrome. The direct route remains as a deep link. - The Pending export control is renamed EXPORT PENDING JSON, because the export is pending-only and "Export Mall Data" implied a catalogue. - It is hidden when Pending is empty. Owner QA found it still offered -- and still downloadable -- beside "No items to show", which is a download of nothing presented as a dataset. The endpoint itself still answers safely with an empty export; this is a UI-visibility fix, not a reason to make the API fail. The Pending list publishes its own count for that gate rather than the control counting separately, so the button and the list on screen can never disagree.
The checker had all the right facts and no hierarchy, and one of its controls broke the page: SHOW RAW VRML expanded the source inline, and a real object's long lines pushed the document wider than the Cybertown frame so the whole page scrolled sideways. Layout, following what a checker actually does -- look at the object, then read what was found in it: - Left: the 3D preview, with Findings directly beneath it. - Right: the thumbnail first (it is what a buyer sees), then WorldInfo, the WorldInfo/CTR comparison, the file facts, the node counts, and a compact moderation panel. Every technical value is kept. Findings now lead with the plain-language sentence and carry the machine code beneath it as "Technical:", so the page reads for someone who knows VRML97 and for someone who only needs to know whether the object is alright. The comparison table gains explicit CTR RECORD / WORLDINFO / RESULT headers rather than three unlabelled columns. Raw source, the full-size thumbnail and the stored-file details each open in a bounded dialog that scrolls internally and cannot widen the document. The source viewer defaults to horizontal scrolling, because a VRML line is a meaningful unit and reflowing it by default would misrepresent the file; "Wrap lines" opts into a soft-wrapped view of the same bytes. The thumbnail is itself the control that opens full size, so the separate THUMBNAIL button is gone. The rejection field is a few lines wide instead of the width of the page. The 2000-character server limit is unchanged. Queue controls read as Previous Item / Next Item / Back to Pending rather than compact developer navigation. The id-based queue logic is untouched, and no global keyboard shortcut was added -- Escape closes a dialog and nothing else, so typing a rejection reason keeps every key. Two columns only above 1024px; below that the panes stack, which is what keeps a 768px portrait tablet inside the frame. No fixed pixel widths.
Owner QA: the downloaded JSON was one enormous minified line and could not realistically be read by eye. The document is now indented with two spaces, and it still streams. Each bounded value -- the schema, the store list, one object entry, the result record -- is serialised and indented on its own before it is written, so the export never holds the whole document in memory and the number of writes still scales with the content. Backpressure, disconnect handling, the stable Pending snapshot, the preflight deadline and the truncation reporting are all untouched. Whitespace is not part of the contract: the regression tests assert the shape and, separately, that the parsed data is identical to what the compact serialisation produced.
Owner QA finding: rejection told the uploader what happened and why, acceptance was silent. Accept now sends the uploader an inbox notice, built the same way the rejection notice is: the uploader, their home place and the object's name are all resolved server-side from the row the moderation transaction returned, so the browser cannot choose who is told or what the notice claims happened. No date is invented. There is no authoritative next-Mall-drop date in this workflow, so the notice says the item is Coming Soon and waiting in the Warehouse for the next drop rather than promising a day nothing guarantees. The notice is sent after the transition commits and never rolls it back: a delivery failure returns a successful acceptance with `notified: false` and asks staff to follow up, rather than a 500 that invites a retry. A losing concurrent Accept -- one that performed no transition -- reports `alreadyAccepted` and sends nothing, so one acceptance produces exactly one notice. The three outcomes are distinct in the UI too: an already-accepted race is not reported as a notification failure.
Three owner-QA findings, all in the checker and the staff lists.
## Queue navigation did not load the next item's files
Previous Item / Next Item updated the checker, but the item's files and
3D preview did not reliably load. A hard refresh fixed it.
Root cause was a contradiction between two designs. `ObjectViewer` creates
exactly one X_ITE browser for its lifetime and swaps the Inline's url as
`objectUrl` changes, documented as deliberate because repeated
create/dispose cycles leave later browsers unable to load a world at all.
But the checker rendered it inside `v-else-if="inspection"` and cleared
`inspection` on every route change -- so each Previous/Next destroyed the
viewer and built a new one, which is exactly the cycle the component was
written to avoid. Its own url-swap path was unreachable in practice.
The url handed to the viewer now lives in `viewerUrl`, which survives
navigation, so the viewer stays mounted and is re-pointed once the next
inspection resolves. `inspection` is still cleared the instant the route
changes, so no stale record and no Accept/Reject/Edit control belonging to
the object just left is on screen or actionable while the next one loads.
A failed load clears the url too, rather than leaving the previous
object's model under an error message as though it were this one.
Proven in a real browser: A -> B -> A -> B twice with no refresh, each
transition reaching ready state with the correct file, one canvas
throughout and no root-node growth.
## The header read as a technical strip
Polish only, not another redesign. The object's name is now what the eye
lands on; its id and status sit under it with the review state in words
("Awaiting Mall review"). The facts are labelled rather than run together
with middots, and say what CTR means rather than what it stores: a null
limit is "Unlimited", and a pending object's absent store is "Not assigned
yet" rather than "no store". A one-line prompt says what to do next.
The queue is a separate block: position, the way out, then the way
through. Its buttons are 40px tall -- explicit px, because this app sets a
13px root and a rem-based target silently came out at 29px. Below 900px
the block moves under the identity instead of squeezing into a column too
narrow to read. Verified at 1024x768 and 768x1024 with no overflow.
## Staff list URLs were noisy
Only what differs from a list's defaults is written, so the Warehouse is
`#/mall/warehouse` rather than `#/mall/warehouse?page=1&limit=10&order=ASC`.
Non-default page, size and sort are still carried, and every existing
explicit URL still restores.
This also fixes a latent mismatch found while reading it: the checker's
queue resolved its page size and sort with hardcoded fallbacks, so a
canonical Stocked URL -- which omits its DESC default -- would have walked
the queue in the opposite order from the list it was opened from. It now
resolves against the originating list's own defaults.
They were the only controls in the staff row using a different button shape from the ones beside them. They now match Edit Name and Update Limit exactly -- same border, radius, padding and height -- and carry the only colour in that row: green for Accept, red for Reject. These two are the irreversible decisions, so being able to tell them apart without reading is the point. Everything around them keeps the site's ordinary button language, and both fade while a request is in flight so a disabled control does not still read as an armed one.
A reachable database is no longer enough to run the MallRepository real-database spec. Its fixture INSERTs and cleanup DELETEs now require CTR_INTEGRATION_TEST_DB to name the configured database exactly -- an explicit assertion that the database is disposable and dedicated to integration testing -- and the fixture rows use database-minted ids instead of predictable constants, so cleanup can only ever delete rows the run itself inserted. Without the opt-in the suite skips visibly, and a guard test proves no connection is even opened.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Staff-facing tooling for the Mall moderation workflow, inside the existing Cybertown shell.
Summary
MALL CHECKcontrol on the Mall's control panel, the checker rendered in the site's normal content region, and staff navigation in the historical right-hand panel. No second, detached admin shell.docs/mall-export-schema.md.No schema migration.
Pending export
schemaVersionis 2.0.0. The document is the submission queue the Mall Checker publishes — not a complete-Mall catalogue. Object scope is Pending only (CTRstatus = 2).WHERE id IN (...)against that fixed list — there is no liveLIMIT/OFFSETpaging — so staff approving or rejecting an object mid-export cannot alter the identity set the export committed to.status,statusName,quantity,limitandctrViewson each entry come from the same preflight rows the document's top-levelctrViewsand counts were built from, so no entry can contradict the document's pending-only scope.status: "truncated"(snapshot_rows_missing) — never a falsecompletecarrying the preflight count.derived=0andderived=1enrich the same object identity set. The mode changes how much is said about each object, never which objects appear.storesis reference data only — the full store list, so a consumer can render a store name it may meet later; its objects are not implied to be present.preflight()runs — and the budget is enforced per row, not per 200-row page.Moderation correctness
FOR UPDATEand its status re-checked inside the transaction.balance = balance + ?), so concurrent rejections of two different objects owned by one uploader cannot lose a refund.notified: falseand a named warning to follow up by hand, rather than a 500 that invites a duplicate moderation retry.alreadyAccepted/alreadyRejectedare reported as distinct outcomes, not as notification failures.Upload / source safety
objectrow insert, are all awaited; a failure cleans up the partial upload directory rather than leaving an orphaned row or files./and\), with the resolved destination verified to stay inside the upload directory. Legitimate filenames are preserved byte-for-byte, since a WRL references its texture by exact name.maxOutputLengthplus a belt-and-braces length check, on the async decompression path.<a>navigation.Error.messageand no filesystem paths in a document that gets passed around.QA / validation
member.service.spec.ts— those tests open a real connection to a MySQL on the default port and failECONNREFUSED 127.0.0.1:3306without one. The 3 other failing suites (role.repository.spec.ts,club.service.spec.ts,wallet.service.spec.ts) are pre-existing empty suites. Every Mall, VRML, export, object-source, controller, real-MySQL atomicity and upload suite passes. The 1 skipped test is a guard proof that runs precisely when the real-database opt-in is absent.tsc --noEmitclean.Known follow-ups (pre-existing, out of scope here)
getAccessLevel()returns a (possibly empty) array and[]is truthy, so several admin endpoints admit any authenticated member regardless of role, andplacesUpdatehas the inverse form. This PR touched only typing/lint in that file and deliberately leaves those authorization semantics unchanged; they need a dedicated security pass with endpoint-specific role expectations and regression tests.node, no runner dependency) and not yet CI-wired.limit = 0the same as unlimited; the export reproduces the page exactly rather than silently correcting it.None of these were introduced by this feature.
Review history
Detailed review and QA history: DJAscendance#13.