Release v0.15.0: Bug fixes for account numbers, addition of commonly used fields and foreign keys #144

fivetran-avinash · 2024-11-12T00:32:48Z

PR Overview

This PR will address the following Issue/Feature: [#67], [#135], [#141]

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

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

Breaking Changes (Full refresh required after upgrading)

Corrected account_number field logic for the netsuite2__balance_sheet model to match the native Balance Sheet report within Netsuite:
- Income statement accounts should use the account number of the system-generated retained earnings account.
- CTA accounts should use the account number of the system-generated CTA account.
- Since this will change the account_number, a --full-refresh after upgrading will be required.

More Breaking Changes: New Fields

Added commonly used fields to specific end models.
netsuite2__balance_sheet
- subsidiary_full_name: Full hierarchical name of the subsidiary.
- is_account_intercompany: Boolean indicating if a general ledger account recording transactions between subsidiaries of the same organization.
- is_account_leftside: Boolean indicating if account has a native debit balance.
netsuite2__balance_sheet, netsuite2__income_statement, netsuite2__transaction_details
- account_display_name: Account name without number or hierarchy.
netsuite2__transaction_details
- is_reversal: Boolean indicating line is reversal.
- reversal_transaction_id: Transaction id of the counterparty in a reversing pair.
- reversal_date: Transaction date of the counterparty in a reversing pair.
- is_reversal_defer: Boolean indicating reversal deferral.
- is_eliminate: Boolean indicating line will auto-eliminate.
- exchange_rate: Exchange rate used on the transaction.
- department_full_name: Full hierarchical name of the department.
- subsidiary_full_name: Full hierarchical name of the subsidiary.
- subsidiary_currency_symbol: Currency of the subsidiary.
- transaction_line_amount: Net amount of the transaction line. This is the actual amount entered when it's in a currency other than the functional currency of the subsidiary.
Added foreign keys to the following end models to make it easier for customers to join back to source tables for better insights.
- netsuite2__income_statement: class_id, location_id, department_id
- netsuite2__transaction_details: customer_id, vendor_id, class_id, location_id, department_id, currency_id, parent_account_id, vendor_category_id (if netsuite2__using_vendor_categories is enabled)

IMPORTANT: All of the affected models have pass-through functionality. If you have already been using passthrough column variables to include the newly added fields (without aliases), you MUST remove the fields from your passthrough variable configuration in order to avoid duplicate column errors.

Feature Updates

You can now leverage passthrough columns in netsuite2__transaction_details to bring in additional fields from the locations and subsidiaries source tables. Make sure you aren't bringing in

Under the Hood

Additional consistency tests added for each Netsuite2 end model in order to be used during integration test validations.

PR Checklist

Basic Validation

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

dbt run –full-refresh && dbt test
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:

Able to test and validate there were no major changes on amounts by transaction ids.

Able to run successfully that passthrough columns bring through new columns.

Validation through internal data.

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

💸

…columns (#136) * Fix account_number Fix account_number for system generated accounts. * Add subsidiaries pass through columns * Add additional fields * Fix is_account_intercompany on balance_sheet * Fix account_number --------- Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>

fivetran-joemarkiewicz

@fivetran-avinash A few change requests and questions but this is looking good! Additionally, would you be able to inspect the results of the validation tests (particularly the amount and count tests). When viewing the results and removing the final where clause I find that some of the results are null and still passing the condition. Could you double check and see if anything needs to be adjusted on the test side or if some code changes are required. Thanks!

integration_tests/tests/consistency/consistency_balance_sheet_count.sql

integration_tests/tests/consistency/consistency_income_statement.sql

integration_tests/tests/consistency/consistency_income_statement_count.sql

integration_tests/tests/consistency/consistency_transaction_details.sql

integration_tests/tests/consistency/consistency_transaction_details_count.sql

CHANGELOG.md

fivetran-joemarkiewicz · 2024-11-12T21:10:55Z

models/netsuite2/netsuite2__balance_sheet.sql

@@ -106,10 +114,17 @@ balance_sheet as (
        else accounts.account_id
            end as account_id,
        case
-        when not accounts.is_balancesheet then null
+        when not accounts.is_balancesheet then (select accounts.account_number from accounts where lower(accounts.special_account_type_id) = 'retearnings' limit 1)


Can you provide a brief explanation of this subquery. I see it's used in a few other places as well.

Via @jmongerlyra in the issue:

"The account number for income statement and CTA accounts in the balance sheet model is currently set to NULL. This is not how it is presented on the native Balance Sheet report in NetSuite.

Income statement accounts should use the account number of the system-generated retained earnings account. CTA should use the account number of the system-generated CTA account."

This logic ensures that the account number pulls the retained earnings or CTA account number rather than generating null values. I added some details in the CHANGELOG to call this out.

This makes sense to me. Thanks for sharing this and calling it out in the CHANGELOG.

CHANGELOG.md

models/netsuite2/netsuite2__transaction_details.sql

fivetran-joemarkiewicz · 2024-11-12T21:49:23Z

models/netsuite2/netsuite2__transaction_details.sql

+    case
+      when lower(accounts.account_type_id) in ('income', 'othincome') then -transaction_lines.netamount
+      else transaction_lines.netamount
+        end as transaction_line_amount  


Can you explain this addition a bit more? Will there ever be a scenario where this does not match the transaction_amount?

This is a new field from the transaction_lines source table, netamount. transaction_amount is being derived from amount in transaction_lines.

According to @jmongerlyra , netamount is the net amount of the transaction line, which is the actual amount entered when it's in a currency other than the functional currency of the subsidiary. Whereas amount is the total amount. I believe these values can be different.

Co-authored-by: Antonio Ruby Barreto <antonio.ruby.barreto@fastnedcharging.com>

fivetran-avinash

@fivetran-joemarkiewicz Pending Buildkite, this is ready for re-review.

Regarding your concern around null test results, I did see this issue in consistency_balance_sheet_amounts because the base of this test is account ids. However, account_ids can be null if accounts have a false is_balancesheet value. So we have a null base that is throwing off the full outer join logic.

For the time being, I created a dummy value for these null cases, and this seemed to prevent the null fanout. But happy to workshop more elegant solutions here if we feel this is a bit hacky.

I didn't see any issues with the other amount test, as it uses a date_day as a base and there are no null transaction dates in our production data. The counts are just comparing row counts, and nothing threw a red flag in terms of these tests.

fivetran-avinash · 2024-11-13T18:17:16Z

integration_tests/tests/consistency/consistency_balance_sheet.sql

@@ -5,11 +5,13 @@

 with prod as (
    select *
+    except(account_type_name, account_number) --this test has been modified for the purposes of validating this PR. Remove this line before merging.


Suggested change

except(account_type_name, account_number) --this test has been modified for the purposes of validating this PR. Remove this line before merging.

fivetran-avinash · 2024-11-13T18:17:25Z

integration_tests/tests/consistency/consistency_balance_sheet.sql

    from {{ target.schema }}_netsuite_prod.netsuite2__balance_sheet
 ),

 dev as (
    select *
+    except(subsidiary_full_name, account_display_name, is_account_intercompany, is_account_leftside, account_type_name, account_number) --this test has been modified for the purposes of validating this PR.Remove this line before merging.


Suggested change

except(subsidiary_full_name, account_display_name, is_account_intercompany, is_account_leftside, account_type_name, account_number) --this test has been modified for the purposes of validating this PR.Remove this line before merging.

fivetran-avinash · 2024-11-13T18:17:52Z

integration_tests/tests/consistency/consistency_income_statement.sql

@@ -10,6 +10,7 @@ with prod as (

 dev as (
    select *
+    except(account_display_name, class_id, location_id, department_id)--this test has been modified for the purposes of validating this PR. Remove this line before merging.


Suggested change

except(account_display_name, class_id, location_id, department_id)--this test has been modified for the purposes of validating this PR. Remove this line before merging.

fivetran-avinash · 2024-11-13T18:18:06Z

integration_tests/tests/consistency/consistency_transaction_details.sql

@@ -10,6 +10,7 @@ with prod as (

 dev as (
    select *
+    except(is_reversal, reversal_transaction_id, reversal_date, is_reversal_defer, account_display_name, is_eliminate, parent_account_id, customer_id, class_id, location_id, vendor_id, vendor_category_id, currency_id, exchange_rate, department_full_name, subsidiary_full_name, subsidiary_currency_symbol, transaction_line_amount) --this test has been modified for the purposes of validating this PR. Remove this line before merging.


Suggested change

except(is_reversal, reversal_transaction_id, reversal_date, is_reversal_defer, account_display_name, is_eliminate, parent_account_id, customer_id, class_id, location_id, vendor_id, vendor_category_id, currency_id, exchange_rate, department_full_name, subsidiary_full_name, subsidiary_currency_symbol, transaction_line_amount) --this test has been modified for the purposes of validating this PR. Remove this line before merging.

reminder to remove these from the tests

fivetran-joemarkiewicz

@fivetran-avinash thanks for addressing all of my comments. My only remaining comment is that it would be great to add a validation test for the income statement amounts to be used in future reviews.

That is no reason to block this approval and move to release review though. This looks good to me!

fivetran-avinash · 2024-11-13T19:38:22Z

@fivetran-avinash thanks for addressing all of my comments. My only remaining comment is that it would be great to add a validation test for the income statement amounts to be used in future reviews.

That is no reason to block this approval and move to release review though. This looks good to me!

@fivetran-joemarkiewicz Good callout! Actually didn't take too long to create a robust income statement amount consistency test and get it to pass! Moving to release review now.

fivetran-reneeli

Approved with just a minor update suggestion!

dbt_project.yml

fivetran-reneeli · 2024-11-13T22:59:34Z

integration_tests/tests/consistency/consistency_transaction_details.sql

@@ -10,6 +10,7 @@ with prod as (

 dev as (
    select *
+    except(is_reversal, reversal_transaction_id, reversal_date, is_reversal_defer, account_display_name, is_eliminate, parent_account_id, customer_id, class_id, location_id, vendor_id, vendor_category_id, currency_id, exchange_rate, department_full_name, subsidiary_full_name, subsidiary_currency_symbol, transaction_line_amount) --this test has been modified for the purposes of validating this PR. Remove this line before merging.


reminder to remove these from the tests

Release v0.15.0

5e407a2

fivetran-avinash self-assigned this Nov 12, 2024

fivetran-avinash and others added 4 commits November 11, 2024 17:09

documentat

5fa4c7b

PR tweaks

fccc4b8

documentation

3d06ebc

fivetran-avinash changed the title ~~Release v0.15.0~~ Release v0.15.0: Bug fixes for account numbers, addition of commonly used fields and foreign keys Nov 12, 2024

fivetran-joemarkiewicz self-requested a review November 12, 2024 20:02

CHANGELOG"

c6801f3

fivetran-joemarkiewicz requested changes Nov 12, 2024

View reviewed changes

fivetran-avinash and others added 3 commits November 12, 2024 14:04

Customer PR adding locations fields (#146)

7ad0e91

Co-authored-by: Antonio Ruby Barreto <antonio.ruby.barreto@fastnedcharging.com>

PR fixes

87a7d63

PR fixes

57118b8

fivetran-avinash commented Nov 13, 2024

View reviewed changes

fivetran-joemarkiewicz approved these changes Nov 13, 2024

View reviewed changes

Final PR changes

bdb3da0

fivetran-reneeli approved these changes Nov 13, 2024

View reviewed changes

Release review

2b8b33c

fivetran-avinash merged commit 72f3fcf into main Nov 13, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release v0.15.0: Bug fixes for account numbers, addition of commonly used fields and foreign keys #144

Release v0.15.0: Bug fixes for account numbers, addition of commonly used fields and foreign keys #144

fivetran-avinash commented Nov 12, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 12, 2024

fivetran-avinash Nov 13, 2024

fivetran-joemarkiewicz Nov 13, 2024

fivetran-joemarkiewicz Nov 12, 2024

fivetran-avinash Nov 13, 2024 •

edited

Loading

fivetran-avinash left a comment

fivetran-avinash Nov 13, 2024

fivetran-avinash Nov 13, 2024 •

edited

Loading

fivetran-avinash Nov 13, 2024

fivetran-avinash Nov 13, 2024

fivetran-reneeli Nov 13, 2024

fivetran-joemarkiewicz left a comment

fivetran-avinash commented Nov 13, 2024

fivetran-reneeli left a comment

fivetran-reneeli Nov 13, 2024

Release v0.15.0: Bug fixes for account numbers, addition of commonly used fields and foreign keys #144

Release v0.15.0: Bug fixes for account numbers, addition of commonly used fields and foreign keys #144

Conversation

fivetran-avinash commented Nov 12, 2024 • edited Loading

PR Overview

Breaking Changes (Full refresh required after upgrading)

More Breaking Changes: New Fields

Feature Updates

Under the Hood

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 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash Nov 13, 2024 • edited Loading

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-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-avinash commented Nov 13, 2024

fivetran-reneeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash commented Nov 12, 2024 •

edited

Loading

fivetran-avinash Nov 13, 2024 •

edited

Loading

fivetran-avinash Nov 13, 2024 •

edited

Loading