-
Notifications
You must be signed in to change notification settings - Fork 76
定期イベントの主催者が追加されたら新しく任命された人に通知する #9593
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
Changes from all commits
2f982f7
c1c39e1
3ba385f
6e20183
b446db4
e8576ed
339a6e8
0c0426a
969e988
cc9f0d2
f9f3ba5
e9945f1
119414c
d5d58b8
e6070b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class OrganizerNotifier | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 口頭でも伝えましたが、 |
||
| def call(_name, _started, _finished, _id, payload) | ||
| regular_event = payload[:regular_event] | ||
| sender = payload[:sender] | ||
| new_organizer_users = payload[:new_organizer_users] | ||
|
|
||
| new_organizer_users.each do |user| | ||
| ActivityDelivery.with(regular_event:, sender:, receiver: user).notify(:added_organizer) | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,13 +118,6 @@ def participated_by?(user) | |
| regular_event_participations.find_by(user_id: user.id).present? | ||
| end | ||
|
|
||
| def assign_admin_as_organizer_if_none | ||
| return if organizers.exists? | ||
|
|
||
| admin_user = User.find_by(login_name: User::DEFAULT_REGULAR_EVENT_ORGANIZER) | ||
| Organizer.new(user: admin_user, regular_event: self).save if admin_user | ||
| end | ||
|
|
||
| def all_scheduled_dates( | ||
| from: Date.current, | ||
| to: Date.current.next_year | ||
|
|
@@ -157,6 +150,29 @@ def publish_with_announcement? | |
| wants_announcement? && !wip? | ||
| end | ||
|
|
||
| def hand_over_organizer(organizer:, sender:) | ||
| before_organizer_user_ids = organizers.pluck(:user_id) | ||
|
|
||
| organizer.delete | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. この機能は削除することになったのでもう関係はないですが、一応コメントします。 もっと綺麗に書けるかもしれませんが、以下のような流れでいけると思います。 if organizers.count > 1
organizer.destory
return
# 主催者が引数で渡ってきた`organizer`一人だけなので`komagata`を主催者に追加した後`organizer.destory`する処理
# ...
notify_new_organizer(sender:, before_organizer_user_ids:) |
||
| assign_admin_as_organizer_if_none | ||
|
|
||
| notify_new_organizer(sender:, before_organizer_user_ids:) | ||
| end | ||
|
|
||
| def notify_new_organizer(sender:, before_organizer_user_ids:) | ||
| new_organizer_user_ids = (organizers.pluck(:user_id) - before_organizer_user_ids) - [sender.id] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return if new_organizer_user_ids.blank? | ||
|
|
||
| new_organizer_users = User.where(id: new_organizer_user_ids) | ||
|
|
||
| ActiveSupport::Notifications.instrument( | ||
| 'organizer.add', | ||
| regular_event: self, | ||
| sender:, | ||
| new_organizer_users: | ||
| ) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def end_at_be_greater_than_start_at | ||
|
|
@@ -197,4 +213,11 @@ def parse_event_time(event_date, event_time) | |
| str_time = event_time.strftime('%R') | ||
| Time.zone.parse([str_date, str_time].join(' ')) | ||
| end | ||
|
|
||
| def assign_admin_as_organizer_if_none | ||
| return if organizers.exists? | ||
|
|
||
| admin_user = User.find_by(login_name: User::DEFAULT_REGULAR_EVENT_ORGANIZER) | ||
| Organizer.new(user: admin_user, regular_event: self).save if admin_user | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -883,12 +883,8 @@ def become_watcher!(watchable) | |
| watches.find_or_create_by!(watchable:) | ||
| end | ||
|
|
||
| def cancel_participation_from_regular_events | ||
| regular_event_participations.destroy_all | ||
| end | ||
|
|
||
| def delete_and_assign_new_organizer | ||
| organizers.each(&:delete_and_assign_new) | ||
| def cancel_participation_from_holding_regular_events | ||
| regular_event_participations.holding.destroy_all | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここでの"開催中の"という意味には
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ご指摘ありがとうございます! すでに複数表現がある状態でさらに新しい概念を混ぜるとさらにややこしくなる懸念があるのと、使用されている箇所を見ると検証箇所がかなり増えてしまう恐れがあるため、今回は退会/休会/研修終了動線のロジックで使われている 表現としては
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yokomaru |
||
| end | ||
|
|
||
| def scheduled_retire_at | ||
|
|
@@ -958,6 +954,12 @@ def reports_with_learning_times | |
| reports.joins(:learning_times).distinct.order(reported_on: :asc) | ||
| end | ||
|
|
||
| def hand_over_organizers_of_holding_regular_events | ||
| organizers.holding.includes(:regular_event).find_each do |organizer| | ||
| organizer.regular_event.hand_over_organizer(organizer:, sender: self) | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def password_required? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| = render '/notification_mailer/notification_mailer_template', | ||
| title: "定期イベント【#{@regular_event.title}】の主催者に追加されました。", | ||
| link_url: @link_url, | ||
| link_text: '定期イベント詳細へ' |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1285,6 +1285,25 @@ class ActivityMailerTest < ActionMailer::TestCase | |||||||||
| assert_empty ActionMailer::Base.deliveries | ||||||||||
| end | ||||||||||
|
|
||||||||||
| test 'added_organizer' do | ||||||||||
| regular_event = RegularEvent.find(ActiveRecord::FixtureSet.identify(:regular_event1)) | ||||||||||
| receiver = User.find(ActiveRecord::FixtureSet.identify(:komagata)) | ||||||||||
|
Comment on lines
+1289
to
+1290
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
こちらの方が可読性が高くて良いと思います。 |
||||||||||
| sender = User.find(ActiveRecord::FixtureSet.identify(:kimura)) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ここも |
||||||||||
|
|
||||||||||
| ActivityMailer.added_organizer( | ||||||||||
| regular_event:, | ||||||||||
| receiver:, | ||||||||||
| sender: | ||||||||||
| ).deliver_now | ||||||||||
|
Comment on lines
+1293
to
+1297
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. プロダクションコードでは他のメール通知も含め引数付きでメール送信処理を呼び出している箇所はなく、今回実装したメール通知も bootcamp/app/models/organizer_notifier.rb Line 10 in e6070b8
ここはチームの方針もあるので難しいですが、プロダクションコードで使われている方だけをテストするのが良いと思います。 |
||||||||||
|
|
||||||||||
| assert_not ActionMailer::Base.deliveries.empty? | ||||||||||
| email = ActionMailer::Base.deliveries.last | ||||||||||
| assert_equal ['noreply@bootcamp.fjord.jp'], email.from | ||||||||||
| assert_equal ['komagata@fjord.jp'], email.to | ||||||||||
| assert_equal '[FBC] 定期イベント【開発MTG】の主催者に追加されました。', email.subject | ||||||||||
| assert_match(/定期イベント/, email.body.to_s) | ||||||||||
| end | ||||||||||
|
|
||||||||||
| private | ||||||||||
|
|
||||||||||
| def mailer_url_options | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -186,4 +186,11 @@ def added_work | |||
|
|
||||
| ActivityMailer.with(work:, sender: user, receiver:).added_work | ||||
| end | ||||
|
|
||||
| def added_organizer | ||||
| regular_event = RegularEvent.find(ActiveRecord::FixtureSet.identify(:regular_event1)) | ||||
| receiver = User.find(ActiveRecord::FixtureSet.identify(:komagata)) | ||||
| sender = User.find(ActiveRecord::FixtureSet.identify(:kimura)) | ||||
| ActivityMailer.with(regular_event:, receiver:, sender:).added_organizer | ||||
|
Comment on lines
+193
to
+194
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. メール通知では基本的にデフォルトの自動送信用メールアドレスを使用するようになっています。
あと大したことではないので修正するほどではないですが、テストデータは実際のシナリオに近いものを選んだ方がわかりやすいです。 |
||||
| end | ||||
| end | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'test_helper' | ||
|
|
||
| class OrganizerNotifierTest < ActiveSupport::TestCase | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| include ActiveJob::TestHelper | ||
|
|
||
| setup do | ||
| @regular_event = regular_events(:regular_event4) | ||
| @sender = users(:kimura) | ||
| end | ||
|
|
||
| test 'sends a notification when a new organizer is added' do | ||
| new_organizer_users = [users(:hatsuno)] | ||
|
|
||
| assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 1 do | ||
| OrganizerNotifier.new.call(nil, nil, nil, nil, { regular_event: @regular_event, new_organizer_users:, sender: @sender }) | ||
| end | ||
| end | ||
|
|
||
| test 'does not send a notification when no new organizers are added' do | ||
| new_organizer_users = [] | ||
|
|
||
| assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 0 do | ||
| OrganizerNotifier.new.call(nil, nil, nil, nil, { regular_event: @regular_event, new_organizer_users:, sender: @sender }) | ||
| end | ||
| end | ||
|
|
||
| test 'sends notifications when multiple organizers are added' do | ||
| new_organizer_users = [users(:hatsuno), users(:hajime)] | ||
|
|
||
| assert_difference -> { AbstractNotifier::Testing::Driver.enqueued_deliveries.count }, 2 do | ||
| OrganizerNotifier.new.call(nil, nil, nil, nil, { regular_event: @regular_event, new_organizer_users:, sender: @sender }) | ||
| end | ||
| end | ||
| end | ||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before_organizer_user_idsを直訳すると"主催者ユーザーIDの(時間的もしくは位置的に)前に"という意味になり変です。他の箇所で、更新後の主催者を
new_organizer_usersとしているのでそれとの対比でold_organizer_user_idsとすると良さそうです。