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

コメント周りのAPIを実装 #12

Merged
merged 8 commits into from
Dec 24, 2024
Merged

コメント周りのAPIを実装 #12

merged 8 commits into from
Dec 24, 2024

Conversation

cp-20
Copy link
Contributor

@cp-20 cp-20 commented Dec 19, 2024

No description provided.

@cp-20 cp-20 requested a review from Takeno-hito December 19, 2024 07:57
Copy link
Member

@Takeno-hito Takeno-hito left a comment

Choose a reason for hiding this comment

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

ざっくりコメント付けました〜

model/comments.go Outdated Show resolved Hide resolved
}

func CreateComment(p *CreateCommentPayload) (*Comment, error) {
if p.ItemID == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

[q] これ、ItemID=0 のものは弾くということであってる?
[q] Invalid な ItemID が来たときは弾かない?弾いたほうが良さそうに見えるけど

model/comments.go Outdated Show resolved Hide resolved
model/comments.go Outdated Show resolved Hide resolved
{
name: "異常系: ItemIDが存在しない",
payload: &CreateCommentPayload{
UserID: "user1",
Copy link
Member

Choose a reason for hiding this comment

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

[imo] 0 を弾くのって話にも関連するけど、この場合 0 値として ItemId=0 がセットされているので、これを弾くのはちょっと気になるかも

return invalidRequest(c, err)
}

me := getAuthorizedUser(c)
Copy link
Member

Choose a reason for hiding this comment

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

[q] これって必ず失敗しない関数なの?(この PR とは無関係の話なので一旦無視しちゃって良いけど)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

内部で呼んでるのは echo.Context.Get です

Copy link
Member

Choose a reason for hiding this comment

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

:naruhodo: ちょっとコード読んだんだけど、AuthMiddleware を通さない状態でこの関数を呼び出すと panic するので気を付けて!外部からは一切のエンドポイントを叩けなくても ok ならこの実装でも良いっちゃよいです
(それはそれとして getAuthorizedUser で panic 起きてもアプリケーションが落ちないようにしたほうがいいかも)

Copy link
Member

@Takeno-hito Takeno-hito Dec 20, 2024

Choose a reason for hiding this comment

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

:naruhodo: ちょっとコード読んだんだけど、AuthMiddleware を通さない状態でこの関数を呼び出すと panic するので気を付けて!外部からは一切のエンドポイントを叩けなくても ok という方針ならこの実装でも良いっちゃよいです
(それはそれとして getAuthorizedUser で panic 起きてもアプリケーションが落ちないようにしたほうがいいかも)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

panic するんですね :hae_zubora:

router/comments.go Outdated Show resolved Hide resolved
router/comments_test.go Outdated Show resolved Hide resolved
router/comments_test.go Outdated Show resolved Hide resolved
router/test_common.go Outdated Show resolved Hide resolved
@cp-20 cp-20 merged commit 94ac3d9 into main Dec 24, 2024
8 checks passed
@cp-20 cp-20 deleted the feat/comments branch December 24, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants