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

商品CSVの分割アップロード #4805

Merged
merged 10 commits into from Dec 16, 2020

Conversation

chihiro-adachi
Copy link
Contributor

@chihiro-adachi chihiro-adachi commented Dec 14, 2020

概要(Overview・Refs Issue)

商品CSV登録にて、CSVを分割して処理するように対応。

方針(Policy)

背景として、CSVの行数が多い場合に、時間がかかり登録が完了しない・メモリを使い切りシステムエラーが発生するなど、ユーザビリティに難がありました。今回のPRでは、以下のようにファイルを分割して登録するように処理方式を変更しています。

Untitled

従来との仕様変更点として、1件でもエラーがあればすべてロールバックされていたのが、分割したファイルごとにトランザクションがかかる形になります。

実装に関する補足(Appendix)

互換性保持のため、CSVの登録ロジック自体に変更はありません。
レスポンス返す部分のみ、ajaxでのリクエストの場合にはjsonで返すようにする修正を入れています。・

テスト(Test)

  • ユニットテスト作成
  • memory_limit:128Mで1万件のCSVを取り込めることを確認

相談(Discussion)

画面UIは以下のように作っています。

csv-out

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

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

レビュワー確認項目

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

@@ -107,6 +111,12 @@ class CsvImportController extends AbstractCsvImportController

private $errors = [];

protected $isXmlHttpRequest = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

XMLHttpRequestかどうかのフラグではなく、分割アップロードかどうかのフラグだと思うので isSplitCsv に変更をお願いします。

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.

途中でエラーになった場合に分割したファイルがサーバーに残り続けてしまいます。

@chihiro-adachi
Copy link
Contributor Author

@kiy0taka
ありがとうございます。修正しました。

@kiy0taka kiy0taka merged commit 7b064c2 into EC-CUBE:4.0 Dec 16, 2020
@chihiro-adachi chihiro-adachi deleted the improve/csv-split branch December 16, 2020 10:14
@kiy0taka kiy0taka added the affected:admin_template 管理画面テンプレートのDOMに影響のある変更 label Dec 16, 2020
@okazy okazy modified the milestones: 4.0.x, 4.0.6 Dec 22, 2020
@chihiro-adachi chihiro-adachi modified the milestones: 4.0.6, 4.1 Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected:admin_template 管理画面テンプレートのDOMに影響のある変更 improvement 機能改善
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants