Skip to content

Enhancement: Qualification Tests#487

Open
baltinerdist wants to merge 13 commits into
amtgard:masterfrom
baltinerdist:feature/qualification-tests
Open

Enhancement: Qualification Tests#487
baltinerdist wants to merge 13 commits into
amtgard:masterfrom
baltinerdist:feature/qualification-tests

Conversation

@baltinerdist

Copy link
Copy Markdown
Contributor

Summary

Full Qualification Tests module for the ORK3 system — kingdom-configurable reeve and corpora testing with question management, test-taking, grading, and reporting.

Core Module (previous commits)

  • Question CRUD with multiple-choice answers and correct answer tracking
  • Per-question statistics (times served, times correct, success rate)
  • Test configuration: question count, passing threshold, retake cooldown
  • Test-taking flow with randomized questions, immediate grading, pass/fail
  • Retake enforcement with configurable cooldown periods
  • Per-kingdom config toggles (QualTestReeveEnabled / QualTestCorporaEnabled)
  • Manager roles: kingdom-level test managers can edit questions and view reports
  • Player-facing test menu with status indicators and retake availability
  • Test results reports for reeve and corpora (kingdom Reports tab)
  • Instructions page with configurable kingdom-specific instructions text

Test Building Tools & UX

  • Bulk import: Paste-based import of questions and answers in structured text format
  • Question duplication: Clone a question with all answers; redirects to edit screen
  • Bulk actions: Multi-select questions with checkbox for batch archive/restore
  • Test preview: Generate a sample test to review question pool quality
  • Quick Answers: True/False and Yes/No pill buttons on question create form
  • Collapsible archived section: Archived questions panel collapsed by default, with deferred DataTable initialization
  • Instant tooltips: CSS pseudo-element tooltips on icon-only action buttons (no browser delay)
  • Config toggle buttons: Yes/No toggle buttons (green/red) for QualTest config on Kingdom admin modal
  • Take page fix: Fixed PHP syntax error (brace vs alternative if/endif mismatch)
  • Unified report template: Single template for both reeve and corpora test results

Confetti Celebration

  • Adds [email protected] (jsDelivr CDN) to both pass flows — standalone take page and profile-embedded modal
  • 2-second side-cannon effect fires when a player passes; zIndex 2000 keeps the canvas above the modal overlay

Rules of Play Version, Correct-Answer Toggle, Dark Mode & Manage Redesign

  • Rules version: New RulesVersion (Reeve, required) shown as a "Based on Amtgard Rules of Play Version {value}" footer on test cards
  • Show-correct-on-incorrect: New ShowCorrectOnIncorrect kingdom toggle gates whether the correct answer is highlighted green when a player picks wrong; defaults off for existing kingdoms
  • Relocation: Test management moves out of its own Kingdom tab into Admin Tasks → Tests; the player profile Qualifications card gets a Take Tests button opening a chooser modal with side-by-side Reeve / Corpora panels (status, last score, retakes used, expiry)
  • Comprehensive dark-mode pass on every qual-test surface: take page, manage admin, question editor, questions list, chooser modal, embedded quiz modal
  • Manage page redesign: fields grouped into Scoring / Player Experience / Reeve Only sections with thin dividers; validity becomes a segmented two-button toggle; behavioural yes/no toggles render as CSS switches; Save Settings pushed to card bottom so both cards line up; navigation row demoted outside the form

Polish Pass — Round 1

Multi-lens review (bug/stability/perf/security/quality/conventions) with implementer + QA agents. All findings either fixed or deferred with rationale.

Security / correctness

  • submittest now enforces the retake limit before scoring — closes a direct-POST bypass that let an at-limit player record a pass (mirrors the gettest gate exactly)
  • resetstats verifies question ownership before clearing stats (cross-kingdom IDOR guard)
  • saveQuestion answer replacement wrapped in an all-or-nothing transaction (was a silent partial-save risk)
  • getCorrectAnswers made deterministic (lowest answer id) so duplicate correct rows can't cause scan-order-dependent scoring

Performance

  • recordQuestionStats → single batch upsert (was N serial round-trips on the submit hot path)
  • getLibraryQuestions → in-PHP dedup replaces a correlated TEXT-equality subquery, plus a result cap
  • saveQuestionBatch → 200-question cap to avoid execution-time blowups
  • New idx_kingdom_type_expires index on ork_qual_result for report queries
  • getPlayerResults expiry computed in SQL (expires_at <= NOW()) — removes a PHP-vs-DB timezone skew

