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

受注編集画面にポイント付与率を追加 #5571

Merged
merged 9 commits into from Sep 14, 2022

Conversation

nobuhiko
Copy link
Contributor

@nobuhiko nobuhiko commented Aug 19, 2022

概要(Overview・Refs Issue)

#5529 へ管理画面側への対応

方針(Policy)

ポイント付与率が一律でbaseinfoに依存しているのでその点の対応

実装に関する補足(Appendix)

テスト(Test)

相談(Discussion)

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

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

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

@chihiro-adachi chihiro-adachi added this to the 4.2.0 milestone Aug 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

Merging #5571 (bb75a2e) into 4.2 (0c30664) will increase coverage by 0.20%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##                4.2    #5571      +/-   ##
============================================
+ Coverage     78.75%   78.95%   +0.20%     
- Complexity     6251     6281      +30     
============================================
  Files           468      469       +1     
  Lines         21010    21092      +82     
============================================
+ Hits          16547    16654     +107     
+ Misses         4463     4438      -25     
Flag Coverage Δ
E2E 64.87% <90.00%> (+0.52%) ⬆️
Unit 77.74% <90.00%> (+0.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Eccube/Service/PointHelper.php 97.91% <83.33%> (-2.09%) ⬇️
src/Eccube/Form/Type/Admin/OrderItemType.php 89.16% <100.00%> (+0.27%) ⬆️
...rvice/PurchaseFlow/Processor/AddPointProcessor.php 96.77% <100.00%> (ø)
...hentication/EccubeAuthenticationFailureHandler.php 57.14% <0.00%> (-28.58%) ⬇️
...hentication/EccubeAuthenticationSuccessHandler.php 57.14% <0.00%> (-28.58%) ⬇️
src/Eccube/EventListener/SecurityListener.php 84.00% <0.00%> (-12.00%) ⬇️
src/Eccube/Log/Processor/TokenProcessor.php 90.00% <0.00%> (-10.00%) ⬇️
src/Eccube/Service/MailService.php 82.26% <0.00%> (-8.08%) ⬇️
src/Eccube/Twig/Template.php 50.00% <0.00%> (-5.56%) ⬇️
src/Eccube/Request/Context.php 96.15% <0.00%> (-3.85%) ⬇️
... and 62 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@carkn carkn added the bugbounty2022:pr:entry バグバウンティ2022 バグ登録 PR登録 label Aug 23, 2022
@nanasess
Copy link
Contributor

nanasess commented Sep 9, 2022

@nobuhiko @chihiro-adachi

  1. 出荷管理画面に遷移
  2. 登録ボタンをクリック
  3. 受注編集画面に戻る
    上記の操作でポイント付与率が空欄になってしまうようなので修正入れました

@nanasess
Copy link
Contributor

nanasess commented Sep 9, 2022

発送済みにしてから、ポイント付与率を変更しても会員に付与されるポイントを変更することはできないので、ポイント付与率のフォームは readonly にしてしまった方がよいかもしれません。

@nanasess
Copy link
Contributor

ポイント付与率のフォームは readonly にしてしまった方がよいかもしれません。

明細ごとにポイント付与率を変更されると、利用ポイントの控除分(200ポイント利用すると、加算ポイント -2 になる仕様)に不整合が発生する可能性が高いため、 hidden で保持するよう修正入れたいと思います

明細ごとにポイント付与率を変更されると、利用ポイントの控除分(200ポイント利用すると、加算ポイント -2 になる仕様)に不整合が発生する可能性が高いため、 hidden で保持するよう修正
- 商品明細が取得できる場合は, 商品明細に設定されたポイント付与率を設定
- 商品明細が取得できない場合は, 店舗基本情報のポイント付与率を設定
@nanasess
Copy link
Contributor

利用ポイントの控除分(200ポイント利用すると、加算ポイント -2 になる仕様)のポイント付与率は、ポイント明細の付与率が使用されるため、商品明細のポイント付与率を設定するよう修正しました

@chihiro-adachi
Copy link
Contributor

chihiro-adachi commented Sep 12, 2022

@nanasess
確認しました。以下point_rateがnullで登録されるパターンがあるようです。

  1. 新規明細追加時に、point_rateがnullで登録される
  • 商品を追加をクリック
  • 任意の商品を追加
  • 受注を登録
  • 追加した明細のpoint_rateがnullで登録される
  1. ポイント明細のpoint_rateがにnullで登録される(※おそらく1に起因すると思われます)
  • 利用ポイントに任意のポイントを登録
  • ポイント明細が追加される
  • 受注を登録
  • その後、何も更新せず受注を登録すると、ポイント明細のpoint_rateがnullで登録される

// 商品別ポイントは未実装なので, 商品明細のポイント付与率はすべて同じ値が設定されているはず
$ProductOrderItem = $itemHolder->getItems()->getProductClasses()->current();
if ($ProductOrderItem instanceof OrderItem) {
$pointRate = $ProductOrderItem->getPointRate();
Copy link
Contributor

Choose a reason for hiding this comment

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

@nanasess
#5571 (comment) の通り、商品明細のポイント付与率がnullの場合もありうるので、商品明細の付与率 => 基本情報の付与率の順に判定したほうがよいかもしれません。

Copy link
Contributor

Choose a reason for hiding this comment

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

$ProductOrderItem instanceof OrderItem && $ProductOrderItem->getPointRate() !== null という条件にしてみました

@nanasess
Copy link
Contributor

@chihiro-adachi point_rate が null の場合は店舗基本情報のポイント付与率が使われるようになっているかなと

@nanasess
Copy link
Contributor

@chihiro-adachi
商品明細のポイント付与率が null になってしまうと、リアルタイムに店舗基本情報のポイント付与率を参照してしまうため、管理画面から商品明細を追加した場合にもポイント付与率を設定するよう修正しました

@chihiro-adachi
Copy link
Contributor

chihiro-adachi commented Sep 13, 2022

@nanasess

  • 基本情報のポイント付与率を1に設定
  • ポイントを利用した受注を作成
  • 基本情報のポイント付与率を10に設定
  • 商品明細を追加
  • 受注を更新

で、ポイント明細の付与率が10で設定されるようです。
既存の商品明細ではなく、追加した商品明細の付与率(10)が反映されてしまうようす。

@nanasess
Copy link
Contributor

@chihiro-adachi このケースの場合、新しく追加した商品明細の付与率は 1 なのが正しい感じですよね🤔

@chihiro-adachi
Copy link
Contributor

@nanasess
ご対応ありがとうございます。こちらについては解消されました!
#5571 (comment)

値引き明細の新規追加時に、付与率がnullで登録されるようです。
値引き明細も控除の計算が行われるため、同様の対応いれていただくことは可能でしょうか?

@nanasess
Copy link
Contributor

@chihiro-adachi 対応しました

@chihiro-adachi
Copy link
Contributor

@nanasess
ご対応ありがとうございます!修正確認できました。

@chihiro-adachi chihiro-adachi merged commit 48d13e8 into EC-CUBE:4.2 Sep 14, 2022
nanasess added a commit to nanasess/ec-cube that referenced this pull request Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:Middle bugbounty2022:pr:entry バグバウンティ2022 バグ登録 PR登録
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants