ベストアンサーなし通知が本番で動作しないバグを修正 #6970

ogawa-tomo · 2023-10-11T12:59:58Z

Issue

概要

下記PRのマージ後に動作確認したところ、本番環境でベストアンサーなし通知が動作していませんでした。
#6578

ベストアンサーなしの通知をキックするコントローラがSchedulerControllerを継承していないことが原因と判明したため、SchedulerControllerを継承させるよう修正しました。
https://bootcamp.fjord.jp/questions/1766#answer_6530

また、こちらのイシューで指摘されている通り、以下の修正を行いました。

環境変数TOKENが未設定だとSchedulerControllerの認証がパスしないようにする
スケジューラを呼び出すテストコードではユーザー認証せずにトークン認証するようにする
/schdulerで始まるパスをリクエストされた && self が SchedulerController を継承していないなら 500エラーとする

上記の修正に合わせ、既存の定期処理の実装も修正しました。

変更確認方法

以下の挙動を確認する。

サーバに環境変数TOKENが設定されていないとき、アクセスすると401エラーになること
サーバに環境変数TOKENが設定されているとき、正しいトークンを指定しないでアクセスすると401エラーになること
サーバに環境変数TOKENが設定されているとき、正しいトークンを指定してアクセスすると問題なく動くこと

なお、非ログイン状態でアクセスできることを確認ため、テストはブラウザのシークレットモードで行う。本番環境で実行するときは、スケジューラが非ログイン状態でURLをキックするため。

bug/notification-on-no-correct-answer-on-productionをローカルに取り込む
bin/rails sでサーバを立ち上げる
適当なユーザー（ここではhatsuno）でログインし、質問を作成する
別の適当なユーザー（ここではkimura）でログインし、4. で作成した質問に対する回答を作成する
Railsコンソールで下記のコメントを実行し、回答の作成日を1週間前にする

> Answer.last.update!(created_at: 1.week.ago)

シークレットモードでブラウザを開き（非ログイン状態で動作を確認するため）、以下のようにtokenパラメータを指定せずにURLにアクセスする
http://localhost:3000/scheduler/daily/notify_certain_period_passed_after_last_answer
401エラーとなることを確認する
いったんRailsサーバを終了し、ターミナルでコマンドを実行し、環境変数TOKENを適当に設定する

> export TOKEN=hoge

再度bin/rails sでサーバを立ち上げる
シークレットモードでブラウザを開き、以下のように正しくないtokenパラメータを指定したURLにアクセスする
http://localhost:3000/scheduler/daily/notify_certain_period_passed_after_last_answer?token=hog
401エラーとなることを確認する
続いて、以下のように正しいtokenパラメータを指定したURLにアクセスする
http://localhost:3000/scheduler/daily/notify_certain_period_passed_after_last_answer?token=hoge
http://localhost:3000/letter_opener/ にアクセスし、質問を作成したユーザー（ここではhatsuno）にメールが飛んでいることを確認する
質問を作成したユーザー（ここではhatsuno）でログインし、web通知が来ていることを確認する

参考PR

この機能が実装されたPR

ogawa-tomo · 2023-10-14T13:27:40Z

@omochiumaiumai こちら、よろしければレビューお願いできますでしょうか？

omochiumaiumai · 2023-10-15T07:56:58Z

@ogawa-tomo
お疲れ様です！
今週は時間が取れず対応できないため、申し訳ありませんが他の方にお願いしていただければ幸いです🙇‍♂💦

ogawa-tomo · 2023-10-15T12:20:44Z

@omochiumaiumai 承知しました、別の方にお願いしようと思います:bow:

ogawa-tomo · 2023-10-15T12:21:30Z

テストが落ちているのと、テストでもトークン認証をチェックできたほうがよいと思い、いったんドラフトに戻す

siso25 · 2023-10-24T05:38:43Z

@ogawa-tomo
お疲れ様です。
こちら修正中かと思いますが、修正範囲について相談させてください。

こちらのpull requestで「定期処理がSchedulerControllerを継承していないときエラーにする」対応を行うと、fetch_external_entryのシステムテストが失敗するようになると思います。（ExternalEntryがSchedulerControllerを継承していないため）

上記を回避するためには「ExternalEntryにScheduleControllerを継承する」対応を行う必要がありますが、以下のpull requestで先に対応してしまうと、今度はtokenを使った定期実行テスト用のヘルパーメソッドの実装が被ってしまいます。
#6941

