Add notification feature #728

kimurabps · 2021-01-28T07:34:14Z

refs #712

対応内容

ジョブ完了通知機能を実装しました。

スクリーンショット

ナビゲーションメニューにベルアイコンを追加

ベルアイコン押下で通知を10件まで表示

「more...」押下で通知一覧ページに遷移

ナビゲーションに「Setting」メニューを追加し、通知粒度とWebhook URLを設定できるようにする

Webhook URLが設定されている場合はSlack通知を行う

config/user_config.sample.yml

lib/tasks/update_schema.rake

kimurabps · 2021-02-04T09:48:53Z

レビュー依頼後で申し訳ございませんが、
ParameterSetに紐づくAnalysisの存在チェックがなぜかうまくいかない問題がありましたので、修正をプッシュしました 🙇

Analysis.last.parameter_set.analyses.last.status
=> :finished

Analysis.last.parameter_set.analyses.exists?
=> false

Analysis.last.parameter_set.analyses.present?
=> true

Analysis.last.parameter_set.analyses.where(status: :finished).present?
=> false

Analysis.where(parameter_set: Analysis.last.parameter_set).where(status: :finished).exists?
=> true

kimurabps · 2021-02-08T08:54:07Z

@yohm Slackにてご指摘いただいた以下の修正をpushいたしました 🙇

* oracle_hostの設定がなければ localhost:3000 にフォールバックする
* 通知が10件を超えたら古いものを消す

yohm · 2021-02-08T09:48:05Z

@kimurabps 早速対応いただきましてありがとうございます。修正を確認しました。

yohm · 2021-02-09T00:38:27Z

@kimurabps 上のコメントで述べたようにuser_config['oacis_host'] の値をslack通知する場合にのみ参照するようにした場合、設定をuser_configではなくSettingsの画面で設定するようにするとより簡単にセットアップができるのではないかと考えています。
Settingsの設定項目として１項目（項目名は"OACIS URL" とか？良い名前があればご提案していただければありがたいです）追加して、設定のデフォルト値をJSでdocument.location.originにしたいと考えています。この仕様で問題なさそうでしょうか？

yohm

とても完成度も高くて、使いやすい機能でした。設定のUIも通知のUIもslackへの連携も思い描いていた通りのものができています。
主に文言や設定方法についてコメントしましたのでご検討いただけるとありがたいです。

yohm · 2021-02-09T00:27:50Z

app/helpers/notification_events_helper.rb

+module NotificationEventsHelper
+  def generate_all_jobs_in_simulator_finished_message(job)
+    simulator_id = job.simulator.id
+    id_link = link_to(shortened_id(simulator_id), simulator_url(simulator_id))


OACISのUI上から参照するリンクは_urlではなく_pathにして、user_config[oacis_host]に依存しないようにできないでしょうか？
slackと連携させずに使う人も多いと思われますので、slackにメッセージを送る際のみoacis_hostの値を参照するようにすると、ユーザーのセットアップがより簡便になると思っています。

yohm · 2021-02-09T00:29:52Z

app/helpers/notification_events_helper.rb

+    simulator_id = job.simulator.id
+    id_link = link_to(shortened_id(simulator_id), simulator_url(simulator_id))
+
+    return "All #{job.class} in SimulatorID #{id_link} finished."


文言ですが、Simulatorの場合のみ、IDではなくnameを表示していただいた方が使いやすいと思います。
id_linkを作る際に shortened_id(simulator_id)ではなく job.simulator.name を使うようにしてもらえませんか？

yohm · 2021-02-09T00:44:25Z

app/controllers/oacis_settings_controller.rb

+
+  def update
+    if @oacis_setting.update(oacis_setting_params)
+      redirect_to edit_oacis_setting_path, notice: 'Oacis setting was successfully updated.'


Slackのwebhook登録時または更新時に、実際にslackに通知("OACIS setting was successfully updated"などの文言)を送っていただけないでしょうか？
ユーザーが、webhookの設定が正しく行われたことを確認できるようにするという意図です。

yohm · 2021-02-09T00:46:52Z

app/views/oacis_settings/edit.html.haml

+    .col-md-2
+      = f.select :notification_level, [1, 2, 3], {}, class: 'form-control', data: tooltip_data(:oacis_setting, :notification_level)
+  .form-group
+    = f.label_c :webhook_url


文言を "Webhook url" から "Slack webhook URL" に変更していただけないでしょうか？

yohm · 2021-02-09T00:50:11Z

app/views/oacis_settings/edit.html.haml

+  .form-group
+    = f.label_c :webhook_url
+    .col-md-5
+      = f.text_field :webhook_url, class: 'form-control', data: tooltip_data(:oacis_setting, :webhook_url)


このテキストフィールドの直下に以下のようなリンクを貼って、ユーザーがどのようにslackを設定するかがわかるようにしたいと考えています。特に問題なさそうであれば追加していただけないでしょうか？

How to setup incoming webhook for slack.

kimurabps · 2021-02-09T03:05:17Z

