Fix/issue439 userAPI #479

Nzt3-gh · 2023-10-26T11:30:08Z

infra/traq/user.go のAPIを呼んでいる部分をgo traqを用いたコードに書き換えました。

ras0q · 2023-10-26T11:32:09Z

これを実行するとコードがフォーマットされるのでまずはこれを実行してほしいです:pray:
https://github.com/traPtitech/knoQ?tab=readme-ov-file#%E3%82%B3%E3%83%BC%E3%83%89%E3%83%95%E3%82%A9%E3%83%BC%E3%83%9E%E3%83%83%E3%83%88

ras0q

ありがとうございます！いくつかレビューしました！

ras0q · 2023-10-26T13:22:38Z

infra/traq/user.go

+	traqconf := traq.NewConfiguration()
+	conf := TraQDefaultConfig
+	traqconf.HTTPClient = conf.Client(context.Background(), token)
+	apiClient := traq.NewAPIClient(traqconf)


ここは多分どのAPIも共通だと思うのでメソッドにしてよさそうです

ras0q · 2023-10-26T13:23:08Z

infra/traq/user.go

+	conf := TraQDefaultConfig
+	traqconf.HTTPClient = conf.Client(context.Background(), token)
+	apiClient := traq.NewAPIClient(traqconf)
+	userDtail, _, err := apiClient.UserApi.GetUser(context.Background(), userID.String()).Execute()


ここのcontextはAPIクライアントを作るときに使ったものと同じものを使ってほしいです！

ras0q · 2023-10-26T13:25:15Z

utils/api.go


-	res, err := http.DefaultClient.Do(req)
+	res, err := apiClient.WebhookApi.PostWebhook(context.Background(), webhookID).XTRAQChannelId(channelID).XTRAQSignature(xTRAQSignature).Embed(int32(embed)).Body(message).Execute()


あまり行が長くなりそうなら分割すると見やすそうです

ras0q · 2023-10-26T13:26:14Z

infra/traq/user.go

+	conf := TraQDefaultConfig
+	traqconf.HTTPClient = conf.Client(context.Background(), token)
+	apiClient := traq.NewAPIClient(traqconf)
+	userDtail, _, err := apiClient.UserApi.GetUser(context.Background(), userID.String()).Execute()


Suggested change

userDtail, _, err := apiClient.UserApi.GetUser(context.Background(), userID.String()).Execute()

userDetail, _, err := apiClient.UserApi.GetUser(context.Background(), userID.String()).Execute()

ras0q · 2023-10-26T13:28:14Z

infra/traq/user.go

+func convertMyUserdetailToUser(userdetail *traq.MyUserDetail) *traq.User {
 	user := new(traq.User)
-	err = json.Unmarshal(data, &user)
-	return user, err
+	user.Id = userdetail.Id
+	user.Name = userdetail.Name
+	user.DisplayName = userdetail.DisplayName
+	user.IconFileId = userdetail.IconFileId
+	user.Bot = userdetail.Bot
+	user.State = userdetail.State
+	user.UpdatedAt = userdetail.UpdatedAt
+	return user
+}
+
+func convertUserdetailToUser(userdetail *traq.UserDetail) *traq.User {
+	user := new(traq.User)
+	user.Id = userdetail.Id
+	user.Name = userdetail.Name
+	user.DisplayName = userdetail.DisplayName
+	user.IconFileId = userdetail.IconFileId
+	user.Bot = userdetail.Bot
+	user.State = userdetail.State
+	user.UpdatedAt = userdetail.UpdatedAt
+	return user
+}
+func convertUsersToUsers(users []traq.User) []*traq.User {
+	new_users := make([]*traq.User, len(users))
+	for i, _user := range users {
+		user := _user
+		new_users[i] = &user
+	}
+	return new_users
 }


これくらいならconvert関数かかなくてもよさそうな気がします
GetUsersの返り値も[]traq.Userにしちゃってよさそう

ras0q

いくつかかきました！

ras0q · 2023-10-27T05:39:17Z