そのため、以下の対応についてはこちらのpull requestで一緒に対応いただいた方が良いかもしれないと思いましたが、いかがでしょうか。

ExternalEntryにSchedulerControllerを継承させる
test/system/external_entries_test.rbで定期実行用のヘルパーメソッド（ogawa-tomoさんに定義していただいたもの）を使うようにする

ogawa-tomo · 2023-10-24T13:39:28Z

@siso25 ご連絡ありがとうございます！たしかにおっしゃる通りですね。その2点についてもこのPRで対応してしまおうと思います。関連する修正なので、小分けにするより一緒にやってしまったほうがよさそうな気もしますし。

siso25 · 2023-10-25T00:29:47Z

@ogawa-tomo
すみません。よろしくお願いします！🙏

ogawa-tomo · 2023-10-25T12:47:09Z

@siso25 上記対応しました！こちらのPR、レビュー依頼もお願いしてしまってもよろしいでしょうか？sisoさんがレビュー依頼可能なステータスなのか分かっていないのですが、sisoさんのPRにも関連しますし、確認はお願いしたく思っています:pray:

ogawa-tomo · 2023-10-25T12:53:48Z

test/system/notification/questions_test.rb

+      mock_env({ 'TOKEN' => 'token' }) do
+        visit_with_query scheduler_daily_notify_certain_period_passed_after_last_answer_path, { token: 'token' }
+      end


@siso25 sisoさんにご提案いただいたコードでは、環境変数TOKENの設定とクエリパラメータtokenを設定してのアクセスという2つのことを1つのメソッドで行っていましたが、両者は分離すべきと考え、別のメソッドとして実装しました。このほうが仕様として上記の2つが必要であることがテストコードから読み取りやすいですし、上記の2つを独立にテストすることも可能になる（今回はやっていませんが）というメリットもあると思います。

修正ありがとうございます！
たしかにこちらの方が環境変数をモックしていることが分かりやすそうです。

siso25 · 2023-10-26T00:28:03Z

@ogawa-tomo
レビューの件承知しました。
今週末までには確認したいと思います！

JunichiIto

コメントしました〜。

JunichiIto · 2023-10-27T04:52:27Z

app/controllers/application_controller.rb

+    return unless request.path_info.start_with?('/scheduler')
+    return if is_a?(SchedulerController)
+
+    head :internal_server_error


Suggested change

return unless request.path_info.start_with?('/scheduler')

return if is_a?(SchedulerController)

head :internal_server_error

if request.path_info.start_with?('/scheduler') && !is_a?(SchedulerController)

head :internal_server_error

end

とした方がコードの意図が明確になりそう。

そして、できたらこの仕組みもテストコードがほしいけど、ちょっと難しいかなあ。。

ガード節が好きでなんとなく使ってしまったのですが、たしかにこちらのほうが今回の意図を肯定形で表現できて分かりやすいですね。
ご提案いただいたコードだとrubocopにガード節か後置ifのどちらかにしろと言われたので、後置ifを用いた書き方に修正しました。
782f084

この仕組みのテストコードは実現方法が思いつかずでして、そのままにしています:pray:

了解です。最低限、一時的にcontrollerの親クラスを書き替えるなどして、手元で期待した動作をしているかどうかだけ確認しておいてほしいです〜。（スクショとかのエビデンスがあればなお良し）

期待した動作が得られなかったので下記の修正を加えています。先にrequire_active_user_loginが評価されて200が帰ってきていたため、require_scheduler_inheritationが先に評価されるように変更しました。
d45e5e5

上記の変更を加えたうえで、下記の箇所でNotifyCertainPeriodPassedAfterLastAnswerControllerの親クラスをApplicationControllerに一時的に書き換え、ディスクリプションに記載した方法で動作確認を行いました。

bootcamp/app/controllers/scheduler/daily/notify_certain_period_passed_after_last_answer_controller.rb

Line 3 in b049748

class Scheduler::Daily::NotifyCertainPeriodPassedAfterLastAnswerController < SchedulerController

その結果、500エラーとなることを確認しました。

親クラスをSchedulerControllerに戻すと正常に動作することも確認しました。

JunichiIto · 2023-10-27T04:55:45Z

app/controllers/scheduler_controller.rb

@@ -8,7 +8,7 @@ class SchedulerController < ApplicationController
  protected

  def require_token
-    return if ENV['TOKEN'] == params[:token]
+    return if ENV['TOKEN'].present? && ENV['TOKEN'] == params[:token]


