Feature/new journal cash models - #78
Conversation
- Add all untracked cash model files: stg_xero__journal_cash, stg_xero__journal_cash_line, stg_xero__journal_cash_line_has_tracking_category (tmp + staging layers), int_xero__journal_cash_line_pivoted_tracking_categories, xero__cash_general_ledger, get_*_columns macros, and integration test seeds - Add consistency_cash_general_ledger.sql integration test to match pattern of all other end models - Fix README source table links pointing to feature/new-union-data (now point to main) - Add not_null tests to journal_id and journal_line_id in stg_xero__journal_cash_line_has_tracking_category Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ensures the second dbt run verifies the package executes cleanly when cash-basis journal models are turned off, closing a coverage gap where all three CI passes ran with xero__using_journal_cash: true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fivetran-catfritz
left a comment
There was a problem hiding this comment.
A tiny yml update needed, but also there are a couple more philosophical questions on columns.
| journal_cash: "{{ source('xero', 'journal_cash') }}" | ||
| journal_cash_line: "{{ source('xero', 'journal_cash_line') }}" | ||
| journal_cash_line_has_tracking_category: "{{ source('xero', 'journal_cash_line_has_tracking_category') }}" | ||
| xero_sources: [] |
There was a problem hiding this comment.
| xero_sources: [] |
I think we should remove this since you provided default values but let me know otherwise.
| description: > | ||
| The source where this data was pulled from. If you are making use of the `union_schemas` variable, this will be the source schema. | ||
| If you are making use of the `union_databases` variable, this will be the source database. If you are not unioining together multiple | ||
| If you are making use of the `union_databases` variable, this will be the source database. If you are not unioning together multiple |
There was a problem hiding this comment.
Haha oh dear... I think this made it into several packages with our last round of updates. Good catch tho.
There was a problem hiding this comment.
Haha it happens
| accounts.account_class, | ||
| 'cash' as accounting_basis, | ||
|
|
||
| case when journals.source_type in ('ACCPAY', 'ACCREC') then journals.source_id end as invoice_id, |
There was a problem hiding this comment.
Question on invoice_id (Claude flagged):
According to Claude, ACCPAY and ACCREC are accrual-basis source types that shouldn't appear in a cash journal, so this will never fire. In the cash context, payment_id already plays the equivalent role — it captures AR/AP transactions at the payment stage rather than invoice creation. So there isn't really a cash-basis equivalent for invoice_id. Should we remove it in favor of keeping only what's conceptually valid for cash, or keep it for structural consistency with xero__general_ledger? Maybe discuss with the team?
There was a problem hiding this comment.
suggested changes of docs updates applied
|
|
||
| select | ||
| first_contact.*, | ||
| contacts.contact_name |
There was a problem hiding this comment.
Another Claude-flagged question:
According to Claude, contact_id and contact_name will be NULL for payment-type source types in xero__cash_general_ledger (since ACCRECPAYMENT, ACCPAYPAYMENT, and the other payment types that map to payment_id). This is because contact resolution only works through the invoice, bank transaction, and credit note joins — and there's no payment staging model in the package to fill the gap.
This same limitation exists in xero__general_ledger, but it's less noticeable there since accrual journals also include the original invoice entries (ACCREC/ACCPAY) which do resolve contacts. In the cash ledger, payments are the only entries, so a larger share of rows will have NULL contact info.
Not suggesting we fix it in this PR — adding a payment model is a bigger lift. But it might be worth updating the contact_id field description in xero.yml to set expectations:
- name: contact_id
description: >
The identifier for the contact associated with the source transaction, where resolvable.
Will be null for payment source types (e.g. ACCRECPAYMENT, ACCPAYPAYMENT) as the package
does not currently include a payment staging model.
Or, we just don't bother with the contact_name.
Maybe another question for the team.
There was a problem hiding this comment.
suggested changes of docs updates applied
| journal_lines.tax_name, | ||
| journal_lines.tax_type, | ||
| accounts.account_class, | ||
| 'cash' as accounting_basis, |
There was a problem hiding this comment.
Perhaps add a similar column 'accrual' as accounting_basis to xero__general_ledger?
There was a problem hiding this comment.
Good call! updated.
…fivetran/dbt_xero into feature/new-journal-cash-models
Ready for re-review once docs get regenerated /Buildkite passes |
…fivetran/dbt_xero into feature/new-journal-cash-models
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog