-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: master
Are you sure you want to change the base?
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
6c322c7 to
a3aa538
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.
| func (c *Client) FindCustomers(ctx context.Context, filter map[string]any) (*FindCustomersResponse, error) { | ||
| url := fmt.Sprintf("%s/v1/customers", c.appAPIBaseURL) | ||
|
|
||
| body, _ := json.Marshal(filter) |
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.
Please check for and handle errors. It's very unlikely here, but better than panicking.
| body, _ := json.Marshal(filter) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewBuffer(body)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) |
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 part of platform, so errors.Wrap is probably a better choice.
This occurs several times throughout, so I'll leave it to you to find the rest.
| if err := json.NewDecoder(resp.Body).Decode(&errResp); err == nil && len(errResp.Errors) > 0 { | ||
| return fmt.Errorf("API error (status %d): %s", resp.StatusCode, errResp.Errors[0].Message) | ||
| } | ||
| return fmt.Errorf("unexpected status code: %d", resp.StatusCode) |
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 often find it more useful to return resp.Status, which includes both the code, and the basic name of the status code as a string. It's sometimes more helpful in the logs.
|
|
||
| type segmentMembershipResponse struct { | ||
| Identifiers []Identifiers `json:"identifiers"` | ||
| IDs []string `json:"ids"` |
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.
Why IDs and Identifiers? Isn't IDs an abbreviation of Identifiers?
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 the response that we get from customer io - we have no control of it.
| 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.
| return nil | ||
| } | ||
|
|
||
| func (w *WebhookProcessor) ensureConsentRecordExists(ctx context.Context, userID string, submission *SubmissionResponse) error { |
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.
Question just to confirm:
The fact that we've received a jotform submission means that they've consented, right?
I seem to remember that was the case, but wanted to be sure.
In theory we're not emailing anyone < 18 years of age, but in the event that we did, do we want to perform any double checks here?
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 the submission is marked as eligible, then we must upsert a consent record. This is the only value that we check. If we don't want to execute this logic, jotform ought to mark the submission is ineligible.
| } | ||
| case OuraRingProductID: | ||
| if err := f.onRingDelivered(ctx, customers.Identifiers[0], event, deliveredProducts.DiscountCode); err != nil { | ||
| logger.WithError(err).Warn("unable to send ring delivered event") |
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.
Typically it's preferable to either log the error, or return it, but not both. When you log then return the error, it tends to get logged multiple times which clutters the log output. Consider using errors.Wrap instead of logging.
| return err | ||
| } | ||
| default: | ||
| logger.Warn("ignoring fulfillment event for unknown product") |
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.
What effect will returning nil here have later down the line?
I don't see a test case covering this. Seems like it would be easier to debug this if an error were returned. The error would get logged and the user would likely get some useful message as well.
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's a fulfilment that we can't process, so returning a nil will prevent shopify from resending the event. Returning an error will make shopify retry the webhook delivery.
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_sizing_kit_delivered event