@kimurabps 上のコメントで述べたようにuser_config['oacis_host'] の値をslack通知する場合にのみ参照するようにした場合、設定をuser_configではなくSettingsの画面で設定するようにするとより簡単にセットアップができるのではないかと考えています。
Settingsの設定項目として１項目（項目名は"OACIS URL" とか？良い名前があればご提案していただければありがたいです）追加して、設定のデフォルト値をJSでdocument.location.originにしたいと考えています。この仕様で問題なさそうでしょうか？

@yohm レビューいただきありがとうございます 🙇
おっしゃるとおり、oacis_urlはslack通知する場合のみ参照し、設定はSettingsの画面から行えるようにした方がよさそうです 🙇
念のため確認させていただきたいのですが、「設定のデフォルト値をJSで document.location.origin にする」というのは、
oacis_urlが未設定の場合に、設定画面の入力ボックスにJSでデフォルト値としてセットするという認識で合っておりますでしょうか？
(未設定のまま使用した場合は document.location.origin は参照できないので、http://localhost:3000 にフォールバックする)

yohm · 2021-02-09T04:35:59Z

@kimurabps

oacis_urlが未設定の場合に、設定画面の入力ボックスにJSでデフォルト値としてセットするという認識で合っておりますでしょうか？
(未設定のまま使用した場合は document.location.origin は参照できないので、http://localhost:3000 にフォールバックする)

おっしゃる通りです。ご指摘の通りフォールバックも必要ですね。

kimurabps · 2021-02-09T05:06:32Z

ご回答ありがとうございます。承知いたしました。

kimurabps · 2021-02-12T02:47:45Z

@yohm 指摘箇所を修正いたしましたので、お手すきの際にレビューをお願いいたします 🙇

yohm

素晴らしいです！ありがとうございます

kimurabps force-pushed the feature/add_notification branch from 5110bbd to 524a143 Compare February 4, 2021 02:56

kimurabps commented Feb 4, 2021

View reviewed changes

config/user_config.sample.yml Outdated Show resolved Hide resolved

kimurabps commented Feb 4, 2021

View reviewed changes

lib/tasks/update_schema.rake Show resolved Hide resolved

kimurabps marked this pull request as ready for review February 4, 2021 08:01

kimurabps requested a review from yohm February 4, 2021 08:02

yohm reviewed Feb 9, 2021

View reviewed changes

kimurabps added 12 commits February 12, 2021 11:40

Add notification feature

a946d17

Add RSpec

35cf0c8

Fix bug

6da9394

Add config fallback and refactor

bbb51bc

Fix to limit notofication events on client side

55b8595

Use _path method for screen and move URL configuration to database

be39c5a

Use simulator name instead of id

c283a3d

Fix label text and add a link to how to setup incoming webhook for slack

24d194a

Fix to send notification message when webhook URL updated

abe9640

Fix color

534ecaa

Fix bug

dd57323

Move to controller

6efb807

kimurabps force-pushed the feature/add_notification branch from 4d5f3dc to 6efb807 Compare February 12, 2021 02:44

kimurabps requested a review from yohm February 12, 2021 02:46

yohm approved these changes Feb 12, 2021

View reviewed changes

yohm merged commit 8a6e06c into develop Feb 12, 2021

yohm deleted the feature/add_notification branch February 12, 2021 04:12

yohm added this to the Improvement-202103 milestone Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notification feature #728

Add notification feature #728

kimurabps commented Jan 28, 2021 •

edited

Loading

kimurabps commented Feb 4, 2021 •

edited

Loading

kimurabps commented Feb 8, 2021

yohm commented Feb 8, 2021

yohm commented Feb 9, 2021

yohm left a comment

yohm Feb 9, 2021

yohm Feb 9, 2021

yohm Feb 9, 2021

yohm Feb 9, 2021

yohm Feb 9, 2021

kimurabps commented Feb 9, 2021

yohm commented Feb 9, 2021 •

edited

Loading

kimurabps commented Feb 9, 2021

kimurabps commented Feb 12, 2021

yohm left a comment

Add notification feature #728

Add notification feature #728

Conversation

kimurabps commented Jan 28, 2021 • edited Loading

対応内容

スクリーンショット

kimurabps commented Feb 4, 2021 • edited Loading

kimurabps commented Feb 8, 2021

yohm commented Feb 8, 2021

yohm commented Feb 9, 2021

yohm left a comment

Choose a reason for hiding this comment

yohm Feb 9, 2021

Choose a reason for hiding this comment

yohm Feb 9, 2021

Choose a reason for hiding this comment

yohm Feb 9, 2021

Choose a reason for hiding this comment

yohm Feb 9, 2021

Choose a reason for hiding this comment

yohm Feb 9, 2021

Choose a reason for hiding this comment

kimurabps commented Feb 9, 2021

yohm commented Feb 9, 2021 • edited Loading

kimurabps commented Feb 9, 2021

kimurabps commented Feb 12, 2021

yohm left a comment

Choose a reason for hiding this comment

kimurabps commented Jan 28, 2021 •

edited

Loading

kimurabps commented Feb 4, 2021 •

edited

Loading

yohm commented Feb 9, 2021 •

edited

Loading