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

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

Merged
merged 18 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
281534a
ベストアンサーなし通知をキックするコントローラにSchedulerControllerを継承させる
ogawa-tomo Oct 11, 2023
670ca83
環境変数TOKENが指定されていない場合、SchedulerControllerの認証をパスさせない
ogawa-tomo Oct 15, 2023
042f9e0
ベストアンサーなし通知のテストコードの修正
ogawa-tomo Oct 15, 2023
878fa49
自動退会テストをログアウト状態で実施する
ogawa-tomo Oct 15, 2023
5e75b03
環境変数TOKENを設定したときに正しいクエリでアクセスすると定期処理ができるテスト
ogawa-tomo Oct 23, 2023
63298c5
定期処理がSchedulerControllerを継承していないときエラーを返す
ogawa-tomo Oct 23, 2023
8e49357
fetch_external_entry_controllerにSchedulerControllerを継承させる
ogawa-tomo Oct 25, 2023
4ce1914
引数の{}を省略
ogawa-tomo Oct 28, 2023
1023269
QueryHelperを削除
ogawa-tomo Oct 28, 2023
132848a
no_textの検証に現在のパスの検証を追加する
ogawa-tomo Oct 28, 2023
d87ce58
通知の数が変化していないことの検証を追加
ogawa-tomo Oct 28, 2023
32973ea
TOKENが未設定の場合と不一致の場合のテスト
ogawa-tomo Oct 29, 2023
fa5565b
ガード節を後置ifに変更
ogawa-tomo Oct 29, 2023
d45e5e5
require_scheduler_inheritationが最初に実行されるようにする
ogawa-tomo Oct 31, 2023
03d531f
token関連のテストをintegration testに置き換える
ogawa-tomo Nov 4, 2023
da12474
コメントを削除
ogawa-tomo Nov 7, 2023
5ba8640
SchedulerControllerの継承を要求するコードを修正
ogawa-tomo Nov 7, 2023
f546eb4
環境変数をモックする処理をstubを使って書き換える
ogawa-tomo Nov 7, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ group :test do
gem 'capybara'
gem 'minitest-ci'
gem 'minitest-retry'
gem 'minitest-stub_any_instance'
gem 'selenium-webdriver'
gem 'vcr'
gem 'webmock'
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ GEM
minitest-retry (0.2.2)
minitest (>= 5.0)
msgpack (1.7.2)
minitest-stub_any_instance (1.0.3)
multi_json (1.15.0)
multi_xml (0.6.0)
multipart-post (2.3.0)
Expand Down Expand Up @@ -625,6 +626,7 @@ DEPENDENCIES
mini_magick
minitest-ci
minitest-retry
minitest-stub_any_instance
net-imap
net-pop
net-smtp
Expand Down
5 changes: 5 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class ApplicationController < ActionController::Base
include PolicyHelper
helper_method :staging?
protect_from_forgery with: :exception
before_action :require_scheduler_inheritation
before_action :basic_auth, if: :staging?
before_action :test_login, if: :test?
before_action :init_user
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end

protected

def staging?
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

class Scheduler::Daily::FetchExternalEntryController < ApplicationController
class Scheduler::Daily::FetchExternalEntryController < SchedulerController
def show
ExternalEntry.fetch_and_save_rss_feeds(User.unretired)
head :ok
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

class Scheduler::Daily::NotifyCertainPeriodPassedAfterLastAnswerController < ApplicationController
class Scheduler::Daily::NotifyCertainPeriodPassedAfterLastAnswerController < SchedulerController
def show
Question.notify_certain_period_passed_after_last_answer
head :ok
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/scheduler_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


head :unauthorized
end
Expand Down
2 changes: 2 additions & 0 deletions test/application_system_test_case.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require 'supports/report_helper'
require 'supports/comment_helper'
require 'supports/tag_helper'
require 'supports/mock_env_helper'

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
include LoginHelper
Expand All @@ -17,6 +18,7 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
include ReportHelper
include CommentHelper
include TagHelper
include MockEnvHelper

if ENV['HEADED']
driven_by :selenium, using: :chrome
Expand Down
28 changes: 28 additions & 0 deletions test/integration/scheduler/daily/auto_retire_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

require 'test_helper'
require 'supports/mock_env_helper'

class Scheduler::Daily::AutoRetireTest < ActionDispatch::IntegrationTest
include MockEnvHelper

