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

退会したらGitHubアカウントとの連携のデータを空になるように修正 #7931

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
32 changes: 1 addition & 31 deletions app/controllers/retirement_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,8 @@ def create
current_user.cancel_participation_from_regular_events
current_user.delete_and_assign_new_organizer
Newspaper.publish(:retirement_create, { user: })
UserRetirement.new(user).execute

destroy_subscription(user)
destroy_card(user)
notify_to_user(user)
notify_to_admins(user)
notify_to_mentors(user)
logout
redirect_to retirement_url
else
Expand All @@ -34,30 +30,4 @@ def create
def retire_reason_params
params.require(:user).permit(:retire_reason, :satisfaction, :opinion, retire_reasons: [])
end

def destroy_subscription(user)
Subscription.new.destroy(user.subscription_id) if user.subscription_id?
end

def destroy_card(user)
Card.destroy_all(user.customer_id) if user.customer_id?
end

def notify_to_user(user)
UserMailer.retire(user).deliver_now
rescue Postmark::InactiveRecipientError => e
logger.warn "[Postmark] 受信者由来のエラーのためメールを送信できませんでした。:#{e.message}"
end

def notify_to_admins(user)
User.admins.each do |admin_user|
ActivityDelivery.with(sender: user, receiver: admin_user).notify(:retired)
end
end

def notify_to_mentors(user)
User.mentor.each do |mentor_user|
ActivityDelivery.with(sender: user, receiver: mentor_user).notify(:retired)
end
end
end
8 changes: 8 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,14 @@ def participated_regular_event_ids
RegularEvent.where(id: regular_event_participations.pluck(:regular_event_id), finished: false)
end

def clear_github_data
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
Member

Choose a reason for hiding this comment

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

@motohiro-mm すみません、ちょっと質問の意図が理解できてないかもです。

基本的にはモデルにメソッドを追加した場合、モデルのテスト(ユニットテスト)の追加は必要になります。

Copy link
Contributor

Choose a reason for hiding this comment

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

@komagata

私が最初にレビューさせていただいたときには、モデルのテストが追加されていなかったため、このようにコメントさせていただきました。
その後、test/models/user_test.rb:740test 'clear_github_data should clear GitHub related fields'が追加されています。

Copy link
Member

Choose a reason for hiding this comment

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

@motohiro-mm なるほどです〜

update(
github_id: nil,
github_account: nil,
github_collaborator: false
)
end

def area
if country_code == 'JP'
subdivision = ISO3166::Country['JP'].subdivisions[subdivision_code]
Expand Down
44 changes: 44 additions & 0 deletions app/models/user_retirement.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# frozen_string_literal: true

class UserRetirement
Copy link
Member

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.

@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

このように既存のテストがある場合でも、追加でテストを作成する必要があるかどうか、ご意見をいただけますでしょうか?
お手数をおかけしますが、アドバイスいただけますと幸いです。
よろしくお願いいたします。

Copy link
Member

Choose a reason for hiding this comment

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

@su-su-su-su まず、 @su-su-su-su さんはどうするべきだと考えますか??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komagata
コメントありがとうございます。
改めて考えてみるとclear_github_dataメソッドなど個々のメソッドに対するテストは既に存在しますが、executeメソッドがこれらのメソッドを正しく呼び出しているかを確認するテストが不足していると気付きました。
そのため、executeメソッドが退会処理を正しく実行しているかを検証するテストを追加したいと思います。

Copy link
Member

Choose a reason for hiding this comment

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

@su-su-su-su 確かにですね。
本来systemテストでカバーしていればモデルのテストでは不要かもですが、ここはかなり重要で心配な処理なので、おっしゃる通りに退会処理が正しくできているかのテストを書いていただければありがたいです。

Copy link
Contributor Author

@su-su-su-su su-su-su-su Nov 17, 2024

Choose a reason for hiding this comment

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

@komagata
executeメソッドが他のメソッドを呼び出しているかを確認するテストを追加いたしました。
再度ご確認よろしくお願いいたします。

def initialize(user)
@user = user
end

def execute
@user.clear_github_data
destroy_subscription
destroy_card
notify_to_user
notify_to_admins
notify_to_mentors
end

private

def destroy_subscription
Subscription.new.destroy(@user.subscription_id) if @user.subscription_id?
end

def destroy_card
Card.destroy_all(@user.customer_id) if @user.customer_id?
end

def notify_to_user
UserMailer.retire(@user).deliver_now
rescue Postmark::InactiveRecipientError => e
Rails.logger.warn "[Postmark] 受信者由来のエラーのためメールを送信できませんでした。:#{e.message}"
end

def notify_to_admins
User.admins.each do |admin_user|
ActivityDelivery.with(sender: @user, receiver: admin_user).notify(:retired)
end
end

def notify_to_mentors
User.mentor.each do |mentor_user|
ActivityDelivery.with(sender: @user, receiver: mentor_user).notify(:retired)
end
end
end
35 changes: 35 additions & 0 deletions test/models/user_retirement_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# frozen_string_literal: true

require 'test_helper'

class UserRetirementTest < ActiveSupport::TestCase
test 'execute method performs all retirement steps' do
user = users(:kimura)
retirement = UserRetirement.new(user)

methods_called = []

user.define_singleton_method(:clear_github_data) do
methods_called << :clear_github_data
end

%i[destroy_subscription destroy_card notify_to_user notify_to_admins notify_to_mentors].each do |method_name|
retirement.define_singleton_method(method_name) do
methods_called << method_name
end
end

retirement.execute

expected_methods = %i[
clear_github_data
destroy_subscription
destroy_card
notify_to_user
notify_to_admins
notify_to_mentors
]

assert_equal expected_methods, methods_called
end
end
14 changes: 14 additions & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -754,4 +754,18 @@ class UserTest < ActiveSupport::TestCase
america_users = [users(:neverlogin), users(:tom)]
assert_equal User.by_area('米国').to_a.sort, america_users.sort
end

test 'clear_github_data should clear GitHub related fields' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test 'clear_github_data should clear GitHub related fields' do
test '#clear_github_data' do

モデルのテストで、代表的なテストだけだったら他と一緒でこういう名前でいいかもです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。

test '#clear_github_data' do

と修正いたしました。

user = users(:kimura)
user.github_id = '12345'
user.github_account = 'github_kimura'
user.github_collaborator = true
user.save!(validate: false)

user.clear_github_data

assert_nil user.github_id
assert_nil user.github_account
assert_not user.github_collaborator
end
end
18 changes: 18 additions & 0 deletions test/system/retirement_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -229,4 +229,22 @@ class RetirementTest < ApplicationSystemTestCase
assert_no_selector '.is-kimura'
assert_selector '.is-komagata'
end

test 'should clear github data on account deletion' do
user = users(:kimura)
user.github_id = '12345'
user.github_account = 'github_kimura'
user.github_collaborator = true
user.save!(validate: false)
visit_with_auth new_retirement_path, 'kimura'
find('label', text: 'とても良い').click
page.accept_confirm '本当によろしいですか?' do
click_on '退会する'
end
assert_text '退会処理が完了しました'
user.reload
assert_nil user.github_id
assert_nil user.github_account
assert_not user.github_collaborator
end
end