Maintainability

  • getQuestionsForTest/getQuestionsForPreview deduped behind a shared _loadQuestionsAndAnswers() helper (player path still never exposes correct answers)
  • reeve_test_results/corpora_test_results collapsed into one shared helper
  • Question author (created_by) now captured on save

UX / house rules

  • Replaced all native confirm()/alert() with pnConfirm (revised-frontend) and per-page in-product modals (default-template pages)
  • Native title= tooltips → data-tip; tooltip wrap/anchor fixes; kn-toggle admin control given a dark-mode override

Deferred (documented): session-token binding of the submitted question set, an ork_mundane expiry sweep, and a file-wide title=data-tip conversion on Playernew edit buttons — each out of scope for a polish pass.

Polish Pass — Round 2 · hardening (latest commit 3646c74a)

A second multi-lens panel (6 lenses) → implementer agents → 2 independent QA critics, every change php -l-clean. Findings split into auto-applied trivial fixes and critic-gated complex fixes.

Security

  • 🔴 Cross-kingdom answer-key leak closedgetLibraryQuestions previously shipped is_correct to any opted-in kingdom admin browsing the shared library (the answer key for every other kingdom's questions). It no longer selects/returns is_correct, and the library renderer no longer marks correct answers.
  • IDORresetplayerretakes now verifies the target player belongs to the acting kingdom before resetting.

Correctness / concurrency

  • saveQuestion: the new-question INSERT now lives inside its transaction (a mid-write fatal/rollback no longer orphans an answerless question); answers written as a single atomic multi-row INSERT, which also closes a latent blank-correct-answer edge.
  • copyQuestionToKingdom + duplicateQuestion wrapped in transactions + batched inserts (no answerless orphans on partial failure).
  • _loadQuestionsAndAnswers prunes any question lacking ≥2 answers or a correct option, so a corrupt question can't soft-lock a live quiz; the player path still never exposes the correct flag.
  • recordResult → atomic INSERT … ON DUPLICATE KEY UPDATE with SQL-computed expiry (DATE_ADD(NOW(), …)), removing both the SELECT-then-write race and PHP-vs-DB clock skew on the stored value.
  • New atomic tryConsumeRetake(); submittest consumes a retake atomically, after question-set validation — no TOCTOU on the cap, and a stale/invalid submission never burns a legitimate retake.

Performance

  • getCorrectAnswers correlated subquery → single GROUP BY aggregation (runs on every submission).
  • saveConfig → UPSERT; getTestResults bounded with LIMIT 2000; player profile skips all qual queries when neither test type is enabled.

Quality / conventions

  • Removed dead esc()/$ok guards; deduped escH (3→1); validity now shows ValidUntil correctly; corrected syncMundaneQual docblock; _test_results sets an error on a null kingdom; ScorePercent escaped in data-order; expired-row dark-mode override.

Product decision (won't-fix): the checkanswer endpoint exposing whether an answer is correct is acceptable by design — qualification tests are open-book with retests allowed, so the answer-probing vector is not worth hardening. This supersedes Round 1's deferred "session-token binding" item.

Deferred as tech-debt (noted, not silently dropped): getAllQuestions correlated-subquery rewrite and getTestReportStats query consolidation (riskiest perf rewrites on infrequent admin/report surfaces, not fully verifiable against the behind-schema local DB); saveConfig 11-param → options-array refactor; template dedup (qtConfirm/.qt-nav-link/[data-tip]) and QualTest_take.tpl monolith split.

Database

  • 8 tables: ork_qual_question, ork_qual_answer, ork_qual_config, ork_qual_result, ork_qual_manager, ork_qual_question_stat, ork_qual_retake, ork_qual_report
  • Migrations in db-migrations/ (incl. 2026-06-12-add-qual-result-index.sql and add-qual-config-rules-version-and-feedback.sql)
  • Note: 2026-03-17-add-qual-library.sql now anchors its column AFTER valid_until so a fresh, alphabetically-ordered migration apply no longer fails on a not-yet-created column

Test Plan

  • Verify kingdom admin can enable/disable reeve and corpora tests via toggle buttons (light + dark mode)
  • Confirm test management now lives under Admin Tasks → Tests, and the player profile Qualifications card has a working Take Tests chooser modal
  • Set a Rules of Play version and confirm the footer renders on test cards
  • Toggle Show-correct-on-incorrect and confirm the green highlight only appears when enabled
  • Create questions with Quick Answers pills (True/False, Yes/No)
  • Duplicate a question and confirm redirect to edit screen
  • Bulk import questions via paste format; confirm >200 is rejected cleanly
  • Use bulk select to archive/restore multiple questions
  • Preview a test to verify randomized question selection
  • Take a test and verify grading, pass/fail, result recording, and confetti on pass
  • Retake gate: exhaust retakes, then confirm a direct submittest POST is blocked
  • Confirm all destructive actions (reset retakes, archive, clear flags, submit test) show an in-product modal — no native browser dialog
  • Walk every qual-test surface in dark mode (take page, manage, editor, list, chooser, embedded modal)
  • Check test results reports under Kingdom > Reports tab
  • Run DB migrations on a fresh database in alphabetical order

🤖 Generated with Claude Code

Follow-up refinements (latest commit 1352e491)

  • Corpora "Based on" attribution: the version-footer concept now covers the Corpora test. Reeve keeps the fixed "Based on Amtgard Rules of Play Version {n}"; Corpora shows a free-text "Based on {value}" so a kingdom names its own document(s) (helper/placeholder example "Corpora 5.1 2026"). Optional for Corpora, still required for Reeve. saveConfig no longer reeve-gates rules_version (same per-test_type column — no migration).
  • Sharing opt-in defaults to yes: getConfig returns ShareQuestions=1 for an unconfigured Reeve config (checkbox pre-checked); corpora stays 0. Only affects kingdoms that have not yet saved a config.
  • Shared-library ordering: library questions now sort by player-report count ascending, so the most-reported ("offending") questions sink to the bottom (and are the first dropped past the 500-row cap). Index-backed via qual_report.idx_question.
  • Library flagged indicator: questions with ≥2 reports show a red flag badge with the count (data-tip tooltip), dark-mode aware.
  • Validity "Until date" warning: an amber infobox recommends 1–2 weeks of buffer for officer changeover, wired to the mode toggle, dark-mode aware.

Avery Krouse and others added 13 commits July 1, 2026 09:26
- New QualTest controller, AJAX controller, and class with full test management
- Reeve and corpora test types with configurable question pools
- Valid For (days) / Valid Until (date) expiry toggle per config
- Max retakes limit with per-player tracking and admin reset
- Question feedback/reporting system with admin flag view and clear/archive actions
- Test Managers widget: kingdom-scoped player autocomplete for non-officer managers
- Global Question Library: opt-in sharing of reeve questions across kingdoms with copy-from-library (deduped by question text)
- Tests tab on Kingdom profile gated behind canManage auth
- Player profile integration: test cards, retake warnings, admin reset buttons
- DB migrations for all new tables/columns

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ts, instructions

- Add QualTestReeveEnabled / QualTestCorporaEnabled kingdom config toggles
  (default disabled, admin sets to 1 to enable) with migration for existing kingdoms
- Gate test-taking (gettest/submittest) on enabled check in QualTestAjax
- Conditionally show/hide Tests tab and individual test cards on kingdom
  and player profiles based on enabled flags; managers see disabled cards
  with badge
- Add Reeve's Test Results and Corpora Test Results reports under kingdom
  Reports > Other with DataTables, stat cards (qualified %, pass rate,
  active questions, flagged questions), and pass/fail badges
- Add test instructions field to qual_config with textarea in manage UI,
  instructions card shown before test begins on player profile
- Fix JS breakage in Reeve report from unescaped apostrophe in filename

Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ts, take page

- Add bulk import, question duplication, bulk status actions, test preview
- Duplicate redirects to edit screen for immediate changes
- Quick Answers pills (True/False, Yes/No) on create question form
- Collapsible archived questions panel (default collapsed, deferred DataTable init)
- CSS instant tooltips on icon-only action buttons
- Yes/No toggle buttons for QualTest config on Kingdom admin modal
- Fix QualTest/take PHP syntax error (brace vs alternative syntax mismatch)
- LAST_INSERT_ID() fixes replacing race-condition-prone patterns
- Move batch status SQL to class.QualTest.php (setQuestionStatusBatch)
- Unified test results report template for both reeve and corpora

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Adds [email protected] (via jsdelivr CDN) to both qualification test
flows — standalone take page and profile-embedded modal — firing a
2-second side-cannon effect when a player passes. zIndex 2000 keeps the
canvas above the modal overlay (z-index 1100).

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Reverse-engineered PRD covering data model (8 new tables + 2 kingdom
configs + ALTER columns), service/controller/template code map,
manager and player workflows, and load-bearing invariants (mundane
sync, valid_until precedence, layered manager auth).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…wer toggle, dark mode, manage redesign

- Adds RulesVersion (Reeve, required) shown as "Based on Amtgard Rules of Play Version {value}" footer on test cards.
- Adds ShowCorrectOnIncorrect kingdom toggle gating whether the correct answer is highlighted in green when a player picks wrong; defaults off for existing kingdoms.
- Moves test management out of its own Kingdom tab into Admin Tasks → Tests sub-section; player profile gets a "Take Tests" button on the Qualifications card that opens a chooser modal with side-by-side Reeve / Corpora panels (status, last score, retakes used, expiry).
- Comprehensive dark-mode pass on every qual-test surface: take page, manage admin, question editor, questions list, chooser modal, embedded quiz modal.
- Manage page UI/UX redesign: fields grouped into Scoring / Player Experience / Reeve Only sections with thin dividers, validity becomes a segmented two-button toggle, behavioural yes/no toggles render as CSS switches, Save Settings pushed to card bottom so both cards line up, navigation row demoted outside the form.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…tness, UX)

Multi-lens review + fixes across the Qualification Tests module:

Security/correctness:
- submittest now enforces the retake limit before scoring (closes direct-POST
  bypass; mirrors gettest gate)
- resetstats verifies question ownership (IDOR guard)
- saveQuestion answer replacement wrapped in an all-or-nothing transaction
- getCorrectAnswers made deterministic (MIN answer id)

Performance:
- recordQuestionStats single batch upsert (was N round-trips on submit)
- getLibraryQuestions in-PHP dedup replaces correlated TEXT subquery (+cap)
- saveQuestionBatch 200-question cap; new idx_kingdom_type_expires index
- getPlayerResults expiry computed in SQL (removes PHP-timezone skew)

Maintainability:
- getQuestionsForTest/Preview deduped via _loadQuestionsAndAnswers helper
- reeve_test_results/corpora_test_results collapsed into shared helper
- question author (created_by) now captured

UX / house rules:
- replaced all native confirm()/alert() with pnConfirm / per-page in-product
  modals; native title= -> data-tip; tooltip wrap/anchor; kn-toggle dark mode

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Per the project architecture rule (all DB work in system/lib/ork3/),
relocated 6 raw $DB->DataSet name-lookups from the controller layer into
class.QualTest.php:

- Add getKingdomName($id) and getMundaneName($id) lib helpers
- controller.QualTest.php: 4 copy-pasted kingdom-name lookups now route
  through getKingdomName()
- controller.QualTestAjax.php: addmanager() persona check uses
  getMundaneName() + ===null
- controller.Reports.php: qual report header uses getKingdomName()
- Remove now-orphaned `global $DB;` declarations

No functional change; all IDs were already int-cast (no injection risk).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…, concurrency, perf)

