#1 delete dynamodb data with slack slash command #34

kdnakt · 2019-03-03T00:27:24Z

🎫 Issue

📝 概要

退会者が出た時に利用するDynamoDBからのデータ削除用のSlackのスラッシュコマンドを実装しました。
Slackで/blog_delete @kdnakt のように利用できる予定です。
テストコードがないのですが、自分のAWSアカウントで簡単に動作確認済みです。
（対象者がいない場合のメッセージ、いる場合の処理）

🙅‍♂️ やってないこと

(このPRではやってないことなど書きましょう)

✔️ 動作確認

(レビューアに確認してほしいことを書きましょう)

CIのテストが全て問題ない

codecov-io · 2019-03-03T00:28:35Z

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #34   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files           4        4           
  Lines         142      142           
=======================================
  Hits          124      124           
  Misses         13       13           
  Partials        5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c5f7bc...0f17827. Read the comment docs.

budougumi0617

コメントしました。

budougumi0617 · 2019-03-03T15:14:43Z

database/database.go

+func DeleteUser(configData config.ConfigData, member WriteBlogEveryWeek) {
+	table := getTableObject(configData)
+	if err := table.Delete("user_id", member.UserID).Run(); err != nil {
+		panic("削除エラー => " + err.Error())


基本goのアプリではpanicはしないでちゃんとerrrorを返してその結果を戻したほうがいいですね。今回の場合だと以下の点で不便だと思います。

panicしたら結果が返らないのでcloud watch見れる人じゃないと結果がわからない

cloud watch見れる人でもSlackコマンド使ったらSlack上で結果を見たいと思う

勉強になります！エラーメッセージを返すよう修正しました。

budougumi0617 · 2019-03-03T15:23:05Z

main.go

@@ -96,3 +99,24 @@ func blogResult() {
 	}
 	// fmt.Println(sendText)
 }
+
+// blogDelete ブログの削除ロジックを実行
+func blogDelete(_ context.Context, rawParams interface{}) (interface{}, error) {


監査的な機能なくいれちゃってダイジョブですかね？コマンドを実行したIDをログに残しておくとかとか（消したレコードもログに出力しておいたほうが復旧しやすそうです）
これってSlackコマンド知っていたら誰でも消せちゃう感じだと、しれっとDMとかでやってメッセージ消したら完全犯罪になってしまうような。

slack.goでSlackから渡ってきたパラメータを出力しました。

と書いたところで、消したレコードがログに記載がないのに気づいたので追加で修正します 🙇

budougumi0617 · 2019-03-03T15:24:25Z

main.go

@@ -96,3 +99,24 @@ func blogResult() {
 	}
 	// fmt.Println(sendText)
 }
+
+// blogDelete ブログの削除ロジックを実行
+func blogDelete(_ context.Context, rawParams interface{}) (interface{}, error) {


戻り値の interface{}はstringですかね？自分で宣言する関数の引数戻り値でinterface{}は使わないほうがいいですね。

修正しました！

budougumi0617

なんどもごめんなさい、追記しました 🙇

main.go

budougumi0617 · 2019-03-04T00:03:49Z

slack/slack.go

+		// また、ユーザー名が送られてきた場合は、先頭の@をtrimする
+		Text: strings.TrimRight(strings.TrimLeft(strings.TrimLeft(params["text"][0], "@"), "<"), ">"),
+	}
+	fmt.Printf("SlackParams: %v", slackParams)


Token: params["token"][0],っていうなんかお漏らししたらいけなそうなフィールドがあるのでUserID(UserName)、Textなど特定のフィールドのみ出力するだけに絞っておいたほうがいいと思います。
（業務でやるときはloggerとかに特定のフィールド名をマスキングする処理仕込んだりするのですが、そこまではするのはめんどくさいので。。。）

token以外のユーザーID、ユーザー名、Textを出力するように修正しました
（ユーザー名だけだとSlack側で変更できてしまうので、IDも念の為出力）

budougumi0617 · 2019-03-04T00:06:35Z

slack/slack.go

@@ -64,11 +65,14 @@ func ParseSlackParams(rawParams interface{}) (result *SlackParams, err error) {
 		return
 	}

-	return &SlackParams{
+	slackParams := SlackParams{


好みの問題（？）ですが、私はこっちはslackParams := &SlackParamsのままにしておきますね。
（これは変えなくてもいいです。）
理由はポインタのほうにメソッド生やしているときが多いので

budougumi0617 · 2019-03-04T00:08:23Z

database/database.go

+// DeleteUser ユーザーデータを削除する
+func DeleteUser(configData config.ConfigData, member WriteBlogEveryWeek) error {
+	table := getTableObject(configData)
+	return table.Delete("user_id", member.UserID).Run()


DynamoDBのuser_idってなんの文字列だったのかよくわかってなかったんですが、SlackのIDだったんですね！（今更）

budougumi0617

最後にひとつだけコメントしました

budougumi0617 · 2019-03-05T22:09:10Z

main.go

@@ -96,3 +99,28 @@ func blogResult() {
 	}
 	// fmt.Println(sendText)
 }
+
+// blogDelete ブログの削除ロジックを実行
+func blogDelete(_ context.Context, rawParams interface{}) (string, error) {


error返すPATHがなくなったのなら戻り値はstringだけでいいと思います。

残念ながらLambdaのハンドラ関数の制約で戻り値1つの場合はerrorでないといけないので、戻り値を2個定義する必要があります……

https://docs.aws.amazon.com/ja_jp/lambda/latest/dg/go-programming-model-handler-types.html

~~なるほど、今まで勘違いしていたのですが、この関数の戻り値がSlackに通知されるわけではないのですね。~~
~~そうするとこの関数で文字列を返す理由はなんでしょうか？~~
~~Slackに結果を通知して関数を正常に完了するだけならば、文字列を slack.SendMessageに渡してこの関数は戻り値なしにすればいいのでは？~~

あー、slack slash commandに紐づけるLambdaとしてはLambda関数の戻り値として設定した値がSlackに通知される感じですかね？
だからblogResultやblogReminderはslack.SendMessage使っているけど、blogRegisterとblogDeleteは戻り値で結果返しているという理解でよいですかね（今更感）
なんか全然検討違いのコメントをしてしまって && ドメイン理解が甘いままレビューしてしまってごめんなさい 🙇 🙇

説明が雑ですみません。。 🙇

ご認識の通り、このQiita記事みたいに、Slack側にレスポンスを返せるようにAPI Gatewayが設定されているはずです。（IAMの権限の都合で見れませんが）
https://qiita.com/G-awa/items/fc803c285cb560dab796

blogRegister関数の動作が同じようになっていて、添付の画像のようにメッセージが表示されます
（main.goの72行目のメッセージが表示されたケースです）

解説ありがとうございます！

budougumi0617

コメントしました。

budougumi0617

昨夜盛大にトンチンカンなコメントをしてしまった気がします。私の不勉強で余計なやりとり発生してしまってごめんなさい 🙇
私の理解が間違っていなければLGTMです

budougumi0617 · 2019-03-07T00:18:29Z

main.go

@@ -96,3 +99,28 @@ func blogResult() {
 	}
 	// fmt.Println(sendText)
 }
+
+// blogDelete ブログの削除ロジックを実行
+func blogDelete(_ context.Context, rawParams interface{}) (string, error) {


あー、slack slash commandに紐づけるLambdaとしてはLambda関数の戻り値として設定した値がSlackに通知される感じですかね？
だからblogResultやblogReminderはslack.SendMessage使っているけど、blogRegisterとblogDeleteは戻り値で結果返しているという理解でよいですかね（今更感）
なんか全然検討違いのコメントをしてしまって && ドメイン理解が甘いままレビューしてしまってごめんなさい 🙇 🙇

kojirock5260

LGTM 🎉

write-blog-every-week#1 delete dynamodb data with slack slash command

03acbc3

kdnakt requested review from budougumi0617 and kojirock5260 March 3, 2019 00:27

budougumi0617 reviewed Mar 3, 2019

View reviewed changes

kdnakt added 3 commits March 4, 2019 08:03

write-blog-every-week#1 return error message without panic

58382ee

write-blog-every-week#1 output slack params to CloudWatch log

c8a9015

write-blog-every-week#1 fix response type to string

df80294

budougumi0617 reviewed Mar 4, 2019

View reviewed changes

write-blog-every-week#1 refines log and error message

0f17827

budougumi0617 reviewed Mar 5, 2019

View reviewed changes

budougumi0617 reviewed Mar 6, 2019

View reviewed changes

budougumi0617 approved these changes Mar 7, 2019

View reviewed changes

kojirock5260 approved these changes Mar 10, 2019

View reviewed changes

kojirock5260 merged commit bc29980 into write-blog-every-week:master Mar 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1 delete dynamodb data with slack slash command #34

#1 delete dynamodb data with slack slash command #34

kdnakt commented Mar 3, 2019

codecov-io commented Mar 3, 2019 •

edited

Loading

budougumi0617 left a comment

budougumi0617 Mar 3, 2019

kdnakt Mar 3, 2019

budougumi0617 Mar 3, 2019

kdnakt Mar 3, 2019 •

edited

Loading

budougumi0617 Mar 3, 2019

kdnakt Mar 3, 2019

budougumi0617 left a comment

budougumi0617 Mar 4, 2019

kdnakt Mar 4, 2019

budougumi0617 Mar 4, 2019

budougumi0617 Mar 4, 2019

budougumi0617 left a comment

budougumi0617 Mar 5, 2019

kdnakt Mar 6, 2019

budougumi0617 Mar 6, 2019 •

edited

Loading

budougumi0617 Mar 7, 2019

kdnakt Mar 7, 2019

budougumi0617 Mar 7, 2019

budougumi0617 left a comment

budougumi0617 left a comment

budougumi0617 Mar 7, 2019

kojirock5260 left a comment

#1 delete dynamodb data with slack slash command #34

#1 delete dynamodb data with slack slash command #34

Conversation

kdnakt commented Mar 3, 2019

🎫 Issue

📝 概要

🙅‍♂️ やってないこと

✔️ 動作確認

codecov-io commented Mar 3, 2019 • edited Loading

Codecov Report

budougumi0617 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdnakt Mar 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budougumi0617 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budougumi0617 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budougumi0617 Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

budougumi0617 left a comment

Choose a reason for hiding this comment

budougumi0617 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kojirock5260 left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 3, 2019 •

edited

Loading

kdnakt Mar 3, 2019 •

edited

Loading

budougumi0617 Mar 6, 2019 •

edited

Loading