test 'token evaluation' do
# サーバー側のTOKENが未設定
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

削除しました!
da12474

mock_env('TOKEN' => '') do
get scheduler_daily_auto_retire_path(token: '')
assert_response 401
end

mock_env('TOKEN' => 'token') do
# リクエストで指定したtokenが不正
get scheduler_daily_auto_retire_path(token: 'invalid')
assert_response 401

# tokenが正しい
Scheduler::Daily::AutoRetireController.stub_any_instance(:auto_retire) do
get scheduler_daily_auto_retire_path(token: 'token')
assert_response 200
end
end
end
end
28 changes: 28 additions & 0 deletions test/integration/scheduler/daily/fetch_external_entry_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

require 'test_helper'
require 'supports/mock_env_helper'

class Scheduler::Daily::FetchExternalEntryTest < ActionDispatch::IntegrationTest
include MockEnvHelper

test 'token evaluation' do
# サーバー側のTOKENが未設定
mock_env('TOKEN' => '') do
get scheduler_daily_fetch_external_entry_path(token: '')
assert_response 401
end

mock_env('TOKEN' => 'token') do
# リクエストで指定したtokenが不正
get scheduler_daily_fetch_external_entry_path(token: 'invalid')
assert_response 401

# tokenが正しい
ExternalEntry.stub(:fetch_and_save_rss_feeds, nil) do
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

get scheduler_daily_fetch_external_entry_path(token: 'token')
assert_response 200
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

require 'test_helper'
require 'supports/mock_env_helper'

class Scheduler::Daily::NotifyCertainPeriodPassedAfterLastAnswerTest < ActionDispatch::IntegrationTest
include MockEnvHelper

test 'token evaluation' do
# サーバー側のTOKENが未設定
mock_env('TOKEN' => '') do
get scheduler_daily_notify_certain_period_passed_after_last_answer_path(token: '')
assert_response 401
end

mock_env('TOKEN' => 'token') do
# リクエストで指定したtokenが不正
get scheduler_daily_notify_certain_period_passed_after_last_answer_path(token: 'invalid')
assert_response 401

# tokenが正しい
Question.stub(:notify_certain_period_passed_after_last_answer, nil) do
get scheduler_daily_notify_certain_period_passed_after_last_answer_path(token: 'token')
assert_response 200
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

require 'test_helper'
require 'supports/mock_env_helper'

class Scheduler::Daily::NotifyComingSoonRegularEventsTest < ActionDispatch::IntegrationTest
include MockEnvHelper

test 'token evaluation' do
# サーバー側のTOKENが未設定
mock_env('TOKEN' => '') do
get scheduler_daily_notify_coming_soon_regular_events_path(token: '')
assert_response 401
end

mock_env('TOKEN' => 'token') do
# リクエストで指定したtokenが不正
get scheduler_daily_notify_coming_soon_regular_events_path(token: 'invalid')
assert_response 401

# tokenが正しい
Scheduler::Daily::NotifyComingSoonRegularEventsController.stub_any_instance(:notify_coming_soon_regular_events) do
get scheduler_daily_notify_coming_soon_regular_events_path(token: 'token')
assert_response 200
end
end
end
end
13 changes: 13 additions & 0 deletions test/supports/mock_env_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

