fix: 管理画面の会員一覧で LIMIT が適用されない不具合を修正 (#1398)#1399
Merged
Merged
Conversation
PR EC-CUBE#1116 で生じた SC_Helper_Customer::sfGetSearchData() の LIMIT 不適用回帰を 回帰防止できるテストを追加する。 - PHPUnit (tests/class/helper/SC_Helper_Customer/): page_max / linemax / OFFSET / limitMode の 4 観点を網羅。修正前は LIMIT/OFFSET 検証 2 件が必ず fail する。 - E2E (e2e-tests/test/admin/customer/list_pagination.test.ts): 30 件のフィクスチャで ページャを操作し, ページ毎に異なる会員 ID が返ることを確認。 - .github/workflows/e2e-tests.yml: 上記 E2E の前提として --customers 数を 1 → 30 に増加。他テストは MAX(customer_id) で最新会員を取得しており影響なし。
PR EC-CUBE#1116 で SC_Query::setLimitOffset() の挙動が MDB2 connection への即時反映 から $this->limit/$this->offset への保存のみへ変更された。 これらの値は SC_Query::getSql() 経由でしか SQL に注入されないため, $objQuery->getAll() に生 SQL を直接渡す SC_Helper_Customer::sfGetSearchData() では LIMIT/OFFSET が一切付与されず, 管理画面の会員一覧 (/admin/customer/) で dtb_customer の全件が取得・表示される回帰が発生していた。 呼び出し側で $objQuery->dbFactory->addLimitOffset() を直接適用することで, SC_Query::setLimitOffset() の内部実装に依存せず確実に LIMIT/OFFSET を SQL へ注入する。SC_Query::getSql() が内部で行っているのと同等の処理であり, LC_Page_Admin_Home::page() や SC_Product::getProductsClassFullByProductId() 等にも同パターンの先例がある。 他の setLimitOffset() 呼び出し箇所は SC_Query::select() / SC_Query::getOne() 等の getSql() 経由のメソッドで結果を受けており, LIMIT/OFFSET が正常に SQL へ反映されるため本件の影響は受けない。
📝 WalkthroughWalkthroughsfGetSearchData() が生成する生SQLへ明示的に LIMIT/OFFSET を適用するよう修正し、その挙動を検証する単体テストと管理画面向けE2E回帰テストを追加、E2E用フィクスチャを30件に増やした。 ChangesPagination Fix and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1399 +/- ##
==========================================
+ Coverage 55.30% 55.88% +0.58%
==========================================
Files 87 87
Lines 11089 11090 +1
==========================================
+ Hits 6133 6198 +65
+ Misses 4956 4892 -64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e-tests/test/admin/customer/list_pagination.test.ts`:
- Line 18: Fix the space-before-function-paren ESLint violation on the
collectCustomerIds function declaration: update the declaration of async
function collectCustomerIds(page: Page): Promise<string[]> to match the
project's ESLint rule (remove the extra space before the opening parenthesis or
add it if your config requires it) so the function signature conforms to the
configured space-before-function-paren rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b967806-50b9-4b20-8c54-316e231e68f3
📒 Files selected for processing (4)
.github/workflows/e2e-tests.ymldata/class/helper/SC_Helper_Customer.phpe2e-tests/test/admin/customer/list_pagination.test.tstests/class/helper/SC_Helper_Customer/SC_Helper_Customer_sfGetSearchDataTest.php
CodeRabbit からの指摘 (EC-CUBE#1399 review): list_pagination.test.ts の collectCustomerIds 関数宣言が .eslintrc.json の `space-before-function-paren: { named: always }` ルールに違反していたため, 関数名と括弧の間にスペースを追加。
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e-tests/test/admin/customer/list_pagination.test.ts (1)
26-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winE2E実行要件（MySQL/PostgreSQL + Playwright 1.52.0）の整合確認
- 本リポジトリの Playwright は package.json / package-lock.json で 1.56.0 になっており、ガイドライン指定の 1.52.0 と不一致です（要件に合わせてバージョンを揃える／PRで使用バージョンを明記）。
- ガイドライン要件どおり、MySQL/PostgreSQL 環境でローカル実行した根拠として「実行コマンド・環境（DB種別/設定）・成功ログ」をPRに追記してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/test/admin/customer/list_pagination.test.ts` around lines 26 - 47, The repository's Playwright version (package.json / package-lock.json) is 1.56.0 but the project guideline requires 1.52.0 and the PR lacks local-run evidence; update package.json and package-lock.json to use Playwright 1.52.0 (or explicitly state and justify using 1.56.0) and regenerate lockfile so tests install the correct version, and then append to this PR a short runnable proof for the e2e test (e2e-tests/test/admin/customer/list_pagination.test.ts) including the exact command used to run Playwright, the DB type and connection/configuration (MySQL or PostgreSQL and relevant settings), and the success log output or a screenshot demonstrating the test passed locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@e2e-tests/test/admin/customer/list_pagination.test.ts`:
- Around line 26-47: The repository's Playwright version (package.json /
package-lock.json) is 1.56.0 but the project guideline requires 1.52.0 and the
PR lacks local-run evidence; update package.json and package-lock.json to use
Playwright 1.52.0 (or explicitly state and justify using 1.56.0) and regenerate
lockfile so tests install the correct version, and then append to this PR a
short runnable proof for the e2e test
(e2e-tests/test/admin/customer/list_pagination.test.ts) including the exact
command used to run Playwright, the DB type and connection/configuration (MySQL
or PostgreSQL and relevant settings), and the success log output or a screenshot
demonstrating the test passed locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75fa2c85-023a-4261-a752-0c74baa88749
📒 Files selected for processing (1)
e2e-tests/test/admin/customer/list_pagination.test.ts
seasoftjapan
approved these changes
May 25, 2026
Contributor
seasoftjapan
left a comment
seasoftjapan
left a comment
There was a problem hiding this comment.
nanasess:issue-1398 をチェックアウトして動作を確認した。問題ない。
実装を確認した。問題ない。
テストはコードまで確認できていないが、確認内容は妥当な内容だと思う。
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
2 participants
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.
Summary
Closes #1398
PR #1116 (5559b6a0, 2025-01-10) で
SC_Query::setLimitOffset()が MDB2 connection への即時反映から\$this->limit/\$this->offsetへの保存のみに変更された結果,\$objQuery->getAll()に生 SQL を直接渡すSC_Helper_Customer::sfGetSearchData()では LIMIT/OFFSET が一切付与されず, 管理画面の会員一覧 (/admin/customer/) でdtb_customerの全件が取得・表示される回帰が発生していました。修正方針 (Issue 修正案 A: 局所修正)
呼び出し側で
\$objQuery->dbFactory->addLimitOffset()を直接適用し,SC_Query::setLimitOffset()の内部実装に依存せず確実に LIMIT/OFFSET を SQL へ注入します。SC_Query::getSql()が内部で行っている処理と同等LC_Page_Admin_Home::page(),SC_Product::getProductsClassFullByProductId(),ItemSearch) の先例ありsetLimitOffset()呼び出し箇所はSC_Query::select()/getOne()等のgetSql()経由メソッドで結果を受けているため LIMIT が SQL に反映されており, 本件の影響は受けないコミット
test:回帰防止用の PHPUnit / E2E テストを追加fix:本体修正 (SC_Helper_Customer::sfGetSearchData())Test plan
tests/class/helper/SC_Helper_Customer/4 テスト passtestSfGetSearchDataRespectsLimit/testSfGetSearchDataPaginationReturnsDifferentRowsが fail (LIMIT/OFFSET 未適用を検知)e2e-tests/test/admin/customer/list_pagination.test.tspass補足
.github/workflows/e2e-tests.ymlの--customers=1を--customers=30に変更しています。これは追加した E2E (ページネーション動作確認) に 10 件以上の会員データが必要なためです。他の E2E はMAX(customer_id)等で最新会員を取得しており, フィクスチャ件数の増加による影響はありません。Summary by CodeRabbit
Bug Fixes
Tests
Chores