提出物一覧ページの「未完了」と「自分の担当」のタブのバッジに表示する件数を適切に増減させる #3690

AudioStakes · 2021-12-07T08:45:30Z

目的

提出物一覧ページの「未完了」と「自分の担当」タブの表示件数を適切に増減させること（以下のバグ修正を含む）

Refs: #3638

やったこと

キャッシュ対象の件数が増減したときにキャッシュを削除するようにした

対象のキャッシュは以下2つ

未完了の提出物件数
任意のユーザー（メンター）の「自分の担当」かつ「未返信」の提出物件数

※ キャッシュを削除する条件の詳細は、↓の「件数が増減する（キャッシュを削除するべき）条件」に記載

テストを追加した

キャッシュの値が適切に増減することを確認するテストを追加

気になっていること（レビューしてもらいたいこと）

今回新たに追加した「キャッシュの更新を確認するモデルテスト」を書いた場所（フォルダやファイル名）が適切かどうか

今までキャッシュに関するモデルテストがなかったため、この PR で初めて追加しました。そのフォルダとファイル名について、判断したことに問題なさそうか確認したいです。

フォルダについて

新たに test/models/caches/*.rb を作成し、そこにモデルテストを置くようにしました。
このフォルダに置くことで「model の実装に関するテスト」かつ「キャッシュに関するテスト」であることがわかりやすくなると考えたためです。

ファイル名について

{キャッシュのキー名}.rb となるようにしました。
キャッシュごとに、どのような条件で削除・更新されるか把握しやすくなると考えたためです。

test/models/{モデル名}.rb に追記しなかった理由について

test/models/{モデル名}.rb に追記した場合、以下のようなメリット・デメリットがあると思います。

メリット
- モデルの振る舞いを把握しやすくなる
デメリット
- キャッシュごとに、どのような条件で削除・更新されるか把握しにくくなる

今回はデメリットの方が大きくなると考え、 test/models/{モデル名}.rb に追記しないこととしました。というのも、キャッシュには複数のモデルから削除されるものもあるため、キャッシュのテストが複数のモデルのテストファイルに散らばる（より一層、削除・更新の条件を把握しにくくなる）と考えたためです。

以下、補足説明

件数が更新されないバッジ（修正対象の件数を表示するバッジ）のキャプチャ

動作確認をするときの注意事項

development モードと test モードの場合はデフォルトでキャッシュが無効になっているため、ローカルでサーバーを起動して動作確認するとき（development モードのとき）や $ rails test xxx を実行するとき（test モードのとき）は、以下のようにして事前にキャッシュを有効にしておく必要があります。

development モード

$ rails dev:cacheを実行してキャッシュのオン/オフを切り替える（オンにする）
（参考）development環境のキャッシュ

test モード

config/environments/test.rb を修正し、キャッシュストアとして :memory_store （ActiveSupport::Cache::MemoryStore ）を使うように設定する

diff --git a/config/environments/test.rb b/config/environments/test.rb
index e080eba6d..c20b14524 100644
--- a/config/environments/test.rb
+++ b/config/environments/test.rb
@@ -25,7 +25,7 @@ Rails.application.configure do
   # Show full error reports and disable caching.
   config.consider_all_requests_local       = true
   config.action_controller.perform_caching = false
-  config.cache_store = :null_store
+  config.cache_store = :memory_store

   # Raise exceptions instead of rendering exception templates.
   config.action_dispatch.show_exceptions = false

#3638 の再現手順で件数が更新されない原因

調査した結果、以下が原因であることがわかった。

「未完了」タブのバッジ（未完了の提出物件数）

未完了の提出物が確認済み（完了）に更新されたときに、未完了件数のキャッシュが削除されていないこと

「自分の担当」タブのバッジ（「自分の担当」かつ「未返信」の提出物件数）

「自分の担当」かつ「未返信」の提出物が返信済みになったとき（担当者がコメントしたとき）に、「自分の担当」かつ「未返信」の件数のキャッシュが削除されていないこと

※ 「自分の担当」の提出物の状態が「未返信」かどうかは、その提出物の最新コメントの作成者が担当者（つまり自分）か否かによって決まる

件数が増減する（キャッシュを削除するべき）条件と、その対応の要不要

#3638 の再現手順以外にもキャッシュが削除されていないケースがないか確認するために調査した。
調査した結果、以下の表の条件で件数が増減することがわかった。また、それぞれの条件について対応の要不要を調べた。

凡例
○: 対応不要（現状のコードでキャッシュが削除されている）
☆: 対応が必要 & #3638 で報告済みの条件
★: 対応が必要 & #3638 で報告されていない条件

未完了の提出物件数（未完了タブのバッジの表示件数）

条件No.	キャッシュ削除の条件	○/☆/★
1	提出物が作成されたとき（作成時は常に未完了）	○
2	未完了の提出物が削除されたとき	○
3	未完了の提出物が確認済み（完了）に更新されたとき	☆
4	確認済み（完了）の提出物が未完了に更新されたとき	★

「自分の担当」かつ「未返信」の提出物件数（自分の担当タブのバッジの表示件数）

条件No.	キャッシュ削除の条件	○/☆/★
5	「担当がいない」かつ「未返信」の提出物の担当になったとき	★
6	「自分の担当」かつ「未返信」の提出物の担当を外れたとき	★
7	担当のいる未返信の提出物に担当がコメントしたとき	☆
8	担当のいる返信済みの提出物の最新コメントが削除され、担当以外のコメントが最新になったとき	★
9	担当のいる返信済みの提出物に担当者以外がコメントしたとき	★
10	担当のいる未返信の提出物の最新コメントが削除され、担当者のコメントが最新となったとき	★

キャッシュ削除の具体的な条件

具体的にどのような条件でキャッシュを削除するか表に整理した。なお、↑の表と同じ条件は「条件No. 」の値を同じにしている。

※表の読み方: 【モデル】のレコードを【作成/更新/削除】するときにレコードが【条件1】かつ【条件2】かつ【条件3】... の場合はキャッシュを削除する。なお、その後はキャッシュに保持する値が【キャッシュの増減値】される。

未完了の提出物件数（未完了タブのバッジの表示件数）

条件No.	モデル	作成/更新/削除	条件	増減値
1	Product	作成	未完了（作成時は未完了なのでチェック不要）	+1
2	Product	削除	未完了	-1
3	Check	作成	チェック対象が提出物	-1
4	Check	削除	チェック対象が提出物	+1

「自分の担当」かつ「未返信」の提出物件数（自分の担当タブのバッジの表示件数）

※項目が異なるため表を2つに分けた

No.	モデル	作成/更新/削除	条件1	条件2	条件3	増減値
5	Product	更新	更新前は担当がいない	更新後は担当がいる	（なし）	+1
6	Product	更新	更新前は担当がいる	更新後は担当がいない	担当から未返信の状態	-1

条件No. 7 ~ 8には以下3点が共通する（表の横幅を小さくするために取り出した）

モデル: Comment
条件1: コメント先が提出物
条件2: 提出物に担当がいる

条件No.	作成/更新/削除	条件3	条件4	条件5	増減値
7	作成	コメント作成者が担当	提出物の最新コメント作成者が担当以外	（なし）	-1
8	作成	コメント作成者が担当以外	提出物の最新コメント作成者が担当	（なし）	+1
9	削除	コメント作成者が担当	(削除前の)2番目の最新コメントの作成者が担当以外	(削除前の)提出物の最新コメント	-1
10	削除	コメント作成者が担当以外	(削除前の)2番目の最新コメントの作成者が担当	(削除前の)提出物の最新コメント	+1

モデルテストで確認したいこと

↑の表のキャッシュ削除条件と合致するデータ変更をしたときに、以下2つを満たせていること（キャッシュとして保持する値が更新されていること）

キャッシュを削除できていること
キャッシュを削除した後、キャッシュ対象の値（オリジナルの値）が増減していること
- もしオリジナルの値が増減しないなら、そのデータ変更のタイミングはキャッシュを削除するタイミングとして不適当となる（オリジナルの値が変わらないならキャッシュを削除しなくて良いため）

AudioStakes · 2022-01-06T04:55:42Z

@unstoppa61e
ご都合の良いときに、こちらのレビューをお願いします！

動作確認をする際に事前準備（詳細は Description に記載しました）が必要であることや、キャッシュを削除する条件の説明量が多いことが影響して、レビューしてもらうのに時間がかかってしまうかもしれません。

もし不明点がありましたらお気軽に質問してください。よろしくお願いします🙏

unstoppa61e

こちらの PR、偉大ですね…👏
僕がちょうどキャッシュのことについて学びたかったところだったので、レビューさせて頂けてありがたかったです🙏
指摘させて頂いたのは、本質的でないものがほとんど全てです。
そういうものにつきましては、どうぞ取捨選択なさった上でご修正くださいませ。
また、テストファイルの置き場所につきましては、良さそうだと思うものの自信はございませんので、komagata さんにお尋ね頂くのが確実かと思います🙇‍♂️

unstoppa61e · 2022-01-07T04:47:36Z

test/models/caches/unchecked_product_count_test.rb

+require 'test_helper'
+
+class UncheckedProductCountTest < ActiveSupport::TestCase
+  test 'increase the cached value of unchecked product count by 1 after create a product' do


Suggested change

test 'increase the cached value of unchecked product count by 1 after create a product' do

test 'cached value of unchecked product count increases by 1 after creating a product' do

個人的には、こうした方が英語として正しいように思いました（こちらの変更はマストではございません）。

なるほど、たしかにそのように書いた方が読みやすいと思いました！他のテストケースについても、添削していただいてありがとうございますー🙏

unstoppa61e · 2022-01-07T04:50:09Z

test/models/caches/unchecked_product_count_test.rb

+    end
+  end
+
+  test 'decreace the cached value of unchecked product count by 1 after destroy an unchecked product' do


Suggested change

test 'decreace the cached value of unchecked product count by 1 after destroy an unchecked product' do

test 'cached value of unchecked product count decreases by 1 after destroying an unchecked product' do

こちらは、少なくともdecreaceという typo があるので、そこは修正した方が良いかと思います。

unstoppa61e · 2022-01-07T04:51:14Z

test/models/caches/unchecked_product_count_test.rb

+    end
+  end
+
+  test 'increase the cached value of unchecked product count by 1 after uncheck a checked product' do


Suggested change

test 'increase the cached value of unchecked product count by 1 after uncheck a checked product' do

test 'cached value of unchecked product count increases by 1 after unchecking a checked product' do

unstoppa61e · 2022-01-07T04:51:51Z

test/models/caches/unchecked_product_count_test.rb

+    end
+  end
+
+  test 'decrease the cached value of unchecked product count by 1 after check an unchecked product' do


Suggested change

test 'decrease the cached value of unchecked product count by 1 after check an unchecked product' do

test 'cached value of unchecked product count decreases by 1 after checking an unchecked product' do

unstoppa61e · 2022-01-07T05:20:34Z