Fix/get all reacted users #18
Conversation
| func (s *slackReactionUsersService) getFullReactions(ctx context.Context, channelID, ts string) ([]*model.SlackReaction, error) { | ||
| return s.slackRepository.GetReactions(ctx, channelID, ts, true) | ||
| } |
There was a problem hiding this comment.
この処理 func に切り出すほどの処理だった？
There was a problem hiding this comment.
trueを直で引数に指定するのが、マジックナンバーみたいに見えるから関数に切り出した感じ（意味がわかるように）。
どっちでも良いとは思ってるけどどうでしょう？
There was a problem hiding this comment.
ここでture直書きにしておきました
fix implementation
| return reactedUserEmails, nil | ||
| } | ||
|
|
||
| // isReactionRefetchNeeded returns true if more fetches is required |
There was a problem hiding this comment.
どういう時に refetch が必要なのか、軽くコメント入れておいてくれると助かる。
There was a problem hiding this comment.
ありがとう！入れておきました
add comment
| UserIDs: []string{"user02", "user03"}, | ||
| Count: 4, // number of counts that need to be re-fetched |
There was a problem hiding this comment.
これって len(UserIDs) > Count のパターンってありえないんだっけ？
There was a problem hiding this comment.
ありえないと思う。APIドキュメントには明示されていなかったけど、実データとして確認しているのは
len(UserIDs) =< Count の時だけかな。
判定部分が != になっていたから、判定部を修正しておきました
There was a problem hiding this comment.
fix implementation でisIncompleteReactionにロジックを移したので、ここのコメントと判定部を修正しておきました
@hotakahattori repository層、まだテスト全然書けていないから、serviceに載せて保護しようかなと思ったけど、確かにそんな気がする。修正しました |
app/internal/repository/slack.go
Outdated
| } | ||
| for _, reaction := range parentMessage.Reactions { |
There was a problem hiding this comment.
変数宣言と使用箇所が無駄に離れてる
| } | |
| for _, reaction := range parentMessage.Reactions { | |
| } | |
| var reactions []*model.SlackReaction | |
| for _, reaction := range parentMessage.Reactions { |
There was a problem hiding this comment.
対応しておきました move reactions(var)
app/internal/repository/slack.go
Outdated
| @@ -18,7 +18,8 @@ package repository | |||
|
|
|||
| import ( | |||
| "context" | |||
| "fmt" | |||
|
|
|||
| slack2 "github.com/slack-go/slack" | |||
|
|
|||
| "github.com/moneyforward/auriga/app/pkg/slack" | |||
There was a problem hiding this comment.
こっち pkgslack とかにしとく？
|
直したのでマージします |
Overview
Changes
ListUsersEmailByReactionto judge whether it has to usereactions.getAPI for fetching full reactions and to fetch all reactions as necessary before filtering reacted users using getReactionUserIDs.model.SlackReactionto allow this determine.Operation check