卒業生に通知がいかないように修正 & テストの修正 #4141

maeda-seina · 2022-02-04T14:01:26Z

issue

「はじめての日報を書きました！」通知は現役生とメンターのみにする #3981

修正PR

「はじめての日報を書きました！」通知は現役生とメンターのみに通知する #4012

概要

#4012 のPRに不備があり、卒業生に「はじめての日報を書きました！」通知がいってしまっていたのでその修正を行いました。

変更内容

app/models/notification_facade.rb:78のif receiver.student_or_trainee?のstudent_or_traineeメソッドでは卒業生も対象になってしまっていたので、そちらのメソッドをstudent_or_trainee_or_retired?に変更しました。
また、test/system/reports_test.rb:6の卒業生のテストが機能していなかったため、機能させるためにwait_for_vuejsメソッドを追加しておきました。

maeda-seina · 2022-02-04T14:22:35Z

@haruguchi-yuma さん
お疲れ様です！
お時間ある時に、こちらのレビューお願い致します🙇‍♂️

haruguchi-yuma

問題ないと思いました。OKです〜！:tada:

今回の変更点ではないんですが、テストの部分で日報を作るところがありますが、titleとdescriptionが意味のあるものだとテストが読みやすかなーと思いました！

test 'the first daily report notification is sent only to current students and mentors' do
    report = users(:muryou).reports.create!(
      title: 'test title',  #ここ
      description: 'test',　#ここ
      reported_on: Date.current
    )
...

本質的な部分ではないので変更はどちらでも結構です〜！

ima1zumi · 2022-02-05T11:36:48Z

app/models/notification_facade.rb

@@ -75,7 +75,7 @@ def self.came_question(question, receiver)
  end

  def self.first_report(report, receiver)
-    Notification.first_report(report, receiver) if receiver.student_or_trainee? || receiver.admin_or_mentor?
+    Notification.first_report(report, receiver) if receiver.student_or_trainee_or_retired? || receiver.admin_or_mentor?


これだとリタイアしたユーザーも通知の対象になってしまうので
卒業生ではない人かつ student または trainee または admin または advisor を対象にすると良さそうです 😄

@ima1zumi さん
レビューして頂きありがとうございます！😊
ご指摘してもらった通り、リタイアしたユーザーへの通知を考慮できていませんでした😅
教えていただいた内容で通知対象を修正をしたいと思います！

メールのこと考えるの忘れてました。確かにリタイヤした人に通知行くのはダメですね、、🥲

maeda-seina · 2022-02-05T15:51:27Z

@haruguchi-yuma さん
レビューして頂き、ありがとうございます!!
おっしゃる通りだと思いましたので、テストのtitleとdescriptionを修正しようと思います！🙇‍♂️ 😄

maeda-seina · 2022-02-05T16:10:36Z

@haruguchi-yuma さん、@ima1zumi さん
お手数おかけしますが、再度レビューのほどよろしくお願い致します🙇‍♂️

haruguchi-yuma · 2022-02-06T05:35:26Z

@maeda-seina (cc @ima1zumi )
リタイヤしたユーザの視点でいくとこの条件でも条件式全体がtrueになってしまうような気がします。😭
以下rails consoleの結果です。studentかどうかの判定ににリタイヤしたユーザがヒットしてしまうので、、、

 retired_user.name
=> "辞目 辞目夫"
 retired_user.student?
=> true
retired_user.student_or_trainee?
=> true
 retired_user.student_or_trainee_or_retired?
=> true

明示的に!retired?という条件を加えるのがいいかも？

maeda-seina · 2022-02-06T06:11:30Z

@haruguchi-yuma さん
再度レビューして頂き、ありがとうございます！！

リタイヤしたユーザの視点でいくとこの条件でも条件式全体がtrueになってしまうような気がします。😭

ほんとうですね...😅（何度も申し訳ないです🙏）
!retired?を条件に加えて修正させて頂きます!!🙏

maeda-seina · 2022-02-07T14:09:33Z

@haruguchi-yuma さん
お疲れ様です。
お手数お掛けしますが、再度レビューのほどよろしくお願い致します🙇‍♂️
app/models/notification_facade.rb:78の条件式receiver.student_or_trainee? && !receiver.graduated? && !receiver.retired?の部分をcurrent_student?というメソッド名で切り出しました。（current_student?メソッドにはuser.rbにある既存のメソッドを使用すると誤解を招いたり複雑になると思ったため、カラムを使用して定義しました。）

@komagata さん、 @machida さん
current_student?メソッドを使用して変更できる箇所がないか探した所、app/views/application/_header.html.slim:7でstudent_or_trainee?メソッドが使用されていまして、student_or_trainee? メソッドの中身はstudent? || trainee? となっており、student?メソッドには卒業生も含まれているため、卒業生でログインしてもヘッダー部分には卒業生ロールが表示されていませんでした。
なので、current_student?メソッドを使用して、卒業生ロールが表示されるよう修正をしました。

修正前

修正後

haruguchi-yuma · 2022-02-07T23:27:27Z

@maeda-seina
確認しました〜！メソッド定義するという判断もナイスだと思います。僕の方からはOKです:rocket:

maeda-seina · 2022-02-08T02:38:52Z

@haruguchi-yuma さん
何度もレビューしていただき、ありがとうございました😊

@komagata さん
お疲れ様です！
お時間ある時に、こちらのレビューお願い致します🙇‍♂️

komagata · 2022-02-09T05:10:39Z

@maeda-seina

なので、current_student?メソッドを使用して、卒業生ロールが表示されるよう修正をしました。

ありがとうございます〜👍

komagata · 2022-02-09T05:14:02Z

test/system/notification/reports_test.rb

