-
Notifications
You must be signed in to change notification settings - Fork 28
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
Feet/bulk group api #2484
base: master
Are you sure you want to change the base?
Feet/bulk group api #2484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビュー遅くなってしまってごめんなさい!貢献ありがとうございます!!!
全体的にかなり良く書けていて凄いです!びっくり!
少し直して欲しいところを書いたので、対応お願いします~!
repository/gorm/user_group.go
Outdated
tx.Clauses(clause.OnConflict{ | ||
Columns: []clause.Column{{Name: "group_id"}, {Name: "user_id"}}, | ||
DoUpdates: clause.AssignmentColumns([]string{"role"}), | ||
}).Create(&users) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
何らかの不具合があった時のために↓の様にエラーハンドリングあった方が良さそうです。OnConflictがあるので、既にグループに居るユーザーを追加しようとしてもエラーで止まることはないです。
if err := tx.Clauses(clause.OnConflict{
Columns: []clause.Column{{Name: "group_id"}, {Name: "user_id"}},
DoUpdates: clause.AssignmentColumns([]string{"role"}),
}).Create(&users).Error; err != nil {
return convertError(err)
}
router/v3/user_groups.go
Outdated
if userID == uuid.Nil { | ||
if err := h.Repo.RemoveUsersFromGroup(g.ID); err != nil { | ||
return herror.InternalServerError(err) | ||
} | ||
return c.NoContent(http.StatusNoContent) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RemoveUserGroupMemberで全てのユーザーがグループから削除される挙動は予期されないと思うので、ここは消した方が良さそう。
router/v3/user_groups.go
Outdated
if err := h.Repo.AddUserToGroup(singleReq.ID, g.ID, singleReq.Role); err != nil { | ||
return herror.InternalServerError(err) | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっとネストが深いので、↓のように早期リターンをしたほうが読みやすいかな~と思ったのですが、好みの範疇だと思うのでどちらでもokです。
// AddUserGroupMember POST /groups/:groupID/members
func (h *Handlers) AddUserGroupMember(c echo.Context) error {
g := getParamGroup(c)
bodyBytes, err := io.ReadAll(c.Request().Body)
if err != nil {
return err
}
var singleReq PostUserGroupMemberRequest
c.Request().Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
if err := bindAndValidate(c, &singleReq); err == nil {
if err := h.Repo.AddUserToGroup(singleReq.ID, g.ID, singleReq.Role); err != nil {
return herror.InternalServerError(err)
}
return c.NoContent(http.StatusNoContent)
}
var reqs []PostUserGroupMemberRequest
c.Request().Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
if err := bindAndValidate(c, &reqs); err != nil {
return err
}
if len(reqs) == 0 {
return herror.BadRequest(errors.New("no users provided"))
}
users := make([]model.UserGroupMember, len(reqs))
for i, req := range reqs {
users[i] = model.UserGroupMember{UserID: req.ID, Role: req.Role, GroupID: g.ID}
}
if err := h.Repo.AddUsersToGroup(users, g.ID); err != nil {
return herror.InternalServerError(err)
}
return c.NoContent(http.StatusNoContent)
}
repository/gorm/user_group.go
Outdated
// RemoveUsersFromGroup implements UserGroupRepository interface. | ||
func (repo *Repository) RemoveUsersFromGroup(groupID uuid.UUID) error { | ||
if groupID == uuid.Nil { | ||
return repository.ErrNilID | ||
} | ||
err := repo.db.Transaction(func(tx *gorm.DB) error { | ||
if err := tx.Where("group_id = ?", groupID).Delete(&model.UserGroupMember{}).Error; err != nil { | ||
return err | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
return err | ||
} | ||
return nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RemoveUserFromGroup
にevent.UserGroupMemberRemoved
があるので、RemoveUsersFromGroup
でもevent.UserGroupMemberRemoved
するべきですね。
各ユーザーの削除を追跡して、それに基づいてイベントを発行する必要があってちょっと大変。
var removedUsers []model.UserGroupMember
if err := tx.Where("group_id = ?", groupID).Find(&members).Error; err != nil {
return err
}
でユーザー一覧を消す前に取得して、削除に成功したら取得したそれぞれのユーザーに対してイベントの発行
for _, userID := range removedUsers {
repo.hub.Publish(hub.Message{
Name: event.UserGroupMemberRemoved,
Fields: hub.Fields{
"group_id": groupID,
"user_id": userID,
},
})
}
をすれば良さそう。
あと以下二つのテストを書きたいですね! |
Co-authored-by: pikachu0310 <pikachu13711@gmail.com>
すみません、時期も時期なのでテストの方は別issueとして扱ってもらえると助かります。 |
グループへのユーザーの複数一括追加とグループの全ユーザーの一括削除