Resolve all 152 fail-classified HTML Link Audit ledger entries (#131) - #154
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: be-framework/BeMart/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (89)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
3e7ccd3 to
693bf65
Compare
1d5de14 to
916b67f
Compare
|
@coderabbitai review |
|
Review (CodeRabbit rate-limited — manual review)Reviewed all three commits shown in this diff (base Acceptance criteria (issue #131)
Other findings (minor, non-blocking)
CSRF / attribution noteFor the record since the diff mixes commits: the Summary: the ledger-emptying mechanics are solid and independently verified (0 fail, test-run confirmed), and the flagship |
693bf65 to
cc8ebea
Compare
…own diff introduced Independent review of #154 found two NEW instances of the exact duplicate-rel defect class this PR exists to resolve (the same pattern as the goMypageHistory/goOrderHistory case this commit's own message already disambiguates via ALPS), both introduced by this PR adding rel attributes to anchors/forms that previously had none: - admin-base.html.twig's template-add nav anchor: stacked rel="goTemplateAdd goTemplateInstall". alps.json has no goTemplateAdd descriptor at all; only goTemplateInstall (rt: #TemplateInstall) is ALPS-backed, and TemplateAdd::onGet() itself carries #[Alps('goTemplateInstall')], not goTemplateAdd. The #[Link(rel: 'goTemplateAdd', ...)] on TemplateList::onGet() predates this PR and was a pre-existing orphaned duplicate this PR's own template change was the first to actually render. Removed the orphaned #[Link] and the stale rel token; renamed the one existing test that named-but-didn't-actually-assert the rel (it only checked the href, which is unaffected). - Admin/Order/ShippingAddress.html.twig's PUT form: stacked rel="doUpdateOrderShippingAddress doUpdateShippingAddress". ALPS defines these as distinct rt states - doUpdateOrderShippingAddress is rt:#Order (admin order-fulfillment, this page), doUpdateShippingAddress is rt:#Shopping (customer checkout, a completely different page/actor). The #[Link(rel: 'doUpdateShippingAddress', ...)] on ShippingAddress.php's onGet/onPost predates this PR; removed both occurrences, kept only the admin-scoped rel. Also (minor, from the same review): added an in-template comment on Admin/Index.html.twig and Admin/System.html.twig's per-page logout forms explaining why they're not a shared admin-base.html.twig topbar form (a dozen other admin pages don't publish csrfToken, so a shared form would render an empty token there and fail CSRF in production) - the rationale existed only in this commit's own message, not in the template a future editor would read.
916b67f to
cd08a62
Compare
|
@coderabbitai review |
|
cc8ebea to
71ae2e2
Compare
BlockList and PaymentList declared doUpdate*/doDelete* on their own onGet -
transitions ALPS assigns to the single-row Block/Payment states, not the
list. Both resources' own docblocks already said as much ("Single-row
affordances ... live at page://self/admin/payment/payment"). The surplus
declarations advertised a Link: header target that can't actually be
followed as declared either - a bare href with no bound row identifier,
since the real per-row delete/update forms are rendered inline in the list
HTML, unrelated to this resource-level attribute.
Per-resource resolution (the two options the issue posed have different
right answers depending on whether ALPS's assigned target resource is
GET-reachable at all):
- Block/Payment: a real single-row GET view exists (Block::onGet,
Payment::onGet), so moved the declaration there - matching ALPS exactly.
Payment::onGet already declared doUpdatePayment; Block::onGet was missing
doUpdateBlock entirely (an oversight, not by design) and Payment::onGet
was missing doDeletePayment the same way - filled both in, or removing
the list's declaration would have made the transition undiscoverable via
hypermedia anywhere, not just corrected.
- TaxRuleList: alps.json's TaxRule state lists doDeleteTaxRule, but there is
no goTaxRule transition and no onGet on TaxRule.php - "intentionally no
onPut... edits flow as delete + create" per its own docblock. There is no
GET-reachable resource to move the declaration to without inventing one,
so this is the one case where the resource is right and ALPS was
incomplete: connected #doDeleteTaxRule into TaxRuleList's descriptor set
instead. Regenerated alps.json.html/alps.svg and synced the docs/ copies
per AGENTS.md.
Updated the two Hypermedia workflow tests that exercised the old (wrong)
navigation - extracting doUpdateBlock/doUpdatePayment/doDeletePayment from
the list response, the only place they used to be declared. Now:
Block: testUpdatesBlock's own response already carries doUpdateBlock (no
extra hop needed); testDeletesBlock re-fetches the single-row Block GET
(Block::onPut doesn't redirect, so there's no Location to follow - a real
client returning to a bookmarked item page would do the same).
Payment: both testUpdatesPayment and testDeletesPayment now follow the
Location header (Payment's create/update both redirect to the single-row
GET) instead of detouring through the list.
Adding doUpdateBlock/doDeletePayment to the single-row GETs surfaced two new
html-link-audit-ledger entries, symmetric to the doDeleteBlock/doUpdatePayment
entries already there: the "new blank form" mode (blockId/paymentId absent)
doesn't render an update/delete form, so the declared link has no matching
affordance (target-missing, resourceOnly - same classification as their
siblings). Also removed two ledger entries that are no longer observed now
that the surplus links are gone from the list resources
(block-list doUpdateBlock, payment-list doUpdatePayment method-mismatch).
Verified: full suite green (2806 tests, 33061 assertions), psalm clean,
asd --validate alps.json clean, HtmlLinkAuditLedgerTest and both Hypermedia
workflow test files pass, all four affected HTML render tests
(AdminBlockHtmlRenderTest, AdminBlockListHtmlRenderTest,
AdminPaymentListHtmlRenderTest, AdminTaxRuleListHtmlRenderTest) pass
unchanged - the rendered per-row forms were never driven by this PHP
attribute, only the Link: HTTP header and the audit ledger were.
…ke Block/Payment The docblock still said doDeleteTaxRule "lives at" the single-row resource, contradicting #136's actual resolution. TaxRule has no GET view (no onGet, only onDelete), so unlike doUpdateBlock/doDeleteBlock/doUpdatePayment/ doDeletePayment (which moved to their single-row Block/Payment GET views), there is nowhere else to declare doDeleteTaxRule without inventing a goTaxRule state ALPS doesn't have. alps.json's TaxRuleList descriptor connects #doDeleteTaxRule accordingly - the list is the ALPS-correct place for this one, not merely a convenient leftover.
Independent review of #152 found Block::class docblock still said "ALPS has no goBlock" even though alps.json defines goBlock (rt: #Block) and Block::onGet() carries #[Alps('goBlock')] three lines below — this PR already fixed the analogous stale claim in TaxRuleList.php but missed this adjacent one in the same diff.
…kList doesn't link to it Second-pass review of the earlier docblock fix (f22f243) found it swapped one inaccuracy for two others: - goBlock carries tag 'alps-route-gate' and the generic boilerplate doc value ('ユーザーが見る状態または行う操作をALPS上で表す。') - it's a route-existence placeholder, not a fully modelled transition. Saying "onGet is ALPS goBlock" without that context implies more than the descriptor actually models. - BlockList::onGet() declares only #[Link(rel: 'doCreateBlock', ...)] - no goBlock Link at all - and alps.json's #BlockList descriptor never references goBlock either. The claim "the list view links to this state via goBlock" was fabricated.
`tests/Html/html-link-audit-ledger.json` tracked 152 "fail" entries across
three reasons: 134 semantic-token-missing (an `<a>`/`<form>` reaches the
right href+method but the rendered markup carries no `rel`/`class` token
matching the resource's `#[Link]`), 12 method-mismatch, and 6 target-missing.
All 152 are now fixed or reclassified with an evidence-based note; the
ledger holds 52 entries (49 resourceOnly, 3 targetOut), zero fail.
Three subagents fixed 105 of the 134 semantic-token-missing entries in
parallel (one `rel="<name>"` addition per page, verified per-page against
the real audit wiring — never against ledger removal, to avoid concurrent
edits on the shared ledger file). Before dispatching them, 29 more
semantic-token-missing entries were resolved by editing two site-wide
shared partials once each: `admin-base.html.twig`'s sidebar (12 admin
`go*List` links spanning ~20 pages) and `IdeaStore/component/header.html.twig`
(goTop / goCart spanning 9 pages) — these had to be excluded from the
subagents' per-page assignments and fixed centrally, since three writers
adding the same token to the same shared `<a>` would have collided.
The remaining 18 (12 method-mismatch, 6 target-missing) needed individual
judgment, done directly:
- `admin/delivery/delivery-list` / `admin/news/news-list` doDelete*:
the row-level delete affordance was a GET `<a href="...&_method=delete">`
with JS rewriting it into a POST on click — the auditor (correctly)
treats an `<a>` as GET regardless of `_method`, so this never worked
without JS. Converted both to real per-row `<form method="post">` with
a hidden `_method`/csrfToken field, matching the pattern #130 already
established for Block/Payment/TaxRule and `Mypage/AddressList.html.twig`.
`admin/news/news-list`'s resource never published `csrfToken` at all
(wired now, matching #139). `admin/tag/tag-list`'s delete was worse —
an `href="#"` anchor with no working submit path at all even with JS —
replaced with the same real-form pattern.
- `admin/delivery/delivery-list` / `admin/news/news-list` doUpdate*:
a false positive — the list's own `goDelivery`/`goNews` edit-icon link
coincidentally shares the PUT target's path, so the auditor offers it
as a method-mismatch candidate. No PUT affordance belongs on a list
page; reclassified `resourceOnly`, same call #130 made for
`doUpdateBlock`/`doUpdatePayment`.
- `admin/layout/layout` doUpdateLayout / `admin/order/edit` doUpdateOrder:
both forms only add their `_method` override (query-string or hidden
field) when a real id is present; the smoke-test's "new" render has
none. Added `rel=` to the real edit-mode form (verified working with a
real id) and reclassified the "new"-mode ledger entry `resourceOnly`,
matching the established "new mode doesn't render X" precedent used
throughout this ledger.
- `admin/index` / `admin/system` doAdminLogout: neither page had a
logout affordance. Added a real per-page logout form to each (not a
shared-frame one — tried that first, reverted it: 12 *other* admin
pages don't publish csrfToken, so a topbar-wide form would render an
empty token on all of them and fail CSRF in production). Both
resources gained `CsrfTokenInterface` + `csrfToken` in body/schema
(the same gap #139 fixed elsewhere, just missed on these two).
`Index.php` also declared a `goAdminLogout` POST Link to `/admin/login`
that exists nowhere in ALPS or anywhere else in the codebase and
pointed at the wrong verb for a "go" transition — a duplicate of what
`doAdminLogout`'s own `rt: #AdminLogin` already implies; removed.
- `admin/mail-template` goOrderMail: genuinely missing UI. Added a
"受注メール送信" link next to the existing toolbar back-link (both
template branches).
- `mypage` goMypageHistory / goOrderHistory: not the duplicate `#[Link]`
the issue guessed — alps.json's `Mypage` state descriptor lists
`#goMypageHistory` and not `goOrderHistory`, so `goMypageHistory` is
canonical, not a mistake. The real bug was in the template: the
"すべての履歴を見る" link pointed at `/mypage/history` (the per-order
detail page, which requires `orderNo` — wrong target) instead of
`/mypage/order-history` (the list page). Fixed the href and added
`rel="goOrderHistory"` there and `rel="goMypageHistory"` on the
per-order "詳細" links already using the correct
`/mypage/history?orderNo=...` — both reclassified `resourceOnly`
regardless, since the audited fixture customer (Alice) has zero
seeded orders, so the panel containing both links never renders in
that specific scenario; a customer with real history sees them.
- `shopping/shipping-multiple` goShoppingShipping: genuinely missing —
added a "配送先選択に戻る" link back to `/shopping/shipping` next to
the existing "ご注文手続きに戻る" link, in both the item-list and
empty-cart branches.
- `index` goHelpAbout/goHelpAgreement/goHelpPrivacy: the storefront
top page's footer already linked goHelpGuide/goHelpTradeLaw but was
missing three of the five `#[Link]`s `Index.php` declares. Added all
three.
Caught during verification, not part of the ledger but broken by these
changes:
- This environment's compiled Twig cache does not reliably invalidate
on template mtime — several fixes silently appeared to fail until
`var/tmp/<context>/twig` was cleared. Cleared before every full-suite
run in this session; not a code change, noted for whoever hits it next.
- `HttpSqlAdminNewsFormTest` pinned the literal string
`/admin/news/news?newsId=X&_method=delete` in the rendered list page —
the exact query-string pattern the doDeleteNews form fix replaced with
a hidden field. Updated the assertion to check the new form's `action`
attribute instead; the test's actual functional delete call (a direct
HTTP POST with `_method=delete` in the query string, unrelated to the
list page's markup) was untouched and still passes.
Verified: full `vendor/bin/phpunit` suite green (2836 tests, 0
failures/errors; the one pre-existing risky test —
`CsrfTokenRenderedTest::testLedgerEntriesAreWellFormed` — is #139's
closed-ledger side effect, unrelated), `vendor/bin/psalm` clean (no
errors). Every individual template/resource change was also verified
against its own `tests/Resource/*HtmlRenderTest.php` /
`*ResourceTest.php` / `tests/Hypermedia/*Test.php` before moving on.
…own diff introduced Independent review of #154 found two NEW instances of the exact duplicate-rel defect class this PR exists to resolve (the same pattern as the goMypageHistory/goOrderHistory case this commit's own message already disambiguates via ALPS), both introduced by this PR adding rel attributes to anchors/forms that previously had none: - admin-base.html.twig's template-add nav anchor: stacked rel="goTemplateAdd goTemplateInstall". alps.json has no goTemplateAdd descriptor at all; only goTemplateInstall (rt: #TemplateInstall) is ALPS-backed, and TemplateAdd::onGet() itself carries #[Alps('goTemplateInstall')], not goTemplateAdd. The #[Link(rel: 'goTemplateAdd', ...)] on TemplateList::onGet() predates this PR and was a pre-existing orphaned duplicate this PR's own template change was the first to actually render. Removed the orphaned #[Link] and the stale rel token; renamed the one existing test that named-but-didn't-actually-assert the rel (it only checked the href, which is unaffected). - Admin/Order/ShippingAddress.html.twig's PUT form: stacked rel="doUpdateOrderShippingAddress doUpdateShippingAddress". ALPS defines these as distinct rt states - doUpdateOrderShippingAddress is rt:#Order (admin order-fulfillment, this page), doUpdateShippingAddress is rt:#Shopping (customer checkout, a completely different page/actor). The #[Link(rel: 'doUpdateShippingAddress', ...)] on ShippingAddress.php's onGet/onPost predates this PR; removed both occurrences, kept only the admin-scoped rel. Also (minor, from the same review): added an in-template comment on Admin/Index.html.twig and Admin/System.html.twig's per-page logout forms explaining why they're not a shared admin-base.html.twig topbar form (a dozen other admin pages don't publish csrfToken, so a shared form would render an empty token there and fail CSRF in production) - the rationale existed only in this commit's own message, not in the template a future editor would read.
…ress, strengthen rel assertion, remove stale references
Deeper review of the ShippingAddress duplicate-Link fix found the resource's own
#[Alps('doUpdateShippingAddress')] attribute on the admin onPut still pointed at the
customer-side transition (rt: #Shopping) instead of the admin one (rt: #Order,
doUpdateOrderShippingAddress) — predates this PR, but left the resource internally
self-contradictory (Link says admin, Alps attribute says customer) after only the
#[Link] duplicates were cleaned up. Fixed the attribute to match.
Also:
- Strengthened the renamed TemplateList test to assert rel="goTemplateInstall"
directly, not just the href — otherwise a future reintroduction of the
goTemplateAdd/goTemplateInstall stacking regresses silently, the same weak-assertion
gap fixed elsewhere in #150.
- Fixed AdminOrderExtrasResourceSqlTest.php's contract-table docblock to name
doUpdateOrderShippingAddress, matching the admin-side rel (AdminOrderExtrasResourceTest.php
was already fixed).
- Removed the stale goAdminLogout line from Index.html.twig's ACTIONS comment — this
PR's own commit already deleted that Link from the resource; the comment was never
updated to match.
cd08a62 to
19a87e2
Compare
|
@coderabbitai review |
|
# Conflicts: # tests/Html/html-link-audit-ledger.json
1 participant
Closes #131 (resolves all 152 fail-classified ledger entries down to 0).
Diamond dependency — base explanation: #131's ledger fixes needed the state produced by BOTH #143 (customer/customer-delivery-edit/product-class dead forms) AND #136 (Block/Payment/TaxRule
#[Link]reconciliation) applied together — e.g.admin/block/block-list doUpdateBlockandadmin/payment/payment-list doUpdatePaymentonly appear as ledger entries after #136's#[Link]additions land. Since GitHub PRs support only one base branch and #143/#136 don't depend on each other, this branch carries #143's commits (pr-143-dead-forms-alps-drift, itself based on #139) plus #136's two commits (already independently reviewable in #152) before #131's own commit. Base is set topr-143-dead-forms-alps-drift, so this PR's diff currently also shows #136's 2 commits — once #139, #143, and #136 all merge to1.x, this will retarget there and its diff will shrink to just the #131-specific commit.Verified: full
vendor/bin/phpunitsuite green (2834 tests, 0 failures/errors; the one pre-existing risky test is #139's closed-ledger side effect, unrelated),vendor/bin/psalmclean.Summary by CodeRabbit
新機能
変更