[refactor] メールテンプレートをEC-CUBE標準のdtb_mail_templateに移行#2
Merged
dotani1111 merged 12 commits intomainfrom Mar 22, 2026
Merged
Conversation
- PluginManager: enable時にMailTemplateレコード作成＋twigファイルコピー、uninstall時に削除 - StockAlertMailBuilder: MailTemplateRepositoryを使って件名・本文を取得、fallback付き - StockAlertConfig: mail_subject / mail_body カラムを削除 - Version20260321000000: 既存インストール向け DROP COLUMN マイグレーション追加 - StockAlertConfigController: テストメール送信アクションを追加（CSRF保護付き） - config.twig: メールテンプレート案内カード＋テストメール送信ボタンを追加 - カスタムメールテンプレート編集画面（Controller/Form/twig）を削除 Co-Authored-By: Claude Sonnet 4.6 <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:
📝 WalkthroughWalkthroughStockAlertConfigから件名/本文を削除し、MailTemplateを参照してメールを生成するよう移行。管理画面に「テスト送信」POSTアクションとボタンを追加。テンプレート編集UI/フォーム/コントローラを削除、マイグレーションでカラム除去を扱い、PluginManagerでTwigテンプレートを配置/削除。 Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as "Admin UI"
participant Controller as "StockAlertConfigController"
participant Builder as "StockAlertMailBuilder"
participant Repo as "MailTemplateRepository"
participant Mailer as "MailerInterface"
Admin->>Controller: POST /plugin/stock-alert/config/send-test (CSRF)
Controller->>Controller: validate CSRF
Controller->>Controller: repo->find(1) (StockAlertConfig)
Controller->>Builder: buildBaseInfo(), resolveRecipients(), generateDummyItems()
Controller->>Builder: buildMailSubject()
Builder->>Repo: findMailTemplate() by `PluginManager::MAIL_TEMPLATE_FILE_NAME`
alt MailTemplate found
Repo-->>Builder: MailTemplate (subject/body)
Builder->>Builder: render body via Twig (MailTemplate.getFileName())
else fallback
Builder->>Builder: render built-in template `@StockAlertMail/Mail/stock_alert.twig`
end
Controller->>Mailer: create Email(from,to,subject,body) and send()
Mailer-->>Controller: success / throws
Controller-->>Admin: redirect with success|failure flash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
Resource/template/admin/config.twig (1)
78-81: インラインイベントハンドラの改善を検討してください。
onclick属性内の JavaScript は動作しますが、翻訳文字列に引用符が含まれる場合に問題が発生する可能性があります。♻️ 改善案: data 属性とイベントリスナーを使用
- <button type="button" class="btn btn-outline-primary" - onclick="if(confirm('{{ 'stock_alert_mail.admin.config.send_test.confirm'|trans }}')){document.getElementById('form-send-test').submit();}"> + <button type="button" class="btn btn-outline-primary" + id="btn-send-test" + data-confirm="{{ 'stock_alert_mail.admin.config.send_test.confirm'|trans|e('html_attr') }}">そして
{% block javascript %}内で:document.getElementById('btn-send-test').addEventListener('click', function() { if (confirm(this.dataset.confirm)) { document.getElementById('form-send-test').submit(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resource/template/admin/config.twig` around lines 78 - 81, Replace the inline onclick handler on the button with a data-confirm attribute and a stable id (use id="btn-send-test") and keep the form id "form-send-test"; then add a JS event listener in the template's {% block javascript %} that reads dataset.confirm, calls confirm(...) and submits the form if true — this avoids quoting issues with translated strings and centralizes behavior in a safe event handler tied to btn-send-test and form-send-test.Controller/Admin/StockAlertConfigController.php (2)
93-101: メール送信ロジックの重複を検討してください。
StockAlertCommandと同様のメール構築・送信ロジックがあります。将来的にメール送信処理をサービスクラスに抽出することで、DRY 原則に沿った設計になります。現時点では許容範囲ですが、今後の拡張時に検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Controller/Admin/StockAlertConfigController.php` around lines 93 - 101, The controller duplicates the email construction/sending logic already present in StockAlertCommand; extract that logic into a reusable service (e.g., a NotificationMailer or StockAlertMailer) and have both StockAlertConfigController and StockAlertCommand call a single method like sendStockAlert(subject, body, fromAddress, toEmails) to build the Email, add recipients, and dispatch it; update StockAlertConfigController (the block that builds $message with Email, Address, ->text and foreach addTo) to call the new service instead, and refactor StockAlertCommand to use the same service so email behavior is centralized and DRY.
105-107:addSuccess/addErrorでの翻訳処理の一貫性を確認してください。Line 59 では
addSuccess('stock_alert_mail.admin.config.save_success', 'admin')と翻訳キーを直接渡していますが、Lines 105, 107 では$this->translator->trans()を事前に呼び出しています。EC-CUBE の
AbstractController::addSuccess/addErrorは内部で翻訳を行いますが、パラメータ付きの翻訳（%emails%など）の場合は事前にtrans()を呼ぶ必要があります。現在の実装は正しいですが、コメントがあると保守性が向上します。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Controller/Admin/StockAlertConfigController.php` around lines 105 - 107, The translation handling is inconsistent: when passing a translation key directly to addSuccess/addError (e.g. addSuccess('stock_alert_mail.admin.config.save_success', 'admin')) EC-CUBE will translate internally, but when the message needs runtime parameters you must call $this->translator->trans(...) beforehand (as done around $this->translator->trans('stock_alert_mail.admin.config.send_test.success', ['%emails%' => implode(', ', $toEmails)]) and the corresponding catch block). Add a short inline comment near the addSuccess/addError calls (and in the catch block) explaining this rule (use direct key for simple messages, pre-call $this->translator->trans when injecting parameters) so future maintainers know why trans() is used before addSuccess/addError; reference the methods addSuccess, addError and $this->translator->trans in the comment.PluginManager.php (1)
116-127: ファイル操作のエラーハンドリングが不足しています。
copy()の戻り値を確認せず、コピー失敗時も処理が続行します。プラグイン有効化時にテンプレートコピーが失敗すると、メール送信時にフォールバックテンプレートが使用されますが、これが意図通りか不明確です。また、
$sourcePathが存在しない場合に静かにリターンするのは、デバッグを困難にする可能性があります。♻️ エラーハンドリングの改善案
$sourcePath = __DIR__.'/Resource/template/Mail/stock_alert.twig'; if (!file_exists($sourcePath)) { - return; + throw new \RuntimeException('Source template not found: '.$sourcePath); } $targetDir = dirname($targetPath); if (!is_dir($targetDir)) { - mkdir($targetDir, 0755, true); + if (!mkdir($targetDir, 0755, true)) { + throw new \RuntimeException('Failed to create directory: '.$targetDir); + } } - copy($sourcePath, $targetPath); + if (!copy($sourcePath, $targetPath)) { + throw new \RuntimeException('Failed to copy template to: '.$targetPath); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PluginManager.php` around lines 116 - 127, sourcePath の存在チェックが無条件に静かに return し、また copy() の戻り値や mkdir の失敗を無視しているためファイル操作の失敗が見えなくなっています。PluginManager.php の該当ブロックで file_exists($sourcePath) が false の場合は単に return するのではなく processLogger（または例外）で明確にエラー/警告を出し、mkdir の戻り値と copy($sourcePath, $targetPath) の戻り値を確認して失敗時は適切にログ出力して処理を中断（または例外を投げる）するように修正してください。エラーメッセージには $sourcePath と $targetPath を含め、必要ならパーミッションや親ディレクトリ作成の失敗理由も含めて記録するようにしてください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Controller/Admin/StockAlertConfigController.php`:
- Around line 93-101: The controller duplicates the email construction/sending
logic already present in StockAlertCommand; extract that logic into a reusable
service (e.g., a NotificationMailer or StockAlertMailer) and have both
StockAlertConfigController and StockAlertCommand call a single method like
sendStockAlert(subject, body, fromAddress, toEmails) to build the Email, add
recipients, and dispatch it; update StockAlertConfigController (the block that
builds $message with Email, Address, ->text and foreach addTo) to call the new
service instead, and refactor StockAlertCommand to use the same service so email
behavior is centralized and DRY.
- Around line 105-107: The translation handling is inconsistent: when passing a
translation key directly to addSuccess/addError (e.g.
addSuccess('stock_alert_mail.admin.config.save_success', 'admin')) EC-CUBE will
translate internally, but when the message needs runtime parameters you must
call $this->translator->trans(...) beforehand (as done around
$this->translator->trans('stock_alert_mail.admin.config.send_test.success',
['%emails%' => implode(', ', $toEmails)]) and the corresponding catch block).
Add a short inline comment near the addSuccess/addError calls (and in the catch
block) explaining this rule (use direct key for simple messages, pre-call
$this->translator->trans when injecting parameters) so future maintainers know
why trans() is used before addSuccess/addError; reference the methods
addSuccess, addError and $this->translator->trans in the comment.
In `@PluginManager.php`:
- Around line 116-127: sourcePath の存在チェックが無条件に静かに return し、また copy() の戻り値や mkdir
の失敗を無視しているためファイル操作の失敗が見えなくなっています。PluginManager.php の該当ブロックで
file_exists($sourcePath) が false の場合は単に return するのではなく
processLogger（または例外）で明確にエラー/警告を出し、mkdir の戻り値と copy($sourcePath, $targetPath)
の戻り値を確認して失敗時は適切にログ出力して処理を中断（または例外を投げる）するように修正してください。エラーメッセージには $sourcePath と
$targetPath を含め、必要ならパーミッションや親ディレクトリ作成の失敗理由も含めて記録するようにしてください。
In `@Resource/template/admin/config.twig`:
- Around line 78-81: Replace the inline onclick handler on the button with a
data-confirm attribute and a stable id (use id="btn-send-test") and keep the
form id "form-send-test"; then add a JS event listener in the template's {%
block javascript %} that reads dataset.confirm, calls confirm(...) and submits
the form if true — this avoids quoting issues with translated strings and
centralizes behavior in a safe event handler tied to btn-send-test and
form-send-test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abf78334-2694-4bee-b2d5-cc2b48828aa5
📒 Files selected for processing (15)
Command/StockAlertCommand.phpController/Admin/StockAlertConfigController.phpController/Admin/StockAlertMailTemplateController.phpDoctrineMigrations/Version20260320000000.phpDoctrineMigrations/Version20260321000000.phpEntity/StockAlertConfig.phpForm/Type/Admin/StockAlertMailTemplateType.phpNav.phpPluginManager.phpResource/locale/messages.en.yamlResource/locale/messages.ja.yamlResource/template/admin/config.twigResource/template/admin/mail_template.twigService/StockAlertMailBuilder.phpTests/StockAlertCommandTest.php
💤 Files with no reviewable changes (6)
- Nav.php
- DoctrineMigrations/Version20260320000000.php
- Resource/template/admin/mail_template.twig
- Entity/StockAlertConfig.php
- Form/Type/Admin/StockAlertMailTemplateType.php
- Controller/Admin/StockAlertMailTemplateController.php
- StockAlertConfig: GeneratedValue を NONE に変更し id=1 固定
- findOneBy([]) を find(1) に統一（Command・Controller・PluginManager・Tests）
- メールテンプレート: 商品IDを表示し同一商品の規格をグルーピング
- コマンドクエリに orderBy('p.id') を追加（グルーピング前提のソート）
- 翻訳キー stock_alert_mail.mail.product_id を追加
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- StockAlertCommandTest: DQL DELETE後にentityManager->clear()を追加し、 テスト間でアイデンティティマップに残存するエンティティを除去する - StockAlertCommand: catch(\Exception)をcatch(\Throwable)に変更し、 PHPの\Error（TypeError等）も確実に捕捉する Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/PluginManagerTest.php (1)
90-97:⚠️ Potential issue | 🟠 Major新しい
MailTemplate/ Twig のライフサイクルをこのテスト群が素通りしています。
enable()/uninstall()は今やdtb_mail_template行とテーマ側Mail/stock_alert.twigも触りますが、ここでは設定やプラグインテーブルしか見ていません。さらにsetUp()/tearDown()がStockAlertConfigしか初期化していないため、前テストのテンプレート行・配置ファイルが残るとcreateMailTemplate()/deleteMailTemplate()が実際には何もしていなくても通ります。file_name = Mail/stock_alert.twigのMailTemplate行と配置先ファイルも各ケースで seed / assert / cleanup して、この PR の主経路を直接検証したいです。Also applies to: 121-144, 189-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/PluginManagerTest.php` around lines 90 - 97, テストが MailTemplate/Twig ライフサイクルを検証していないため、testEnable()（および同様の箇所：lines ~121-144, 189-211）で pluginManager->enable() / uninstall() の実動作を保証するために、setUp() で dtb_mail_template に file_name = 'Mail/stock_alert.twig' の行と実際の配備先テンプレートファイルをシードし、test 内で enable()/uninstall() 実行後に createMailTemplate()/deleteMailTemplate() の期待効果（テーブルの追加/削除とファイルの存在/不在）を明示的にアサートし、tearDown() でそのテンプレート行とファイルを確実にクリーンアップするように修正してください（StockAlertConfig 初期化だけでなく MailTemplate 行とファイルの seed/assert/cleanup を追加すること）。
🧹 Nitpick comments (2)
Tests/PluginManagerTest.php (1)
199-204:testUninstallThenReinstall()が旧スキーマを復元しています。
recreateTablesIfNeeded()でmail_subject/mail_bodyをまだ作っているので、PR で削除したカラムへの再依存をこの再インストールテストが見逃します。手組み DDL は現行スキーマに合わせるか、可能なら実マイグレーション相当の経路を使った方が安全です。♻️ 例
$table->addColumn('threshold', 'integer', ['default' => 5]); $table->addColumn('alert_emails', 'string', ['length' => 1000, 'notnull' => false]); - $table->addColumn('mail_subject', 'string', ['length' => 500, 'notnull' => false]); - $table->addColumn('mail_body', 'text', ['notnull' => false]); $table->addColumn('create_date', 'datetimetz'); $table->addColumn('update_date', 'datetimetz');Also applies to: 223-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/PluginManagerTest.php` around lines 199 - 204, The test testUninstallThenReinstall is accidentally recreating removed columns because recreateTablesIfNeeded still builds mail_subject/mail_body; update the test setup so recreateTablesIfNeeded aligns with the current schema (remove creation of those deleted columns) or, preferably, replace the manual DDL with running the real migration path (use the same migration runner the app uses) so resyncTransactionState($conn) runs against the up-to-date schema and the re-install sequence in enable()/flush() is validated against the current migrations.Resource/template/Mail/stock_alert.twig (1)
10-15: 商品グルーピングが入力順に依存しています。現状は
StockAlertCommandがp.idで並べているので成立しますが、このロジック自体は同一Product.idが連続している前提です。StockAlertMailBuilder::buildMailBody()は配列をそのまま渡すだけなので、別経路から未ソート配列が来ると同じ商品の見出しが分裂します。グルーピングはサービス側で確定してからテンプレートへ渡す方が保守しやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resource/template/Mail/stock_alert.twig` around lines 10 - 15, The template groups lowStockItems by relying on consecutive items with the same Product.id which is brittle; update the service before rendering (e.g. in StockAlertMailBuilder::buildMailBody()) to either sort the lowStockItems array by Product.id or, preferably, transform it into an explicit grouped structure (map keyed by Product.id with name and its items) and pass that to the template so the template no longer assumes input order; adjust callers such as StockAlertCommand to stop relying on ordering if you choose grouping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PluginManager.php`:
- Around line 76-83: The current flow can create a DB-only MailTemplate when
copyTwigTemplate() fails silently; change PluginManager so filesystem operations
are guaranteed before committing DB rows: make copyTwigTemplate() throw on
mkdir()/copy() failure (do not swallow errors) and/or run the template copy
before inserting/returning when checking $repository->findOneBy(['file_name' =>
self::MAIL_TEMPLATE_FILE_NAME]); after copying verify the target file (e.g.
Mail/stock_alert.twig) exists and throw an exception to abort registration if
not; apply the same fix to the other block referenced (lines ~118-127) so both
code paths validate file presence and propagate errors instead of leaving a
DB-only MailTemplate that breaks StockAlertMailBuilder::getFileName().
In `@Resource/template/Mail/stock_alert.twig`:
- Line 14: 現在のテンプレート embeds 全角括弧 around the translated product id so those
brackets appear even in English; update the template so the parentheses are
handled by the translation key or made locale‑neutral: move the brackets into
the translation resource key 'stock_alert_mail.mail.product_id' (so translators
can supply locale-appropriate surrounding punctuation and wording like "(Product
ID: ...)" vs "（商品ID：...）"), or change the template to use a locale‑neutral
delimiter (e.g. " - " or ":" placed outside the translation) and reference
item.Product.id via the same 'stock_alert_mail.mail.product_id' key; adjust the
corresponding translation entries to include the surrounding text accordingly.
---
Outside diff comments:
In `@Tests/PluginManagerTest.php`:
- Around line 90-97: テストが MailTemplate/Twig
ライフサイクルを検証していないため、testEnable()（および同様の箇所：lines ~121-144, 189-211）で
pluginManager->enable() / uninstall() の実動作を保証するために、setUp() で dtb_mail_template に
file_name = 'Mail/stock_alert.twig' の行と実際の配備先テンプレートファイルをシードし、test 内で
enable()/uninstall() 実行後に createMailTemplate()/deleteMailTemplate()
の期待効果（テーブルの追加/削除とファイルの存在/不在）を明示的にアサートし、tearDown()
でそのテンプレート行とファイルを確実にクリーンアップするように修正してください（StockAlertConfig 初期化だけでなく MailTemplate
行とファイルの seed/assert/cleanup を追加すること）。
---
Nitpick comments:
In `@Resource/template/Mail/stock_alert.twig`:
- Around line 10-15: The template groups lowStockItems by relying on consecutive
items with the same Product.id which is brittle; update the service before
rendering (e.g. in StockAlertMailBuilder::buildMailBody()) to either sort the
lowStockItems array by Product.id or, preferably, transform it into an explicit
grouped structure (map keyed by Product.id with name and its items) and pass
that to the template so the template no longer assumes input order; adjust
callers such as StockAlertCommand to stop relying on ordering if you choose
grouping.
In `@Tests/PluginManagerTest.php`:
- Around line 199-204: The test testUninstallThenReinstall is accidentally
recreating removed columns because recreateTablesIfNeeded still builds
mail_subject/mail_body; update the test setup so recreateTablesIfNeeded aligns
with the current schema (remove creation of those deleted columns) or,
preferably, replace the manual DDL with running the real migration path (use the
same migration runner the app uses) so resyncTransactionState($conn) runs
against the up-to-date schema and the re-install sequence in enable()/flush() is
validated against the current migrations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 007b009d-86d6-461e-b62e-44f0dd05f610
📒 Files selected for processing (9)
Command/StockAlertCommand.phpController/Admin/StockAlertConfigController.phpEntity/StockAlertConfig.phpPluginManager.phpResource/locale/messages.en.yamlResource/locale/messages.ja.yamlResource/template/Mail/stock_alert.twigTests/PluginManagerTest.phpTests/StockAlertCommandTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
- Command/StockAlertCommand.php
- Entity/StockAlertConfig.php
- Resource/locale/messages.en.yaml
- Controller/Admin/StockAlertConfigController.php
- Resource/locale/messages.ja.yaml
- Tests/StockAlertCommandTest.php
SMTP接続失敗によるテスト失敗を解消。MailerAssertionsTraitは nullトランスポートでも正常に動作する。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
142-150:MAILER_DSNをセットアップ工程でも統一してください。Line 165 は
null://nullになっていますが、Line 149 が SMTP のままです。MailHog サービスなし構成なので、セットアップ中にメール送信が発生した場合の不安定要因になります。Setup database schema側も同じ DSN に揃えるのが安全です。差分案
- MAILER_DSN: 'smtp://127.0.0.1:1025' + MAILER_DSN: 'null://null'Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 142 - 150, In the "Setup database schema" GitHub Actions step, the MAILER_DSN environment variable is still set to the SMTP DSN while a later step uses null://null; change the MAILER_DSN in that step's env block to null://null so the setup run uses the same no-mail transport as the later step (update the MAILER_DSN entry in the step named "Setup database schema" to match the null://null value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 142-150: In the "Setup database schema" GitHub Actions step, the
MAILER_DSN environment variable is still set to the SMTP DSN while a later step
uses null://null; change the MAILER_DSN in that step's env block to null://null
so the setup run uses the same no-mail transport as the later step (update the
MAILER_DSN entry in the step named "Setup database schema" to match the
null://null value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc596466-9aef-44d3-84c3-3c81bda22f8f
📒 Files selected for processing (2)
.github/workflows/ci.ymlTests/StockAlertCommandTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/StockAlertCommandTest.php
CIではeccube:plugin:enableでコピーされるテーマ側テンプレート (app/template/default/Mail/)が存在しない場合があるため、 テストではMailTemplateレコードを削除して@StockAlertMailネームスペースの テンプレートにフォールバックさせる。 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Tests/StockAlertCommandTest.php (2)
86-88:find(1)固定はテストの将来的な不安定要因になりえます。setUpで作成した
StockAlertConfigの実IDを保持して参照すると、DBシーケンス状態に依存せず堅牢です。💡 例: setUpで作成したIDを使う差分
class StockAlertCommandTest extends EccubeTestCase { use MailerAssertionsTrait; @@ /** `@var` CommandTester */ private $commandTester; + /** `@var` int */ + private $configId; @@ $this->entityManager->persist($config); $this->entityManager->flush(); + $this->configId = $config->getId(); @@ - $config = $this->configRepository->find(1); + $config = $this->configRepository->find($this->configId); $config->setThreshold(-1); @@ - $config = $this->configRepository->find(1); + $config = $this->configRepository->find($this->configId); $config->setThreshold(-1); @@ - $config = $this->configRepository->find(1); + $config = $this->configRepository->find($this->configId); $config->setThreshold(-1);Also applies to: 128-130, 159-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/StockAlertCommandTest.php` around lines 86 - 88, The tests use a hardcoded find(1) which makes them brittle; instead capture and store the real ID of the StockAlertConfig created in setUp (e.g. save the entity's getId() into a property like $this->configId) and update all usages of $this->configRepository->find(1) (including the occurrences around the other blocks referenced) to $this->configRepository->find($this->configId) so the tests do not depend on DB sequence values and will reliably reference the config created in setUp.
198-200: 本文アサート前に null チェックを追加してください。
getTextBody()が null の場合に、失敗理由をより明確にできます。💡 例: null 明示チェックを追加
$body = $message->getTextBody(); + $this->assertNotNull($body, 'Text body should not be null.'); $this->assertStringContainsString('管理者様', $body); $this->assertStringContainsString('在庫', $body);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/StockAlertCommandTest.php` around lines 198 - 200, Add an explicit null check for the email body before asserting its contents: after assigning $body = $message->getTextBody() in Tests/StockAlertCommandTest.php, assertNotNull($body, 'Expected text body to be present') (or an equivalent null check) and return/fail early if null so the subsequent assertions on $body and the calls to assertStringContainsString('管理者様', $body) and assertStringContainsString('在庫', $body) don’t throw unclear errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Tests/StockAlertCommandTest.php`:
- Around line 86-88: The tests use a hardcoded find(1) which makes them brittle;
instead capture and store the real ID of the StockAlertConfig created in setUp
(e.g. save the entity's getId() into a property like $this->configId) and update
all usages of $this->configRepository->find(1) (including the occurrences around
the other blocks referenced) to $this->configRepository->find($this->configId)
so the tests do not depend on DB sequence values and will reliably reference the
config created in setUp.
- Around line 198-200: Add an explicit null check for the email body before
asserting its contents: after assigning $body = $message->getTextBody() in
Tests/StockAlertCommandTest.php, assertNotNull($body, 'Expected text body to be
present') (or an equivalent null check) and return/fail early if null so the
subsequent assertions on $body and the calls to
assertStringContainsString('管理者様', $body) and assertStringContainsString('在庫',
$body) don’t throw unclear errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b63e15aa-91f6-45ef-b5f8-4aa8e066f54b
📒 Files selected for processing (1)
Tests/StockAlertCommandTest.php
- PluginManager: copyTwigTemplateのソース欠落・mkdir・copy失敗を 例外化し、DB登録前にファイル存在を保証 - StockAlertMailBuilder: MailTemplateのファイルが見つからない場合に @StockAlertMailネームスペースのテンプレートへフォールバック Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
PluginManager.php (1)
76-83:⚠️ Potential issue | 🟠 Majorテンプレート欠落時の自己修復がまだ効きません。
Line 77 の早期 return により、
dtb_mail_templateレコードだけ残って Twig ファイルが消えているケースでcopyTwigTemplate()が実行されません。ファイル存在保証を先に行ってから DB 存在チェックする順序にしてください。🔧 修正案
- // 既に登録済みの場合はスキップ - if ($repository->findOneBy(['file_name' => self::MAIL_TEMPLATE_FILE_NAME]) !== null) { - return; - } - - // プラグインのデフォルトテンプレートを app/template/default/Mail/ にコピー - $this->copyTwigTemplate($container); + // まずテンプレート実体を保証（欠落時の自己修復） + $this->copyTwigTemplate($container); + + // 既に登録済みの場合はDB登録のみスキップ + if ($repository->findOneBy(['file_name' => self::MAIL_TEMPLATE_FILE_NAME]) !== null) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PluginManager.php` around lines 76 - 83, The early return after repository->findOneBy(['file_name' => self::MAIL_TEMPLATE_FILE_NAME]) prevents copyTwigTemplate($container) from running when the DB row exists but the Twig file is missing; move the file-existence/self-heal step before the DB existence check (i.e., call copyTwigTemplate($container) or a new ensureTwigTemplatesExist() helper first), then perform the repository->findOneBy(...) check and return only after templates are guaranteed present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Service/StockAlertMailBuilder.php`:
- Around line 66-70: The subject fallback currently only checks for null on
$mailTemplate and can still produce an empty subject when getMailSubject()
returns an empty string or whitespace; update the logic in
Service\StockAlertMailBuilder (the $subjectSuffix assignment that reads
$mailTemplate->getMailSubject()) to treat null or empty/whitespace-only values
as missing by using trim() or an empty() check and fall back to the default
'在庫アラート通知', then build $subject as before using $this->BaseInfo->getShopName().
---
Duplicate comments:
In `@PluginManager.php`:
- Around line 76-83: The early return after repository->findOneBy(['file_name'
=> self::MAIL_TEMPLATE_FILE_NAME]) prevents copyTwigTemplate($container) from
running when the DB row exists but the Twig file is missing; move the
file-existence/self-heal step before the DB existence check (i.e., call
copyTwigTemplate($container) or a new ensureTwigTemplatesExist() helper first),
then perform the repository->findOneBy(...) check and return only after
templates are guaranteed present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8894730f-5442-418c-bb8e-9be09ca4430d
📒 Files selected for processing (2)
PluginManager.phpService/StockAlertMailBuilder.php
- 空文字/空白のみの件名もデフォルト件名にフォールバック - テンプレートの全角括弧を翻訳キーに移動し多言語対応 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DB シーケンス値に依存しないよう $this->configId を使用 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ファイルコピーをDB存在チェックの前に移動し、DB行だけ残る半壊状態を修復できるようにした Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- testMailSubjectFallbackWhenEmpty: 空白のみ件名がデフォルトにフォールバックされること - testEnableRestoresMissingTwigTemplate: DB行あり・Twigファイル欠損時の自己修復 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merged
3 tasks
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.
Summary
enable()時にdtb_mail_templateレコード作成 + twigファイルをテーマディレクトリへコピー。uninstall()時にDBレコードとtwigファイルを削除/admin/setting/shop/mail）で件名・本文を編集できるように移行MailTemplateRepository経由で件名・本文を取得、テンプレート未設定時はプラグイン内蔵の@StockAlertMail/Mail/stock_alert.twigにfallbackmail_subject/mail_bodyカラムを削除。マイグレーションVersion20260321000000で既存DBから DROP COLUMNStockAlertMailTemplateController,StockAlertMailTemplateType,mail_template.twig（カスタム編集画面）Test plan
dtb_mail_templateにレコードが作成されることapp/template/default/Mail/stock_alert.twigがコピーされることbin/phpunit app/Plugin/StockAlertMail/がパスすること🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
変更
削除