Skip to content
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

Fix/issue 400 #461

Merged
merged 4 commits into from
Nov 11, 2023
Merged

Fix/issue 400 #461

merged 4 commits into from
Nov 11, 2023

Conversation

iChemy
Copy link
Contributor

@iChemy iChemy commented Sep 24, 2023

No description provided.

@iChemy
Copy link
Contributor Author

iChemy commented Sep 24, 2023

そもそもフロント側が/api/users/me/groups?relation=belongsを叩いている気がするのでこことどのようにやりくりするべきでしょうか?(ここ)
relation=belongsで自分はbelongしてないがadminではあるチームを送るべきなのかと思い新しいクエリパラメータを暫定的に作ってコミットしました.

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

レビューしました

@@ -38,6 +39,8 @@ func GetUserRelationQuery(values url.Values) UserRelation {
return RelationBelongs
case "admins":
return RelationAdmins
case "belongsoradmins":
Copy link
Member

Choose a reason for hiding this comment

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

URLに指定するのでbelongs-or-adminsの方がいいかな
openapiにも追記してもらえると助かります:pray:

@@ -38,6 +39,8 @@ func GetUserRelationQuery(values url.Values) UserRelation {
return RelationBelongs
case "admins":
return RelationAdmins
case "belongsoradmins":
return RelationBelongsOrAdmins
}

return RelationBelongs
Copy link
Member

Choose a reason for hiding this comment

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

デフォルトをどうするかも考慮したい
個人的にはRelationBelongsOrAdminsが自然な気がします

router/groups.go Outdated
for id := range uniqueIDs {
groupIDs = append(groupIDs, id)
}

Copy link
Member

Choose a reason for hiding this comment

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

最後の空行は消してもらえると助かります:pray:

router/groups.go Outdated
Comment on lines 144 to 154
for _, id := range belongingGroupIDs {
uniqueIDs[id] = struct{}{}
}

for _, id := range adminGroupIDs {
uniqueIDs[id] = struct{}{}
}

for id := range uniqueIDs {
groupIDs = append(groupIDs, id)
}
Copy link
Member

Choose a reason for hiding this comment

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

allGroupIDs := append(belongingGroupIDs, adminGroupIDs)

uniqueIDMap := make(map[uuid.UUID]struct{})
uniqueIDs := make([]uuid.UUID, 0, len(allGroupIDs))
for _, id := range allGroupIDs {
    if _, ok := uniqueIDMap[id]; ok {
        continue
    }

    uniqueIDMap[id] = struct{}{}
    uniqueIDs = append(uniqueIDs, id)
}

みたいに書けそうです:eyes:

@ras0q
Copy link
Member

ras0q commented Sep 24, 2023

そもそもフロント側が/api/users/me/groups?relation=belongsを叩いている気がするのでこことどのようにやりくりするべきでしょうか?(ここ) relation=belongsで自分はbelongしてないがadminではあるチームを送るべきなのかと思い新しいクエリパラメータを暫定的に作ってコミットしました.

これは新しいパラメータを作っていいと思います!
マージしたらフロント側にも置き換えるように依頼しましょう

@iChemy
Copy link
Contributor Author

iChemy commented Oct 16, 2023

遅くなりました.
確認のほどよろしくお願いします.

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

いくつか書きました

Comment on lines 1203 to 1206
description: どのような関係性でユーザーと結びつけるか。
|取り得る値は、admins(ユーザーが管理者), belongs(ユーザーが所属している), belongs-or-admins(ユーザーが管理者または所属している)
|イベントはさらに、attendees(not absent)
|値がない場合は、belongs として振る舞う
Copy link
Member

Choose a reason for hiding this comment

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

description: |だけで複数行かけるのでそれでいいはず

Copy link
Member

Choose a reason for hiding this comment

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

イベントはさらに、、の行言ってること分かりにくいけど思い出したら別PRで書き換えときます

Comment on lines 30 to 32
RelationBelongs = iota
RelationAdmins = iota
RelationBelongsOrAdmins = iota
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RelationBelongs = iota
RelationAdmins = iota
RelationBelongsOrAdmins = iota
RelationBelongs UserRelation = iota
RelationAdmins
RelationBelongsOrAdmins

かも?今のままだとuntyped intとかになってそう

@iChemy
Copy link
Contributor Author

iChemy commented Nov 6, 2023

遅くなりました.
お時間ある際に確認のほどお願いします.

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

見逃してました 🙇
よさそうです!

@iChemy iChemy merged commit ecb2c44 into main Nov 11, 2023
4 checks passed
@iChemy iChemy deleted the fix/issue-400 branch November 11, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants