WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@systemfriend-tsuji
Copy link
Contributor

purchaseFlowで呼び出すメソッドがflowTypeごとにソートされていなかったため、flowTypeごとにソートされるように調整しました。

概要(Overview・Refs Issue)

#6257 の修正を行いました。

方針(Policy)

実装に関する補足(Appendix)

flow_typeごとにpriority順にソートを行うため
いったん情報を配列に取り込み、あとから配列をソートするという手順を踏んでいます。

indexを追加しているのはpriorityが同等のものの場合に
「findAndSortTaggedServices」で同等のときの処理が行われているのを引き継ぐためです。

テスト(Test)

  1. プラグインで以下を用いた場合に最後尾に追加されること
    @Cartflow
    @ShoppingFlow
    @orderflow
  2. 異なるプラグインでpriorityが同じプロセッサーを追加し、修正前後で同じ結果が得られること
    ※ 今回の影響範囲外の部分でのチェック
    ※ インストール順ではなくプラグインのフォルダ名称順になっている
  3. flow_type:shoppingにてAddPointProcessorとPaymentValidatorの間に入る850のpriorityを指定して意図通りに入ること
  4. priorityの入力がなくてもシステムエラーにならず、priorityは0で判定されること
  5. 存在しないfolow_typeが設定されていてもシステムエラーにならず、PurchaseFlowに反映もされないこと

※ PurchaseFlowに意図通りに入るかどうかはPurchaseFlow::dumpを出力して確かめております。

相談(Discussion)

上記、テストで確かめている
「異なるプラグインでpriorityが同じプロセッサーを追加し、修正前後で同じ結果が得られること」について
正しい挙動がわかっていません。

以下で正しいようでしたら、問題なしと判断できますが
異なるようでしたら、ご教示いただけるでしょうか。
「インストール順ではなくプラグインのフォルダ名称順になっている」

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

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

レビュワー確認項目

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

purchaseFlowで呼び出すメソッドがflowTypeごとにソートされていなかったため、flowTypeごとにソートされるように調整
@codecov
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.71%. Comparing base (c972044) to head (4a7d085).
Report is 48 commits behind head on 4.3.

Additional details and impacted files
@@           Coverage Diff           @@
##              4.3    #6258   +/-   ##
=======================================
  Coverage   82.70%   82.71%           
=======================================
  Files         480      480           
  Lines       26456    26465    +9     
=======================================
+ Hits        21881    21890    +9     
  Misses       4575     4575           
Flag Coverage Δ
E2E 82.71% <100.00%> (+<0.01%) ⬆️
Unit 82.71% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dotani1111 dotani1111 added this to the 4.3.x milestone Jul 31, 2024
@dotani1111
Copy link
Contributor

dotani1111 commented Jul 31, 2024

@dotani1111
PRありがとうございます!
修正方針は問題ないと思います。

「異なるプラグインでpriorityが同じプロセッサーを追加し、修正前後で同じ結果が得られること」

こちら確認したいと思います。

@dotani1111 dotani1111 self-requested a review August 5, 2024 05:20
@dotani1111
Copy link
Contributor

@systemfriend-tsuji

「インストール順ではなくプラグインのフォルダ名称順になっている」

こちらで問題ありませんでした。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants