お知らせにあるWIPの処理をnewspaperに置き換える #5616

akingo55 · 2022-10-02T06:33:16Z

Issue

[newspaper] お知らせにあるWIPの処理をnewspaperに置き換えたい #5494

変更点

お知らせのWIPの処理をnewspaperに置き換えました。
お知らせの新規作成時、更新時にお知らせを公開するときの処理に影響があります。
見た目上、動作上の変更はありません。

確認方法

※ローカルにfeature/replace-announce-processing-from-callback-to-newspaperブランチを落として切り替えてください。

mentormentaroでログインする
お知らせを新規作成し、「全員」に「公開」し、お知らせが公開されていることを確認する

一度ログアウトし、別のユーザーでログインする（今回はkomagataでログインしました）
通知にさきほど公開したお知らせの通知が来ていることを確認する

bin/rails cで最新のお知らせを取得し、published_atに公開日時が入っていることを確認する

irb(main):004:0> Announcement.last
  Announcement Load (0.8ms)  SELECT "announcements".* FROM "announcements" ORDER BY "announcements"."id" DESC LIMIT $1  [["LIMIT", 1]]
=>
#<Announcement:0x000000010aba2538
 id: 1061722995,
 title: "おしらせテスト４",
 description: "ああああああああ",
 created_at: Sun, 02 Oct 2022 13:56:42.444950000 JST +09:00,
 updated_at: Sun, 02 Oct 2022 13:57:11.395091000 JST +09:00,
 user_id: 534981761,
 target: "all",
 wip: false,
 published_at: Sun, 02 Oct 2022 13:57:11.394889000 JST +09:00>
irb(main):005:0>

akingo55 · 2022-10-02T07:14:40Z

app/models/announcement_notifier.rb

+    return if announce.wip?
+    return unless !announce.wip && announce.published_at.nil?


もともとafter_first_publishメソッドは、after_createとafter_updateメソッドから呼び出されているので、同一条件下で呼び出されるようにしています。
https://github.com/fjordllc/bootcamp/blob/main/app/models/announcement_callbacks.rb#L26

akingo55 · 2022-10-02T07:20:25Z

@AyakaTakashima
お疲れ様です！
以前newspaperの置き換えのIssueを対応されていたかと思うので、こちらのPRをレビューしていただきたいです！:pray:
お手隙によろしくお願いします！

AyakaTakashima

@akingo55
お疲れ様です！
レビュー依頼ありがとうございます！
動作、コード共に問題ないと思います🎉
approveさせていただきます🙌

akingo55 · 2022-10-03T12:45:06Z

@AyakaTakashima
ありがとうございます！

@komagata
メンバーのレビューが通りましたので、お手隙にご確認お願いします🙏🏻

komagata

conflictの修正をお願いします〜

akingo55 · 2022-10-05T15:09:42Z

@komagata
コンフリクト解消しましたので、再度確認お願いします:pray:

komagata · 2022-10-06T18:46:27Z

@akingo55 すみません、またconflictが起こってしまったので修正をお願いします〜

akingo55 · 2022-10-10T09:13:14Z

@komagata
遅くなりすみません:pray:
再度コンフリクト解消しましたので、ご確認お願いします！

komagata · 2022-10-11T06:11:47Z

app/models/announcement_notifier.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class AnnouncementNotifier


やっていることがpublished_atの更新なので、AnnouncementNotifer（お知らせ通知）という名前だと見る人が混乱するかもです。

komagata · 2022-10-11T06:12:53Z

app/models/announcement_notifier.rb

+    return if announce.wip?
+    return unless !announce.wip && announce.published_at.nil?


元々は別々のところにあったコードなので、この条件であればもっとシンプルに書けると思います。

akingo55 · 2022-10-18T15:19:14Z

@komagata
すみません、遅くなりました:pray:
修正完了しましたので、再度レビューお願いします。

komagata

conflictの修正をお願いします〜。

それと、既にあればOKですが、お知らせのWIPの処理がちゃんと動いているかのテストがなければ追加をお願いします〜
（この処理が間違っていると、お知らせの通知が全ユーザーに飛んでしまったりと影響範囲が大きいためです。）

akingo55 · 2022-10-26T10:30:11Z

@komagata
コンフリクト修正＆announcementの処理を1つにまとめました。（処理が散らばっておりわかりづらかったため）
開発環境にて以下の動作確認済みです。

お知らせがwipの時は通知飛ばないこと（published_atはnullでwipがtrueであること）
お知らせ公開された時は通知が飛ぶこと
公開時にはpublished_atに日時が入り, wipカラムがfalseに更新されること

テストはこちらのPRで追加されていたので、新規追加分はなさそうです。
https://github.com/fjordllc/bootcamp/pull/5662/files

再レビューお願いします。:pray:

akingo55 · 2022-10-29T06:55:36Z

@komagata
お手数ですが、こちらレビューお願いいたします。🙇‍♀️

komagata · 2022-10-31T05:27:47Z

app/models/announcement.rb

@@ -18,9 +18,6 @@ class Announcement < ApplicationRecord
  belongs_to :user
  alias sender user

-  after_create AnnouncementCallbacks.new


komagata · 2022-10-31T05:28:17Z

app/models/announcement_notifier.rb

@@ -2,6 +2,11 @@

 class AnnouncementNotifier
  def call(announce)
+    return if announce.wip? || !announce.published_at.nil?


Suggested change

return if announce.wip? || !announce.published_at.nil?

return if announce.wip? || announce.published_at.present?

二重否定よりこちらの方が良いかもです〜

@komagata
ありがとうございます！
こちら修正しました:pray:
a21f8e2

@komagata
こちら再度確認お願いしますmm

app/models/announcement_notifier.rb

akingo55 · 2022-11-11T04:00:41Z

@komagata 確認お願いします！

komagata · 2022-11-11T07:01:35Z

@akingo55 マージコミットが含まれているようなので、rebaseの方式で、マージコミットができないようにお願いします〜

Co-authored-by: Masaki Komagata <[email protected]>

akingo55 · 2022-11-13T04:54:31Z

@komagata
すみません！マージコミット消しましたので再度確認お願いします！

komagata

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

akingo55 commented Oct 2, 2022

View reviewed changes

akingo55 requested a review from AyakaTakashima October 2, 2022 07:18

akingo55 self-assigned this Oct 2, 2022

AyakaTakashima approved these changes Oct 3, 2022

View reviewed changes

komagata reviewed Oct 4, 2022

View reviewed changes

akingo55 force-pushed the feature/replace-announce-processing-from-callback-to-newspaper branch from fb596f7 to c576708 Compare October 5, 2022 13:53

akingo55 force-pushed the feature/replace-announce-processing-from-callback-to-newspaper branch from 9b00164 to beae3a4 Compare October 10, 2022 09:11

komagata requested changes Oct 11, 2022

View reviewed changes

akingo55 force-pushed the feature/replace-announce-processing-from-callback-to-newspaper branch 2 times, most recently from b89d3aa to 909edab Compare October 17, 2022 15:26

komagata reviewed Oct 21, 2022

View reviewed changes

akingo55 force-pushed the feature/replace-announce-processing-from-callback-to-newspaper branch from 3ecf1e0 to fd6b3ef Compare October 25, 2022 14:43

komagata reviewed Oct 31, 2022

View reviewed changes

akingo55 mentioned this pull request Nov 3, 2022

お知らせのWatch処理をnewspaperに置き換える #5723

Merged

komagata reviewed Nov 7, 2022

View reviewed changes

app/models/announcement_notifier.rb Outdated Show resolved Hide resolved

akingo55 added 5 commits November 13, 2022 13:42

お知らせにあるWIPの処理をnewspaperに置き換える

c99a281

debug用コメントを削除

1b8ccae

命名修正&リファクタ

f2a7c5b

不要なメソッド削除

dd6df8e

条件式の修正

ce97bb4

akingo55 and others added 7 commits November 13, 2022 13:42

