Bug/last touch attribution #53

fivetran-catfritz · 2025-08-25T22:05:01Z

PR Overview

Package version introduced in this PR:

v1.1.0-a1
v1.1.0-a2
v1.1.0

This PR addresses the following Issue/Feature(s):

GA-1001660

Summary of changes:

Fixes attribution for order lifecycle events. Fulfilled Order and similar order-related events now inherit the touch from their corresponding Placed Order, rather than the nearest intervening event.

Submission Checklist

Alignment meeting with the reviewer (if needed)
- Timeline and validation requirements discussed
Provide validation details:
- Validation Steps: Check for unintentional effects (e.g., add/run consistency & integrity tests)
- Testing Instructions: Confirm the change addresses the issue(s)
- Focus Areas: Complex logic or queries that need extra attention
Merge any relevant open PRs into this PR

Changelog

Draft changelog for PR
Final changelog for release review

fivetran-catfritz · 2025-08-25T22:09:07Z

macros/staging/remove_string_from_numeric.sql

 {% macro default__remove_string_from_numeric(column_name) %}

-    cast(regexp_replace(cast({{ column_name }} as {{ dbt.type_string() }}), '[^0-9.]*', '') as {{ dbt.type_numeric() }})
+    cast(nullif(regexp_replace(cast({{ column_name }} as {{ dbt.type_string() }}), '[^0-9.]*', ''), '') as {{ dbt.type_numeric() }})


In our internal data, one example was malformed, so this prevents a run failure.

fivetran-catfritz · 2025-08-25T22:10:50Z

models/intermediate/int_klaviyo__event_attribution.sql


-final as (
-
+order_seq as (


The general approach is to group the events based on the prior placed order event, than use the group to assign the touch_id.

fivetran-catfritz · 2025-08-25T22:23:13Z

models/intermediate/int_klaviyo__event_attribution.sql

+        order_seq.*,
+        case
+            when 
+                lower(order_seq.type) in ('fulfilled order', 'fulfilled partial order', 'delivered shipment', 'marked out for delivery', 'confirmed shipment', 'refunded order') 


I will ask the customer for feedback on what event types should be tied to a placed order, but these seemed reasonable to me for now.

@fivetran-catfritz These make sense to me, but are they standard event types from Klaviyo? Or is there a list you referenced to find these?

They are from this list. I added a comment like you did elsewhere so we don't lose it!

fivetran-jamie

Looks great, couple of questions

fivetran-jamie · 2025-08-26T16:05:31Z

models/intermediate/int_klaviyo__event_attribution.sql

+        order_seq.*,
+        case
+            when 
+                lower(order_seq.type) in ('fulfilled order', 'fulfilled partial order', 'delivered shipment', 'marked out for delivery', 'confirmed shipment', 'refunded order') 


@fivetran-catfritz These make sense to me, but are they standard event types from Klaviyo? Or is there a list you referenced to find these?

fivetran-jamie · 2025-08-26T16:11:08Z

CHANGELOG.md

+[PR #53](https://github.com/fivetran/dbt_klaviyo/pull/53) includes the following updates:
+
+## Bug Fixes
+- Updated attribution for order lifecycle events. `Fulfilled Order` and similar order-related events now inherit the touch from their corresponding `Placed Order`, rather than the nearest intervening event.


I think it'd be good to include all affected event types

Good call. Added.

fivetran-jamie · 2025-08-26T17:33:57Z

models/intermediate/int_klaviyo__event_attribution.sql

+            else order_seq.last_touch_id
+        end as adjusted_last_touch_id
+    from order_seq
+    left join placed_index


I wonder if we should place some sort of time limit, like the 'placed order' event needs to have happened within the previous X days/weeks/months. Just in case 'fulfilled order' or any of these other event types were logged without an order placement for some reason (not sure how possible/common this is though).

@fivetran-catfritz thoughts?

That makes sense. I added line 135 and set it at 3 months for now. I tested it again and no change as expected in the case I was looking at. What do you think?

fivetran-catfritz

Thanks for the comments @fivetran-jamie. Made some updates!

fivetran-catfritz · 2025-08-26T17:59:28Z

models/intermediate/int_klaviyo__event_attribution.sql

+        order_seq.*,
+        case
+            when 
+                lower(order_seq.type) in ('fulfilled order', 'fulfilled partial order', 'delivered shipment', 'marked out for delivery', 'confirmed shipment', 'refunded order') 


They are from this list. I added a comment like you did elsewhere so we don't lose it!

fivetran-catfritz · 2025-08-26T18:11:44Z

CHANGELOG.md

+[PR #53](https://github.com/fivetran/dbt_klaviyo/pull/53) includes the following updates:
+
+## Bug Fixes
+- Updated attribution for order lifecycle events. `Fulfilled Order` and similar order-related events now inherit the touch from their corresponding `Placed Order`, rather than the nearest intervening event.


Good call. Added.

fivetran-catfritz · 2025-08-26T18:13:41Z

models/intermediate/int_klaviyo__event_attribution.sql

+            else order_seq.last_touch_id
+        end as adjusted_last_touch_id
+    from order_seq
+    left join placed_index


That makes sense. I added line 135 and set it at 3 months for now. I tested it again and no change as expected in the case I was looking at. What do you think?

fivetran-jamie

Two suggestions that shouldn't block approval!

fivetran-jamie · 2025-08-26T19:15:29Z

models/intermediate/int_klaviyo__event_attribution.sql

+                -- list of possible Shopify order events --> https://help.klaviyo.com/hc/en-us/articles/115005080447
+                lower(order_seq.type) in ('fulfilled order', 'fulfilled partial order', 'delivered shipment', 'marked out for delivery', 'confirmed shipment', 'cancelled order', 'refunded order') 
+                and order_seq.placed_order_group > 0
+                and {{ dbt.datediff('placed_order_index.placed_order_occurred_at', 'order_seq.occurred_at', 'month') }} <= 3


I am 99.9% sure that if placed_order_index.placed_order_occurred_at is null this will work OK and the clause will be treated basically as false, but perhaps we wanna add a coalesce like this to be safe?

coalesce({{ dbt.datediff('placed_order_index.placed_order_occurred_at', 'order_seq.occurred_at', 'month') }}, 100) <= 3

I'll leave that up to you 🤓

Ahh true--better to be safe. Added!

fivetran-jamie · 2025-08-26T19:16:23Z

CHANGELOG.md

+[PR #53](https://github.com/fivetran/dbt_klaviyo/pull/53) includes the following updates:
+
+## Bug Fixes
+- Updated attribution for Shopify order lifecycle events. The following event types now inherit the touch from their corresponding `Placed Order` event, rather than the nearest intervening event.


Last changelog suggestion: Include the 3-month timeframe guardrail you just added

fivetran-jamie

Looks great!

CHANGELOG.md

models/intermediate/int_klaviyo__event_attribution.sql

fivetran-jamie

This is looking really good. Couple of minor code suggestions and some documentation comments/questions

CHANGELOG.md

fivetran-jamie · 2025-09-15T21:11:38Z

CHANGELOG.md

+
+| Data Model(s) | Change type | Old | New | Notes |
+| ---------- | ----------- | -------- | -------- | ----- |
+| `klaviyo__events`<br>`int_klaviyo__event_attribution`<br>`stg_klaviyo__event` | New column | | `event_attribution` | New field sourced from `property_attribution` in the `EVENT` source table. Contains Klaviyo's native attribution data for events. |


I think it'd be good to briefly highlight here that we're now using this field by default for attribution (that's the big breaking change)

CHANGELOG.md

fivetran-jamie · 2025-09-15T21:18:32Z

DECISIONLOG.md

+- Events with `property_attribution` are used directly
+- Events without it inherit from parent events via `attributed_event_id`


This is a lil unclear to me

I see that. reworded!

fivetran-jamie · 2025-09-15T21:19:43Z

DECISIONLOG.md

+| `klaviyo__email_attribution_lookback` | `120` | Lookback window (hours) for email attribution |
+| `klaviyo__sms_attribution_lookback` | `24` | Lookback window (hours) for SMS attribution |
+| `klaviyo__eligible_attribution_events` | `['email open', 'email click', 'sms open']` | Event types eligible for attribution |


Might wanna note here that these ones are only applicable when using_native_attribution is False

models/intermediate/int_klaviyo__event_attribution.sql

fivetran-jamie · 2025-09-16T14:07:29Z

models/intermediate/int_klaviyo__event_attribution.sql

+    {% if using_native_attribution %}
+    left join inherited
+        on events.unique_event_id = inherited.unique_event_id
+    {% else %}
+    left join session_calculated
+        on events.unique_event_id = session_calculated.unique_event_id
+    {% endif %}


Gotta join on source_relation

DECISIONLOG.md

fivetran-jamie · 2025-09-16T15:00:12Z

CHANGELOG.md

+  - For background on why this method was chosen, see the [DECISIONLOG.md](https://github.com/fivetran/dbt_klaviyo/blob/main/DECISIONLOG.md).
+
+## Breaking Changes
+- **Native attribution is now enabled by default** (`using_native_attribution: true`)


This isn't 100% accurate, right? The default is really determined by whether you have the property_attribution field (if you do, then yah using_native_attribution is basically true)

yes. removed.

README.md

Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-jamie

Looks great! Couple of documentation comments that shouldn't block this moving forward. Approved as long as those are addressed

fivetran-jamie · 2025-09-16T21:28:05Z

DECISIONLOG.md

+- Events with `property_attribution` are used directly
+- Events without it use the following session-based attribution


This kinda sounds like we're talking about singular event records instead of the EVENT table overall

CHANGELOG.md

DECISIONLOG.md

Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-joemarkiewicz

LGTM with just a few small notes that don't require blocking approval. Once the update is applied then this will be ready to go!

DECISIONLOG.md

fivetran-joemarkiewicz · 2025-09-17T15:49:28Z

macros/staging/get_event_columns.sql

    {"name": "uuid", "datatype": dbt.type_string()},
-    {"name": "property_value", "datatype": dbt.type_string()}
+    {"name": "property_value", "datatype": dbt.type_string()},
+    {"name": "property_attribution", "datatype": dbt.type_string()}


This needs to be called out as a breaking change since a user could have been passing this field through as a passthrough column.

Co-authored-by: Joe Markiewicz <[email protected]>

…ran/dbt_klaviyo into bug/last-touch-attribution merge

fivetran-catfritz added 3 commits August 25, 2025 16:57

bug/last-touch-attribution

28cf0b9

add tests

40f2653

bump version

6afd41b

fivetran-catfritz self-assigned this Aug 25, 2025

fivetran-catfritz commented Aug 25, 2025

View reviewed changes

formatting

b9374ae

fivetran-jamie reviewed Aug 26, 2025

View reviewed changes

updates from review

de005bc

fivetran-catfritz commented Aug 26, 2025

View reviewed changes

fivetran-jamie approved these changes Aug 26, 2025

View reviewed changes

round 2 suggestions

3475e10

fivetran-catfritz added the pre-release Triggers the auto-releaser workflow. label Aug 26, 2025

make breaking change

a6d9f85

fivetran-catfritz added pre-release Triggers the auto-releaser workflow. and removed pre-release Triggers the auto-releaser workflow. labels Aug 26, 2025

fivetran-catfritz added 6 commits September 2, 2025 10:48

add event_attribution

4c638c9

seed fixes

aef4ad5

fix yml

10649bc

update fallback to false

ec91c73

changelog

892444e

fix bigquery

906dc9a

fivetran-catfritz added pre-release Triggers the auto-releaser workflow. and removed pre-release Triggers the auto-releaser workflow. labels Sep 2, 2025

fivetran-catfritz added 2 commits September 2, 2025 15:40

formatting

47eb568

fix version

b16ffc0

fivetran-catfritz added pre-release Triggers the auto-releaser workflow. and removed pre-release Triggers the auto-releaser workflow. labels Sep 2, 2025

fivetran-jamie reviewed Sep 2, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

models/intermediate/int_klaviyo__event_attribution.sql Show resolved Hide resolved

fivetran-jamie reviewed Sep 16, 2025

View reviewed changes

fivetran-catfritz and others added 9 commits September 16, 2025 10:22

Apply suggestions from code review

4e3449f

Co-authored-by: Jamie Rodriguez <[email protected]>

fix union

a1ab7ee

fix join

5d70634

update ref for get_cols

0118515

add depends

b56fa65

fix source relation again

6cf73b1

fix comma

7346e05

review updates

ea2713e

readme

a853071

fivetran-catfritz added docs:ready Triggers the docs generator workflow. and removed docs:ready Triggers the docs generator workflow. labels Sep 16, 2025

github-actions bot and others added 2 commits September 16, 2025 18:34

Generate dbt docs via GitHub Actions

b15d2b4

update comment

fd8de41

fivetran-jamie approved these changes Sep 17, 2025

View reviewed changes

fivetran-catfritz commented Sep 17, 2025

View reviewed changes

DECISIONLOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

718bfd7

Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-catfritz added docs:ready Triggers the docs generator workflow. and removed docs:ready Triggers the docs generator workflow. labels Sep 17, 2025

github-actions bot and others added 2 commits September 17, 2025 15:41

Generate dbt docs via GitHub Actions

0a62a8b

Update models/intermediate/int_klaviyo__event_attribution.sql

57111ef

fivetran-catfritz added docs:ready Triggers the docs generator workflow. and removed docs:ready Triggers the docs generator workflow. labels Sep 17, 2025

Generate dbt docs via GitHub Actions

e2acd53

fivetran-joemarkiewicz approved these changes Sep 17, 2025

View reviewed changes

fivetran-catfritz and others added 4 commits September 17, 2025 11:23

Apply suggestions from code review

a29e52c

Co-authored-by: Joe Markiewicz <[email protected]>

changelog

3c16f65

Merge branch 'bug/last-touch-attribution' of https://github.com/fivet…

77b09e0

…ran/dbt_klaviyo into bug/last-touch-attribution merge

changelog

2fa4985

fivetran-catfritz merged commit e0d58be into main Sep 17, 2025
9 checks passed

		- Events with `property_attribution` are used directly
		- Events without it inherit from parent events via `attributed_event_id`

		- Events with `property_attribution` are used directly
		- Events without it use the following session-based attribution


		final as (

		order_seq as (

Bug/last touch attribution #53

Bug/last touch attribution #53

Uh oh!

Conversation

fivetran-catfritz commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Overview

Submission Checklist

Changelog

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivetran-jamie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivetran-jamie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fivetran-jamie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fivetran-jamie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

fivetran-catfritz commented Aug 25, 2025 •

edited

Loading