TOKENが未設定だとエラー、paramsのtokenが不一致だったらエラー、というパターンもテストしたい。
（門番が門番として機能していることをテストする）

それぞれのテストについて、TOKENが未設定のケースと不一致のケースのテストを加えました。
580816e
ちょっと冗長かなとも思ったのですが、テストは必ずしもDRYでなくてよいという伊藤さんの記事も参考にしつつ、全てのテストについて愚直に繰り返し書いています。
https://qiita.com/jnchito/items/eb3cfa9f7db752dcb796

JunichiIto · 2023-10-27T04:57:09Z

test/system/auto_retire_test.rb

@@ -16,7 +16,9 @@ class AutoRetireTest < ApplicationSystemTestCase
    user = users(:kyuukai)
    travel_to Time.zone.local(2020, 7, 2, 0, 0, 0) do
      VCR.use_cassette 'subscription/update' do
-        visit_with_auth scheduler_daily_auto_retire_path, 'komagata'
+        mock_env({ 'TOKEN' => 'token' }) do


Suggested change

mock_env({ 'TOKEN' => 'token' }) do

mock_env('TOKEN' => 'token') do

でよさそう

修正しました！引数の最後がハッシュだった場合は{}が省略できるのですね。
03e3b7e

JunichiIto · 2023-10-27T04:57:25Z

test/system/auto_retire_test.rb

@@ -16,7 +16,9 @@ class AutoRetireTest < ApplicationSystemTestCase
    user = users(:kyuukai)
    travel_to Time.zone.local(2020, 7, 2, 0, 0, 0) do
      VCR.use_cassette 'subscription/update' do
-        visit_with_auth scheduler_daily_auto_retire_path, 'komagata'
+        mock_env({ 'TOKEN' => 'token' }) do
+          visit_with_query scheduler_daily_auto_retire_path, { token: 'token' }


Suggested change

visit_with_query scheduler_daily_auto_retire_path, { token: 'token' }

visit_with_query scheduler_daily_auto_retire_path, token: 'token'

でよさそう

そもそも、

Suggested change

visit_with_query scheduler_daily_auto_retire_path, { token: 'token' }

visit scheduler_daily_auto_retire_path(token: 'token')

でいいのでは？

なるほど、それで書けるのですね。修正し、QueryHelperを削除しました。
1d66013

JunichiIto · 2023-10-27T04:58:27Z

test/system/notification/questions_test.rb

+      mock_env({ 'TOKEN' => 'token' }) do
+        visit_with_query scheduler_daily_notify_certain_period_passed_after_last_answer_path, { token: 'token' }
+      end
+      visit_with_auth '/notifications', 'kimura'

      assert_no_text 'Q&A「テストの質問」のベストアンサーがまだ選ばれていません。'


no textの検証だけはなるべく避けたい

システムスペックやフィーチャスペックで「〜が表示されないこと」だけを検証するのはちょっと危険、という話 #Ruby - Qiita

なるほどです。通知一覧のページにアクセスできていることの検証を追加しました。
ba0c596

↑だけだと通知のメッセージが変わったときも通っちゃうな、と思い直したので、通知の件数が変わっていないことの検証も追加しました:bow:
a6e1f58

ogawa-tomo · 2023-11-06T12:12:17Z

@JunichiIto コメントいただいた点を修正しましたのでご確認お願いします:pray:

JunichiIto

修正ありがとうございます！コメントしました〜。

JunichiIto · 2023-11-07T06:30:00Z

app/controllers/application_controller.rb

@@ -50,6 +51,10 @@ def require_subscription
    redirect_to root_path, notice: 'サブスクリプション登録が必要です。' unless current_user&.subscription?
  end

+  def require_scheduler_inheritation
+    head :internal_server_error if request.path_info.start_with?('/scheduler') && !is_a?(SchedulerController)


before_action :require_scheduler_inheritation, if: -> { request.path_info.start_with?('/scheduler') }

とした上で

Suggested change

head :internal_server_error if request.path_info.start_with?('/scheduler') && !is_a?(SchedulerController)

head :internal_server_error unless is_a?(SchedulerController)

とした方がコードの意図が明確になりそうな気がしました

こちらで修正しました。
5ba8640

JunichiIto · 2023-11-07T06:31:18Z

test/integration/scheduler/daily/auto_retire_test.rb

+  include MockEnvHelper
+
+  test 'token evaluation' do
+    # サーバー側のTOKENが未設定


