Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

フリガナ欄のオプション化 #4642

Open
wants to merge 8 commits into
base: 4.3
Choose a base branch
from

Conversation

eternalharvest
Copy link
Contributor

概要(Overview・Refs Issue)

#3872 で議論で議題に上がっているように、会員登録や受注編集等の際にカナの入力を強制することは、カゴ落ちの原因になったり、エントリ業務の効率化の妨げになるのでカナ入力を強制しないよう設定変更できるようにしました。

また、これに関連して #4636 で報告されているバグをついでに修正しています。
フリガナ欄のオプション化に際しては互換性を損なわないよう最善の注意を払っていますが、新しい機能をバグ修正パッチと一緒に取り込むことに問題がある場合は、バグ修正コミットを別に分けようと思いますのでご連絡ください。あるいは、チェリーピックしていただいても構いません。

方針(Policy)

互換性を損なわないよう、店舗設定の基本情報で設定できるようにしデフォルトはこれまで通りフリガナの入力を強制します。
オプションの設定内容に関わらず、カナ項目が必須であることを示すバッジ以外の要素が物理的になくなったりすることはありません。

実装に関する補足(Appendix)

テスト(Test)

私の書いたコードの範囲はほぼ UI に関わる部分なのでテストコードは書いていませんが、考えうるコードパスはほぼ全て手動でテストして正しいことを確認しています。客観的に証明出来なくてすいません。

相談(Discussion)

フリガナ項目の非表示化の是非

今回はオプション化のみの実装に留めていますが、そもそもオプションにするくらいならフリガナの項目事態を UI 上からは消し去ってしまったほうが良いのではないか?と悩みましたが、それ以上は本体でやるというよりカスタマイズでやるべき内容なのかなとも思っています。皆さんの意見が聞きたいです。

フォローアップ

この PR を作成したことで、いろんなフォームのコードに目を通すことができました。各フォームで結構、仕様に統一感がなかったりしたのでそういった点はおいおい改善していければ良いなと思っています。フォームはいろんなところに影響が及びそうなので互換性には気をつけないけませんが。

特に、非会員での購入時のショッピングページでお客様情報を編集するフォームだけが AJAX になっていて、他とは異質な存在になっているように思います。もちろん、AJAX ならではのメリットもあるのですが下記 Issue でも指摘されているような問題を抱えています。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更
  • フックポイントの呼び出しタイミングの変更
  • フックポイントのパラメータの削除・データ型の変更
  • twigファイルに渡しているパラメータの削除・データ型の変更
  • Serviceクラスの公開関数の、引数の削除・データ型の変更
  • 入出力ファイル(CSVなど)のフォーマット変更

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

@nanasess
Copy link
Contributor

早速のPRありがとうございます!
UIのテストに関しては、 Codeception で自動化していますので、お時間ありましたらぜひ!

まとまった公式ドキュメントがないのですが、こちらが参考になると思います
https://qiita.com/kazumiiiiiiiiiii/items/612b541d1dffcf026098

@kiy0taka kiy0taka added this to the 4.1 milestone Aug 11, 2020
@eternalharvest
Copy link
Contributor Author

@nanasess
情報ありがとうございます。
時間があるときに、Codeception のテスト書いてみます。

既存のテストコードを編集するより、別ファイルで作った方が良いでしょうか?
どうやらテストはナンバリングされているようですが、ナンバリングの規則がよくわからないので好きな名前でテスト書いて別ファイルにした方が良いかなと思っています。

@nanasess
Copy link
Contributor

@eternalharvest ありがとうございます。一応、機能ごとにPHPファイルがわかれていますので、別ファイルにせず、今回修正した機能ごとにテストを追加していただければ嬉しいです。
ナンバリングは、以下の結合試験項目書に対応しています
https://github.com/EC-CUBE/eccube3-doc/tree/4.0/IntegrationTest

また、既存の PHPUnit が落ちているようですので、そちらも合わせて修正していただけると大変助かります。

@eternalharvest
Copy link
Contributor Author

@nanasess

phpunit の件は修正させていただきました。
まだ、codeception がコケていますが。

もともと、コードを書き換える段階で問い合わせフォームだけなぜかカナフィールドが必須ではないのが気になっていたのですが、他と統一した方が良いのではと思い勝手に仕様を変更していました。

なぜ、問い合わせフォームだけカナがもともと必須フィールドでなかったのかについては理由はわからないですが、設計者の意図があるようにも感じます。問い合わせフォームのテストでカナが必須ではないことを検証するようになっているので、ここの仕様は変えるべきではないと判断しました。私はカナフィールドいらない派なので、問い合わせフォームがカナを必須にできないことは私にとっては問題ではありません。

みなさんの意見を聞いて必要であれば、仕様の変更を検討したいと思います。

@nanasess
Copy link
Contributor

@eternalharvest ご対応ありがとうございます!お問い合わせフォームは、必要最小限の入力でお問い合わせできるようにという配慮から、必須が外れているようです

@eternalharvest eternalharvest force-pushed the feature/support_option_require_kana branch 2 times, most recently from d6d3c59 to a078f8b Compare August 15, 2020 00:17
@okazy okazy added the enhancement 機能追加 label Aug 20, 2020
@kiy0taka kiy0taka changed the base branch from 4.0 to 4.1-feature March 2, 2021 06:05
@kiy0taka kiy0taka self-assigned this Mar 2, 2021
@kiy0taka kiy0taka force-pushed the feature/support_option_require_kana branch from a078f8b to 72c3934 Compare March 2, 2021 06:19
@kiy0taka kiy0taka force-pushed the feature/support_option_require_kana branch from 72c3934 to a137073 Compare March 2, 2021 06:32
@chihiro-adachi chihiro-adachi modified the milestones: 4.1, 4.1.x Sep 3, 2021
@chihiro-adachi chihiro-adachi changed the base branch from 4.1-feature to 4.1 September 5, 2021 23:45
@chihiro-adachi chihiro-adachi modified the milestones: 4.1.x, 4.x Apr 19, 2022
@shinya shinya changed the base branch from 4.1 to 4.3 April 17, 2024 07:29
@shinya
Copy link
Contributor

shinya commented Apr 17, 2024

ソースコードを現最新Versionに更新した後、Conflictを解消しマージするように動きます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants