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.

Sign up for GitHub

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

Jump to bottom

handlerのgame_roleのテスト #1092

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

handlerのgame_roleのテスト #1092

wants to merge 9 commits into from

Conversation

ikura-hamu
Copy link
Member

@ikura-hamu ikura-hamu commented Jan 6, 2025 •
edited by github-actions bot
Loading

User description

fix #1086
一部バグ修正含む

  • 🐛 PatchGameRoleでvisibilityを返してなかった
  • 🐛 PatchGameRoleでジャンルを取得してなかった
  • ♻️ convertGameVisibility 関数を追加
  • 🐛 roleのtypeのnilチェックを追加
  • ✅ handlerのPatchGameRoleのテストを追加
  • 🐛 DeleteGameRoleでジャンルとvisibilityを返すように修正
  • 🩹 英語がおかしい
  • ✅ DeleteGameRoleのテスト追加

PR Type

Tests, Bug fix

Description

  • PatchGameRoleにおけるバグ修正とテスト追加

    • visibilityとジャンルの取得と返却を追加
    • roleのtypeがnilの場合のチェックを追加

  • DeleteGameRoleにおけるバグ修正とテスト追加

    • visibilityとジャンルの返却を追加
    • エラーメッセージの修正

  • 新しいユーティリティ関数convertGameVisibilityの追加

  • PatchGameRoleとDeleteGameRoleのユニットテストを大幅に追加

Changes walkthrough 📝

Relevant files
Enhancement
game.go
ゲームのvisibility変換関数を追加                                                                       

src/handler/v2/game.go

  • 新しい関数convertGameVisibilityを追加
  • visibilityの変換処理を実装
 +14/-0   
Bug fix
game_role.go
PatchGameRoleとDeleteGameRoleのバグ修正と改善                                         

src/handler/v2/game_role.go

  • PatchGameRoleでvisibilityとジャンルを返却する処理を追加
  • roleのtypeがnilの場合のチェックを追加
  • エラーメッセージの修正
  • DeleteGameRoleでvisibilityとジャンルを返却する処理を追加
    		• +56/-27 
    Tests
    game_role_test.go
    PatchGameRoleとDeleteGameRoleのユニットテスト追加                                     

    src/handler/v2/game_role_test.go

  • PatchGameRoleのユニットテストを追加
  • DeleteGameRoleのユニットテストを追加
  • 各種エラーハンドリングのテストケースを追加
    		• +650/-0 

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

    @github-actions GitHub Actions
    Copy link

    github-actions bot commented Jan 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    1086 - Fully compliant

    Fully compliant requirements:

    • Add unit tests for handler/game_role.
    • Fix bugs in PatchGameRole and DeleteGameRole handlers.
    • Ensure PatchGameRole returns visibility and genre.
    • Add nil check for role type in PatchGameRole.
    • Add utility function convertGameVisibility.
    • Ensure DeleteGameRole returns visibility and genre.

    Not compliant requirements:
    []

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The convertGameVisibility function could potentially return an empty string for invalid visibility values. Ensure this is handled correctly in all cases.

    func convertGameVisibility(value values.GameVisibility) (openapi.GameVisibility, error) {
	switch value {
	case values.GameVisibilityTypePublic:
		return openapi.Public, nil
	case values.GameVisibilityTypeLimited:
		return openapi.Limited, nil
	case values.GameVisibilityTypePrivate:
		return openapi.Private, nil
	default:
		return "", fmt.Errorf("invalid game visibility: %v", value)
	}
}
    Error Handling

    Error handling for convertGameVisibility in PatchGameRole and DeleteGameRole should be reviewed to ensure proper logging and user feedback.

    var visibility openapi.GameVisibility
visibility, err = convertGameVisibility(newGameInfo.Game.GetVisibility())
if err != nil {
	log.Printf("error: failed to convert game visibility: %v\n", err)
	return echo.NewHTTPError(http.StatusInternalServerError, "failed to convert game visibility")
    Test Coverage

    Ensure all edge cases for PatchGameRole and DeleteGameRole are covered, especially for invalid visibility and genre values.

    func TestPatchGameRole(t *testing.T) {
	t.Parallel()

	ctrl := gomock.NewController(t)
	defer ctrl.Finish()

	game := domain.NewGame(values.NewGameID(), "game1", "game1 description", values.GameVisibilityTypePrivate, time.Now())
	user := service.NewUserInfo(values.NewTrapMemberID(uuid.New()), "user1", values.TrapMemberStatusActive)
	genre1 := domain.NewGameGenre(values.NewGameGenreID(), "genre1", time.Now())

	var (
		roleTypeOwner      openapi.GameRoleType = openapi.Owner
		roleTypeMaintainer openapi.GameRoleType = openapi.Maintainer
		roleTypeInvalid    openapi.GameRoleType = "invalid"
	)

	ownerRequestBody := &openapi.PatchGameRoleJSONRequestBody{
		Id:   openapi.UserID(user.GetID()),
		Type: &roleTypeOwner,
	}

	validAuthSession := domain.NewOIDCSession(values.NewOIDCAccessToken("token"), time.Now().Add(time.Hour))

	testCases := map[string]struct {
		gameID                        openapi.GameIDInPath
		sessionExist                  bool
		authSession                   *domain.OIDCSession
		reqBody                       *openapi.PatchGameRoleJSONRequestBody
		executeEditGameManagementRole bool
		EditGameManagementRoleErr     error
		executeGetGame                bool
		newGameInfo                   *service.GameInfoV2
		GetGameErr                    error
		isErr                         bool
		err                           error
		statusCode                    int
		expectedResponse              *openapi.Game
	}{
		"ownerでも問題なし": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			executeGetGame:                true,
			newGameInfo: &service.GameInfoV2{
				Game:   game,
				Owners: []*service.UserInfo{user},
				Genres: []*domain.GameGenre{genre1},
			},
			expectedResponse: &openapi.Game{
				Id:          openapi.GameID(game.GetID()),
				Name:        openapi.GameName(game.GetName()),
				Description: openapi.GameDescription(game.GetDescription()),
				Visibility:  openapi.Private,
				Owners:      []openapi.UserName{string(user.GetName())},
				CreatedAt:   game.GetCreatedAt(),
				Genres:      &[]openapi.GameGenreName{string(genre1.GetName())},
			},
		},
		"maintainerでも問題なし": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			sessionExist: true,
			authSession:  validAuthSession,
			reqBody: &openapi.PatchGameRoleJSONRequestBody{
				Id:   openapi.UserID(user.GetID()),
				Type: &roleTypeMaintainer,
			},
			executeEditGameManagementRole: true,
			executeGetGame:                true,
			newGameInfo: &service.GameInfoV2{
				Game:        game,
				Owners:      []*service.UserInfo{},
				Maintainers: []*service.UserInfo{user},
				Genres:      []*domain.GameGenre{genre1},
			},
			expectedResponse: &openapi.Game{
				Id:          openapi.GameID(game.GetID()),
				Name:        openapi.GameName(game.GetName()),
				Description: openapi.GameDescription(game.GetDescription()),
				Visibility:  openapi.Private,
				Owners:      []openapi.UserName{},
				Maintainers: &[]openapi.UserName{string(user.GetName())},
				CreatedAt:   game.GetCreatedAt(),
				Genres:      &[]openapi.GameGenreName{string(genre1.GetName())},
			},
		},
		"roleTypeがnilでも問題なし": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			sessionExist: true,
			authSession:  validAuthSession,
			reqBody: &openapi.PatchGameRoleJSONRequestBody{
				Id: openapi.UserID(user.GetID()),
			},
			executeEditGameManagementRole: false,
			executeGetGame:                true,
			newGameInfo: &service.GameInfoV2{
				Game:        game,
				Owners:      []*service.UserInfo{user},
				Maintainers: []*service.UserInfo{},
				Genres:      []*domain.GameGenre{genre1},
			},
			expectedResponse: &openapi.Game{
				Id:          openapi.GameID(game.GetID()),
				Name:        openapi.GameName(game.GetName()),
				Description: openapi.GameDescription(game.GetDescription()),
				Visibility:  openapi.Private,
				Owners:      []openapi.UserName{string(user.GetName())},
				CreatedAt:   game.GetCreatedAt(),
				Genres:      &[]openapi.GameGenreName{string(genre1.GetName())},
			},
		},
		"sessionがないので401": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			sessionExist: false,
			statusCode:   http.StatusUnauthorized,
			isErr:        true,
		},
		"authSessionが無効なので401": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			sessionExist: true,
			authSession:  nil,
			statusCode:   http.StatusUnauthorized,
			isErr:        true,
		},
		"無効なroleTypeなので400": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			sessionExist: true,
			authSession:  validAuthSession,
			reqBody: &openapi.PatchGameRoleJSONRequestBody{
				Id:   openapi.UserID(user.GetID()),
				Type: &roleTypeInvalid,
			},
			statusCode: http.StatusBadRequest,
			isErr:      true,
		},
		"EditGameManagementRoleがErrNoGameManagementRoleUpdatedなので400": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			EditGameManagementRoleErr:     service.ErrNoGameManagementRoleUpdated,
			statusCode:                    http.StatusBadRequest,
			isErr:                         true,
		},
		"EditGameManagementRoleがErrNoGameなので404": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			EditGameManagementRoleErr:     service.ErrNoGame,
			statusCode:                    http.StatusNotFound,
			isErr:                         true,
		},
		"EditGameManagementRoleがErrInvalidUserIDなので400": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			EditGameManagementRoleErr:     service.ErrInvalidUserID,
			statusCode:                    http.StatusBadRequest,
			isErr:                         true,
		},
		"EditGameManagementRoleがErrCannotEditOwnersなので400": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			EditGameManagementRoleErr:     service.ErrCannotEditOwners,
			statusCode:                    http.StatusBadRequest,
			isErr:                         true,
		},
		"EditGameManagementRoleがエラーなので500": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			EditGameManagementRoleErr:     errors.New("error"),
			statusCode:                    http.StatusInternalServerError,
			isErr:                         true,
		},
		"GetGameがErrNoGameなので404": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			executeGetGame:                true,
			GetGameErr:                    service.ErrNoGame,
			statusCode:                    http.StatusNotFound,
			isErr:                         true,
		},
		"GetGameがエラーなので500": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			executeGetGame:                true,
			GetGameErr:                    errors.New("error"),
			statusCode:                    http.StatusInternalServerError,
			isErr:                         true,
		},
		"GetGameのvisibilityが無効なので500": {
			gameID:                        openapi.GameIDInPath(game.GetID()),
			sessionExist:                  true,
			authSession:                   validAuthSession,
			reqBody:                       ownerRequestBody,
			executeEditGameManagementRole: true,
			executeGetGame:                true,
			newGameInfo: &service.GameInfoV2{
				Game:   domain.NewGame(game.GetID(), game.GetName(), game.GetDescription(), values.GameVisibility(100), game.GetCreatedAt()),
				Owners: []*service.UserInfo{user},
				Genres: []*domain.GameGenre{genre1},
			},
			statusCode: http.StatusInternalServerError,
			isErr:      true,
		},
	}

	for name, testCase := range testCases {
		t.Run(name, func(t *testing.T) {
			t.Parallel()

			mockConf := mockConfig.NewMockHandler(ctrl)
			mockConf.
				EXPECT().
				SessionKey().
				Return("key", nil)
			mockConf.
				EXPECT().
				SessionSecret().
				Return("secret", nil)
			sess, err := common.NewSession(mockConf)
			if err != nil {
				t.Fatalf("failed to create session: %v", err)
				return
			}
			session, err := NewSession(sess)
			if err != nil {
				t.Fatalf("failed to create session: %v", err)
				return
			}

			bodyBytes, err := json.Marshal(testCase.reqBody)
			require.NoError(t, err)

			buf := bytes.NewBuffer(bodyBytes)

			e := echo.New()
			req := httptest.NewRequest(
				http.MethodPatch,
				fmt.Sprintf("/api/v2/games/%s/role", testCase.gameID.String()),
				buf,
			)
			req.Header.Set(echo.HeaderContentType, "application/json")
			rec := httptest.NewRecorder()
			c := e.NewContext(req, rec)

			if testCase.sessionExist {
				sess, err := session.New(req)
				if err != nil {
					t.Fatal(err)
				}

				if testCase.authSession != nil {
					sess.Values[accessTokenSessionKey] = string(testCase.authSession.GetAccessToken())
					sess.Values[expiresAtSessionKey] = testCase.authSession.GetExpiresAt()
				}

				err = sess.Save(req, rec)
				if err != nil {
					t.Fatalf("failed to save session: %v", err)
				}

				setCookieHeader(c)

				sess, err = session.Get(req)
				if err != nil {
					t.Fatal(err)
				}

				c.Set("session", sess)
			}

			mockGameRoleService := mock.NewMockGameRoleV2(ctrl)
			if testCase.executeEditGameManagementRole {
				mockGameRoleService.
					EXPECT().
					EditGameManagementRole(gomock.Any(), gomock.Any(), values.GameID(testCase.gameID), values.NewTrapMemberID(testCase.reqBody.Id), gomock.Any()).
					Return(testCase.EditGameManagementRoleErr)
			}

			mockGameService := mock.NewMockGameV2(ctrl)
			if testCase.executeGetGame {
				mockGameService.
					EXPECT().
					GetGame(gomock.Any(), gomock.Any(), values.GameID(testCase.gameID)).
					Return(testCase.newGameInfo, testCase.GetGameErr)
			}

			gameRole := NewGameRole(mockGameRoleService, mockGameService, session)

			err = gameRole.PatchGameRole(c, testCase.gameID)

			if testCase.isErr {
				if testCase.statusCode != 0 {
					var httpErr *echo.HTTPError
					if assert.ErrorAs(t, err, &httpErr) {
						assert.Equal(t, testCase.statusCode, httpErr.Code)
					}
				} else if testCase.err != nil {
					assert.ErrorIs(t, err, testCase.err)
				} else {
					assert.Error(t, err)
				}
			} else {
				assert.NoError(t, err)
			}

			if testCase.isErr {
				return
			}

			var responseGame openapi.Game
			err = json.NewDecoder(rec.Body).Decode(&responseGame)
			require.NoError(t, err)

			assert.Equal(t, testCase.expectedResponse.Id, responseGame.Id)
			assert.Equal(t, testCase.expectedResponse.Name, responseGame.Name)
			assert.Equal(t, testCase.expectedResponse.Description, responseGame.Description)
			assert.Equal(t, testCase.expectedResponse.Visibility, responseGame.Visibility)
			for i := range testCase.expectedResponse.Owners {
				assert.Equal(t, testCase.expectedResponse.Owners[i], responseGame.Owners[i])
			}
			if testCase.expectedResponse.Maintainers != nil {
				for i := range *testCase.expectedResponse.Maintainers {
					assert.Equal(t, (*testCase.expectedResponse.Maintainers)[i], (*responseGame.Maintainers)[i])
				}
			}
			assert.WithinDuration(t, testCase.expectedResponse.CreatedAt, responseGame.CreatedAt, time.Second)
			if testCase.expectedResponse.Genres != nil {
				for i := range *testCase.expectedResponse.Genres {
					assert.Equal(t, (*testCase.expectedResponse.Genres)[i], (*responseGame.Genres)[i])
				}
			}
		})
	}
}

