登録メール送信時の警告を修正 #982

clicktx commented 2 months ago

fixed #982

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 56.29%. Comparing base (66445b3) to head (d989c3d). Report is 9 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #989 +/- ## ========================================== + Coverage 55.63% 56.29% +0.66% ========================================== Files 75 75 Lines 8917 8917 ========================================== + Hits 4961 5020 +59 + Misses 3956 3897 -59 ``` | [Flag](https://app.codecov.io/gh/EC-CUBE/ec-cube2/pull/989/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EC-CUBE) | Coverage Δ | | |---|---|---| | [tests](https://app.codecov.io/gh/EC-CUBE/ec-cube2/pull/989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EC-CUBE) | `56.29% <100.00%> (+0.66%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=EC-CUBE#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

clicktx commented 2 months ago

LC_Page_Admin_xxxクラスにメール送信処理が散らばっていて、テストしにくかったので後回しにしたのですが、影響ありましたね。。。

とりあえずテンプレートを戻すだけにしても良いかも知れませんが、回避案のようにSC_Helper_Mail::sfSendRegistMail() にまとめた方がスマートですね

clicktx commented 2 months ago

どっちに書くべきか迷ったのですが、こちらに。

        if ($arrCustomerData['status'] == 1
             && (CUSTOMER_CONFIRM_MAIL == true || $resend_flg == true)
         ) {...

PR頂いて、条件式のリファクタリング部分を見ていて思ったのですが、 $arrCustomerData['status'] == 1 = 「仮会員」の状態の時は「会員登録のご確認」メールを出すはずですよね？

条件を満たさないと「会員登録のご完了」メールが送信されるようになっている、と。そうなると実は && (CUSTOMER_CONFIRM_MAIL == true || $resend_flg == true) も不要なのではないかと思いました。

CUSTOMER_CONFIRM_MAIL == trueじゃないと仮会員にならないはず
$resend_flg はそもそも不要なのでは説

https://github.com/EC-CUBE/ec-cube2/blob/fc5a36a25f2fff5179670351d65f890228b86e13/data/class/pages/entry/LC_Page_Entry.php#L194-L195

https://github.com/EC-CUBE/ec-cube2/blob/fc5a36a25f2fff5179670351d65f890228b86e13/data/class/pages/regist/LC_Page_Regist.php#L71-L72

$arrCustomerData['status'] == 1の状態の会員に「会員登録のご完了」メールを送る状況ってあるのでしょうか？？

seasoftjapan commented 2 months ago

@clicktx 今回は、あくまでリファクタリングの範囲に留めて処理内容は変更していませんが、私も漠然と違和感はありました。

もしかすると、CUSTOMER_CONFIRM_MAIL を途中変更した場合の考慮でしょうか。(実際に目的を達成しているか未検証です。)

第1引数・第2引数の被りも違和感ありますし、第3引数も今となっては不要なはずですし、色々と改善の余地はありそうに思います。本 Issue でどこまで扱うか難しいところですが。

nanasess commented 2 months ago

@seasoftjapan @clicktx 単に考慮できていなかっただけのように見えますね。別の issues で対応しましょうか。

clicktx commented 2 months ago

@seasoftjapan @clicktx 単に考慮できていなかっただけのように見えますね。別の issues で対応しましょうか。

issueたてました https://github.com/EC-CUBE/ec-cube2/issues/1000#issue-2511357656

nanasess commented 2 months ago

古いフォーマットのメールテンプレートの互換テストが欲しいので別途 issues 登録しますね

EC-CUBE / ec-cube2

登録メール送信時の警告を修正 #982 #989

Codecov Report