Conversation
出荷編集画面で注文者情報や他の出荷先の住所をコピーできる機能を追加しました。 - 「注文者情報をコピー」ボタンを追加 - 複数出荷先がある場合、「他の出荷情報からコピー」ドロップダウンを追加 - E2Eテストを追加 refs EC-CUBE#6595 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough配送編集画面に「注文者情報をコピー」と「他の出荷情報からコピー」機能を追加。Pageオブジェクトに4つの公開メソッドを追加し、テンプレートへUI/JSを組み込み、受け入れテストと英日ロケール文字列を追加しています。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
codeception/acceptance/EA04OrderCest.php (1)
574-582: コピー検証にprefとcompany_nameも含めてください。住所コピー機能の回帰検知として、都道府県・会社名までアサートしておくとテストの実効性が上がります。
✅ 追加アサート例
$I->assertEquals($Order->getPostalCode(), $ShippingEditPage->出荷情報のフィールド値を取得(0, 'postal_code')); + $I->assertEquals((string) $Order->getPref()->getId(), $ShippingEditPage->出荷情報のフィールド値を取得(0, 'pref')); $I->assertEquals($Order->getAddr01(), $ShippingEditPage->出荷情報のフィールド値を取得(0, 'addr01')); $I->assertEquals($Order->getAddr02(), $ShippingEditPage->出荷情報のフィールド値を取得(0, 'addr02')); $I->assertEquals($Order->getPhoneNumber(), $ShippingEditPage->出荷情報のフィールド値を取得(0, 'phone_number')); + $I->assertEquals($Order->getCompanyName(), $ShippingEditPage->出荷情報のフィールド値を取得(0, 'company_name')); @@ $I->assertEquals('1000001', $ShippingEditPage->出荷情報のフィールド値を取得(1, 'postal_code')); + $I->assertEquals('13', $ShippingEditPage->出荷情報のフィールド値を取得(1, 'pref')); $I->assertEquals('千代田区', $ShippingEditPage->出荷情報のフィールド値を取得(1, 'addr01')); $I->assertEquals('1-1-1', $ShippingEditPage->出荷情報のフィールド値を取得(1, 'addr02')); $I->assertEquals('0312345678', $ShippingEditPage->出荷情報のフィールド値を取得(1, 'phone_number')); + $I->assertEquals('', $ShippingEditPage->出荷情報のフィールド値を取得(1, 'company_name'));Also applies to: 624-632
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeception/acceptance/EA04OrderCest.php` around lines 574 - 582, Add assertions to verify that prefecture and company name are copied: in EA04OrderCest.php, inside the block using $Order and $ShippingEditPage (the 出荷情報のフィールド値を取得(...) calls), add assertions comparing $Order->getPref() and $Order->getCompanyName() to ShippingEditPage->出荷情報のフィールド値を取得(0, 'pref') and 出荷情報のフィールド値を取得(0, 'company_name') respectively; apply the same additions to the similar assertion block around lines 624-632.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeception/_support/Page/Admin/ShippingEditPage.php`:
- Around line 286-301: The method 出荷情報のフィールド値を取得 currently indexes
$fieldMap[$field] directly and lacks type hints; change the signature to accept
typed parameters (e.g. int $shippingNo, string $field) and a typed return (e.g.
?string), validate the $field against $fieldMap using isset() or
array_key_exists() and throw a \InvalidArgumentException on unknown fields, then
use the mapped key to build the id and return the result (casting or allowing
null to match grabValueFrom's behavior); reference symbols: method
出荷情報のフィールド値を取得, local $fieldMap, and $this->tester->grabValueFrom.
- Around line 237-302: The four public methods lack PHP parameter and/or return
type declarations; update the signatures: add a return type of self to
goByOrderId(AcceptanceTester $I) (keep the existing param type), change
注文者情報をコピー to declare int $shippingNo = 0 and return self, change 他の出荷情報からコピー to
declare int $targetShippingNo, int $sourceShippingNo and return self, and change
出荷情報のフィールド値を取得 to declare int $shippingNo, string $field and a return type of
string; adjust any callers/tests if strict types require strict typing
compatibility and ensure imports/namespace references still resolve for self.
In `@codeception/acceptance/EA04OrderCest.php`:
- Around line 545-632: The two new test methods order_出荷編集_注文者情報をコピー and
order_出荷編集_他の出荷情報からコピー are missing explicit return type declarations; update
their method signatures in EA04OrderCest.php to include the PHP void return type
(add ": void" after the parameter list) so both methods become public function
order_出荷編集_注文者情報をコピー(AcceptanceTester $I): void and public function
order_出荷編集_他の出荷情報からコピー(AcceptanceTester $I): void.
---
Nitpick comments:
In `@codeception/acceptance/EA04OrderCest.php`:
- Around line 574-582: Add assertions to verify that prefecture and company name
are copied: in EA04OrderCest.php, inside the block using $Order and
$ShippingEditPage (the 出荷情報のフィールド値を取得(...) calls), add assertions comparing
$Order->getPref() and $Order->getCompanyName() to
ShippingEditPage->出荷情報のフィールド値を取得(0, 'pref') and 出荷情報のフィールド値を取得(0,
'company_name') respectively; apply the same additions to the similar assertion
block around lines 624-632.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
codeception/_support/Page/Admin/ShippingEditPage.phpcodeception/acceptance/EA04OrderCest.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/admin/Order/shipping.twig
- ShippingEditPage: 引数・戻り値の型宣言を追加 - ShippingEditPage: フィールド名のバリデーションを追加 - EA04OrderCest: テストメソッドに戻り値型voidを追加 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codeception/acceptance/EA04OrderCest.php (1)
602-607: Line 603 の固定待機は不安定化要因です。
wait(3)では環境差でフレークしやすいので、要素出現待ちに置き換えるのが安全です。🔧 変更例
// 出荷先を追加 $ShippingEditPage->出荷先を追加(); - $I->wait(3); + $I->waitForElementVisible(['id' => 'form_shippings_1_name_name01']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeception/acceptance/EA04OrderCest.php` around lines 602 - 607, Replace the fixed sleep ($I->wait(3)) with a deterministic wait for a DOM change that signals the shipping edit page has reloaded; after calling ShippingEditPage->出荷先を追加() use $I->waitForElementVisible or $I->waitForElement to wait for a specific selector or element exposed by ShippingEditPage (for example the shipping list row, a success flash, or a known page root used in ShippingEditPage::at) before re-instantiating ShippingEditPage::at($I); this removes the flaky fixed delay and ensures the test proceeds only when the page is ready.codeception/_support/Page/Admin/ShippingEditPage.php (1)
250-257: Line 254 と Line 273 の固定wait(1)は条件待機に置き換えた方が安定します。コピー反映を時間依存で待つと、実行環境差で不安定になります。DOM状態の変化を条件に待機してください。
🔧 変更例
public function 注文者情報をコピー(int $shippingNo = 0): self { $this->tester->scrollTo('#shipmentOverview_'.$shippingNo); $this->tester->click('.copy-orderer[data-shipping-no="'.$shippingNo.'"]'); - $this->tester->wait(1); + $this->tester->waitForJS( + "return document.getElementById('form_shippings_{$shippingNo}_name_name01').value !== ''", + 5 + ); return $this; } @@ public function 他の出荷情報からコピー(int $targetShippingNo, int $sourceShippingNo): self { $this->tester->scrollTo('#shipmentOverview_'.$targetShippingNo); // ドロップダウンを開く $this->tester->click('#shipmentOverview_'.$targetShippingNo.' .btn-group .dropdown-toggle'); $this->tester->waitForElementVisible('#shipmentOverview_'.$targetShippingNo.' .dropdown-menu'); // コピー元を選択 $this->tester->click('.copy-other-shipping[data-shipping-no="'.$targetShippingNo.'"][data-source-no="'.$sourceShippingNo.'"]'); - $this->tester->wait(1); + $this->tester->waitForJS( + "return document.getElementById('form_shippings_{$targetShippingNo}_name_name01').value !== ''", + 5 + ); return $this; }Also applies to: 265-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeception/_support/Page/Admin/ShippingEditPage.php` around lines 250 - 257, The fixed wait(1) in 注文者情報をコピー should be replaced with a conditional wait that detects the DOM update instead of sleeping; in the method 注文者情報をコピー(int $shippingNo = 0) use the tester's waitFor* helpers (e.g. waitForElement, waitForText, or waitForJS) to wait until the shipment overview or the specific fields inside '#shipmentOverview_'.$shippingNo reflect the copied data (or until a success indicator appears) before returning; apply the same replacement for the other similar block around lines 265-276 so both spots wait for a DOM condition rather than a fixed timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeception/acceptance/EA04OrderCest.php`:
- Around line 587-632: The test order_出荷編集_他の出荷情報からコピー currently never asserts
that the "他の出荷情報からコピー" dropdown is hidden when there is only one shipping entry;
add a case that, after opening the ShippingEditPage (use
ShippingEditPage::goByOrderId / ShippingEditPage::at) and before calling
出荷先を追加(), asserts the copy dropdown is not visible for index 0 (e.g. call the
page object method that checks visibility or add a new
ShippingEditPage->他の出荷情報からコピーが表示されているか/非表示か helper and assert false). Then
proceed with the existing flow that adds a second entry and verifies copying via
他の出荷情報からコピー and 出荷情報のフィールド値を取得 to keep the rest unchanged.
---
Nitpick comments:
In `@codeception/_support/Page/Admin/ShippingEditPage.php`:
- Around line 250-257: The fixed wait(1) in 注文者情報をコピー should be replaced with a
conditional wait that detects the DOM update instead of sleeping; in the method
注文者情報をコピー(int $shippingNo = 0) use the tester's waitFor* helpers (e.g.
waitForElement, waitForText, or waitForJS) to wait until the shipment overview
or the specific fields inside '#shipmentOverview_'.$shippingNo reflect the
copied data (or until a success indicator appears) before returning; apply the
same replacement for the other similar block around lines 265-276 so both spots
wait for a DOM condition rather than a fixed timeout.
In `@codeception/acceptance/EA04OrderCest.php`:
- Around line 602-607: Replace the fixed sleep ($I->wait(3)) with a
deterministic wait for a DOM change that signals the shipping edit page has
reloaded; after calling ShippingEditPage->出荷先を追加() use $I->waitForElementVisible
or $I->waitForElement to wait for a specific selector or element exposed by
ShippingEditPage (for example the shipping list row, a success flash, or a known
page root used in ShippingEditPage::at) before re-instantiating
ShippingEditPage::at($I); this removes the flaky fixed delay and ensures the
test proceeds only when the page is ready.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeception/_support/Page/Admin/ShippingEditPage.phpcodeception/acceptance/EA04OrderCest.php
出荷先が1件の場合、「他の出荷情報からコピー」ドロップダウンが 表示されないことを検証するテストケースを追加 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codeception/acceptance/EA04OrderCest.php (1)
549-557: テストセットアップの共通化を検討してください。受注作成〜出荷編集画面遷移の前処理が3メソッドで重複しているため、privateヘルパーにまとめると保守性が上がります。
Also applies to: 591-599, 641-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeception/acceptance/EA04OrderCest.php` around lines 549 - 557, The repeated setup that creates a new order and navigates to the shipping edit page should be extracted into a private helper on EA04OrderCest (e.g., prepareOrderAndOpenShippingEdit) that calls the existing Fixtures functions (createCustomer, createOrders) to create an Order with OrderStatus::NEW and then invokes ShippingEditPage::goByOrderId($I, $order->getId()); replace the three duplicated blocks (including the occurrences at the other noted ranges) with calls to this helper so creation and navigation logic is centralized and reusable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeception/acceptance/EA04OrderCest.php`:
- Around line 574-581: Add assertions to verify the prefecture (都道府県) is copied:
call the order getter (e.g. Order->getPrefecture() or the correct prefecture
accessor on the Order object) and compare it to the
ShippingEditPage->出荷情報のフィールド値を取得(0, 'prefecture') using $I->assertEquals(...);
add the same prefecture assertion in the other block referenced (lines around
624-631) to mirror the existing name/postal/address/phone checks.
---
Nitpick comments:
In `@codeception/acceptance/EA04OrderCest.php`:
- Around line 549-557: The repeated setup that creates a new order and navigates
to the shipping edit page should be extracted into a private helper on
EA04OrderCest (e.g., prepareOrderAndOpenShippingEdit) that calls the existing
Fixtures functions (createCustomer, createOrders) to create an Order with
OrderStatus::NEW and then invokes ShippingEditPage::goByOrderId($I,
$order->getId()); replace the three duplicated blocks (including the occurrences
at the other noted ranges) with calls to this helper so creation and navigation
logic is centralized and reusable.
- 注文者情報コピーテストに都道府県の検証を追加 - 他の出荷情報からコピーテストに都道府県の検証を追加 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.3 #6643 +/- ##
=========================================
Coverage 79.33% 79.33%
Complexity 6638 6638
=========================================
Files 475 475
Lines 26568 26568
=========================================
Hits 21078 21078
Misses 5490 5490
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:
|
|
@kurozumi |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Eccube/Resource/template/admin/Order/shipping.twig (2)
378-380:javascript:URL ではなくbutton要素を使ってください。Line 378 は画面遷移ではなく単なるアクションなので、
href="javascript:;"よりtype="button"の方が自然です。javascript:依存は CSP やアクセシビリティ観点でも避けたいです。🔧 置き換え例
- <a class="dropdown-item copy-other-shipping" href="javascript:;" data-shipping-no="{{ currentShippingNo }}" data-source-no="{{ loop.index0 }}"> + <button type="button" class="dropdown-item copy-other-shipping" data-shipping-no="{{ currentShippingNo }}" data-source-no="{{ loop.index0 }}"> {{ 'admin.order.shipping__card_title'|trans }}({{ loop.index }}) - </a> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Eccube/Resource/template/admin/Order/shipping.twig` around lines 378 - 380, Replace the anchor tag used for an in-page action with a button element: change the <a class="dropdown-item copy-other-shipping" href="javascript:;" data-shipping-no="{{ currentShippingNo }}" data-source-no="{{ loop.index0 }}">...</a> into a <button type="button" class="dropdown-item copy-other-shipping" data-shipping-no="{{ currentShippingNo }}" data-source-no="{{ loop.index0 }}">...</button>, preserving the class "dropdown-item copy-other-shipping", the data attributes data-shipping-no and data-source-no, and the inner translatable text {{ 'admin.order.shipping__card_title'|trans }}({{ loop.index }}); ensure no href remains so the element is a semantic button suitable for CSP and accessibility.
39-73: コピー対象フィールドの代入ロジックは共通化した方が安全です。Line 44-53 と Line 63-72 で同じフィールド一覧を二重管理しているので、次回フィールド追加や名称変更が入ると片方だけ更新漏れになりやすいです。ヘルパー化して 1 か所に寄せておくと保守事故を減らせます。
♻️ 共通化イメージ
+ function setShippingFields(prefix, values) { + var fields = [ + 'name_name01', + 'name_name02', + 'kana_kana01', + 'kana_kana02', + 'postal_code', + 'address_pref', + 'address_addr01', + 'address_addr02', + 'phone_number', + 'company_name' + ]; + + $.each(fields, function(_, field) { + $(prefix + field).val(values[field]); + }); + } + // 注文者情報をコピー $(document).on('click', '.copy-orderer', function() { var shippingNo = $(this).data('shipping-no'); var prefix = '#form_shippings_' + shippingNo + '_'; - - $(prefix + 'name_name01').val(ordererInfo.name01); - $(prefix + 'name_name02').val(ordererInfo.name02); - $(prefix + 'kana_kana01').val(ordererInfo.kana01); - $(prefix + 'kana_kana02').val(ordererInfo.kana02); - $(prefix + 'postal_code').val(ordererInfo.postal_code); - $(prefix + 'address_pref').val(ordererInfo.pref_id); - $(prefix + 'address_addr01').val(ordererInfo.addr01); - $(prefix + 'address_addr02').val(ordererInfo.addr02); - $(prefix + 'phone_number').val(ordererInfo.phone_number); - $(prefix + 'company_name').val(ordererInfo.company_name); + setShippingFields(prefix, { + name_name01: ordererInfo.name01, + name_name02: ordererInfo.name02, + kana_kana01: ordererInfo.kana01, + kana_kana02: ordererInfo.kana02, + postal_code: ordererInfo.postal_code, + address_pref: ordererInfo.pref_id, + address_addr01: ordererInfo.addr01, + address_addr02: ordererInfo.addr02, + phone_number: ordererInfo.phone_number, + company_name: ordererInfo.company_name + }); }); // 他の出荷情報からコピー $(document).on('click', '.copy-other-shipping', function() { var targetNo = $(this).data('shipping-no'); var sourceNo = $(this).data('source-no'); var targetPrefix = '#form_shippings_' + targetNo + '_'; var sourcePrefix = '#form_shippings_' + sourceNo + '_'; - - $(targetPrefix + 'name_name01').val($(sourcePrefix + 'name_name01').val()); - $(targetPrefix + 'name_name02').val($(sourcePrefix + 'name_name02').val()); - $(targetPrefix + 'kana_kana01').val($(sourcePrefix + 'kana_kana01').val()); - $(targetPrefix + 'kana_kana02').val($(sourcePrefix + 'kana_kana02').val()); - $(targetPrefix + 'postal_code').val($(sourcePrefix + 'postal_code').val()); - $(targetPrefix + 'address_pref').val($(sourcePrefix + 'address_pref').val()); - $(targetPrefix + 'address_addr01').val($(sourcePrefix + 'address_addr01').val()); - $(targetPrefix + 'address_addr02').val($(sourcePrefix + 'address_addr02').val()); - $(targetPrefix + 'phone_number').val($(sourcePrefix + 'phone_number').val()); - $(targetPrefix + 'company_name').val($(sourcePrefix + 'company_name').val()); + setShippingFields(targetPrefix, { + name_name01: $(sourcePrefix + 'name_name01').val(), + name_name02: $(sourcePrefix + 'name_name02').val(), + kana_kana01: $(sourcePrefix + 'kana_kana01').val(), + kana_kana02: $(sourcePrefix + 'kana_kana02').val(), + postal_code: $(sourcePrefix + 'postal_code').val(), + address_pref: $(sourcePrefix + 'address_pref').val(), + address_addr01: $(sourcePrefix + 'address_addr01').val(), + address_addr02: $(sourcePrefix + 'address_addr02').val(), + phone_number: $(sourcePrefix + 'phone_number').val(), + company_name: $(sourcePrefix + 'company_name').val() + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Eccube/Resource/template/admin/Order/shipping.twig` around lines 39 - 73, The two click handlers '.copy-orderer' and '.copy-other-shipping' duplicate the same field assignment list; extract that logic into a single helper (e.g., copyShippingFields) that accepts a targetPrefix and either an ordererInfo object or a sourcePrefix (DOM selector) and performs the assignments for name_name01, name_name02, kana_kana01, kana_kana02, postal_code, address_pref, address_addr01, address_addr02, phone_number, company_name; then replace the inline assignments in the '.copy-orderer' and '.copy-other-shipping' handlers to call copyShippingFields(targetPrefix, source) so the field list is maintained in only one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Eccube/Resource/template/admin/Order/shipping.twig`:
- Around line 378-380: Replace the anchor tag used for an in-page action with a
button element: change the <a class="dropdown-item copy-other-shipping"
href="javascript:;" data-shipping-no="{{ currentShippingNo }}"
data-source-no="{{ loop.index0 }}">...</a> into a <button type="button"
class="dropdown-item copy-other-shipping" data-shipping-no="{{ currentShippingNo
}}" data-source-no="{{ loop.index0 }}">...</button>, preserving the class
"dropdown-item copy-other-shipping", the data attributes data-shipping-no and
data-source-no, and the inner translatable text {{
'admin.order.shipping__card_title'|trans }}({{ loop.index }}); ensure no href
remains so the element is a semantic button suitable for CSP and accessibility.
- Around line 39-73: The two click handlers '.copy-orderer' and
'.copy-other-shipping' duplicate the same field assignment list; extract that
logic into a single helper (e.g., copyShippingFields) that accepts a
targetPrefix and either an ordererInfo object or a sourcePrefix (DOM selector)
and performs the assignments for name_name01, name_name02, kana_kana01,
kana_kana02, postal_code, address_pref, address_addr01, address_addr02,
phone_number, company_name; then replace the inline assignments in the
'.copy-orderer' and '.copy-other-shipping' handlers to call
copyShippingFields(targetPrefix, source) so the field list is maintained in only
one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f61ce298-a931-455d-9839-2f2cae776045
📒 Files selected for processing (3)
src/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/admin/Order/shipping.twig
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Eccube/Resource/locale/messages.ja.yaml
- src/Eccube/Resource/locale/messages.en.yaml
Summary
Test plan
refs #6595
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
テスト
ローカライズ