Multi-lens polish of the qualification-tests module:

Security
- Close cross-kingdom answer-key leak: getLibraryQuestions no longer
  selects/returns is_correct, and the library renderer no longer marks
  correct answers.
- resetplayerretakes now verifies the target player belongs to the kingdom (IDOR).

Correctness / concurrency
- saveQuestion: new-question INSERT now inside its transaction; answers written
  as a single atomic multi-row INSERT (also guards a blank correct-answer edge).
- copyQuestionToKingdom + duplicateQuestion wrapped in transactions + batched inserts.
- _loadQuestionsAndAnswers prunes answerless/keyless questions so a broken
  question can't soft-lock a live quiz.
- recordResult -> atomic UPSERT with SQL-computed expiry (no PHP/MySQL clock skew).
- New atomic tryConsumeRetake(); submittest consumes a retake atomically after
  validation (no TOCTOU, no slot burned on a stale/invalid set).

Performance
- getCorrectAnswers correlated subquery -> GROUP BY; saveConfig -> UPSERT;
  getTestResults bounded with LIMIT; player profile skips qual queries when off.

Quality
- Remove dead esc()/$ok guards; dedupe escH; show ValidUntil correctly;
  correct syncMundaneQual docblock; _test_results null guard; escape ScorePercent;
  expired-row dark-mode override.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…lt, library polish

