Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add created_at and updated at fields #58

Merged
merged 10 commits into from
Nov 26, 2024

Conversation

fivetran-avinash
Copy link
Contributor

@fivetran-avinash fivetran-avinash commented Nov 18, 2024

PR Overview

This PR will address the following Issue/Feature: #144 in dbt_quickbooks

This PR will result in the following new package version:

Added fields into various models, but nothing in particular will break for the customer I believe.

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

Feature Updates

  • We introduced these pre-existing timestamp fields from these sources to their equivalent staging models to better analyze real-time transaction data:
    • created_at: bill, bill_payment, credit_memo, invoice, payment, transfer
    • updated_at: bill, bill_payment, credit_memo, deposit, invoice, journal_entry, payment, purchase, refund_receipt, sales_receipt, transfer, vendor_credit
  • These new fields are then being brought in downstream in the dbt_quickbooks package in the quickbooks__general_ledger model via the double entry transaction intermediate models. You can learn more about these changes in the v0.17.0 release of the dbt_quickbooks package.

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:

Ensured the created_at and updated_at fields were being brought into the relevant models that were used in the double entry transaction models. Example:
Screenshot 2024-11-20 at 6 03 54 AM

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

@fivetran-avinash fivetran-avinash self-assigned this Nov 20, 2024
@fivetran-avinash fivetran-avinash marked this pull request as ready for review November 20, 2024 15:08
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these updates @fivetran-avinash, just a few change requests before approval.

CHANGELOG.md Outdated Show resolved Hide resolved
dbt_project.yml Outdated Show resolved Hide resolved
integration_tests/dbt_project.yml Outdated Show resolved Hide resolved
models/stg_quickbooks.yml Show resolved Hide resolved
Copy link
Contributor Author

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-avinash a couple suggestions!

CHANGELOG.md Outdated
[PR #58](https://github.com/fivetran/dbt_quickbooks_source/pull/57) introduces the following updates:

## Breaking Changes
- We introduced these pre-existing timestamp fields from these sources to their equivalent staging models to better analyze real-time transaction data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would list the field and the exact staging models they were added to instead of requiring the user to have to match them up on their own.

Suggested change
- We introduced these pre-existing timestamp fields from these sources to their equivalent staging models to better analyze real-time transaction data:
- Introduced the following timestamp fields to the listed staging models to better analyze real-time transaction data:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-catfritz Listing all the staging models would get a bit wordy, so I did a slight tweak to your suggestion. Let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True--this makes sense!

CHANGELOG.md Outdated
- `created_at`: `bill`, `bill_payment`, `credit_memo`, `invoice`, `payment`, `transfer`
- `updated_at`: `bill`, `bill_payment`, `credit_memo`, `deposit`, `invoice`, `journal_entry`, `payment`, `purchase`, `refund_receipt`, `sales_receipt`, `transfer`, `vendor_credit`
- These new fields are then being brought in downstream in the `dbt_quickbooks` package in the `quickbooks__general_ledger` model via the double entry transaction intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).
- As this introduces new columns to our staging models and changes our schema, this will be a breaking change.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- As this introduces new columns to our staging models and changes our schema, this will be a breaking change.
- As this introduces new columns to our staging models and changes our schema, this is breaking change.

CHANGELOG.md Outdated
- We introduced these pre-existing timestamp fields from these sources to their equivalent staging models to better analyze real-time transaction data:
- `created_at`: `bill`, `bill_payment`, `credit_memo`, `invoice`, `payment`, `transfer`
- `updated_at`: `bill`, `bill_payment`, `credit_memo`, `deposit`, `invoice`, `journal_entry`, `payment`, `purchase`, `refund_receipt`, `sales_receipt`, `transfer`, `vendor_credit`
- These new fields are then being brought in downstream in the `dbt_quickbooks` package in the `quickbooks__general_ledger` model via the double entry transaction intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- These new fields are then being brought in downstream in the `dbt_quickbooks` package in the `quickbooks__general_ledger` model via the double entry transaction intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).
- These new fields are incorporated in the `dbt_quickbooks` package's `quickbooks__general_ledger` model via the `*_double_entry` intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).

Copy link
Contributor Author

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-catfritz Addressed your comments!

Copy link
Contributor

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-avinash One more teensy item, but lgtm!

CHANGELOG.md Outdated
- Introduced the following timestamp fields to the listed `stg_quickbooks__*` models to better analyze real-time transaction data:
- `created_at`: `bill`, `bill_payment`, `credit_memo`, `invoice`, `payment`, `transfer`
- `updated_at`: `bill`, `bill_payment`, `credit_memo`, `deposit`, `invoice`, `journal_entry`, `payment`, `purchase`, `refund_receipt`, `sales_receipt`, `transfer`, `vendor_credit`
- - These new fields are incorporated in the `dbt_quickbooks` package's `quickbooks__general_ledger` model via the `*_double_entry` intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed the double dash.

Suggested change
- - These new fields are incorporated in the `dbt_quickbooks` package's `quickbooks__general_ledger` model via the `*_double_entry` intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).
- These new fields are incorporated in the `dbt_quickbooks` package's `quickbooks__general_ledger` model via the `*_double_entry` intermediate models. You can learn more about these changes in the [v0.17.0 release of the `dbt_quickbooks` package](https://github.com/fivetran/dbt_quickbooks/releases/tag/v0.17.0).

CHANGELOG.md Outdated
[PR #58](https://github.com/fivetran/dbt_quickbooks_source/pull/57) introduces the following updates:

## Breaking Changes
- We introduced these pre-existing timestamp fields from these sources to their equivalent staging models to better analyze real-time transaction data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True--this makes sense!

@fivetran-avinash fivetran-avinash merged commit 6e98ab6 into main Nov 26, 2024
7 checks passed
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.

3 participants