infra/traq/user.go


 	"github.com/gofrs/uuid"
 	"github.com/traPtitech/go-traq"
 	"golang.org/x/oauth2"
 )

 func (repo *TraQRepository) GetUser(token *oauth2.Token, userID uuid.UUID) (*traq.User, error) {
-	URL := fmt.Sprintf("%s/users/%s", repo.URL, userID)
-	req, err := http.NewRequest(http.MethodGet, URL, nil)
+	apiClient := MakeApiClient(token)


ここの引数でcontextを含めるとapiClientとリクエストで単一のcontextを使えるのでそうして欲しいです🙏

ras0q · 2023-11-10T15:20:36Z

infra/traq/traq.go

@@ -77,3 +78,11 @@ func (repo *TraQRepository) doRequest(token *oauth2.Token, req *http.Request) ([
 	}
 	return io.ReadAll(resp.Body)
 }
+
+func MakeApiClient(token *oauth2.Token, ctx context.Context) *traq.APIClient {


contextは第1引数に取るのが慣例なのでそうして欲しいです🙏

ras0q · 2023-11-10T15:31:03Z

infra/traq/user.go

-	req, err := http.NewRequest(http.MethodGet, URL, nil)
+	ctx := context.TODO()
+	apiClient := MakeApiClient(token, ctx)
+	userDetail, _, err := apiClient.UserApi.GetUser(ctx, userID.String()).Execute()


レスポンスのStatus Codeもチェックするといいと思います

ras0q · 2023-11-10T15:33:02Z

infra/traq/user.go

 	user := new(traq.User)
-	err = json.Unmarshal(data, &user)
+	user.Id = userDetail.Id
+	user.Name = userDetail.Name
+	user.DisplayName = userDetail.DisplayName
+	user.IconFileId = userDetail.IconFileId
+	user.Bot = userDetail.Bot
+	user.State = userDetail.State
+	user.UpdatedAt = userDetail.UpdatedAt
 	return user, err


個人的には

user := traq.User { Id: userDetail.Id, // ... }

で一気に書く方が見やすいと思います

ras0q · 2023-11-10T15:34:34Z

utils/api.go


-	res, err := http.DefaultClient.Do(req)
+	res, err := apiClient.WebhookApi.PostWebhook(context.Background(), webhookID).


動作変わらないのでどっちでもいいんですが他のところでcontext.TODO使ってくれてるので合わせると良さそう

ras0q · 2023-11-10T15:35:15Z

utils/api.go

-	if channelID != "" {
-		req.Header.Set("X-TRAQ-Channel-Id", channelID)
-	}
+	xTRAQSignature := calcSignature(message, secret)


定義から使用までが少し離れているのでPostWebhookの行の手前とかに定義すると読みやすいと思います

ras0q · 2023-11-10T15:43:33Z

infra/traq/traq.go

@@ -77,3 +78,11 @@ func (repo *TraQRepository) doRequest(token *oauth2.Token, req *http.Request) ([
 	}
 	return io.ReadAll(resp.Body)
 }
+
+func MakeApiClient(token *oauth2.Token, ctx context.Context) *traq.APIClient {


ApiよりもAPIの方が好まれるので直してもらいたいです🙏
ref: https://google.github.io/styleguide/go/decisions#initialisms

あとポインタを返すならMakeよりNewっぽい気がする(割とこの関数を頻繁につかうのでNewも100%しっくり来てるわけじゃないけどGetとかRetrieveもうーんって感じなのでNewでいいです)

Nzt3-gh · 2023-12-20T13:27:37Z

修正したので確認をお願いします。

ras0q

動作確認しました！よさそうです！

Nzt3-gh added 7 commits September 24, 2023 17:49

use go-traq

df321ca

disable previous code

2566f5f

use go-traq GetUser (WIP)

9a6859d

revert

5c3a168

use go-traq GetUserMe add converter MyUserdetail -> User

86d2298

use go-traq userAPI

8c0016b

disable debag code

a071658

Nzt3-gh linked an issue Oct 26, 2023 that may be closed by this pull request

traQへのリクエストにgo-traqを使う #439

Closed

Nzt3-gh removed a link to an issue Oct 26, 2023

traQへのリクエストにgo-traqを使う #439

Closed

Nzt3-gh added 2 commits October 26, 2023 21:14

Delete unused code

82393b7

delete unused variable

862b47a

ras0q requested changes Oct 26, 2023

View reviewed changes

Nzt3-gh added 7 commits October 26, 2023 23:01

fix format

5c7fa49

make func MakeApiClient with token

b2613cd

expand converter func UserDetail,MyUserDetail->User

2f0d599

add context to func MakeApiClient

7608d82

change return value of GetUsers

d352c8a

delete unused converter

43166d6

set context.TODO

3956f65

ras0q requested changes Nov 10, 2023

View reviewed changes

Nzt3-gh added 4 commits December 19, 2023 21:02

fix

866f601

fix AIPClient context

bee1a24

fix context

eab8062

change comverter

965039f

Nzt3-gh marked this pull request as ready for review December 20, 2023 13:39

ras0q approved these changes Dec 21, 2023

View reviewed changes

Nzt3-gh merged commit 8715875 into main Dec 25, 2023
4 checks passed

Nzt3-gh deleted the fix/issue439 branch December 25, 2023 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/issue439 userAPI #479

Fix/issue439 userAPI #479

Nzt3-gh commented Oct 26, 2023

ras0q commented Oct 26, 2023

ras0q left a comment

ras0q Oct 26, 2023

ras0q Oct 26, 2023

ras0q Oct 26, 2023

ras0q Oct 26, 2023

ras0q Oct 26, 2023

ras0q left a comment

ras0q Oct 27, 2023

ras0q Nov 10, 2023

ras0q Nov 10, 2023

ras0q Nov 10, 2023

ras0q Nov 10, 2023

ras0q Nov 10, 2023

ras0q Nov 10, 2023

Nzt3-gh commented Dec 20, 2023

ras0q left a comment


		res, err := http.DefaultClient.Do(req)
		res, err := apiClient.WebhookApi.PostWebhook(context.Background(), webhookID).XTRAQChannelId(channelID).XTRAQSignature(xTRAQSignature).Embed(int32(embed)).Body(message).Execute()

	userDtail, _, err := apiClient.UserApi.GetUser(context.Background(), userID.String()).Execute()
	userDetail, _, err := apiClient.UserApi.GetUser(context.Background(), userID.String()).Execute()

Fix/issue439 userAPI #479

Fix/issue439 userAPI #479

Conversation

Nzt3-gh commented Oct 26, 2023

ras0q commented Oct 26, 2023

ras0q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nzt3-gh commented Dec 20, 2023

ras0q left a comment

Choose a reason for hiding this comment