@@ -20,16 +20,20 @@ class Notification::ReportsTest < ApplicationSystemTestCase
    )

    notification_message = 'muryouさんがはじめての日報を書きました！'
-    visit_with_auth '/notifications', 'komagata'
+    visit_with_auth '/notifications', 'machida'
+    wait_for_vuejs


これ、中身は単なるsleepなので、できれば使いたくない感じなんですよね。
vue.jsで表示しているものが表示されるまで待つような処理をきちんと書きたい感じがします。

理由はsleepだと不要な場合でも必ずその秒数待つのでテストが遅くなるのと、重さなどでその秒数を単に超えるとエラーになり、flakyなテストになっちゃうからです。

@komagata さん
了解いたしました！
前教えていただいた、findメソッドを使ってテスト作成できるかやってみます！💪

maeda-seina · 2022-02-09T14:02:09Z

@komagata さん
お疲れ様です。
再度レビューのほどよろしくお願い致します🙏
（先日komagataさんに教えて頂いたコードを参考にテストコードの修正を行いました。）

参考
https://github.com/fjordllc/bootcamp/blob/main/test/system/comments_test.rb#L52
https://github.com/fjordllc/bootcamp/blob/main/app/javascript/comments.vue

komagata

確認しました、OKですー🙆‍♂️

maeda-seina self-assigned this Feb 4, 2022

maeda-seina requested a review from haruguchi-yuma February 4, 2022 14:22

haruguchi-yuma approved these changes Feb 5, 2022

View reviewed changes

ima1zumi reviewed Feb 5, 2022

View reviewed changes

maeda-seina force-pushed the bug/fix-first-daily-report-notification branch from cccdc5a to 6bcc9c5 Compare February 5, 2022 15:50

maeda-seina force-pushed the bug/fix-first-daily-report-notification branch from e541bd2 to c7f1e3f Compare February 7, 2022 14:49

komagata requested changes Feb 9, 2022

View reviewed changes

maeda-seina added 8 commits February 9, 2022 22:05

卒業生に通知がいかないように修正 & テストの修正

17b77e9

通知対象にリタイアしたユーザーを含んでしまっていたのを修正 & テスト内容の一部修正

2240c14

リタイアした人にサイト内通知がいかないよう再修正

0b22d64

current_student?メソッドを追加

a813396

卒業生のロールがheaderに表示されるよう修正

29da337

current_student?メソッドを修正

d4eff22

adminとmentorに通知がいくよう修正 & テスト内容に不備があったので修正

62f6264

wait_for_vuejsメソッドの使用をやめて、vue.jsで表示しているものが表示されるまで待つような処理に修正

bf6b5af

maeda-seina force-pushed the bug/fix-first-daily-report-notification branch from c7f1e3f to bf6b5af Compare February 9, 2022 13:05

komagata approved these changes Feb 14, 2022

View reviewed changes

komagata merged commit 6ca71eb into main Feb 14, 2022

komagata deleted the bug/fix-first-daily-report-notification branch February 14, 2022 06:02

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

Release 2022-02-18 02:36:10 #4129

Merged

54 tasks

Paru871 mentioned this pull request Feb 17, 2022

sadがついた日報にコメントを書いて『確認OKにする』ボタンを押した際に『今日の気分は「sad」ですが、コメント無しで確認しますか？』が表示されないようにした #4239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

卒業生に通知がいかないように修正 & テストの修正 #4141

卒業生に通知がいかないように修正 & テストの修正 #4141

maeda-seina commented Feb 4, 2022 •

edited

Loading

maeda-seina commented Feb 4, 2022

haruguchi-yuma left a comment

ima1zumi Feb 5, 2022

maeda-seina Feb 5, 2022

haruguchi-yuma Feb 6, 2022

maeda-seina commented Feb 5, 2022

maeda-seina commented Feb 5, 2022

haruguchi-yuma commented Feb 6, 2022 •

edited

Loading

maeda-seina commented Feb 6, 2022

maeda-seina commented Feb 7, 2022 •

edited

Loading

haruguchi-yuma commented Feb 7, 2022

maeda-seina commented Feb 8, 2022

komagata commented Feb 9, 2022

komagata Feb 9, 2022

maeda-seina Feb 9, 2022 •

edited

Loading

maeda-seina commented Feb 9, 2022

komagata left a comment

卒業生に通知がいかないように修正 & テストの修正 #4141

卒業生に通知がいかないように修正 & テストの修正 #4141

Conversation

maeda-seina commented Feb 4, 2022 • edited Loading

issue

修正PR

概要

変更内容

maeda-seina commented Feb 4, 2022

haruguchi-yuma left a comment

Choose a reason for hiding this comment

ima1zumi Feb 5, 2022

Choose a reason for hiding this comment

maeda-seina Feb 5, 2022

Choose a reason for hiding this comment

haruguchi-yuma Feb 6, 2022

Choose a reason for hiding this comment

maeda-seina commented Feb 5, 2022

maeda-seina commented Feb 5, 2022

haruguchi-yuma commented Feb 6, 2022 • edited Loading

maeda-seina commented Feb 6, 2022

maeda-seina commented Feb 7, 2022 • edited Loading

haruguchi-yuma commented Feb 7, 2022

maeda-seina commented Feb 8, 2022

komagata commented Feb 9, 2022

komagata Feb 9, 2022

Choose a reason for hiding this comment

maeda-seina Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

maeda-seina commented Feb 9, 2022

komagata left a comment

Choose a reason for hiding this comment

maeda-seina commented Feb 4, 2022 •

edited

Loading

haruguchi-yuma commented Feb 6, 2022 •

edited

Loading

maeda-seina commented Feb 7, 2022 •

edited

Loading

maeda-seina Feb 9, 2022 •

edited

Loading