Follow-up refinements to the Qualification Tests module:

- Corpora "Based on" attribution: extend the version-footer concept to the
  Corpora test. Reeve keeps the fixed "Based on Amtgard Rules of Play Version
  {n}"; Corpora shows a free-text "Based on {value}" so kingdoms can name their
  own document(s) (helper/placeholder example "Corpora 5.1 2026"). Optional for
  Corpora, still required for Reeve. saveConfig no longer reeve-gates
  rules_version (same per-test_type column, no migration).

- Sharing opt-in defaults to yes: getConfig now returns ShareQuestions=1 for an
  unconfigured Reeve config (checkbox pre-checked); corpora stays 0. Only
  affects kingdoms that have not yet saved a config.

- Shared library ordering: sort library questions by player-report count
  ascending so the most-reported ("offending") questions sink to the bottom
  (and are first dropped past the 500-row cap). Index-backed via
  qual_report.idx_question.

- Library flagged indicator: questions with >= 2 reports show a red flag badge
  with the count (data-tip tooltip), dark-mode aware.

- Validity "Until date": show an amber warning infobox recommending 1-2 weeks of
  buffer for officer changeover, wired to the mode toggle, dark-mode aware.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
1) Bulk Import > Parse & Preview did nothing — the click handler was bound
   but threw ReferenceError inside because escH() is only defined inside
   the Test Preview IIFE, not the bulk-import IIFE. Define a local escH in
   the bulk-import module so the preview renders.