rubocop修正

2b7f9d0

newpaperの呼び出しを統一化させる

099d673

お知らせのnewpaper処理をまとめる

51dab54

rubocop指摘修正

936a606

nil?をpresent?に置き換える

2b33c79

お知らせのWatch処理をnewspaperに置き換える

417105d

announce.published_at?に変更する

afc04b7

Co-authored-by: Masaki Komagata <[email protected]>

akingo55 force-pushed the feature/replace-announce-processing-from-callback-to-newspaper branch from 798db1e to afc04b7 Compare November 13, 2022 04:43

komagata approved these changes Nov 14, 2022

View reviewed changes

komagata merged commit 01002d3 into main Nov 14, 2022

komagata deleted the feature/replace-announce-processing-from-callback-to-newspaper branch November 14, 2022 14:09

github-actions bot mentioned this pull request Nov 14, 2022

Release 2022-11-17 08:19:56 #5765

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

お知らせにあるWIPの処理をnewspaperに置き換える #5616

お知らせにあるWIPの処理をnewspaperに置き換える #5616

akingo55 commented Oct 2, 2022 •

edited

Loading

akingo55 Oct 2, 2022

akingo55 commented Oct 2, 2022

AyakaTakashima left a comment

akingo55 commented Oct 3, 2022

komagata left a comment

akingo55 commented Oct 5, 2022 •

edited

Loading

komagata commented Oct 6, 2022

akingo55 commented Oct 10, 2022

komagata Oct 11, 2022

komagata Oct 11, 2022

akingo55 commented Oct 18, 2022

komagata left a comment

akingo55 commented Oct 26, 2022 •

edited

Loading

akingo55 commented Oct 29, 2022

komagata Oct 31, 2022

komagata Oct 31, 2022

akingo55 Oct 31, 2022 •

edited

Loading

akingo55 Nov 5, 2022

akingo55 commented Nov 11, 2022

komagata commented Nov 11, 2022

akingo55 commented Nov 13, 2022

komagata left a comment

		return if announce.wip?
		return unless !announce.wip && announce.published_at.nil?

		@@ -0,0 +1,10 @@
		# frozen_string_literal: true

		class AnnouncementNotifier

	return if announce.wip? \|\| !announce.published_at.nil?
	return if announce.wip? \|\| announce.published_at.present?

お知らせにあるWIPの処理をnewspaperに置き換える #5616

お知らせにあるWIPの処理をnewspaperに置き換える #5616

Conversation

akingo55 commented Oct 2, 2022 • edited Loading

Issue

変更点

確認方法

akingo55 Oct 2, 2022

Choose a reason for hiding this comment

akingo55 commented Oct 2, 2022

AyakaTakashima left a comment

Choose a reason for hiding this comment

akingo55 commented Oct 3, 2022

komagata left a comment

Choose a reason for hiding this comment

akingo55 commented Oct 5, 2022 • edited Loading

komagata commented Oct 6, 2022

akingo55 commented Oct 10, 2022

komagata Oct 11, 2022

Choose a reason for hiding this comment

komagata Oct 11, 2022

Choose a reason for hiding this comment

akingo55 commented Oct 18, 2022

komagata left a comment

Choose a reason for hiding this comment

akingo55 commented Oct 26, 2022 • edited Loading

akingo55 commented Oct 29, 2022

komagata Oct 31, 2022

Choose a reason for hiding this comment

komagata Oct 31, 2022

Choose a reason for hiding this comment

akingo55 Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

akingo55 Nov 5, 2022

Choose a reason for hiding this comment

akingo55 commented Nov 11, 2022

komagata commented Nov 11, 2022

akingo55 commented Nov 13, 2022

komagata left a comment

Choose a reason for hiding this comment

akingo55 commented Oct 2, 2022 •

edited

Loading

akingo55 commented Oct 5, 2022 •

edited

Loading

akingo55 commented Oct 26, 2022 •

edited

Loading

akingo55 Oct 31, 2022 •

edited

Loading