Add `created_at` and `updated_at` fields #146

fivetran-avinash · 2024-11-18T16:27:10Z

PR Overview

This PR will address the following Issue/Feature: [#144]

This PR will result in the following new package version: v0.17.0

Although this should not impact the existing fields, the general ledger model now is different, so I decided to bump it up. I'm open to moving it back down to 0.16.1 if we don't feel the change is significant enough.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Feature Updates

We have introduced the following fields in the quickbooks__general_ledger.
- created_at: The time a transaction was first created.
- updated_at: The time a transaction was last updated.
- This required updates to all our double entry transaction models to bring in these specific fields for each transaction type.
This isn't necessarily a breaking change, but as this is a new version of this model that introduces new fields, we determined it was best to bump the version up.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
[NA] dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Confirmed that created_at and updated_at are brought into the general ledger model.

Validation tests passed with integration tests seed data.

If you had to summarize this PR in an emoji, which would it be?

⌛

fivetran-joemarkiewicz

@fivetran-avinash this is looking great! I just have a few small requests and one small question before approving.

fivetran-joemarkiewicz · 2024-11-20T22:44:04Z

CHANGELOG.md

+  - `created_at`: The time a transaction was first created.
+  - `updated_at`: The time a transaction was last updated.
+  - This required updates to all our double entry transaction models to bring in these specific fields for each transaction type. 
+- This isn't necessarily a breaking change, but as this introduces new fields to an end model, we determined it was best to bump the version up.


This is an unnecessary bullet in the CHANGELOG. Let's remove it as this really doesn't pertain to the customer.

Suggested change

- This isn't necessarily a breaking change, but as this introduces new fields to an end model, we determined it was best to bump the version up.

README.md

packages.yml

fivetran-joemarkiewicz · 2024-11-20T22:54:18Z

integration_tests/dbt_project.yml

@@ -1,6 +1,6 @@
 name: 'quickbooks_integration_tests'

-version: '0.16.0'
+version: '0.17.0'


Not related to this change, but why do we specify the materialization of so many specific types of models in the models section? I worry we could be masking errors by changing the materialization of the default package.

This must be an artifact from testing on seed data. Removing from the actual model.

packages.yml

fivetran-avinash

@fivetran-joemarkiewicz This is ready for re-review.

fivetran-joemarkiewicz

LGTM

fivetran-catfritz

@fivetran-avinash a couple comments/questions!

fivetran-catfritz · 2024-11-22T22:35:55Z

CHANGELOG.md

+[PR #146](https://github.com/fivetran/dbt_quickbooks/pull/146) introduces the following updates:
+
+## Breaking Changes
+- We have introduced the following fields in the `quickbooks__general_ledger` to better analyze real-time transaction data:


Suggested change

- We have introduced the following fields in the `quickbooks__general_ledger` to better analyze real-time transaction data:

- Introduced the following fields in the `quickbooks__general_ledger` model to better analyze real-time transaction data:

fivetran-catfritz · 2024-11-22T22:36:35Z

CHANGELOG.md

+  - `created_at`: The time a transaction was first created.
+  - `updated_at`: The time a transaction was last updated.
+  - This required updates to all our double entry transaction models to bring in these specific fields for each transaction type.
+- This is a breaking change as this adds new fields to the existing schema for one of our end models. 


Suggested change

- This is a breaking change as this adds new fields to the existing schema for one of our end models.

- This is a breaking change as this adds new fields to the existing schema.

fivetran-catfritz · 2024-11-22T22:37:59Z

CHANGELOG.md

+- We have introduced the following fields in the `quickbooks__general_ledger` to better analyze real-time transaction data:
+  - `created_at`: The time a transaction was first created.
+  - `updated_at`: The time a transaction was last updated.
+  - This required updates to all our double entry transaction models to bring in these specific fields for each transaction type.


Suggested change

- This required updates to all our double entry transaction models to bring in these specific fields for each transaction type.

- Updated the `*_double_entry` models to add these fields for each transaction type.

fivetran-catfritz · 2024-11-22T22:41:38Z

models/intermediate/int_quickbooks__retained_earnings.sql

+revenue_starter as (

    select
        period_first_day,
        source_relation,
-        sum(case when account_class = 'Revenue' then period_net_change else 0 end) as revenue_net_change,
-        sum(case when account_class = 'Revenue' then period_net_converted_change else 0 end) as revenue_net_converted_change,
-        sum(case when account_class = 'Expense' then period_net_change else 0 end) as expense_net_change,
-        sum(case when account_class = 'Expense' then period_net_converted_change else 0 end) as expense_net_converted_change
+        sum(period_net_change) as revenue_net_change,
+        sum(period_net_converted_change) as revenue_net_converted_change
    from general_ledger_balances
+
+    where account_class = 'Revenue'
+


Can you explain these changes in the changelog? It's just not obviously related to the created/update_at if it's part of those changes.

Good call out. These changes are from the pre-release of this package that went out last week, but is not currently live to customers in an actual release. However this was accidentally merged to main, instead of onto its own branch.

So I'm not certain if we should call out changes that aren't live in our latest release, since to their eyes, nothing will change, even if there are changes currently contained in main itself.

Got it. I'll leave it to your judgement!

fivetran-avinash

@fivetran-catfritz Addressed your comments!

fivetran-catfritz

@fivetran-avinash lgtm!

fivetran-avinash added 3 commits November 18, 2024 08:26

Add created at and updated at fields

a89c9e7

Add created at and updated at fields

3b1d3f4

Add created at and updated at

a736775

fivetran-avinash self-assigned this Nov 20, 2024

fivetran-avinash marked this pull request as ready for review November 20, 2024 16:21

Add created at and updated at

9ab70d3

fivetran-avinash changed the title ~~Add created at and updated at fields~~ Add created_at and updated_at fields Nov 20, 2024

fivetran-joemarkiewicz self-requested a review November 20, 2024 22:42

fivetran-joemarkiewicz requested changes Nov 20, 2024

View reviewed changes

PR fixes

c65bad5

fivetran-avinash commented Nov 21, 2024

View reviewed changes

packages.yml Outdated Show resolved Hide resolved

fivetran-avinash commented Nov 21, 2024

View reviewed changes

fivetran-joemarkiewicz approved these changes Nov 21, 2024

View reviewed changes

fivetran-catfritz reviewed Nov 22, 2024

View reviewed changes

release review

9835893

fivetran-avinash commented Nov 25, 2024

View reviewed changes

fivetran-catfritz approved these changes Nov 25, 2024

View reviewed changes

Update packages.yml

2e600db

fivetran-avinash merged commit c57a9c2 into main Nov 26, 2024
8 checks passed

fivetran-joemarkiewicz mentioned this pull request Dec 18, 2024

[Bug Fix] Net income adjustment for no revenue/expense lines #149

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `created_at` and `updated_at` fields #146

Add `created_at` and `updated_at` fields #146

fivetran-avinash commented Nov 18, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 20, 2024

fivetran-avinash Nov 21, 2024

fivetran-joemarkiewicz Nov 20, 2024

fivetran-avinash Nov 21, 2024

fivetran-avinash left a comment

fivetran-joemarkiewicz left a comment

fivetran-catfritz left a comment

fivetran-catfritz Nov 22, 2024

fivetran-avinash Nov 25, 2024

fivetran-catfritz Nov 22, 2024

fivetran-avinash Nov 25, 2024

fivetran-catfritz Nov 22, 2024

fivetran-avinash Nov 25, 2024

fivetran-catfritz Nov 22, 2024

fivetran-avinash Nov 25, 2024 •

edited

Loading

fivetran-catfritz Nov 25, 2024

fivetran-avinash left a comment

fivetran-catfritz left a comment

	- We have introduced the following fields in the `quickbooks__general_ledger` to better analyze real-time transaction data:
	- Introduced the following fields in the `quickbooks__general_ledger` model to better analyze real-time transaction data:

	- This is a breaking change as this adds new fields to the existing schema for one of our end models.
	- This is a breaking change as this adds new fields to the existing schema.

	- This required updates to all our double entry transaction models to bring in these specific fields for each transaction type.
	- Updated the `*_double_entry` models to add these fields for each transaction type.

Add created_at and updated_at fields #146

Add created_at and updated_at fields #146

Conversation

fivetran-avinash commented Nov 18, 2024 • edited Loading

PR Overview

Feature Updates

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Add `created_at` and `updated_at` fields #146

Add `created_at` and `updated_at` fields #146

fivetran-avinash commented Nov 18, 2024 •

edited

Loading

fivetran-avinash Nov 25, 2024 •

edited

Loading