refactor(characters): consolidate downloadPdf onto DocuDeskPdfRenderer, closing gate-49 - #267
Merged
Merged
Conversation
…r, closing gate-49 CharactersController failed hydra gate-49 (controller-exception-translation) on both downloadPdf() and requirementReport(): each calls RegisterObjectFetcher::getObject() but caught only \Exception, so an OpenRegister DoesNotExistException was never translated explicitly. The obvious fix -- importing DoesNotExistException -- is a net regression on its own. Measured on PHP 8.4.22 / PHPMD 2.15.0: import alone: CouplingBetweenObjects 12 -> 13, shipped findings 0 -> 1 DocuDeskPdfRenderer already existed and its docblock already claimed both the character-sheet and the event run-sheet export as callers -- but downloadPdf() never actually used it and re-implemented the whole pipeline inline. Routing downloadPdf() through the renderer (as EventsController::downloadRunsheet already does) drops IAppManager, ContainerInterface and LoggerInterface from the controller, which buys back the budget the exception import needs. before: CBO 12, shipped PHPMD findings 0, gate-49 FAIL (2 methods) after: CBO 11, shipped PHPMD findings 0, gate-49 PASS No behaviour change. The existing downloadPdf tests were kept driving a REAL DocuDeskPdfRenderer built from the same IAppManager/Container/Logger mocks the controller used to hold, so they still exercise the identical end-to-end pipeline and act as a behaviour-preservation check rather than an assertion about the new internal shape. Tests: 153/541 -> 173/577, all passing. Adds the first tests for DocuDeskPdfRenderer, which had none despite already serving EventsController, and pins the details this refactor could have silently changed: the template body, render context and page options handed to DocuDesk, the derived download filename, and template-id lower-casing. Fail-proof (mutation testing, each reverted): isDocuDeskAvailable -> true kills 3 tests (incl. controller 424) normaliseTemplateId -> passthrough kills 8 tests (incl. controller 400) render error -> '' instead of null kills 2 tests (incl. controller 500) No suppressions, baselines, thresholds or waivers were added or changed.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ❌ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ❌ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-05 18:04 UTC
Download the full PDF report from the workflow artifacts.
… this change touches gate-28 (license-triangle) is diff-scoped, so editing CharactersController pulled a pre-existing header drift into scope: the file declared AGPL-3.0 while composer.json declares EUPL-1.2. Fixed properly rather than waived, on the two files this change touches plus the new test. DashboardController, DeepLinkRegistrationListener and LarpingAppAdmin carry the same pre-existing drift but are outside this diff and are already covered by chore/eupl-license-normalisation-2026-08-05; left alone to avoid conflicting with that branch.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ❌ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ❌ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-05 18:47 UTC
Download the full PDF report from the workflow artifacts.
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
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.
What
CharactersControllerfailed hydra gate-49 (controller-exception-translation). This closes it by first removing responsibility from the controller, then adding the exception import the gate wants.Why the obvious fix was rejected
Adding
use OCP\AppFramework\Db\DoesNotExistException;on its own is a net regression, and an FQCN is no escape becauseMissingImportis enabled in this repo's ruleset. Measured, not assumed (PHP 8.4.22 / PHPMD 2.15.0):developmentThe actual fix
DocuDeskPdfRendereralready existed, and its own docblock already claimed both the character-sheet and the event run-sheet export as callers — butdownloadPdf()never used it and re-implemented the entire pipeline inline (dependency check, UUID validation, template lookup, render, error logging).Routing
downloadPdf()through the renderer — exactly asEventsController::downloadRunsheet()already does — dropsIAppManager,ContainerInterfaceandLoggerInterfacefrom the controller. That buys back the coupling budget the exception import needs.Before / after
CouplingBetweenObjectsBoth
downloadPdf()andrequirementReport()were flagged; both are fixed. Of the 63 gate verdicts, exactly one changed (gate-49 FAIL→PASS); the other 5 failures are pre-existing and untouched.Behaviour preservation
The existing
downloadPdftests were kept driving a realDocuDeskPdfRendererbuilt from the sameIAppManager/ContainerInterface/Loggermocks the controller used to hold — not a mock of the renderer. They therefore still exercise the identical end-to-end pipeline and act as a behaviour-preservation check rather than an assertion about the new internal shape.Adds the first tests for
DocuDeskPdfRenderer, which had none despite already servingEventsController, and pins the details this refactor could have silently changed: the template body, render context and page options handed to DocuDesk, the derived download filename, and template-id lower-casing.Fail-proof
A passing test is evidence about the test until it has been shown it can fail. Each mutation applied to the renderer, then reverted:
isDocuDeskAvailable()→truenormaliseTemplateId()→ passthrough''instead ofnullThe PHPMD rig itself was positive-controlled before the first measurement with a deliberate 3-violation probe, which it reported exactly.
Reported honestly: the two new
DoesNotExistException→ 404 tests pass with or without the specific catch clause, because the pre-existingcatch (\Exception)already caught it. They pin the 404 translation behaviour but do not discriminate the gate-49 fix — gate-49 itself is the verifier for that.Not done
psalmcannot run locally —psalm.xmlreferences a stub atvendor/nextcloud/ocp/OCP.bak/…that does not exist after a freshcomposer install. Verified pre-existing: it fails identically on the pristine tree. Not touched, because editing the stub config to make the check pass would weaken it.No
@SuppressWarnings, baseline, threshold change,.skip, waiver,continue-on-erroror weakened assertion was added. larpingapp still has nophpmd.baseline.xml.