退会したらGitHubアカウントとの連携のデータを空になるように修正 #7931
Conversation
su-su-su-su
force-pushed
the
chore/clear-github-data-on-account-deletion
branch
from
b123f0f
to
66416fb
Compare
|
お疲れ様です。 |
|
承知いたしました！ |
| @@ -823,6 +823,14 @@ def scheduled_retire_at | |||
| hibernated_at + User::HIBERNATION_LIMIT if hibernated_at? | |||
| end | |||
|
|
|||
| def clear_github_data | |||
There was a problem hiding this comment.
今回こちらのメソッドをモデルに追加しているので、このメソッド自体のテストは必要かと思ったのですがいかがでしょうか？
There was a problem hiding this comment.
@motohiro-mm すみません、ちょっと質問の意図が理解できてないかもです。
基本的にはモデルにメソッドを追加した場合、モデルのテスト（ユニットテスト）の追加は必要になります。
There was a problem hiding this comment.
私が最初にレビューさせていただいたときには、モデルのテストが追加されていなかったため、このようにコメントさせていただきました。
その後、test/models/user_test.rb:740のtest 'clear_github_data should clear GitHub related fields'が追加されています。
test/system/retirement_test.rb
Outdated
| visit_with_auth new_retirement_path, 'kimura' | ||
| find('label', text: 'とても良い').click | ||
| click_on '退会する' | ||
| page.driver.browser.switch_to.alert.accept |
There was a problem hiding this comment.
このコードはSeleniumがドライバーの時に使えるようです。
https://www.selenium.dev/ja/documentation/webdriver/interactions/alerts/
現状ドライバーはSeleniumなので問題ないのですが、今後ドライバーを変更するとこのコードは動かなくなります。
Capybaraのpage.accept_confirmを使えばドライバーを変更しても同じ動作ができるようになるので、そちらの書き方のほうが望ましいのかなと思いました！
私が参考にしたリンクを一応貼っておきます↓
https://trends.codecamp.jp/blogs/media/difference-word6
https://qiita.com/at-946/items/403d85d45cb02615c323
https://bootcamp.fjord.jp/reports/98175#comment_170875
There was a problem hiding this comment.
こちらもありがとうございます！
参考リンク、日報もとても勉強になりました！
ドライバーを変更するとコードが動かなくなることが把握できていなかったです。
page.accept_confirm '本当によろしいですか？' do
click_on '退会する'
endに変更しました！
|
レビューありがとうございました。 |
|
ご確認ありがとうございました！ |
|
@komagata |
| @@ -823,6 +823,14 @@ def scheduled_retire_at | |||
| hibernated_at + User::HIBERNATION_LIMIT if hibernated_at? | |||
| end | |||
|
|
|||
| def clear_github_data | |||
| @@ -736,4 +736,18 @@ class UserTest < ActiveSupport::TestCase | |||
| test '.users_job returns all users when invalid job is passed' do | |||
| assert_equal User.all, User.users_job('destroy_all') | |||
| end | |||
|
|
|||
| test 'clear_github_data should clear GitHub related fields' do | |||
There was a problem hiding this comment.
| test 'clear_github_data should clear GitHub related fields' do | |
| test '#clear_github_data' do |
モデルのテストで、代表的なテストだけだったら他と一緒でこういう名前でいいかもです。
There was a problem hiding this comment.
ご指摘ありがとうございます。
test '#clear_github_data' doと修正いたしました。
|
@komagata |
|
@komagata |
| notify_to_mentors(user) | ||
| logout | ||
| redirect_to retirement_url | ||
| begin |
There was a problem hiding this comment.
こちらのファイルの今回と関係ない部分が変更されているようです。
こちらはどういった意図でしょうか。
There was a problem hiding this comment.
お疲れ様です。
当初今回のイシューの処理をcreateメソッドに追加したのですが20行を超えた為、Metrics/MethodLengthというエラーがでてしまいました。
その為今回の変更と関係のないコードを分割したという経緯です。
There was a problem hiding this comment.
なるほどです。
ControllerがFatになっているのでしたら、モデルクラスに切り出すのが良さそうです。
こんな感じがいいかもです。
class UserRetirement
def initialize(user)
@user = user
end
def execute
# ...
end
endThere was a problem hiding this comment.
アドバイスありがとうございます。
現在UserRetirementクラスにclear_github_data処理だけを切り出したのですが、このクラス名から考えると、退会に関する他の処理（サブスクリプションの解約、通知の送信など）もこのクラスにまとめるべきでしょうか？
There was a problem hiding this comment.
@su-su-su-su はい。UserRetirementに相応しい処理は全てそちらに書くべきです。
There was a problem hiding this comment.
回答ありがとうございました。UserRetirementに退会に関する処理を追加しました。
ご確認よろしくお願い致します。
su-su-su-su
force-pushed
the
chore/clear-github-data-on-account-deletion
branch
from
eb2db3f
to
61d4b4e
Compare
|
@su-su-su-su conflictの修正をお願いします〜 |
su-su-su-su
force-pushed
the
chore/clear-github-data-on-account-deletion
branch
from
f8c510e
to
b4c9d98
Compare
|
@komagata |
| @@ -0,0 +1,44 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class UserRetirement | |||
There was a problem hiding this comment.
@komagata
ご指摘ありがとうございます。
UserRetirement のテストを作成しているのですが、どこまでテストをカバーすべきか確認したく、ご相談させてください。
clear_github_dataメソッドのテストについては以前test/models/user_test.rbに作成しました。
退会時の通知に関してはtest/deliveries/activity_delivery_test.rbファイル内に以下のようなテストが既に存在しています。
test '.notify(:retired)' do
params = {
sender: users(:yameo),
receiver: users(:mentormentaro)
}
assert_difference -> { AbstractNotifier::Testing::Driver.deliveries.count }, 1 do
ActivityDelivery.notify!(:retired, **params)
end
assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 1 do
ActivityDelivery.notify(:retired, **params)
end
assert_difference -> { AbstractNotifier::Testing::Driver.deliveries.count }, 1 do
ActivityDelivery.with(**params).notify!(:retired)
end
assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 1 do
ActivityDelivery.with(**params).notify(:retired)
end
endこのように既存のテストがある場合でも、追加でテストを作成する必要があるかどうか、ご意見をいただけますでしょうか？
お手数をおかけしますが、アドバイスいただけますと幸いです。
よろしくお願いいたします。
There was a problem hiding this comment.
@su-su-su-su まず、 @su-su-su-su さんはどうするべきだと考えますか？？
There was a problem hiding this comment.
@komagata
コメントありがとうございます。
改めて考えてみるとclear_github_dataメソッドなど個々のメソッドに対するテストは既に存在しますが、executeメソッドがこれらのメソッドを正しく呼び出しているかを確認するテストが不足していると気付きました。
そのため、executeメソッドが退会処理を正しく実行しているかを検証するテストを追加したいと思います。
There was a problem hiding this comment.
@su-su-su-su 確かにですね。
本来systemテストでカバーしていればモデルのテストでは不要かもですが、ここはかなり重要で心配な処理なので、おっしゃる通りに退会処理が正しくできているかのテストを書いていただければありがたいです。
There was a problem hiding this comment.
@komagata
executeメソッドが他のメソッドを呼び出しているかを確認するテストを追加いたしました。
再度ご確認よろしくお願いいたします。
su-su-su-su
force-pushed
the
chore/clear-github-data-on-account-deletion
branch
from
b4c9d98
to
7a51f8b
Compare
Issue
概要
変更確認方法
rails consoleでRailsコンソールを立ち上げるUser.where(name: 'Kensyu Seiko').select(:name, :github_account, :github_id, :github_collaborator)でKensyu SeikoのGitHubアカウントとの連携のデータを確認foreman start -f Procfile.devでサーバーを立ち上げるUser.where(name: 'Kensyu Seiko').select(:name, :github_account, :github_id, :github_collaborator)でKensyu SeikoのGitHubアカウントとの連携のデータが空になっていることを確認Screenshot
変更前
変更後