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

Task/34 活動タブ:チケットの一番古い履歴が最新のチケットステータスと同じになる問題 #37

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

ishikawa999
Copy link
Contributor

@ishikawa999 ishikawa999 commented Mar 13, 2021

  • activity_testで問題を再現するテストを追加
  • 問題を修正
  • Issue#event_statusのテストを追加
  • activity_testでケースを増やす

@juno-nishizaki
Copy link
Contributor

4/10 のパッチ会で追加のテストを実装しました。

  • test_activity_contains_issue_status_update_events2 : 新規チケット作成 → ステータス以外の項目だけを更新 → ステータスとステータス以外の項目を同時に更新
  • test_activity_contains_issue_status_update_events3 : 新規チケット作成 → チケットコメントだけを更新 → ステータスを更新

動作の面では問題なさそうです。(テストがコケてるのは Ruby 2.4 が drop されたからです)

@ishikawa999

パッチが accept されるようにテストコードの改善が必要と考えているので、以下の観点でレビューやフォローをお願いしたいです。(他の方もサポートいただけると助かります)

  • テストで何を確認しようとしているのか意図が伝わるようなコードにしたい
    • 今のところ日本語のコメントを入れているが、少なくともここは削除するか英語に書き換える必要がある
    • コメントを入れなくてもコードを読むだけで意図が伝わるのが理想ではある
  • Redmine テストの(明示的/暗黙的な)コード規約に沿ってないのであれば規約にあわせるようにしたい
  • テストコードの書き方として不適切または冗長な部分があればリファクタリングしたい

それでもなお、Performance issue があると指摘される可能性はあるかなとは思っていますが…残念ながら、今のところそれに対する解は持っていないです。(活動タブに表示される チケットの数だけ event_status メソッドが呼び出されることになるため、いわゆる N+1 問題に相当するような挙動を示すはず)

@ishikawa999
Copy link
Contributor Author

毎回event_statusが呼び出されることを失念していました。
Activity画面は結構高頻度で表示される画面で、なおかつ件数によるページネーションが存在しない(全体で設定された日付分を表示するため、期間内に大量の変更があった場合表示件数も大量になる)ため現在の仕組みだとコミットされづらい気がします。

時間のあるときに考えてみます

@github-actions
Copy link

Patch can be downloaded here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants