feat: handle radom webhook events #2640
Conversation
There was a problem hiding this comment.
few questions to just make sure we're not accidentally introducing issues, but overall I think this looks good.
Noting that privacy policy updates still needed, but I spoke with @mattmcalister and we're going to handle that before enabling the flag
clD11
changed the title
services/skus/controllers.go
Outdated
| if err := svc.processRadomEvent(ctx, event); err != nil { | ||
| l.Err(err).Msg("failed to process radom event") | ||
|
|
||
| return handlers.WrapError(model.ErrSomethingWentWrong, "something went wrong", http.StatusInternalServerError) | ||
| } |
There was a problem hiding this comment.
question (blocking): Do we want to respond to any error with a 500? There could be legitimate reasons for this to not return an error to the 3rd-party. Please take a look at handleWebhookPlayStoreH, handleWebhookAppStoreH, and handleStripeWebhook which handle errors returned from the business logic.
There was a problem hiding this comment.
@clD11 This is an open important question.
There was a problem hiding this comment.
For the mobile handlers i.e. handleWebhookPlayStoreH and handleWebhookAppStoreH things are quite different with regards to the order i.e. in many cases we will not have an order due to not linking however this should never be the case for Radom.
Also looking at the other handlers its not clear how model.ErrNoRowsChangedOrderPayHistory can ever be thrown. Unless I am missing something the current code will either insert or throw a unique constraint violation so model.ErrNoRowsChangedOrderPayHistory will never been returned. Whats puzzling is how this is working for the other handlers with regards to duplicates and I see we are setting the paid at timestamp to the time we process the message rather than the actual paid at time hence it always inserts a new row even for duplicates, this will not be the case for Radom as it uses the actual paid at time which will be consistent.
Lets take this one 'offline' and discuss how we want to handle the events as there are a couple of things we might want to address here.
There was a problem hiding this comment.
Yes, let's discuss it offline.
The main point I've made in/by the comment is that responding with a 500 in all possible errors here is not necessarily good enough. Maybe some granularity, if possible, is good.
| } | ||
| } | ||
|
|
||
| func TestService_processRadomEventTx(t *testing.T) { |
There was a problem hiding this comment.
suggestion: At some point, it would be nice to see more test cases covering some of the important branches of the method. Currently it covers only the happy path, which does not guarantee the expected behaviour under error conditions.
nit: Above and below, the variable for service should hold a pointer as that's what Service usually is, and that's how it's declared in other places. I know that Go will properly handle this when calling the method, however, visually, when reading the code, the inconsistency is noticeable and distracting – there is this urge to fix it.
* fix imports * update gateway to pointer * update radom checkout session request to pointer * rename order items to line items function * add radom product id getter on metadata
* feat: add radom checkout session integration for new orders * test: add no metadata test order item req * style: add new line to slice init in radom gateway * fix: set radom authorization header without bearer prefix * fix: rectify skus model tests after rebase * refactor: rename method createRadomSessID to createRadomSession * refactor: address comments * fix imports * update gateway to pointer * update radom checkout session request to pointer * rename order items to line items function * add radom product id getter on metadata * refactor: remove unused variable emptyCreateCheckoutSessionResp * refactor: remove unnecessary convert to UTC * refactor: sort imports * refactor: remove unused func mustDecimalFromString * feat: handle radom webhook events (#2640) * feat: add radom checkout session integration for new orders * fix: rectify skus model tests after rebase * refactor: address comments * fix imports * update gateway to pointer * update radom checkout session request to pointer * rename order items to line items function * add radom product id getter on metadata * feat: add radom checkout session integration for new orders * feat: handle radom webhook events * test: remove duplicate test and fix pointer after rebase * feat: set radom webhook verification key * refactor: use pointers for radom events * refactor: fix import order skus controller * refactor: simplify radom event processing checks * style: fix parse apostrophe * refactor: use pointer for radom subscription response * refactor: remove call to error when calling err on logging * refactor: use index directly on radom mdata * refactor: make radom action error naming less generic * refactor: use pointer for service in test * refactor: sort imports in radom.go * refactor: update radom empty payments error message * style: return from func call opposed to explicit return nil * refactor: use string in radom get subscription * refactor: add on conflict do nothing to order history insert statement * refactor: rename radom event to radom notification * test: remove duplicate test after rebase
Summary
This PR adds support for radom webhook events. Namely
Things to note:
radom-verification-keywhich is checked when we receive the event from Radom. The key is stored in AWS secret manager.RADOM-ENABLEDresolves #2642
Type of Change
Tested Environments
Before Requesting Review
Manual Test Plan