Skip to content
This repository has been archived by the owner on Aug 17, 2023. It is now read-only.

refs #56 slackbot バージョンアップ #200

Merged
merged 5 commits into from
Sep 9, 2020
Merged

Conversation

fumi232323
Copy link
Contributor

@fumi232323 fumi232323 commented Sep 8, 2020

チケットURL

このレビューで確認してほしい点

  • ライブラリのバージョンアップの指定が適切であること (slackbot, slacker)

  • ライブラリバージョンアップに伴うコードの変更 (random コマンド) が適切であること

    • ローカル環境のapp + 以下チャンネルにて動作確認済みです
      • public: #bot-test-fumi23
      • private: p-bot-test-fumi23
      • shared: #s-bot-test-fumi23

レビューチェックリスト

  • C2 体を表す名前の公理:あらかじめ決められている以外の汎用的な名前のモジュールを作らない
  • C3 汎用名のモジュール内に長々と具体的処理を書かない
  • C4 単純な処理の長さで分割しない
  • C5 引数の数を減らす
  • C6 パッケージ間で共通した定数を作らない
  • C7 継承の利用を最小限にする
  • C8 親クラスのテストを子クラスでも実行すること
  • C9 オーバーライドを減らす
  • C10 継承やオーバーライドを明示する

@fumi232323 fumi232323 self-assigned this Sep 8, 2020
@@ -149,7 +149,7 @@ $ (cd beproudbot/deployment && ~/venv_ansible/bin/ansible-playbook -i hosts --co

- `$water count`: 現在の残数を返す
- `$water num`: 水を取り替えた時に使用。指定した数だけ残数を減らす(numが負数の場合、増やす)
- `$water hitsory <num>`: 指定した件数分の履歴を返す(default=10)
- `$water history <num>`: 指定した件数分の履歴を返す(default=10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[typo]

typo 修正

Comment on lines +17 to +18
slackbot==1.0.0
slacker==0.14.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[変更]

廃止予定のAPI channelsgroups の代わりに新しいほうのAPI conversations を使えるようにするため、バージョンアップしました

[参考]

https://api.slack.com/changelog/2020-01-deprecating-antecedents-to-the-conversations-api


@respond_to('^random$')
@respond_to('^random\s+(active|help)$')
def random_command(message, subcommand=None):
"""
チャンネルにいるメンバーからランダムに一人を選んで返す
- https://github.com/os/slacker
- https://api.slack.com/methods/channels.info
- https://api.slack.com/methods/conversations.members
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[変更]

channels.info => conversations.members に変更

[参考]

https://api.slack.com/methods/conversations.members

@@ -33,16 +36,11 @@ def random_command(message, subcommand=None):
channel = message.body['channel']
webapi = slacker.Slacker(settings.API_TOKEN)
try:
cinfo = webapi.channels.info(channel)
members = cinfo.body['channel']['members']
cinfo = webapi.conversations.members(channel)
Copy link
Contributor Author

@fumi232323 fumi232323 Sep 8, 2020

Choose a reason for hiding this comment

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

conversations.info だと、メンバー情報は取得できないため、 conversations. members を使用します

[参考]

https://api.slack.com/methods/conversations.info

except slacker.Error:
try:
cinfo = webapi.groups.info(channel)
members = cinfo.body['group']['members']
Copy link
Contributor Author

@fumi232323 fumi232323 Sep 8, 2020

Choose a reason for hiding this comment

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

conversations.members で private チャンネルのメンバーも取得できるので、こちらのくだりは削除

  • conversations.members は public, private, shared チャンネルのメンバーが取得できる。
  • 動作確認済み

# TODO: 例外で判定しないように修正する
# チャンネルに紐付かない場合はreturn
return
logger.exception('An error occurred while fetching members: channel=%s', channel)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

エラー発生時は、ログを出力するようにしました。

@@ -6,16 +6,16 @@ geopy==1.11.0
idna==2.6
kml2geojson==4.0.2
Mako==1.0.7
MarkupSafe==1.0
MarkupSafe==1.1.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[変更]

CI で、pallets/markupsafe#116 と同事象が発生したため、バージョンアップしました。 (最新の 1.1.1であれば発生しない)

  • (wan さんが教えてくれたとおり)このライブラリは、alembic -> maco -> markupsafe という依存関係がある
  • => alembic はマイグレーションに使用しているので、ローカル環境でバージョンアップしてマイグレーションの動作確認をした
  • => 問題なかったです

@fumi232323 fumi232323 requested a review from wanshot September 9, 2020 02:40
@fumi232323 fumi232323 changed the title [wip] refs #56 slackbot バージョンアップ refs #56 slackbot バージョンアップ Sep 9, 2020
Copy link
Contributor

@wanshot wanshot left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@fumi232323 fumi232323 merged commit 2b3d713 into master Sep 9, 2020
@fumi232323 fumi232323 deleted the t56-upgrade-slackbot branch September 9, 2020 05:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants