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 /genres/:genreID`の実装 #1023

Merged
merged 12 commits into from
Oct 27, 2024
Merged

PATCH /genres/:genreID`の実装 #1023

merged 12 commits into from
Oct 27, 2024

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Oct 25, 2024

User description

fix: #720

  • ✨ serviceのUpdateGameGenreの実装
  • 🩹 repositoryのinterface定義変更
  • 🩹 Updateで返すエラーを変更
  • ✨ repositoryでUpdateGameGenreを実装
  • ✨ repositoryでGetGameGenre実装
  • 🩹 serviceのエラーハンドリング修正
  • ✨ handlerでジャンル情報の変更を行う処理を実装
  • ✨ serviceでGetGameGenreを実装
  • ✨ repositoryでGetGamesByGenreIDを実装

PR Type

Enhancement, Tests


Description

  • PATCH /genres/{gameGenreID}の実装を行い、ゲームジャンルの情報を更新する機能を追加しました。
  • ハンドラー、サービス、リポジトリ層での実装を行い、各層でのエラーハンドリングを強化しました。
  • 新たに追加されたメソッドに対する単体テストを実装し、正常系および異常系の動作を確認しました。
  • リポジトリ層では、GORMを用いたデータベース操作を実装し、重複エラーや更新がない場合のエラーを適切に処理しました。

Changes walkthrough 📝

Relevant files
Enhancement
6 files
game_genre.go
Implement PATCH method for updating game genre                     

