line item enhanced updates #98
Conversation
CHANGELOG.md
Outdated
| - Since charges and refunds are both types of balance transactions, included an additional join between refunds and balance transactions to bring in refunds at the same level as charges. | ||
| - Fixed `fee_amount` logic to sum together charge and refund amounts. | ||
| - Updated `transaction_type` logic to not only bring in `refund`, but return `charge+refund` if the balance transaction has a charge and a refund associated with it. | ||
| - Updated conditional logic for invoice-only records to bring in only non-zero rather than not null `fee_amounts`, as summing charge and refund amounts together brings in only not null `fee_amount` values. |
There was a problem hiding this comment.
| - Updated conditional logic for invoice-only records to bring in only non-zero rather than not null `fee_amounts`, as summing charge and refund amounts together brings in only not null `fee_amount` values. | |
| - Updated conditional logic for invoice-only records to bring in only non-zero rather than not null `fee_amount`, as summing charge and refund amounts together brings in only not null `fee_amount` values. |
There was a problem hiding this comment.
Is the update about the filter or about us coalescing fee_amount with 0 now for non-header rows?
There was a problem hiding this comment.
Both. Before it was possible for fee_amount to be null because of the left join. Because we are summing, summing a null with a not null would make the value null, which is not what we would want. So I coalesced to zero, meaning it could only be zero or not zero, so the filter had to change too.
There was a problem hiding this comment.
I see, then I would make it clearer that we added a coalesce with 0 for non-header rows before getting into this filter logic
There was a problem hiding this comment.
Good point, cleaned up the CHANGELOG.
CHANGELOG.md
Outdated
| - Updated the logic in `stripe__line_item_enhanced` to properly bring in refund data by adjusting the joins on balance transactions, refunds and charges. | ||
| - Since charges and refunds are both types of balance transactions, included an additional join between refunds and balance transactions to bring in refunds at the same level as charges. | ||
| - Fixed `fee_amount` logic to sum together charge and refund amounts. | ||
| - Updated `transaction_type` logic to not only bring in `refund`, but return `charge+refund` if the balance transaction has a charge and a refund associated with it. |
There was a problem hiding this comment.
| - Updated `transaction_type` logic to not only bring in `refund`, but return `charge+refund` if the balance transaction has a charge and a refund associated with it. | |
| - Updated `transaction_type` logic to not only bring in `charge`, but also return `charge + refund` if the balance transaction has a charge and a refund associated with it. |
Unless we also want to return refund?
There was a problem hiding this comment.
Fixed.
I don't believe a refund can exist in Stripe without a charge so for now won't add refund in (documentation)
There was a problem hiding this comment.
Adding payment intent + refund as a transaction type based on Jamie's research.
| on bt_charge.connected_account_id = connected_account.account_id | ||
| and bt_charge.source_relation = connected_account.source_relation |
There was a problem hiding this comment.
I am wondering if the adding the following would offer more coverage:
| on bt_charge.connected_account_id = connected_account.account_id | |
| and bt_charge.source_relation = connected_account.source_relation | |
| on coalesce(bt_charge.connected_account_id, bt_refund.connected_account_id) = connected_account.account_id | |
| and coalesce(bt_charge.source_relation, bt_refund.source_relation) = connected_account.source_relation |
There was a problem hiding this comment.
Wouldn't they be the same value? bt_charge and bt_refund are both from the balance_transaction source, so the connected_account_id values would be the same.
There was a problem hiding this comment.
Yeah but their join conditions are different -- bt_charge joins with the charge table and bt_refund joins with the refund table. So there could be cases where one is null and the other is not, right? It sounds like a refund can exist without a charge if it is associated with a payment intent instead
There was a problem hiding this comment.
This makes sense, went ahead and added.
| @@ -33,7 +33,7 @@ models: | |||
| - name: product_type | |||
| description: Product type | |||
| - name: transaction_type | |||
| description: Balance transaction type | |||
| description: Balance transaction type. If refunds and charges are tied to one invoice, it's designated as 'refund+charge'. | |||
There was a problem hiding this comment.
| description: Balance transaction type. If refunds and charges are tied to one invoice, it's designated as 'refund+charge'. | |
| description: Balance transaction type. If refunds and charges are tied to one invoice, it's designated as 'charge + refund'. |
Should we include the other type(s) as well?
There was a problem hiding this comment.
Good call. Added!
There was a problem hiding this comment.
@fivetran-jamie Ready for re-review! Let me know your thoughts on the refund/charge situation. I'm fairly certain a refund must have a charge tied to it, but I can also add in the logic if we feel it's best.
|
@fivetran-jamie Addressed your follow up comments, should be ready for re-review now! |
CHANGELOG.md
Outdated
| - Since charges and refunds are both types of balance transactions, included an additional join between refunds and balance transactions to bring in refunds at the same level as charges. | ||
| - Updated balance transactions join on `connected_account_id` and `source_relation` to look at both charge and refund balance transactions. | ||
| - Fixed `fee_amount` logic to sum together charge and refund amounts. | ||
| - Updated conditional logic for invoice-only records to ensure `fee_amount` is not null by coalescing to zero for summing non-header rows. |
There was a problem hiding this comment.
| - Updated conditional logic for invoice-only records to ensure `fee_amount` is not null by coalescing to zero for summing non-header rows. | |
| - Coalesced `fee_amount` with zero for invoice-only (non-header) rows and updated downstream summing logic accordingly. |
There was a problem hiding this comment.
Oh yeah that's much clearer. Thanks @fivetran-jamie !
There was a problem hiding this comment.
@fivetran-avinash a few comments on this PR before release approval
| when bt_refund.balance_transaction_id is not null and bt_charge.balance_transaction_id is not null then 'charge + refund' | ||
| when bt_charge.balance_transaction_id is not null then 'charge' | ||
| when bt_refund.balance_transaction_id is not null then 'payment intent + refund' | ||
| else null |
There was a problem hiding this comment.
Is there a reason we default to null here? Should we default to the base balance_transaction.type field?
There was a problem hiding this comment.
Especially since I see there are other "types" of transaction_types listed in our data and I'm unsure if null is entirely accurate.
There was a problem hiding this comment.
Good catch, updated. I was hesitant to do this since we are technically only joining in on charge and refund, but I think it makes sense that we should bring in whatever transaction type is associated with a line item.
| @@ -123,7 +129,7 @@ with invoice_line_item as ( | |||
| cast(payment_method.payment_method_id as {{ dbt.type_string() }}) as payment_method_id, | |||
| cast(payment_method.type as {{ dbt.type_string() }}) as payment_method, | |||
| cast(charge.created_at as {{ dbt.type_timestamp() }}) as payment_at, | |||
| cast(balance_transaction.fee as {{ dbt.type_numeric() }}) as fee_amount, | |||
| cast(coalesce(bt_charge.fee, 0) as {{ dbt.type_numeric() }}) + cast(coalesce(bt_refund.fee, 0) as {{ dbt.type_numeric() }}) as fee_amount, | |||
There was a problem hiding this comment.
Just want to confirm here. Is there any case where the signs of a charge.fee and a refund.fee would be different (i.e. a charge fee is positive and a refund fee is negative)?
There was a problem hiding this comment.
Checking our internal data, a refund fee is always negative (or 0) in our internal data as it's returning fees that are not longer being incurred. Whereas a charge fee will always be positive.
I think the logic makes sense, since deducting refund fees will give us how much fees are actually being incurred on the line item. But open to changing this if we think there are reasons they should be accumulated differently (or creating a new column, and delineating between charge and refund fees).
There was a problem hiding this comment.
(or creating a new column, and delineating between charge and refund fees).
Unfortunately with this model we can't take the approach to add or remove columns since this is required to be the same schema for all *__line_item_enhanced models in the other platforms (Zuora, Recharge, Recurly, Shopify). If we add a new column here then we will need to add the same column in the other platforms which may not work for their use cases.
Thanks for the explanation here. This sounds appropriate to keep as is.
There was a problem hiding this comment.
@fivetran-joemarkiewicz Sorry I realized I never submitted this. This is ready for re-review.
| @@ -123,7 +129,7 @@ with invoice_line_item as ( | |||
| cast(payment_method.payment_method_id as {{ dbt.type_string() }}) as payment_method_id, | |||
| cast(payment_method.type as {{ dbt.type_string() }}) as payment_method, | |||
| cast(charge.created_at as {{ dbt.type_timestamp() }}) as payment_at, | |||
| cast(balance_transaction.fee as {{ dbt.type_numeric() }}) as fee_amount, | |||
| cast(coalesce(bt_charge.fee, 0) as {{ dbt.type_numeric() }}) + cast(coalesce(bt_refund.fee, 0) as {{ dbt.type_numeric() }}) as fee_amount, | |||
There was a problem hiding this comment.
(or creating a new column, and delineating between charge and refund fees).
Unfortunately with this model we can't take the approach to add or remove columns since this is required to be the same schema for all *__line_item_enhanced models in the other platforms (Zuora, Recharge, Recurly, Shopify). If we add a new column here then we will need to add the same column in the other platforms which may not work for their use cases.
Thanks for the explanation here. This sounds appropriate to keep as is.
PR Overview
This PR will address the following Issue/Feature: [#93]
This PR will result in the following new package version: 0.15.1
No schema changes--we are modifying the logic significantly to correct for bug issues, but this is in a good spot.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
stripe__line_item_enhancedto properly bring in refund data by adjusting the joins on balance transactions, refunds and charges.fee_amountlogic to sum together charge and refund amounts.transaction_typelogic to not only bring inrefund, but returncharge+refundif the balance transaction has a charge and a refund associated with it.fee_amounts, as summing charge and refund amounts together brings in only not nullfee_amountvalues.Under the Hood
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
Was able to pass through refund amounts to the dev models, which were previously not populating because of the join on balance transactions.
All validation tests passed, with exclusions on the
consistency_line_item_enhanced modelon the tables who saw their underlying logic changed (fee_amount, andtransaction_type) or had the join changed to bring in not null data (refund_amount).If you had to summarize this PR in an emoji, which would it be?
⌨️