Skip to content
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

提出物に関するSlack通知のテキストを修正する #421

Merged
merged 5 commits into from Jul 26, 2018

Conversation

ikaruga777
Copy link
Contributor

@ikaruga777 ikaruga777 commented Jul 25, 2018

fixes #409
SlackのメッセージAPIでは独自記法をサポートしています。
Basic message formatting | Slack

提出物のコメント、確認時にこの記法が崩れるのでキャプチャのように修正します。
2018-07-25 10 27 50

  • 通知文言の修正
  • :development時に通知テキストを確認可能にする

@komagata komagata temporarily deployed to bootcamp-fjord-jp-pr-421 July 25, 2018 01:31 Inactive
@komagata komagata temporarily deployed to bootcamp-fjord-jp-pr-421 July 25, 2018 06:10 Inactive
@ikaruga777
Copy link
Contributor Author

slack-notifierはdevelopmentでも環境変数必須…

@sanfrecce-osaka
Copy link
Contributor

@ikaruga777 @komagata
dotenvconfigを使ってはどうでしょう?
自分はconfigを使っています。

@ikaruga777
Copy link
Contributor Author

@sanfrecce-osaka productionに干渉する必要はないのですが、herokuの環境変数にいる想定であれば、development用の設定だけおいておけばよさそうですね。

@sanfrecce-osaka
Copy link
Contributor

sanfrecce-osaka commented Jul 25, 2018

@ikaruga777
dotenvならそうなりますね。
configを使うなら現在ENVを直接代入している箇所をSettings.プロパティ名とし、config/settings/production.ymlには環境変数を、config/settings/development.ymlに値を直書きまたはconfig/settings/settings.local.ymlに各自で書いてもらうことになるかと思います。
development.ymlはGitHubで値が見えてしまうので止めたほうがいいですね。
dotenvにしてもconfigにしても設定する値の共有方法は考えないといけないですね。

@ikaruga777
Copy link
Contributor Author

Production環境以外でSlackの通知をしっかりと確認したいということがなければ、無理にgemを入れなくてもslack-notifierを挟まずににログ出力すればよいのかなと思いました。

@ikaruga777 ikaruga777 changed the title [WIP]提出物に関するSlack通知のテキストを修正する 提出物に関するSlack通知のテキストを修正する Jul 25, 2018
@ikaruga777 ikaruga777 self-assigned this Jul 26, 2018
@ikaruga777 ikaruga777 requested a review from komagata July 26, 2018 01:17
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます!

@komagata komagata merged commit 0b8fae4 into master Jul 26, 2018
@komagata komagata deleted the fix-product-notification branch July 26, 2018 05:51
@@ -38,6 +38,8 @@ def notify(text, options = {})

notifier = Slack::Notifier.new ENV["SLACK_WEBHOOK_URL"], username: username
notifier.ping text, icon_url: icon_url, attachments: attachments
else
Rails.logger.info "Notify\ntext:#{text}\nparams:#{options}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Rails.logger.info "Notify\ntext:#{text}\nparams:#{options}"
+logger.info "Notify\ntext:#{text}\nparams:#{options}"

で大丈夫そう

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

開発中、Railsを付けないと、No method errorでテストが落ちることがあり、付けて対応したものです。
今確認したところ、Rails外してもテストが通ったので、次回付近を修正する時に一緒に直します!

@june29
Copy link

june29 commented Jul 30, 2018

マージ後なのでおもに思考の共有のために書きます 📝

  • 「Slack への投稿をいい感じにしたい」というシチュエーションなので、開発環境でも Slack に投稿して表示を確認しながら作業したい、という気持ちが強くあるケースだな〜と感じました
  • 自分だったら ENV["SLACK_WEBHOOK_URL"] が設定されていれば Slack に通知するし、されていなければ Slack に通知しない、という分岐にしていたかもなぁ

@bluerabbit
Copy link
Collaborator

自分だったら ENV["SLACK_WEBHOOK_URL"] が設定されていれば Slack に通知するし、されていなければ Slack に通知しない、という分岐にしていたかもなぁ

👍

いまRails.env.production?で分岐してるんですね。

Notifierみたいなクラスでslackをラップしたクラス作って、@notifierがなかったらログ出力みたいにすること多いです。

class Notifier
  def initialize(webhook_url: ENV['SLACK_WEBHOOK_URL'], channel:)
    if webhook_url.present?
      @notifier = Slack::Notifier.new(webhook_url, channel: channel)
    end
  end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slackに表示される提出物へのリンクURLがおかしくなる
5 participants