Skip to content

Conversation

@toddkazakov
Copy link
Contributor

@toddkazakov toddkazakov commented Dec 16, 2025

  • BACK-4162
  • BACK-4164
  • BACK-4165
  • BACK-4041
  • BACK-3959
  • BACK-4166
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
Loading

@toddkazakov toddkazakov force-pushed the tk-oura-webhooks-shopify branch from 8e9edd8 to 3935ecd Compare December 16, 2025 14:31
@toddkazakov toddkazakov force-pushed the tk-oura-webhooks-shopify branch from 3935ecd to 9a99e16 Compare December 16, 2025 14:49
@toddkazakov
Copy link
Contributor Author

/deploy dev1

@toddkazakov
Copy link
Contributor Author

/deploy dev

@tidebot
Copy link
Collaborator

tidebot commented Dec 16, 2025

toddkazakov updated values.yaml file in dev1

@tidebot
Copy link
Collaborator

tidebot commented Dec 16, 2025

toddkazakov updated flux policies file in dev1

@tidebot
Copy link
Collaborator

tidebot commented Dec 16, 2025

toddkazakov deployed platform tk-oura-webhooks-shopify branch to dev1 namespace

@toddkazakov toddkazakov force-pushed the tk-oura-webhooks-shopify branch from 37d1eac to 27ec01d Compare December 17, 2025 15:03
@toddkazakov toddkazakov force-pushed the tk-oura-webhooks-shopify branch 6 times, most recently from 91c0bc2 to 6c322c7 Compare December 18, 2025 20:34
@toddkazakov toddkazakov force-pushed the tk-oura-webhooks-shopify branch 2 times, most recently from 3997ec3 to 22aae96 Compare December 23, 2025 10:21
@toddkazakov toddkazakov force-pushed the tk-oura-webhooks-shopify branch from 22aae96 to 9995641 Compare December 23, 2025 14:44
@toddkazakov toddkazakov requested a review from ewollesen January 7, 2026 13:00
@toddkazakov toddkazakov changed the title Add shopify webhooks for OURA Add webhooks for OURA Jan 8, 2026
Copy link
Contributor

@ewollesen ewollesen left a 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 {
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@ewollesen
Copy link
Contributor

I believe there's a typo in your process diagram.

The last event is Send oura_sizing_kit_delivered event, but I think it should be something like Send oura_ring_kit_delivered event?

Copy link
Contributor

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.

Copy link
Contributor Author

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

ewollesen
ewollesen previously approved these changes Jan 9, 2026
Copy link
Contributor

@darinkrauss darinkrauss left a 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.

@toddkazakov toddkazakov changed the base branch from master to BACK-3960-oura-connection January 13, 2026 17:38
Copy link
Contributor

@darinkrauss darinkrauss left a 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.

@toddkazakov toddkazakov requested a review from ewollesen January 14, 2026 07:36
}

if len(errResp.Errors) > 0 {
return errors.Newf("API error (status %d): %s", res.StatusCode, errResp.Errors[0].Message)
Copy link
Contributor

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.

Copy link
Contributor Author

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{
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@toddkazakov toddkazakov merged commit 818a57d into BACK-3960-oura-connection Jan 14, 2026
3 checks passed
@toddkazakov toddkazakov deleted the tk-oura-webhooks-shopify branch January 14, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants