Fix 3 of 5 dead admin forms and resolve 15 of 19 ALPS reference drifts (#143) - #153
Merged
Merged
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 (16)
📒 Files selected for processing (44)
✨ 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 |
Collaborator
Author
|
@coderabbitai review |
|
koriym
force-pushed
the
pr-143-dead-forms-alps-drift
branch
from
September 20, 2026 12:43
3e7ccd3 to
693bf65
Compare
Collaborator
Author
|
@coderabbitai review |
|
Collaborator
Author
Review (CodeRabbit rate-limited — manual review)Cross-checked against issue #143's own acceptance criteria, then verified Issue #143 acceptance criteria
Verification performed
Findings
Comment qualityThe new docblocks (AUTHZ check-ordering lists, No other correctness or security issues found — AUTHZ ordering (session → existence → ownership, session check always first) is consistent and correct across all three new write paths, CSRF ( |
koriym
force-pushed
the
pr-143-dead-forms-alps-drift
branch
from
September 20, 2026 14:54
693bf65 to
cc8ebea
Compare
Collaborator
Author
|
@coderabbitai review |
|
#143) Admin `/admin/customer`, `/admin/customer-delivery-edit`, and `/admin/product/product-class` rendered `<form method="post">` with no onPost/onPut/onDelete to answer it (405). The reference implementation lives on the unmerged `origin/post-redirect-get-303` branch (same merge-base as this branch, `4beee6a7`); cherry-picked `894a92df` and `5e05cb19` for exactly these three resources and reverted everything else those commits touched that was out of #143's scope: - TwoFactorAuthEdit.php / FileManager.php / CustomerList.php: scope decisions #143 explicitly defers to a separate issue. - A whole admin "flash message" feature (AdminFlash.php, admin-base.html.twig calling `admin_flashes()`): the Twig extension never defined that function, so every admin page render would have thrown. Reverted in full rather than half-wire it. - All three template conflicts (Customer/CustomerDeliveryEdit/ ProductClass) resolved to the current idea-admin-* design already on this branch, not the older Bootstrap-table markup the reference branch had — this branch's templates are the newer ones. Each write path got its own admin-specific Be Input/Final (AdminUpdateCustomerInput → AdminCustomerUpdated, Admin{Create,Update,Delete}CustomerDeliveryAddressInput, RegisterProductClassInput → ProductClassRegistered) rather than reusing the storefront transitions: `UpdateCustomerAddressInput` derives the owner from the customer session by design (a customer cannot reassign someone else's address by tampering with the body), and an admin editing a different customer's data is the opposite authorization model. This also resolves #143's actor-naming question in favor of option (B) — separate ids for the admin path (doUpdateCustomerProfile, doUpdateCustomerDeliveryAddress, doRegisterProductClass) rather than widening the storefront ids' docs. ProductClass.php's onGet was never publishing `csrfToken` (same gap GET JSON schema. Its template had two bugs: the hidden parent-id field was named `parentProductCode` but onPost reads `productCode`, and the CSRF token had no hidden field to carry it — both fixed. Added 8 new alps.json descriptors for the ids these fixes introduced (goCustomerDeliveryEdit, doUpdateCustomerDeliveryAddress, doDeleteCustomerDeliveryAddress, doUpdateCustomerProfile, goProductClass, doRegisterProductClass, goLog, goOrderPdf) plus the CustomerDeliveryEdit/Log/OrderPdf states they return to; validated with `asd --validate` and regenerated alps.json.html/alps.svg (root + docs/ copies). Dropped `#[Alps]` from the 8 route-gate/fallback ids (doActionRedirect, doAdminActionRedirect, doAdminUnsupportedRoute, doUnsupportedRoute, goActionRedirect, goAdminActionRedirect, goAdminUnsupportedRoute, goUnsupportedRoute) and the goAdminEmptyPage placeholder — these answer URLs EC-CUBE has that BeMart deliberately doesn't model as application transitions, so a client can't discover them anyway. goAdminLog/goAdminOrderOrderPdf/goAdminOrderMailConfirm/ goAdminTemplateTemplateAdd were renamed in the cherry-picked commits (the last two to already-existing profile ids goOrderMailConfirm/ goTemplateInstall; the first two are new — goAdminOrderOrderPdf is NOT a rename of goExportOrderPdf, it's the distinct options-form screen that links to it). Net: 19 drifted references down to 4 (doCreateMailTemplate, goAdminContentFileManager, goAdminTwoFactorAuthEdit, goShoppingShippingMultipleEdit) — updated `AlpsReferenceTest`'s SCREEN_GAP/ROUTE_GATE/PLACEHOLDER lists and `TemplateFormActionTest`'s KNOWN_DEAD list to match, and refreshed `docs/migration-status.md` §2.1/§2.2 and the feature matrix. Fixes made during verification, beyond the cherry-pick: - Customer::onPost($password), AdminUpdateCustomerInput::$password, and AdminCustomerUpdated::$passwordHash were missing #[SensitiveParameter] (CredentialParameterSensitiveParameterTest / CredentialConstructorParameterSensitiveParameterTest caught this). - Password/PasswordHash Semantic validators made nullable: the admin edit flow passes null to mean "leave the current hash untouched" (EC-CUBE's default-password sentinel); a supplied empty string still fails the length floor. - ExportOrderPdf gained an onPost (EC-CUBE's admin_order_pdf_download route) so OrderPdf.php's submit button reaches a real handler instead of 405 — reuses the existing goExportOrderPdf id since both verbs perform the same transition. The cherry-picked smoke-test fixture expected this to return 200, but OrderPdfCompatibilityService::export() unconditionally throws OrderPdfNotSupportedException (a Phase-A stub, tracked in docs/migration-status.md §4 item 5) — fixed the fixture to expect 501, and dropped an unrelated `?category_id=` the cherry-pick had added to the GET /products fixture line (broke JSON Schema validation; unrelated to this issue). - Removed the stale `POST admin/two-factor-auth-edit` smoke-test fixture entry the cherry-pick added — that resource still has no onPost since TwoFactorAuthEdit stays deferred. - HttpSqlAdminProductClassFormTest (a new real-browser/real-MySQL regression from the cherry-pick) assumed a pre-existing `admin-active-001` product/class row that nothing in this environment seeds; added setUp/tearDown that inserts and cleans up the parent dtb_product + dtb_product_class rows directly, since the fixture helper it was written against (SqlFixturesTrait) doesn't exist in this repo (removed by `47016e64`, already documented in #139's commit). - HttpSqlAdminProductClassFormTest's two markup assertions pinned the reverted Bootstrap-table template's `id="product_class_new_row"` and "登録" button text; updated to the kept idea-admin-* template's actual id and "追加" label. - The new #[Link] declarations on Customer/CustomerDeliveryEdit/ ProductClass introduced 5 HTML Link Audit warnings (the templates don't render a matching `rel="..."` token); classified all 5 in html-link-audit-ledger.json — 4 as semantic-token-missing/fail (real markup gaps, tracked), 1 (doDeleteCustomerDeliveryAddress) as method-mismatch/resourceOnly (the edit form has no delete affordance at all; deleting a delivery address is out of this editor's scope). Deferred, per #143: `/admin/content/file-manager` and `/admin/two-factor-auth-edit` stay dead forms (multipart file I/O and admin-editing-another-member 2FA semantics both need their own scope decision first). 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), `asd --validate alps.json` clean.
…unts, UI-reachability limitation Post-commit review of `716a8a44`/`a0245c36` surfaced one real SSOT gap and a few stale numbers that `AlpsReferenceTest` can't catch on its own: - `CustomerDeliveryEdit.php` declares `#[Link(rel: 'doCreateCustomerDeliveryAddress', ...)]` on its onGet (for HATEOAS discoverability — the create/update split only exists at the Link level; `#[Alps]` uses a single `doUpdateCustomerDeliveryAddress` id for both branches). No alps.json descriptor backed it. `AlpsReferenceTest` only scans `#[Alps]` attributes, not `#[Link]` rels, so this drift was invisible to every test in the suite. Added the descriptor and swept the other 7 files #143 touched (Customer/CustomerDeliveryEdit/ProductClass/Log/OrderPdf/MailConfirm/ TemplateAdd/ExportOrderPdf) for the same gap — none found. - `OrderPdf` state was missing the `src-template` tag every other screen state in this profile carries (Security, ChangePassword, Customer, CustomerAddress, CustomerDeliveryEdit, Log all have it). - `docs/migration-status.md`: the ALPS transition count (216, now 217 after the descriptor above) and three feature-matrix row counts (flow-manage-product 24→26, flow-manage-order 13→14, flow-manage-customer 6→11) were left at their pre-#143 values. - Documented a real, previously-silent completeness gap in `CustomerDeliveryEdit.php`: the rendered form has no `addressId` hidden field and no existing-address list, so `onPost` can only ever take the create branch and `onDelete` has no click path at all — both are exercised directly by `AdminCustomerDeliveryEditResourceTest` but unreachable from the page today. Not a regression from #143 (the cherry-picked template never had this either), and building the edit/delete affordance (an address list on `/admin/customer`, or addressId-aware `onGet`) is a materially larger UI slice than the dead-form fix #143 scoped — documented rather than silently left, so it isn't mistaken for done. Re-verified (independently reviewed and confirmed already correct, no change needed): Customer.php/CustomerDeliveryEdit.php/ProductClass.php constructors carry every dependency from both merge sides with no duplicate imports; TwoFactorAuthEdit.php's only diff from origin/1.x is FileManager.php/CustomerList.php/AdminTwoFactorAuthEditResourceTest.php are byte-identical to origin/1.x (cleanly reverted); the news-list delete dialog's real submission form (`#news-delete-form`) already carries a csrfToken hidden field; `Page/Mypage/navi.html.twig` is unused dead code, not a live shared partial. Verified: `asd --validate alps.json` clean, regenerated alps.json.html / alps.svg (root + docs/ copies), full `vendor/bin/phpunit` suite green (2836 tests, 0 failures/errors; same pre-existing risky test as before), `vendor/bin/psalm` clean.
…ality The feature-matrix row said customer-delivery-edit was simply 'done' alongside customer edit. Per eaabac5's finding (and the docblock it added to CustomerDeliveryEdit.php), the rendered form only ever posts an empty addressId, so update/delete are unreachable from the UI — create-only. Matches the same honesty already applied to the doDeleteCustomerDeliveryAddress ledger note in a0245c3.
…-status.md omission Independent review found two real gaps: - RegisterProductClassInput/ProductClassRegistered broke the established Admin*-prefix naming convention used by all 49+ other admin-mutation Input/Final class pairs in be/src/Input and be/src/Final (AdminCreateCustomerInput -> AdminCustomerCreating, AdminDeleteProductInput -> AdminProductDeleted, etc.). docs/migration-status.md's own prose already cited the correctly-conventioned name (AdminRegisterProductClassInput) that didn't exist in code. Renamed both classes (AdminRegisterProductClassInput, AdminProductClassRegistered) and updated every reference in src/Resource/Page/Admin/Product/ProductClass.php. - migration-status.md's admin-Input list for the #143 dead-form fixes omitted AdminCreateCustomerDeliveryAddressInput even though CustomerDeliveryEdit.php actually wires three Input classes (Create/Update/Delete), not two.
koriym
force-pushed
the
pr-143-dead-forms-alps-drift
branch
from
September 20, 2026 15:40
cc8ebea to
71ae2e2
Compare
Collaborator
Author
|
@coderabbitai review |
|
# Conflicts: # alps.json.html # alps.svg # docs/alps.json.html # docs/alps.svg
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.
Closes #143 partially (3 of 5 dead forms fixed:
customer,customer-delivery-edit,product-class;file-managerandtwo-factor-auth-editremain, per the issue's own note that those two need a separate scope decision first). Resolves 15 of 19 ALPS reference drifts.Depends on #139 (
pr-139-csrf-token-publish-test) — this branch is stacked on it becauseCustomer.php/CustomerDeliveryEdit.phpboth need the csrfToken wiring #139 adds before #143's own write-handler changes to those same files apply cleanly. Base is set to that branch; once #139 merges to1.x, GitHub will retarget this PR automatically.Also includes two follow-up commits from post-commit review:
CustomerDeliveryEdit.phpdeclared#[Link(rel: 'doCreateCustomerDeliveryAddress', ...)]with no matching alps.json descriptor (invisible toAlpsReferenceTest, which only scans#[Alps]attributes, not#[Link]rels). Added the descriptor.docs/migration-status.mdcounts (ALPS transition total, three feature-matrix flow-area counts) left at pre-管理画面の死んだフォーム 5 件と #[Alps] 参照ドリフト 19 件を解消する #143 values, plus an honest correction: the feature-matrix claimedcustomer-delivery-editwas simply "done," but the rendered form only ever posts an emptyaddressId, so update/delete are unreachable from the UI today — softened to reflect that (matching the honesty already in thedoDeleteCustomerDeliveryAddressledger note).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,asd --validate alps.jsonclean with regeneratedalps.json.html/alps.svg(root +docs/copies).Summary by CodeRabbit
新機能
セキュリティ
ドキュメント