src/handler/v2/game_genre.go

  • PatchGameGenre method implemented for updating game genre information.
  • Error handling for invalid request body and genre name validation
    added.
  • JSON response for successful updates implemented.
  • +50/-13 
    game_genre.go
    Implement repository methods for game genre updates           

    src/repository/game_genre.go

  • Added UpdateGameGenre method to update game genre information.
  • Added GetGameGenre method to retrieve game genre by ID.
  • Added GetGamesByGenreID method to retrieve games by genre ID.
  • +12/-0   
    game_genre.go
    Implement GORM repository methods for game genre                 

    src/repository/gorm2/game_genre.go

  • Implemented UpdateGameGenre method for database updates.
  • Implemented GetGameGenre and GetGamesByGenreID methods.
  • Error handling for database operations added.
  • +91/-0   
    errors.go
    Define new error types for game genre service                       

    src/service/errors.go

  • Added new error types for game genre operations.
  • Defined ErrNoGameGenreUpdated and ErrDuplicateGameGenreName.
  • +2/-0     
    game_genre.go
    Extend service interface for game genre updates                   

    src/service/game_genre.go

  • Added UpdateGameGenre method to service interface.
  • Defined error handling for game genre updates.
  • +5/-0     
    game_genre.go
    Implement service logic for updating game genre                   

    src/service/v2/game_genre.go

  • Implemented UpdateGameGenre method in service.
  • Handled transactions and error scenarios.
  • Integrated repository methods for game genre updates.
  • +46/-0   
    Tests
    3 files
    game_genre_test.go
    Add tests for PATCH game genre functionality                         

    src/handler/v2/game_genre_test.go

  • Added test cases for PatchGameGenre method.
  • Validated error scenarios and successful updates.
  • Utilized mock services for testing.
  • +207/-0 
    game_genre_test.go
    Add repository tests for game genre methods                           

    src/repository/gorm2/game_genre_test.go

  • Added tests for UpdateGameGenre repository method.
  • Added tests for GetGameGenre and GetGamesByGenreID.
  • Validated database operations and error scenarios.
  • +330/-0 
    game_genre_test.go
    Add service tests for updating game genre                               

    src/service/v2/game_genre_test.go

  • Added tests for UpdateGameGenre service method.
  • Validated logic and error handling.
  • Used mock repositories for testing.
  • +163/-0 

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

    Copy link

    github-actions bot commented Oct 25, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit c376176)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    720 - Fully compliant

    Fully compliant requirements:

    • service実装
    • repository実装
    • handler実装
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    エラーハンドリングのロジックが複雑で、特定のエラーケースで適切なHTTPステータスコードが返されているかの確認が必要

    Database Error Handling
    データベース操作時のエラーハンドリングが適切かどうか確認する必要がある。特に、エラーコードのチェックとその後のエラーハンドリングのロジック

    Copy link

    github-actions bot commented Oct 25, 2024

    PR Code Suggestions ✨

    Latest suggestions up to c376176
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    専用のロギングライブラリを使用してログの一貫性を向上させる。


    PatchGameGenre関数内でlog.Printfを使用していますが、エラーロギングのための専用のロギングライブラリを使用することで、ログの一貫性と検索性を向上させることができます。

    src/handler/v2/game_genre.go [188]

    -log.Printf("failed to validate genre name: %v\n", err)
    +logger.Error("failed to validate genre name", zap.Error(err))
    Suggestion importance[1-10]: 7

    Why: Replacing log.Printf with a structured logging library like zap can improve log consistency and searchability. This suggestion is beneficial for enhancing logging practices, assuming the project can accommodate the additional dependency.

    7
    エラーレスポンスの生成をヘルパー関数で行うことで一貫性を保つ。


    PatchGameGenre関数内で複数の場所でecho.NewHTTPErrorを使用していますが、これをエラーレスポンスを生成するヘルパー関数に置き換えることで、エラーメッセージの一貫性を保ちやすくなります。

    src/handler/v2/game_genre.go [183]

    -return echo.NewHTTPError(http.StatusBadRequest, "genre name must not be empty")
    +return newHTTPError(http.StatusBadRequest, "genre name must not be empty")
    Suggestion importance[1-10]: 6

    Why: Using a helper function for generating HTTP errors can enhance consistency across the codebase. This suggestion is valid as it promotes best practices, although it requires additional implementation for the helper function.

    6
    Maintainability
    リクエストボディのバインディングエラー処理を関数化して再利用性を向上させる。


    PatchGameGenre関数内でリクエストボディのバインディング後、直接HTTPエラーを返していますが、これを独自のエラーハンドリング関数に抽象化することで、コードの再利用性とメンテナンス性を向上させることができます。

    src/handler/v2/game_genre.go [176-177]

     if err := c.Bind(&reqBody); err != nil {
    -    return echo.NewHTTPError(http.StatusBadRequest, "invalid request body")
    +    return handleBadRequest("invalid request body")
     }
    Suggestion importance[1-10]: 5

    Why: Abstracting the error handling into a separate function could improve code reusability and maintainability. However, the suggestion lacks details on how the handleBadRequest function should be implemented, reducing its immediate applicability.

    5

    Previous suggestions

    Suggestions up to commit 91224f3
    CategorySuggestion                                                                                                                                    Score
    Enhancement
    gameGenreName.Validate()のエラーハンドリングを改善して、適切なHTTPステータスコードを返すように修正します。


    PatchGameGenreメソッド内でgameGenreName.Validate()のエラーハンドリングを改善してください。現在の実装では、values.ErrGameGenreNameEmptyvalues.ErrGameGenreNameTooLong以外のエラーが発生した場合、内部サーバーエラーを返していますが、これはクライアントにとって誤解を招く可能性があります。エラーの種類に応じた適切なHTTPステータスコードを返すように修正してください。

    src/handler/v2/game_genre.go [180-189]

     if err := gameGenreName.Validate(); err != nil {
         if errors.Is(err, values.ErrGameGenreNameEmpty) {
             return echo.NewHTTPError(http.StatusBadRequest, "genre name must not be empty")
         }
         if errors.Is(err, values.ErrGameGenreNameTooLong) {
             return echo.NewHTTPError(http.StatusBadRequest, "genre name is too long")
         }
    -    log.Printf("failed to validate genre name: %v\n", err)
    -    return echo.NewHTTPError(http.StatusInternalServerError, "failed to validate genre name")
    +    // ここで適切なエラーハンドリングを行う
    +    return echo.NewHTTPError(http.StatusBadRequest, "invalid genre name")
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential issue where the current error handling for gameGenreName.Validate() may return a misleading internal server error for validation issues. By proposing to return a more appropriate HTTP status code, it enhances the clarity and accuracy of error responses to the client, improving the robustness of the API.

    7
    Possible bug
    UpdateGameGenre関数のエラーハンドリングを強化する。


    UpdateGameGenreのエラーハンドリングが不十分です。UpdateGameGenreErrnilの場合でもエラーが発生する可能性があるため、エラーがnilかどうかを確認する処理を追加してください。

    src/service/v2/game_genre_test.go [413]

     err := gameGenreService.UpdateGameGenre(ctx, testCase.gameGenre.GetID(), testCase.newGameGenreName)
    +if err != nil {
    +    t.Errorf("Unexpected error: %v", err)
    +}
    Suggestion importance[1-10]: 7

    Why: The suggestion enhances error handling by checking if the error is nil after calling UpdateGameGenre. This is a good practice to ensure that unexpected errors are caught and reported, improving the robustness of the test cases.

    7

    Copy link

    codecov bot commented Oct 25, 2024

    Codecov Report

    Attention: Patch coverage is 84.00000% with 24 lines in your changes missing coverage. Please review.

    Project coverage is 49.58%. Comparing base (5cc251f) to head (c376176).
    Report is 13 commits behind head on main.

    Files with missing lines Patch % Lines
    src/repository/gorm2/game_genre.go 69.86% 17 Missing and 5 partials ⚠️
    src/handler/v2/game_genre.go 94.87% 2 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main    #1023      +/-   ##
    ==========================================
    + Coverage   49.10%   49.58%   +0.48%     
    ==========================================
      Files         121      121              
      Lines       10742    10892     +150     
    ==========================================
    + Hits         5275     5401     +126     
    - Misses       5140     5159      +19     
    - Partials      327      332       +5     

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

    @ikura-hamu ikura-hamu marked this pull request as draft October 27, 2024 09:14
    @ikura-hamu ikura-hamu marked this pull request as ready for review October 27, 2024 09:22
    Copy link

    Persistent review updated to latest commit c376176

    @ikura-hamu ikura-hamu merged commit 77614ff into main Oct 27, 2024
    11 checks passed
    @ikura-hamu ikura-hamu deleted the feat/patch_game_genre branch October 27, 2024 09:44
    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 /genres/{gameGenreID}実装
    1 participant