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

PATCH /games/{gameID}でvisibilityを変更できるようにする #996

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Sep 23, 2024

User description

fix: #778

  • ✨ serviceでvisibilityを更新できるようにした
  • ✨ repositoryでvisibilityを変更する実装を追加
  • ✅ getVisibilityのテスト
  • ✨ handlerでvisibilityを更新できるようにした

PR Type

enhancement, tests


Description

  • ゲームの可視性を更新する機能を追加しました。これにより、ゲームの可視性をPATCH /games/{gameID}エンドポイントで変更できるようになりました。
  • visibilityフィールドのバリデーションを追加し、無効な可視性が指定された場合はエラーメッセージを返すようにしました。
  • テストケースを追加し、可視性の変更が正しく行われることを確認しました。
  • データベースで可視性タイプを管理するためのロジックを追加しました。

Changes walkthrough 📝

Relevant files
Enhancement
game.go
ゲームの可視性を更新する機能を追加                                                                               

src/handler/v2/game.go

  • visibilityフィールドの追加と更新処理を実装
  • visibilityのバリデーションを追加
  • GameInfovisibilityを含めるように変更
  • +31/-0   
    v2_game.go
    ゲームの可視性をデータベースで管理する機能を追加                                                                 

    src/repository/gorm2/v2_game.go

    • getVisibilityメソッドを追加
    • VisibilityTypeIDの設定を追加
    +30/-2   
    visibility.go
    可視性タイプの変換関数を追加                                                                                     

    src/repository/gorm2/visibility.go

    • convertVisibilityType関数を追加
    +21/-0   
    game.go
    ゲームの可視性を更新するサービスロジックを追加                                                                   

    src/service/v2/game.go

    • visibilityパラメータをUpdateGameメソッドに追加
    • ゲームの可視性を更新するロジックを追加
    +14/-7   
    v2_game.go
    ゲーム更新メソッドに可視性パラメータを追加                                                                       

    src/service/v2_game.go

    • UpdateGameメソッドのシグネチャを変更
    +1/-1     
    Tests
    game_test.go
    ゲームの可視性更新に関するテストを追加                                                                           

    src/handler/v2/game_test.go

  • visibilityのテストケースを追加
  • visibilityがnilの場合のテストを追加
  • visibilityのアサーションを追加
  • +46/-1   
    v2_game_test.go
    可視性タイプの取得に関するテストを追加                                                                           

    src/repository/gorm2/v2_game_test.go

    • 可視性タイプの取得テストを追加
    • 可視性タイプのIDをテストケースに追加
    +73/-7   
    game_test.go
    ゲームの可視性更新に関するサービスのテストを追加                                                                 

    src/service/v2/game_test.go

    • visibilityのテストケースを追加
    • UpdateGameメソッドのテストを更新
    +119/-22

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    エラーメッセージがハードコードされています。これは保守性の低下を招く可能性があります。エラーメッセージを定数として定義するか、より柔軟なエラーハンドリング戦略を検討してください。

    Code Duplication
    visibilityの変換ロジックが複数の場所(handlerとtest)で重複しています。このロジックを共通のヘルパー関数に抽出することで、コードの重複を減らし、将来的な変更に対して一元管理が可能になります。

    Logic Check
    UpdateGameメソッドにおいて、visibilitynilの場合のロジックがありますが、これが意図した動作をしているか確認が必要です。特に、nilが渡された場合に既存の可視性を保持するという挙動が正しいかレビューが必要です。

    Copy link

    github-actions bot commented Sep 23, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    不正な可視性値を受け取った場合にログを記録する

    switch 文のデフォルトケースで echo.NewHTTPError
    を返す代わりに、ログにエラーを記録してからエラーを返すことを検討してください。これにより、問題の診断が容易になります。

    src/handler/v2/game.go [410]

     default:
    +    log.Printf("error: received invalid visibility value: %v", *req.Visibility)
         return echo.NewHTTPError(http.StatusBadRequest, "invalid visibility")
     
    Suggestion importance[1-10]: 8

    Why: Logging the error before returning an HTTP error response can aid in diagnosing issues, making this a valuable enhancement for debugging purposes.

    8
    visibilitynil の場合にデフォルト値を設定する

    visibility ポインタが nil の場合、デフォルトの可視性を設定することを検討してください。これにより、可視性が意図せず nil
    になるのを防ぐことができます。

    src/handler/v2/game.go [369]

    -if visibility == nil || game.GetVisibility() == *visibility {
    +if visibility == nil {
    +    visibility = &defaultVisibility // defaultVisibility はデフォルトの可視性を定義してください
    +}
    +if game.GetVisibility() == *visibility {
     
    Suggestion importance[1-10]: 7

    Why: Setting a default visibility when visibility is nil can prevent unintended nil values, improving robustness. However, the suggestion assumes the existence of a defaultVisibility, which is not defined in the current context.

    7
    Maintainability
    visibility 変数のスコープを if ブロック内に限定する

    visibility 変数のスコープを限定するために、if ブロック内で vis 変数を直接参照するように変更してください。これにより、コードの可読性が向上します。

    src/handler/v2/game.go [401-412]

    -var vis values.GameVisibility
    -switch {
    -case *req.Visibility == openapi.Public:
    -    vis = values.GameVisibilityTypePublic
    -case *req.Visibility == openapi.Limited:
    -    vis = values.GameVisibilityTypeLimited
    -case *req.Visibility == openapi.Private:
    -    vis = values.GameVisibilityTypePrivate
    -default:
    -    return echo.NewHTTPError(http.StatusBadRequest, "invalid visibility")
    +if req.Visibility != nil {
    +    switch {
    +    case *req.Visibility == openapi.Public:
    +        visibility = &values.GameVisibilityTypePublic
    +    case *req.Visibility == openapi.Limited:
    +        visibility = &values.GameVisibilityTypeLimited
    +    case *req.Visibility == openapi.Private:
    +        visibility = &values.GameVisibilityTypePrivate
    +    default:
    +        return echo.NewHTTPError(http.StatusBadRequest, "invalid visibility")
    +    }
     }
    -visibility = &vis
     
    Suggestion importance[1-10]: 6

    Why: Limiting the scope of the visibility variable to the if block improves code readability and maintainability, but it is a minor improvement.

    6
    Best practice
    game.GetVisibility() の結果を一時変数に格納してから比較する

    switch 文内で game.GetVisibility()
    の結果を直接比較するのではなく、一時変数に格納してから比較することを検討してください。これにより、デバッグ時に可視性の値を簡単に確認できるようになります。

    src/handler/v2/game.go [430-439]

    -switch game.GetVisibility() {
    +currentVisibility := game.GetVisibility()
    +switch currentVisibility {
     case values.GameVisibilityTypePublic:
         apiVisibility = openapi.Public
     case values.GameVisibilityTypeLimited:
         apiVisibility = openapi.Limited
     case values.GameVisibilityTypePrivate:
         apiVisibility = openapi.Private
     default:
    -    log.Printf("error: failed to get game visibility: %v\n", game.GetVisibility())
    +    log.Printf("error: failed to get game visibility: %v\n", currentVisibility)
         return echo.NewHTTPError(http.StatusInternalServerError, "failed to get game visibility")
     }
     
    Suggestion importance[1-10]: 5

    Why: Storing the result of game.GetVisibility() in a temporary variable before comparison can make debugging easier, but it is a minor best practice improvement. The existing code is already functional without this change.

    5

    Copy link

    codecov bot commented Sep 23, 2024

    Codecov Report

    Attention: Patch coverage is 72.97297% with 20 lines in your changes missing coverage. Please review.

    Project coverage is 49.10%. Comparing base (e8c3e2b) to head (c2c6546).
    Report is 42 commits behind head on main.

    Files with missing lines Patch % Lines
    src/handler/v2/game.go 59.25% 11 Missing ⚠️
    src/repository/gorm2/v2_game.go 62.50% 6 Missing and 3 partials ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #996      +/-   ##
    ==========================================
    - Coverage   50.77%   49.10%   -1.67%     
    ==========================================
      Files         120      121       +1     
      Lines        8705    10742    +2037     
    ==========================================
    + Hits         4420     5275     +855     
    - Misses       3961     5140    +1179     
    - Partials      324      327       +3     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @ikura-hamu ikura-hamu merged commit 5cc251f into main Oct 25, 2024
    9 of 10 checks passed
    @ikura-hamu ikura-hamu deleted the feat/patch_game_visibility branch October 25, 2024 07:30
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PATCH /games/{gameID}の修正
    1 participant