コメントいらないんじゃないかなー。
（僕のサンプルコードのコメントはあくまで説明用だったので）

削除しました！
da12474

JunichiIto · 2023-11-07T06:32:21Z

test/integration/scheduler/daily/fetch_external_entry_test.rb

+      assert_response 401
+
+      # tokenが正しい
+      ExternalEntry.stub(:fetch_and_save_rss_feeds, nil) do


質問）nilって付けないと問題が起きるんでしょうか？

stubメソッドでは第2引数で戻り値を指定する必要があるようです。省略するとエラーになりました。ここでは戻り値はとくに問題ではないので、nilを指定することにしました。
https://www.rubydoc.info/gems/minitest/Object:stub

JunichiIto · 2023-11-07T06:49:06Z

test/supports/mock_env_helper.rb

+  def mock_env(partial_env_hash)
+    old = ENV.to_hash
+    ENV.update partial_env_hash
+    begin
+      yield
+    ensure
+      ENV.replace old
+    end
+  end


Suggested change

def mock_env(partial_env_hash)

old = ENV.to_hash

ENV.update partial_env_hash

begin

yield

ensure

ENV.replace old

end

end

def mock_env(hash_for_mock)

env_hash = ENV.to_hash.merge(hash_for_mock)

ENV.stub(:[], ->(key) { env_hash[key] }) do

yield

end

end

ENVを書き替えるのは事故が怖いのでstubメソッドのした方が安心かも

こちらで修正しました。
f546eb4
rubocopでこの警告が出たため、yieldを使わない形に書き換えています。
https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/ExplicitBlockArgument

ogawa-tomo · 2023-11-07T11:58:20Z

@JunichiIto 再度ご確認お願いします:pray:

JunichiIto

1点コメントしましたが、その点が問題なければこれでOKです〜 🙆‍♂️

JunichiIto · 2023-11-08T01:39:25Z

app/controllers/application_controller.rb

@@ -6,6 +6,7 @@ class ApplicationController < ActionController::Base
  include PolicyHelper
  helper_method :staging?
  protect_from_forgery with: :exception
+  before_action :require_scheduler_inheritation, if: -> { request.path_info.start_with?('/scheduler') }


テストを書いてないのでちゃんと動くかどうか不安だけど、ローカルでは動作確認済みですよね？

こちらと同じ方法で確認しました！
#6970 (comment)

ogawa-tomo · 2023-11-08T12:01:16Z

@JunichiIto @siso25 レビューありがとうございました:bow:

@komagata メンバーのレビューが完了しましたので、レビューをお願いします:bow:

komagata

確認させて頂きました。OKです〜🙆‍♂️

ogawa-tomo · 2023-11-15T11:24:10Z

📝 ステージング環境での動作確認
こちらと同様の手順で確認（ただし、ブラウザで定期処理URLにアクセスする箇所はシークレットモードを用いる）
#6578 (comment)
定期処理URLにアクセスした際にBASIC認証のID/PWを要求されたが、それを入力するとweb通知・メール通知ともに正常に届いた

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 9936c06 to 048b62f Compare October 14, 2023 03:15

ogawa-tomo marked this pull request as draft October 14, 2023 04:00

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 175a0d4 to 048b62f Compare October 14, 2023 10:52

ogawa-tomo changed the base branch from main to bug/debug-flaky-regular-events-test October 14, 2023 12:43

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 048b62f to 89655c4 Compare October 14, 2023 12:56

ogawa-tomo marked this pull request as ready for review October 14, 2023 13:26

ogawa-tomo requested a review from omochiumaiumai October 14, 2023 13:28

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 50969e4 to 248b2e9 Compare October 15, 2023 11:59

ogawa-tomo marked this pull request as draft October 15, 2023 12:21

ogawa-tomo mentioned this pull request Oct 15, 2023

みんなのブログが更新されない。 #6929

Closed

ogawa-tomo removed the request for review from omochiumaiumai October 15, 2023 12:26

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 9f08365 to 248b2e9 Compare October 16, 2023 12:33

Base automatically changed from bug/debug-flaky-regular-events-test to main October 16, 2023 18:12

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 956f4fa to 2e6e84b Compare October 23, 2023 12:35

ogawa-tomo mentioned this pull request Oct 23, 2023

SchedulerControllerの認証方法とテストコードの見直し #6961

Closed

ogawa-tomo mentioned this pull request Oct 25, 2023