module MockEnvHelper
def mock_env(partial_env_hash)
old = ENV.to_hash
ENV.update partial_env_hash
begin
yield
ensure
ENV.replace old
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

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メソッドのした方が安心かも

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end
32 changes: 24 additions & 8 deletions test/system/auto_retire_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 scheduler_daily_auto_retire_path(token: 'token')
end
end
assert_equal Date.current, user.reload.retired_on
end
Expand Down Expand Up @@ -49,7 +51,9 @@ class AutoRetireTest < ApplicationSystemTestCase
user = users(:kyuukai)
travel_to Time.zone.local(2020, 7, 1, 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 scheduler_daily_auto_retire_path(token: 'token')
end
end
assert_nil user.reload.retired_on
end
Expand All @@ -64,7 +68,9 @@ class AutoRetireTest < ApplicationSystemTestCase
logout

travel_to Time.zone.local(2020, 7, 2, 0, 0, 0) do
visit_with_auth scheduler_daily_auto_retire_path, 'komagata'
mock_env('TOKEN' => 'token') do
visit scheduler_daily_auto_retire_path(token: 'token')
end
assert_nil user.reload.retired_on
end
end
Expand All @@ -76,7 +82,9 @@ class AutoRetireTest < ApplicationSystemTestCase
user.update!(retired_on: retired_date)

travel_to Time.zone.local(2020, 7, 2, 0, 0, 0) do
visit_with_auth scheduler_daily_auto_retire_path, 'komagata'
mock_env('TOKEN' => 'token') do
visit scheduler_daily_auto_retire_path(token: 'token')
end
assert_equal retired_date, user.reload.retired_on
end
end
Expand All @@ -89,7 +97,9 @@ class AutoRetireTest < ApplicationSystemTestCase

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 scheduler_daily_auto_retire_path(token: 'token')
end
end
assert_equal Date.current, user.reload.retired_on
end
Expand All @@ -106,7 +116,9 @@ class AutoRetireTest < ApplicationSystemTestCase
travel_to Time.zone.local(2020, 7, 2, 0, 0, 0) do
Discord::Server.stub(:delete_text_channel, true) do
VCR.use_cassette 'subscription/update' do
visit_with_auth scheduler_daily_auto_retire_path, 'komagata'
mock_env('TOKEN' => 'token') do
visit scheduler_daily_auto_retire_path(token: 'token')
end
end
end
assert_equal Date.current, user.reload.retired_on
Expand All @@ -123,7 +135,9 @@ class AutoRetireTest < ApplicationSystemTestCase
UserMailer.stub(:auto_retire, stub_postmark_error) do
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 scheduler_daily_auto_retire_path(token: 'token')
end
end
assert_equal Date.current, user.reload.retired_on
end
Expand All @@ -140,7 +154,9 @@ class AutoRetireTest < ApplicationSystemTestCase

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 scheduler_daily_auto_retire_path(token: 'token')
end
end
assert_equal Date.current, user.reload.retired_on
end
Expand Down
4 changes: 3 additions & 1 deletion test/system/external_entries_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ class ExternalEntriesTest < ApplicationSystemTestCase
assert_difference 'ExternalEntry.count', 26 do
VCR.use_cassette 'external_entry/fetch2', vcr_options do
VCR.use_cassette 'external_entry/fetch' do
visit_with_auth '/scheduler/daily/fetch_external_entry', 'komagata'
mock_env('TOKEN' => 'token') do
visit scheduler_daily_fetch_external_entry_path(token: 'token')
end
end
end
end
Expand Down
20 changes: 14 additions & 6 deletions test/system/notification/questions_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -303,15 +303,23 @@ class Notification::QuestionsTest < ApplicationSystemTestCase
)

travel_to Time.zone.local(2022, 11, 6, 0, 0, 0) do
visit_with_auth '/scheduler/daily/notify_certain_period_passed_after_last_answer', 'kimura'
visit '/notifications'

assert_no_text 'Q&A「テストの質問」のベストアンサーがまだ選ばれていません。'
assert_no_difference 'questioner.notifications.count' do
mock_env('TOKEN' => 'token') do
visit scheduler_daily_notify_certain_period_passed_after_last_answer_path(token: 'token')
end
visit_with_auth '/notifications', 'kimura'

assert_no_text 'Q&A「テストの質問」のベストアンサーがまだ選ばれていません。'
assert_current_path(notifications_path(_login_name: 'kimura'))
end
end
logout

travel_to Time.zone.local(2022, 11, 7, 0, 0, 0) do
visit_with_auth '/scheduler/daily/notify_certain_period_passed_after_last_answer', 'kimura'
visit '/notifications'
mock_env('TOKEN' => 'token') do
visit scheduler_daily_notify_certain_period_passed_after_last_answer_path(token: 'token')
end
visit_with_auth '/notifications', 'kimura'

assert_text 'Q&A「テストの質問」のベストアンサーがまだ選ばれていません。'
end
Expand Down
4 changes: 3 additions & 1 deletion test/system/notification/regular_events_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ class Notification::RegularEventsTest < ApplicationSystemTestCase
headers: { 'Content-Type' => 'application/json' })

travel_to Time.zone.local(2023, 5, 5, 6, 0, 0) do
visit '/scheduler/daily/notify_coming_soon_regular_events'
mock_env('TOKEN' => 'token') do
visit scheduler_daily_notify_coming_soon_regular_events_path(token: 'token')
end
end

assert_requested(stub_message)
Expand Down