-
Notifications
You must be signed in to change notification settings - Fork 3
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
Correction webhook eventhandler #815
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #815 +/- ##
==========================================
- Coverage 25.25% 25.22% -0.03%
==========================================
Files 147 147
Lines 30735 30752 +17
==========================================
- Hits 7761 7757 -4
- Misses 22088 22109 +21
Partials 886 886 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とりあえず
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とりあえずこれだけ
service/webhook.go
Outdated
if err != nil { | ||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここlog残したいな(後でissueにする)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
よく分けられてます
/api/requests
以下と/api/transactions
以下でmiddleware.BodyDump
に渡す関数を分けたら嬉しそうだなと思ったんですが、お願いできますか?
/requests
以下はWebhookEventHandler
を経由してWebhookRequestsEventHandler
を呼び出すのではなくWebhookRequestsEventHandler
を直接渡して、
Lines 49 to 67 in 0f91889
apiRequests := api.Group("/requests", h.CheckLoginMiddleware) | |
{ | |
apiRequests.GET("", h.GetRequests) | |
apiRequests.POST("", h.PostRequest, middleware.BodyDump(service.WebhookEventHandler)) | |
apiRequestIDs := apiRequests.Group("/:requestID", retrieveRequestCreator) | |
{ | |
apiRequestIDs.GET("", h.GetRequest) | |
apiRequestIDs.PUT( | |
"", | |
h.PutRequest, | |
middleware.BodyDump(service.WebhookEventHandler), | |
h.CheckRequestCreatorMiddleware) | |
apiRequestIDs.POST( | |
"/comments", | |
h.PostComment, | |
middleware.BodyDump(service.WebhookEventHandler)) | |
apiRequestIDs.PUT("/status", h.PutStatus, h.CheckAdminOrRequestCreatorMiddleware) | |
} | |
} |
/api/transactions
以下も同様にWebhookTransactionsEventHandler
を直接渡す形を想定してます
Lines 69 to 83 in 0f91889
apiTransactions := api.Group("/transactions", h.CheckLoginMiddleware) | |
{ | |
apiTransactions.GET("", h.GetTransactions) | |
apiTransactions.POST( | |
"", | |
h.PostTransaction, | |
middleware.BodyDump(service.WebhookEventHandler), | |
h.CheckAdminMiddleware) | |
apiTransactions.GET("/:transactionID", h.GetTransaction) | |
apiTransactions.PUT( | |
"/:transactionID", | |
h.PutTransaction, | |
middleware.BodyDump(service.WebhookEventHandler), | |
h.CheckAdminMiddleware) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
#769
Transaction
をTransactionNewCreate
とTransactionCorrection
に分離webhook.goも構造体を分け、POSTとPUTで処理を分けた