Discordのmentorチャンネルに提出物の溜まり具合を定期的に投稿する

[gallardo16]

issue

#2399

概要

何故この機能が必要なのか

提出物は1週間以内にレビューする、というSLAになっているが、ときどきSLAを超えてしまいそうな提出物がぽろぽろ出てくる。
しかし、メンターが意識的にその情報を見に行かないと、そういう提出物があることに気づけない。
なので、PULL型ではなくPUSH型で情報が届くようにする。

機能の説明

毎日決まった時間に締切が近い提出物の件数をSlackのmentorチャンネルに投稿する。

• 投稿内容の案

（提出後日数が経っている提出物）

- 5日経過 = 12件
- 6日経過 = 5件
- 7日以上経過 = 2件（OUT！！）

https://bootcamp.fjord.jp/products/not_responded

• 該当する提出物が1件もない場合

（提出後日数が経っている提出物）

5日以上経過している提出物は1件もありません！🎉

https://bootcamp.fjord.jp/products/not_responded

このissueの進捗について

私(@gallardo16)の担当としては「JSON APIで未返信の提出物のうち5日、6日、7日以上経ったものの数をそれぞれ取得する」まで行いました。(2e815b3)
後の「Discordに通知する」に関しては駒形さんにお願いする形になります。

[komagata]

📝 基本的には今の感じでOK。（JSON APIができたところで完了にします〜）

[gallardo16]

@komagata
了解です。コメントありがとうございます。

[gallardo16]

@alto-I
お手隙の際に、コードレビューをよろしくお願いいたします🙇‍♂️

[alto-I]

@gallardo16
自分の分かる範囲でですが問題ないのではと思います！😄

あと質問なのですが、これはテストは書いた方がいいissueなのでしょうか？
経験不足でテストが必要かどうかの判断付かなかったです…😥

[gallardo16]

@alto-I
返信が遅れてしましました。大変申し訳ありません。

レビューありがとうございます。

あと質問なのですが、これはテストは書いた方がいいissueなのでしょうか？

テストを追加しましたので再度レビューをよろしくお願いします🙇‍♂️

[alto-I]

@gallardo16
こちらこそ遅くなってすいません！
テストも書いていただきありがとうございます😄
大丈夫だと思います！

[sinsoku]

これは ago を使うと短く書けます。

  created_at: <%= 1.day.ago %>
  updated_at: <%= 1.day.ago %>
  published_at: <%= 1.day.ago %>

[gallardo16]

レビューありがとうございます。
修正してみます！

[sinsoku]

モデルに経過日数を返すメソッドを用意して、 Array#count を使う実装にするのはどうでしょう？
ActiveSupport::Duration.build は経過した時間を計算できるクラスです。

# 公開日もしくは作成日からの経過日数を返す。
def elapsed_days
  t = published_at || created_at
  ActiveSupport::Duration.build(Time.current - t).parts[:days]
end

コントローラー側の処理がこんな感じでシンプルになる。

@products_submitted_just_5days_count = products.count { |product| product.elapsed_days == 5 }
@products_submitted_just_6days_count = products.count { |product| product.elapsed_days == 6 }
@products_submitted_over_7days_count = products.count { |product| product.elapsed_days >= 7 }

[gallardo16]

確かにシンプル！！
提案していただいた形で修正したいと思います。

[komagata]

@sinsoku レビューありがとうございます！

[sinsoku]

@gallardo16 @komagata
今回のケースだとあまり問題にならないと思いますが、変更後のコードは each が3回呼ばれる（countは内部で each を呼ぶ）点には注意が必要です。

業務のコードだと、こういう変更で意図せずパフォーマンスの劣化が起きて問題になるケースもあるため、補足のコメントもしておきます。

元のコード

products.each do |product|
  # productsの件数が100件の場合、中の処理は100回実行される。
end

変更後のコード

# productsの件数が100件の場合、合計で300回の処理が実行される。
products.count { |product| product.elapsed_days == 5 }
products.count { |product| product.elapsed_days == 6 }
products.count { |product| product.elapsed_days >= 7 }

[gallardo16]

@sinsoku
レビューありがとうございました。
提案していただいた形で修正したのですが上手くいかなかったので質問があります。

products.count { |product| product.elapsed_days == 5 }

上記の様に5日経過したものだけをカウントしたかったのですが、「12日経過したもの」もカウントされてしまいます。色々試した結果、12日経過したものは{:weeks=>1, :days=>5}(1週間と5日)で判定されているのかなと考えました。

そのため以下のようにメソッド等追加しました。

  def elapsed_days
    t = published_at || created_at
    ActiveSupport::Duration.build(Time.current - t).parts[:days]
  end

  def elapsed_weeks
    t = published_at || created_at
    ActiveSupport::Duration.build(Time.current - t).parts[:weeks]
  end

    @products_submitted_just_5days_count = products.count { |product| product.elapsed_days == 5 && product.elapsed_weeks.nil? }
    @products_submitted_just_6days_count = products.count { |product| product.elapsed_days == 6 && product.elapsed_weeks.nil? }
    @products_submitted_over_7days_count = products.count { |product| product.elapsed_weeks.to_i >= 1 }

提案していただいたコードで詰まってしまい、色々付け足してしまいました。
どうすればいいかわからなくなってしまったので、ご助言いただけたらなと思います。よろしくお願いいたします。

[sinsoku]

@gallardo16
{:weeks=>1, :days=>5} で返ってくるのは気づきませんでした 🙈

elapsed_days で経過日数を返すように直して、 elapsed_weeks などは作らない方が良いかなと思います。
weeksの扱いが微妙なので ActiveSupport::Duration を使わず、 ((Time.current - t) / 1.day).to_i で日数を出す方法にするとどうでしょうか？

[gallardo16]

ありがとうございます。
修正できました。

[gallardo16]

@alto-I
レビューありがとうございました。

[gallardo16]

@komagata
コードレビューをよろしくお願いいたします。

[komagata]

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

ありがとうございます！