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

管理画面の検索機能をDateTimeTypeに対応 #4619

Merged
merged 8 commits into from
Jul 21, 2020

Conversation

okazy
Copy link
Contributor

@okazy okazy commented Jul 8, 2020

概要(Overview・Refs Issue)

管理画面の検索機能をDateTimeTypeに対応

互換性を確保するため yyyy-MM-ddyyyy-MM-dd HH:mm:ss のどちらのフォーマットでも検索可能なようにしました。

close #4618

Chrome

image

Safari

image

FireFox

image

方針(Policy)

  • カスタマイズさてていることを考慮して既存の DateType は残しつつ DateTimeType を新たに追加
  • ブラウザ間の差異をなくすためにUIは datetimepicker に統一
  • 実装はできるだけ今のものを利用できるように配慮

実装に関する補足(Appendix)

  • カレンダーのUIはブラウザごとにHTML5と datetimepicker が混在していたのを統一した
    • シンプルで使いやすいHTML5にしたかったが対応ブラウザが限られてしまうので諦めた
  • datetimepicker で検索条件が消えてしまう課題にも対応
  • ShippingRepository::getQueryBuilderBySearchDataForAdmin() は利用されていなかったので @ deprecated とした
  • DateType と DateTimeType のどちらも指定されていた場合には DateTimeType を優先する

テスト(Test)

UnitTestを追加

相談(Discussion)

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

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

レビュワー確認項目

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

@okazy okazy added the enhancement 機能追加 label Jul 8, 2020
@okazy okazy added this to the 4.0.x milestone Jul 8, 2020
@okazy okazy force-pushed the feat/datetime-search branch from ca7776e to 47004df Compare July 9, 2020 02:36
@okazy okazy changed the title [WIP] 管理画面の検索機能をDateTimeTypeに対応 管理画面の検索機能をDateTimeTypeに対応 Jul 9, 2020
@okazy
Copy link
Contributor Author

okazy commented Jul 9, 2020

2点修正が必要とのご意見をいただきました。

  • 時分の補完時にDateTimeTypeかどうかのチェック(Formがカスタマイズされていた場合の考慮)
  • datetimepicker の初期値の設定

@okazy
Copy link
Contributor Author

okazy commented Jul 9, 2020

クロスブラウザで日時入力UIの調整は地獄でした。

@okazy okazy force-pushed the feat/datetime-search branch from eee0f5b to ab82ebc Compare July 9, 2020 11:13
@k-yamamura
Copy link
Contributor

各フォームタイプにある、

'class' => 'datetimepicker-input',

も不要になったので削除しておいてください。

@okazy
Copy link
Contributor Author

okazy commented Jul 10, 2020

@k-yamamura
指摘ありがとうございます!

いただいた指摘を試してみたところ、入力フォームのフォーカスを解いても明示的に閉じてやらないとカレンダーが消えなくなってしまいました。

image

原因の特定まではできなかったのですが、js で datetimepicker の対象のフォームを特定するのにも利用していますので、そのまま残す方向でどうでしょうか。

https://github.com/EC-CUBE/ec-cube/pull/4619/files#diff-81ef87e3d0e00a02b177bccd6fa3e517R47

@okazy okazy added affected:admin_template 管理画面テンプレートのDOMに影響のある変更 affected:外部仕様 外部仕様の変更や追加 labels Jul 10, 2020
@okazy
Copy link
Contributor Author

okazy commented Jul 10, 2020

カレンダー入力のライブラリ自体を変更するのはどうかという提案をいただきました。
ライブラリは以下
https://flatpickr.js.org/

@okazy
Copy link
Contributor Author

okazy commented Jul 10, 2020

カレンダー入力ライブラリの変更は影響範囲がこちらのプルリクの内容と異なりますので別でIssueを作成しました。
#4622

Copy link
Contributor

@kiy0taka kiy0taka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

検索条件に登録日(終了)2018/09/28 19:13 を指定すると、登録日が 2018/09/28 19:14 の商品が検索されてしまいます。

image

@okazy
Copy link
Contributor Author

okazy commented Jul 10, 2020

ありがとうございます!確認します。

@okazy
Copy link
Contributor Author

okazy commented Jul 10, 2020

Repository を変更する方針で考えています。

            $date = clone $searchData['order_date_end'];
            $date = $date
                ->modify('+1 days');

            $date = $searchData['order_date_end'];

ShippingRepository->getQueryBuilderBySearchDataForAdmin() がどこにも使われていない気がするので対応考えます。

@okazy
Copy link
Contributor Author

okazy commented Jul 13, 2020

#4619 (comment)

こちらの修正方法にしようかと思っていたのですが、Repositoryの挙動が変わるのはさすがにまずいかと思いました。
冗長にはなってしまいますが、新しいFormを追加する対応がいいかと思いましたのでそちらで実装して見ます。

@okazy
Copy link
Contributor Author

okazy commented Jul 13, 2020

指摘いただいたこと

  • 検索条件で引っかからないパターン or 2重で引っかかることはないか確認( <=< など)
  • 今は分までとなっているが、秒まで指定したい

@okazy
Copy link
Contributor Author

okazy commented Jul 13, 2020

検索条件で引っかからないパターン or 2重で引っかかることはないか確認( <= と < など)

開始は含み: >=
終了は含まない: <

ですので漏れなくだぶりなく検索可能です。

@okazy okazy force-pushed the feat/datetime-search branch from 26c08ef to 4459932 Compare July 14, 2020 00:24
@okazy
Copy link
Contributor Author

okazy commented Jul 14, 2020

秒まで指定できるようにしました。
管理画面からも秒まで指定できるようにしています。
補完などは用意せず一旦シンプルな作りにしています。

@kiy0taka kiy0taka removed the affected:外部仕様 外部仕様の変更や追加 label Jul 17, 2020
@kiy0taka kiy0taka merged commit 75f2e1b into EC-CUBE:4.0 Jul 21, 2020
@okazy okazy modified the milestones: 4.0.x, 4.0.5 Jul 30, 2020
@okazy okazy mentioned this pull request Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected:admin_template 管理画面テンプレートのDOMに影響のある変更 enhancement 機能追加 Status: ready-for-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

管理画面の受注一覧の検索条件で注文日の時刻まで指定したい
3 participants