func TestDeleteGameRole(t *testing.T) {
	t.Parallel()

	ctrl := gomock.NewController(t)
	defer ctrl.Finish()

	game := domain.NewGame(values.NewGameID(), "game1", "game1 description", values.GameVisibilityTypePrivate, time.Now())
	user := service.NewUserInfo(values.NewTrapMemberID(uuid.New()), "user1", values.TrapMemberStatusActive)
	user2 := service.NewUserInfo(values.NewTrapMemberID(uuid.New()), "user2", values.TrapMemberStatusActive)
	genre1 := domain.NewGameGenre(values.NewGameGenreID(), "genre1", time.Now())

	validAuthSession := domain.NewOIDCSession(values.NewOIDCAccessToken("token"), time.Now().Add(time.Hour))

	testCases := map[string]struct {
		gameID                          openapi.GameIDInPath
		userID                          openapi.UserIDInPath
		sessionExist                    bool
		authSession                     *domain.OIDCSession
		executeDeleteGameManagementRole bool
		DeleteGameManagementRoleErr     error
		executeGetGame                  bool
		newGameInfo                     *service.GameInfoV2
		GetGameErr                      error
		isErr                           bool
		err                             error
		statusCode                      int
		expectedResponse                *openapi.Game
	}{
		"特に問題なく削除できる": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			executeGetGame:                  true,
			newGameInfo: &service.GameInfoV2{
				Game:        game,
				Owners:      []*service.UserInfo{user},
				Maintainers: []*service.UserInfo{user2},
				Genres:      []*domain.GameGenre{genre1},
			},
			expectedResponse: &openapi.Game{
				Id:          openapi.GameID(game.GetID()),
				Name:        openapi.GameName(game.GetName()),
				Description: openapi.GameDescription(game.GetDescription()),
				Visibility:  openapi.Private,
				Owners:      []openapi.UserName{string(user.GetName())},
				CreatedAt:   game.GetCreatedAt(),
				Genres:      &[]openapi.GameGenreName{string(genre1.GetName())},
			},
		},
		"sessionがないので401": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			userID:       openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist: false,
			statusCode:   http.StatusUnauthorized,
			isErr:        true,
		},
		"authSessionが無効なので401": {
			gameID:       openapi.GameIDInPath(game.GetID()),
			userID:       openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist: true,
			authSession:  nil,
			statusCode:   http.StatusUnauthorized,
			isErr:        true,
		},
		"RemoveGameManagementRoleがErrInvalidRoleなので404": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			DeleteGameManagementRoleErr:     service.ErrInvalidRole,
			statusCode:                      http.StatusNotFound,
			isErr:                           true,
		},
		"RemoveGameManagementRoleがErrCannotDeleteOwnerなので400": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(user.GetID()),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			DeleteGameManagementRoleErr:     service.ErrCannotDeleteOwner,
			statusCode:                      http.StatusBadRequest,
			isErr:                           true,
		},
		"RemoveGameManagementRoleがErrNoGameなので400": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			DeleteGameManagementRoleErr:     service.ErrNoGame,
			statusCode:                      http.StatusBadRequest,
			isErr:                           true,
		},
		"RemoveGameManagementRoleがエラーなので500": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			DeleteGameManagementRoleErr:     errors.New("error"),
			statusCode:                      http.StatusInternalServerError,
			isErr:                           true,
		},
		"GetGameがErrNoGameなので404": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			executeGetGame:                  true,
			GetGameErr:                      service.ErrNoGame,
			statusCode:                      http.StatusNotFound,
			isErr:                           true,
		},
		"GetGameがエラーなので500": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			executeGetGame:                  true,
			GetGameErr:                      errors.New("error"),
			statusCode:                      http.StatusInternalServerError,
			isErr:                           true,
		},
		"GetGameのvisibilityが無効なので500": {
			gameID:                          openapi.GameIDInPath(game.GetID()),
			userID:                          openapi.UserIDInPath(values.NewTrapMemberID(uuid.New())),
			sessionExist:                    true,
			authSession:                     validAuthSession,
			executeDeleteGameManagementRole: true,
			executeGetGame:                  true,
			newGameInfo: &service.GameInfoV2{
				Game:        domain.NewGame(game.GetID(), game.GetName(), game.GetDescription(), values.GameVisibility(100), game.GetCreatedAt()),
				Owners:      []*service.UserInfo{user},
				Maintainers: []*service.UserInfo{user2},
				Genres:      []*domain.GameGenre{genre1},
			},
			statusCode: http.StatusInternalServerError,
			isErr:      true,
		},
	}

	for name, testCase := range testCases {
		t.Run(name, func(t *testing.T) {
			t.Parallel()

			mockConf := mockConfig.NewMockHandler(ctrl)
			mockConf.
				EXPECT().
				SessionKey().
				Return("key", nil)
			mockConf.
				EXPECT().
				SessionSecret().
				Return("secret", nil)
			sess, err := common.NewSession(mockConf)
			if err != nil {
				t.Fatalf("failed to create session: %v", err)
				return
			}
			session, err := NewSession(sess)
			if err != nil {
				t.Fatalf("failed to create session: %v", err)
				return
			}

			e := echo.New()
			req := httptest.NewRequest(
				http.MethodDelete,
				fmt.Sprintf("/api/v2/games/%s/role/%s", testCase.gameID.String(), testCase.userID.String()),
				nil,
			)
			rec := httptest.NewRecorder()
			c := e.NewContext(req, rec)

			if testCase.sessionExist {
				sess, err := session.New(req)
				if err != nil {
					t.Fatal(err)
				}

				if testCase.authSession != nil {
					sess.Values[accessTokenSessionKey] = string(testCase.authSession.GetAccessToken())
					sess.Values[expiresAtSessionKey] = testCase.authSession.GetExpiresAt()
				}

				err = sess.Save(req, rec)
				if err != nil {
					t.Fatalf("failed to save session: %v", err)
				}

				setCookieHeader(c)

				sess, err = session.Get(req)
				if err != nil {
					t.Fatal(err)
				}

				c.Set("session", sess)
			}

			mockGameRoleService := mock.NewMockGameRoleV2(ctrl)
			if testCase.executeDeleteGameManagementRole {
				mockGameRoleService.
					EXPECT().
					RemoveGameManagementRole(gomock.Any(), values.GameID(testCase.gameID), values.NewTrapMemberID(testCase.userID)).
					Return(testCase.DeleteGameManagementRoleErr)
			}

			mockGameService := mock.NewMockGameV2(ctrl)
			if testCase.executeGetGame {
				mockGameService.
					EXPECT().
					GetGame(gomock.Any(), gomock.Any(), values.GameID(testCase.gameID)).
					Return(testCase.newGameInfo, testCase.GetGameErr)
			}

			gameRole := NewGameRole(mockGameRoleService, mockGameService, session)

			err = gameRole.DeleteGameRole(c, testCase.gameID, testCase.userID)

			if testCase.isErr {
				if testCase.statusCode != 0 {
					var httpErr *echo.HTTPError
					if assert.ErrorAs(t, err, &httpErr) {
						assert.Equal(t, testCase.statusCode, httpErr.Code)
					}
				} else if testCase.err != nil {
					assert.ErrorIs(t, err, testCase.err)
				} else {
					assert.Error(t, err)
				}
			} else {
				assert.NoError(t, err)
			}

			if testCase.isErr {
				return
			}

			var responseGame openapi.Game
			err = json.NewDecoder(rec.Body).Decode(&responseGame)
			require.NoError(t, err)

			assert.Equal(t, testCase.expectedResponse.Id, responseGame.Id)
			assert.Equal(t, testCase.expectedResponse.Name, responseGame.Name)
			assert.Equal(t, testCase.expectedResponse.Description, responseGame.Description)
			assert.Equal(t, testCase.expectedResponse.Visibility, responseGame.Visibility)
			for i := range testCase.expectedResponse.Owners {
				assert.Equal(t, testCase.expectedResponse.Owners[i], responseGame.Owners[i])
			}
			if testCase.expectedResponse.Maintainers != nil {
				for i := range *testCase.expectedResponse.Maintainers {
					assert.Equal(t, (*testCase.expectedResponse.Maintainers)[i], (*responseGame.Maintainers)[i])
				}
			}
			assert.WithinDuration(t, testCase.expectedResponse.CreatedAt, responseGame.CreatedAt, time.Second)
			if testCase.expectedResponse.Genres != nil {
				for i := range *testCase.expectedResponse.Genres {
					assert.Equal(t, (*testCase.expectedResponse.Genres)[i], (*responseGame.Genres)[i])
				}
			}
		})
	}
}

    @github-actions GitHub Actions
    Copy link

    github-actions bot commented Jan 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    エラーハンドリングのステータスコードを一貫性のあるものに修正します。


    DeleteGameRole関数内でRemoveGameManagementRoleのエラーハンドリングにおいて、エラーメッセージの一貫性を保つために、ErrNoGameのステータスコードを404に変更することを検討してください。

    src/handler/v2/game_role.go [468-469]

     if errors.Is(err, service.ErrNoGame) {
-    return echo.NewHTTPError(http.StatusBadRequest, "no game")
+    return echo.NewHTTPError(http.StatusNotFound, "no game")
 }
    Suggestion importance[1-10]: 8

    Why: Changing the status code for ErrNoGame from 400 to 404 ensures consistency with standard HTTP status codes, improving the API's clarity and correctness. This is a significant improvement for API design.

    		8
    エラーメッセージに型情報を追加してデバッグを容易にします。


    convertGameVisibility関数で無効な値を処理する際、エラーメッセージに値の型情報を含めることで、デバッグ時の情報をより明確にすることを検討してください。

    src/handler/v2/game_role.go [38]

    -return "", fmt.Errorf("invalid game visibility: %v", value)
+return "", fmt.Errorf("invalid game visibility: %v (type: %T)", value, value)
    Suggestion importance[1-10]: 7

    Why: Adding type information to the error message in convertGameVisibility enhances debugging by providing more context about the invalid value. This is a minor but useful improvement for maintainability and debugging.

    		7
    nilチェックの早期リターンでコードの可読性を改善します。

    PatchGameRole関数内でreq.Typeがnilの場合の処理を明確にするため、早期リターンを使用してコードの可読性を向上させることを検討してください。

    src/handler/v2/game_role.go [45-54]

    -if req.Type != nil {
-    var roleType values.GameManagementRole
-    switch *req.Type {
-    case "owner":
-        roleType = values.GameManagementRoleAdministrator
-    case "maintainer":
-        roleType = values.GameManagementRoleCollaborator
-    default:
-        return echo.NewHTTPError(http.StatusBadRequest, "role type is invalid")
-    }
-    ...
+if req.Type == nil {
+    return nil
 }
+var roleType values.GameManagementRole
+switch *req.Type {
+case "owner":
+    roleType = values.GameManagementRoleAdministrator
+case "maintainer":
+    roleType = values.GameManagementRoleCollaborator
+default:
+    return echo.NewHTTPError(http.StatusBadRequest, "role type is invalid")
+}
+...
    Suggestion importance[1-10]: 6

    Why: Introducing an early return for nil checks improves code readability and reduces nesting. However, the current implementation is already functional and clear, so the impact is moderate.

    		6
    テストケースのエラーメッセージを具体的にしてデバッグを容易にします。


    テストケースでGetGameのvisibilityが無効なので500のシナリオにおいて、無効な値を明示的にログ出力することで、失敗時のデバッグを容易にすることを検討してください。

    src/handler/v2/game_role_test.go [231]

    -GetGameErr: errors.New("error"),
+GetGameErr: errors.New("invalid visibility value encountered"),
    Suggestion importance[1-10]: 5

    Why: Making the error message in the test case more specific aids debugging by clearly indicating the issue. While helpful, this change has a limited impact as it only affects test output.

    		5

    @codecov Codecov
    Copy link

    codecov bot commented Jan 6, 2025

    Codecov Report

    Attention: Patch coverage is 93.44262% with 4 lines in your changes missing coverage. Please review.

    Project coverage is 51.16%. Comparing base (a52a8bc) to head (f22d35f).
    Report is 2 commits behind head on main.

    Files with missing lines Patch % Lines
    src/handler/v2/game.go 60.00% 4 Missing ⚠️
    Additional details and impacted files 
    @@            Coverage Diff             @@
##             main    #1092      +/-   ##
==========================================
+ Coverage   50.01%   51.16%   +1.14%     
==========================================
  Files         122      122              
  Lines       11026    11060      +34     
==========================================
+ Hits         5515     5659     +144     
+ Misses       5174     5061     -113     
- Partials      337      340       +3

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

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 4
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    handler/game_roleのユニットテスト
    1 participant
    @ikura-hamu