-
Notifications
You must be signed in to change notification settings - Fork 20
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 fields to support account hierarchies #45
Feature/add fields to support account hierarchies #45
Conversation
Add extra fields for intercompany reconciliation.
* Align sspecacct with upstream * Add currency_id to stg_netsuite2__subsidiaries * Fix currency_id * Update get_subsidiaries_columns.sql * Add type_based_document_number
Add fields for PR-107 in dbt_netsuite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but just have a few comments and requests to make small adjustments and clarifications in the CHANGELOG.
CHANGELOG.md
Outdated
## 🎉 Feature Updates 🎉 | ||
- Fields added to support account hierarchies in the `dbt_netsuite` end model. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) | ||
- Added `is_elimination` in `stg_netsuite2__subsidiaries`. | ||
- Added `is_eliminate` and `net_amount` in `stg_netsuite2__transaction_lines`. | ||
|
||
## Documentation Updates | ||
- Added `display_name` in `stg_netsuite2__accounts`; added `exchange_rate` in `stg_netsuite2__transaction_accounting_lines`. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small style request to change the PR links to be in parenthesis.
## 🎉 Feature Updates 🎉 | |
- Fields added to support account hierarchies in the `dbt_netsuite` end model. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) | |
- Added `is_elimination` in `stg_netsuite2__subsidiaries`. | |
- Added `is_eliminate` and `net_amount` in `stg_netsuite2__transaction_lines`. | |
## Documentation Updates | |
- Added `display_name` in `stg_netsuite2__accounts`; added `exchange_rate` in `stg_netsuite2__transaction_accounting_lines`. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) | |
## 🎉 Feature Updates 🎉 | |
- Fields added to support account hierarchies in the `dbt_netsuite` end model. ([PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45)) | |
- Added `is_elimination` in `stg_netsuite2__subsidiaries`. | |
- Added `is_eliminate` and `net_amount` in `stg_netsuite2__transaction_lines`. | |
## Documentation Updates | |
- Added `display_name` in `stg_netsuite2__accounts`; added `exchange_rate` in `stg_netsuite2__transaction_accounting_lines`. ([PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
CHANGELOG.md
Outdated
@@ -1,3 +1,18 @@ | |||
# dbt_netsuite_source v0.10.0 | |||
|
|||
This is part of a batch release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is all that necessary and can probably be removed.
This is part of a batch release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
CHANGELOG.md
Outdated
This is part of a batch release. | ||
|
||
## 🎉 Feature Updates 🎉 | ||
- Fields added to support account hierarchies in the `dbt_netsuite` end model. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth calling out that this is for Netsuite2
- Fields added to support account hierarchies in the `dbt_netsuite` end model. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) | |
- Fields added to support account hierarchies in the `dbt_netsuite` Netsuite2 end model. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
CHANGELOG.md
Outdated
## 🎉 Feature Updates 🎉 | ||
- Fields added to support account hierarchies in the `dbt_netsuite` end model. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) | ||
- Added `is_elimination` in `stg_netsuite2__subsidiaries`. | ||
- Added `is_eliminate` and `net_amount` in `stg_netsuite2__transaction_lines`. | ||
|
||
## Documentation Updates | ||
- Added `display_name` in `stg_netsuite2__accounts`; added `exchange_rate` in `stg_netsuite2__transaction_accounting_lines`. [PR #45](https://github.com/fivetran/dbt_netsuite_source/pull/45) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some of the new fields listed under feature updates and some listed under documentation updates? Can we instead list them all as feature updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, we should call out for the new accounts
, subsidiaries
, and transaction_lines
fields that if they are currently being passed through in the respective variables, that they will need to be removed. Otherwise they may see an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates applied.
Co-authored-by: Joe Markiewicz <[email protected]>
PR Overview
This PR will address the following Issue/Feature:
dbt_netsuite
issue 106 fivetran/dbt_netsuite#106This PR will result in the following new package version: 10.0 (this will be merged into a release branch I believe)
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
🎉 Feature Updates 🎉
dbt_netsuite
end model. PR #45is_elimination
instg_netsuite2__subsidiaries
.is_eliminate
andnet_amount
instg_netsuite2__transaction_lines
.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: See Height ticket.