定期処理にWeb APIのテストを追加する #6993

Closed

ogawa-tomo requested a review from siso25 October 25, 2023 12:47

ogawa-tomo commented Oct 25, 2023

View reviewed changes

ogawa-tomo marked this pull request as ready for review October 25, 2023 12:55

siso25 mentioned this pull request Oct 26, 2023

記事ごとの登録日時を持たない場合も例外が発生しないよう修正 #6941

Merged

JunichiIto reviewed Oct 27, 2023

View reviewed changes

ogawa-tomo added 12 commits November 6, 2023 20:51

自動退会テストをログアウト状態で実施する

878fa49

環境変数TOKENを設定したときに正しいクエリでアクセスすると定期処理ができるテスト

5e75b03

定期処理がSchedulerControllerを継承していないときエラーを返す

63298c5

fetch_external_entry_controllerにSchedulerControllerを継承させる

8e49357

引数の{}を省略

4ce1914

QueryHelperを削除

1023269

no_textの検証に現在のパスの検証を追加する

132848a

通知の数が変化していないことの検証を追加

d87ce58

TOKENが未設定の場合と不一致の場合のテスト

32973ea

ガード節を後置ifに変更

fa5565b

require_scheduler_inheritationが最初に実行されるようにする

d45e5e5

token関連のテストをintegration testに置き換える

03d531f

ogawa-tomo force-pushed the bug/notification-on-no-correct-answer-on-production branch from 432e576 to 03d531f Compare November 6, 2023 11:51

ogawa-tomo requested a review from JunichiIto November 6, 2023 12:12

JunichiIto reviewed Nov 7, 2023

View reviewed changes

ogawa-tomo added 3 commits November 7, 2023 20:22

コメントを削除

da12474

SchedulerControllerの継承を要求するコードを修正

5ba8640

環境変数をモックする処理をstubを使って書き換える

f546eb4

ogawa-tomo requested a review from JunichiIto November 7, 2023 11:58

JunichiIto approved these changes Nov 8, 2023

View reviewed changes

ogawa-tomo requested a review from komagata November 11, 2023 02:54

komagata approved these changes Nov 15, 2023

View reviewed changes

komagata merged commit e64f379 into main Nov 15, 2023
8 checks passed

komagata deleted the bug/notification-on-no-correct-answer-on-production branch November 15, 2023 07:03

github-actions bot mentioned this pull request Nov 15, 2023

Release 2023-11-16 04:01:48 #7047

Merged

8 tasks

Takuya-Sakai91 mentioned this pull request Apr 6, 2024

分報チャンネル作成後、生成されたチャンネルのURLをユーザーのプロフィールに自動で設定 #7564

Merged

	mock_env({ 'TOKEN' => 'token' }) do
	mock_env('TOKEN' => 'token') do

	visit_with_query scheduler_daily_auto_retire_path, { token: 'token' }
	visit_with_query scheduler_daily_auto_retire_path, token: 'token'

	visit_with_query scheduler_daily_auto_retire_path, { token: 'token' }
	visit scheduler_daily_auto_retire_path(token: 'token')

	head :internal_server_error if request.path_info.start_with?('/scheduler') && !is_a?(SchedulerController)
	head :internal_server_error unless is_a?(SchedulerController)

ベストアンサーなし通知が本番で動作しないバグを修正 #6970

ベストアンサーなし通知が本番で動作しないバグを修正 #6970

Conversation

ogawa-tomo commented Oct 11, 2023 • edited Loading

Issue

概要

変更確認方法

参考PR

ogawa-tomo commented Oct 14, 2023

omochiumaiumai commented Oct 15, 2023

ogawa-tomo commented Oct 15, 2023

ogawa-tomo commented Oct 15, 2023

siso25 commented Oct 24, 2023 • edited Loading

ogawa-tomo commented Oct 24, 2023

siso25 commented Oct 25, 2023

ogawa-tomo commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siso25 commented Oct 26, 2023

JunichiIto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo commented Nov 6, 2023

JunichiIto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo commented Nov 7, 2023

JunichiIto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogawa-tomo commented Nov 8, 2023

komagata left a comment

Choose a reason for hiding this comment

ogawa-tomo commented Nov 15, 2023

ogawa-tomo commented Oct 11, 2023 •

edited

Loading

siso25 commented Oct 24, 2023 •

edited

Loading

ogawa-tomo Nov 4, 2023 •

edited

Loading