2) Corpora / Reeve's Test Results reports never appeared on the Kingdom
   Reports > Other group, even after enabling the toggle. Root cause: the
   qual-tests reads at controller.Kingdom::profile() were sitting above
   the $knConfigs assignment (the AwardRecsPublic block owns it on master,
   and the merge left the qual reads reaching for an undefined variable
   that always resolved false via isset()). Hoist the get_configs() call
   above both consumers, drop the duplicate below.

3) Add Staff Member picker rendered the same player twice when the query
   used an abbreviation prefix like "nb:ff alt". The staff-modal fallback
   fires a second scope=exclude fetch when scope=own returns <5 rows, but
   the backend's prefix parser overrides scope entirely — both fetches
   return the same rows. Dedup by MundaneId on the concat, matching the
   pattern used elsewhere in revised.js.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Qualification tests could previously only ask single-correct questions —
one right answer, one radio button, one equality check to score. This
adds a per-question mode toggle so an admin can flip a question to
"multiple correct" and require the player to pick every right answer.

Schema (new migration 2026-07-06-add-qual-question-answer-mode.sql):
 - ork_qual_question gets an answer_mode ENUM('single','multi') column,
   default 'single'. Existing questions keep single-correct semantics
   without any data migration.

Scoring — all-or-nothing:
 - getCorrectAnswers() now returns
     [qid => ['Mode' => 'single'|'multi', 'AnswerIds' => [int,...]]]
 - scoreTest() delegates to a shared _scoreOne() predicate, used by
   recordQuestionStats() too so per-question stats never disagree with
   the aggregate. Multi requires the submitted set to exactly equal the
   correct set (no missing, no extra).

AJAX shape:
 - checkanswer + submittest accept AnswerIds[] (array) as well as
   the legacy scalar AnswerId. Server sanitizes to int arrays.
 - checkanswer returns correct_answer_ids[] (full set) alongside the
   back-compat scalar correct_answer_id.

Question editor:
 - New Answer Mode toggle (Single / Multi). Flipping swaps the correct
   inputs between radio and checkbox in place, preserving picks.
 - Submit validator: multi needs >=2 correct; single rejects >1.
 - Question row in the list gets a small "multi" badge.

Bulk import:
 - Parser auto-detects multi from >=2 *-prefixed answers. Existing
   "exactly 1 correct" rule relaxed to ">=1 correct" so 2+ * marks
   the question as multi automatically.
 - Preview UI shows a "select all that apply" hint for multi rows.
 - Backend saveQuestionBatch forwards AnswerMode through to saveQuestion.

Take page:
 - Multi questions render checkboxes + a "Submit Answer" button and a
   "Select all that apply, then submit." hint. Selection tracks a local
   set; submit routes through the same checkAnswer() the single flow
   uses so scoring and feedback stay identical.
 - Wrong-set feedback: highlights each PICKED answer as right (green)
   or wrong (red), and each MISSED correct answer also as right, so
   the player can see exactly what they should have selected (when
   the kingdom has ShowCorrectOnIncorrect enabled).

Verified end-to-end on dev: saveQuestion + getQuestion round-trip;
getCorrectAnswers shape; scoreTest all-or-nothing across three
scenarios (both correct → 1/1, one of two → 0/1, one correct plus one
wrong → 0/1).

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants