Add RequestProjectRole API #353
Conversation
|
セキュリティレビューを実施しました。 By RISKEN review |
| return nil, err | ||
| } | ||
| default: | ||
| a.logger.Warnf(ctx, "This notification_type is unimprement. type: %v", notification.Type) |
There was a problem hiding this comment.
エラーを返しても良いかもです。
通知されてないのに、正常系のレスポンスになってしまいそうなので
There was a problem hiding this comment.
すごい細かいですが、unimprement -> unimplement のtypoがありそうです。
ここで使うならunimplementedの方が適切かもと思いました。
There was a problem hiding this comment.
通知されてないのに、正常系のレスポンスになってしまいそうなので
すごい細かいですが、unimprement -> unimplement のtypoがありそうです。ここで使うならunimplementedの方が適切かもと思いました。
こちらについては自分も同感なので、修正します。
また、こちらはTestNotificationのほぼコピペとなっていたので、TestNotificationにも同様の問題（タイポ、エラーを返していないなど）がありそうです。意図していない実装になっている場合、このPRで一緒に修正してしまいます。
core/pkg/server/alert/alert_notification.go
Line 199 in 0e3af5c
|
slackとの疎通確認に成功 |
| } | ||
| default: | ||
| a.logger.Warnf(ctx, "This notification_type is unimplemented. type: %v", notification.Type) | ||
| return nil, fmt.Errorf("This notification_type is unavailable. type: %v", notification.Type) |
There was a problem hiding this comment.
コメント通りにここにエラーを追加しました。ログメッセージではunimplementedですがエラーメッセージとしてはユーザー目線でunavailableとしました。
There was a problem hiding this comment.
エラーメッセージの場合は最初小文字でお願いします。
| return nil, err | ||
| } | ||
| notifications, err := a.repository.ListNotification(ctx, req.ProjectId, "slack", 0, time.Now().Unix()) | ||
| if err != nil { |
There was a problem hiding this comment.
ここの書き方は他の部分では
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return &empty.Empty{}, nil
}
return nil, err
}
となっていますが、
- このAPIは管理者に権限リクエストを送るもので、何らかのバグで送ることができなかった場合にはエラーをそのまま返してしまうのがよいと考えた
ので、errをそのまま返却するようにしました。
| } | ||
| default: | ||
| a.logger.Warnf(ctx, "This notification_type is unimplemented. type: %v", notification.Type) | ||
| return nil, fmt.Errorf("This notification_type is unavailable. type: %v", notification.Type) |
There was a problem hiding this comment.
エラーメッセージの場合は最初小文字でお願いします。
| cases := []struct { | ||
| name string | ||
| input *alert.RequestProjectRoleNotificationRequest | ||
| want *empty.Empty |
There was a problem hiding this comment.
ここは、レスポンス内容は問わないので、削っても良いかもと思いました（各テストケースでエラーが返されるか返されないかだけ見れば良い（wantErrのみ見れば良い））
|
@gassara-kys |
背景
https://github.com/ca-risken/internal-community/issues/673
概要
アクセス権をリクエストするAPIを追加します。ユーザーが403画面からプロジェクト管理者ユーザーにアクセス許可をリクエストするために使用します。
APIの説明
なお、proto以下のディレクトリに差分が発生していますが、
proto/alert/service.proto以外はバージョンアップによる差分です。観点
このPRのコメントに追加してあります。
参考
https://github.com/ca-risken/internal-community/issues/673
ca-risken/web#360