-
-
Notifications
You must be signed in to change notification settings - Fork 12
Add webhooks for OURA #901
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
Add webhooks for OURA #901
Conversation
8e9edd8 to
3935ecd
Compare
3935ecd to
9a99e16
Compare
|
/deploy dev1 |
|
/deploy dev |
|
toddkazakov updated values.yaml file in dev1 |
|
toddkazakov updated flux policies file in dev1 |
|
toddkazakov deployed platform tk-oura-webhooks-shopify branch to dev1 namespace |
…w subscribing to webhooks from the shopify UI
37d1eac to
27ec01d
Compare
91c0bc2 to
6c322c7
Compare
3997ec3 to
22aae96
Compare
22aae96 to
9995641
Compare
ewollesen
left a comment
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.
A few things small things that I think can be quickly fixed. Nothing majorly blocking, though the use of the platform's error package might take a fair bit of cut and paste, if it's decided that it should be done at all.
In general, while there are tests, they seem to focus on successful completion, i.e. testing that things work, not that things that shouldn't work don't work, or that things that don't work fail in the expected manner. Given the relatively temporary nature of this code, that's not too concerning, but as always, today's hot fix is tomorrow production code. So... Shrug, something to think about maybe.
| ctx := req.Context() | ||
| responder := request.MustNewResponder(res, req) | ||
|
|
||
| if err := req.ParseMultipartForm(multipartMaxMemory); err != nil { |
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.
It would be simpler I think to call to PostFormValue. It will call the appropriate ParseForm method as needed, and return the first matching field.
| OuraRingProductID = "15496765964675" // Todd's test store | ||
| OuraRingDiscountCodeTitle = "Oura Ring Discount Code" | ||
|
|
||
| DiscountCodeLength = 12 |
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.
If these aren't being manually input, and I think they're not... is there any reason not to use the full output of rand.Text()?
The result contains at least 128 bits of randomness, enough to prevent brute force guessing attacks and to make the likelihood of collisions vanishingly small.
I don't have a problem with 12 characters, but better safe than sorry, especially if no one needs to copy the codes, right?
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.
We are sending those in emails. I'd prefer to keep them short. The worst that can happen is to generate a duplicate value the shopify rejects. The reconciliation process should retry this.
| }, | ||
| } | ||
|
|
||
| return f.customerIOClient.SendEvent(ctx, identifiers.ID, sizingKitDelivered) |
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.
I don't see any sort of retry logic here, maybe that'll be covered by the task / job that gets run 1x / day to reconcile?
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.
The job will be run more frequently (every hour or so) and will reprocessed failures.
|
I believe there's a typo in your process diagram. The last event is |
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.
nit: if this file lived in oura/jotformapi/router.go, then it would have a clear name, and not need any special naming when importing, i.e.
import "github.com/tidepool-org/platform/oura/jotformapi"
instead of
import (
jotformAPI "github.com/tidepool-org/platform/oura/jotform/api"
)
By convention, go imports are all lowercase, so seeing jotformAPI can throw people off, and make them wonder if it's a variable (the capital letters hint that maybe it is) or an import.
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.
I'd like to adhere to established structure and import aliasing in the project even if it is not perfect
darinkrauss
left a comment
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.
A few comments for your consideration.
darinkrauss
left a comment
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! I'll let @ewollesen review his comments and the updates to give final approval.
| } | ||
|
|
||
| if len(errResp.Errors) > 0 { | ||
| return errors.Newf("API error (status %d): %s", res.StatusCode, errResp.Errors[0].Message) |
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.
Has there ever been more than one error here? If so, it might be useful to in some way at least log the other errors—they might provide useful hints as to what went wrong.
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.
I've only seen a single error during testing
| ctx = log.NewContextWithLogger(ctx, c.logger) | ||
| url := c.trackClient.ConstructURL("api", "v1", "customers", cid, "events") | ||
|
|
||
| mutators := []request.RequestMutator{ |
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.
This is fine, but I feel like maybe we should include a way to include default mutators with a client, so this doesn't need to be run on every request. Maybe a slice of mutators could be passed to the constructor. Thoughts @darinkrauss?
Either way, work for another day / PR.
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.
FWIW, I'm in favor of moving to slog now that there is standard structured logging library in go. The way we instantiate loggers and pass them in context is far from ideal.
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.
Sounds like a conversation to be had. Totally in favor of moving to slog, but I still want to retain the ability to add values to any log message further down the stack. Right now that is attached to the Logger attached to the Context. Oh, and I really, really don't want to add a Logger parameter to every function call, Context is enough.
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.
If a logger is required for a function to work then that dependency should be made explicit. I've been bitten too many times by nil pointer derefs because a function down the stack requires a logger but one isn't present in the context. Another option is to just use a global logger. We can (an I agree that we should) use the context to add additional context to log messages.
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.
Ah, okay, yea that makes sense.
sequenceDiagram actor User participant JotForm participant Platform participant Customer.io participant Shopify User->>JotForm: Submits survey JotForm->>Platform: Webhook call with submission ID Platform->>JotForm: Pull submission details JotForm-->>Platform: Return submission data Platform->>Platform: Check eligibility criteria Platform->>Customer.io: Check if participant ID exists Customer.io-->>Platform: Return participant status Platform->>Shopify: Generate discount code Shopify-->>Platform: Return discount code Platform->>Customer.io: Send oura_eligibility_survey_completed event Customer.io->>Customer.io: Trigger Sizing Kit Campaign User->>Shopify: Place Sizing Kit Order Shopify->>Platform: Send orders/create notification Platform->>Customer.io: Send oura_sizing_kit_ordered event Shopify->>Platform: Send fulfillments/update or fulfillments/create notification Platform->>Shopify: Generate discount code Shopify-->>Platform: Return discount code Platform->>Shopify: Send oura_sizing_kit_delivered event Customer.io->>Customer.io: Trigger Ring Campaign User->>Shopify: Place Ring Order Shopify->>Platform: Send orders/create notification Platform->>Customer.io: Send oura_ring_ordered event Shopify->>Platform: Send fulfillments/update or fulfillments/create notification Platform->>Shopify: Send oura_ring_delivered event