bug/account-type-id #84

fivetran-catfritz · 2023-09-25T17:06:30Z

PR Overview

This PR will address the following Issue/Feature:

[Bug] Income Statement model not accounting for renamed account types #83

This PR will result in the following new package version:

v0.10.0
Breaking change since are changing how records are categorized and some may be different than before (for example, if a prior issue was now corrected).

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:

Updated the following models to reference account_type_id instead of type_name:
- int_netsuite2__tran_with_converted_amounts
- netsuite2__balance_sheet
- netsuite2__income_statement
- netsuite2__transaction_details
This change was implemented because type_name was previously utilized to categorize records, which was causing issues for users that customized the type_name values. Utilizing the unique identifier account_type_id instead produces more accurate results in the final models.
Removed accepted_values test from column account_type_names in model netsuite2__transaction_details since logic is now based on account_type_id instead, and type names can be changed by the user.
Updated documents with descriptions for account_type_id

** ❗ Note - references to "prepaid" were removed since they became moot when moving to using ids.

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:

The appropriate issue has been linked and tagged
You are assigned to the corresponding issue and this PR
BuildKite integration tests are passing

Detailed Validation

Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":

You have validated these changes and assure this PR will address the respective Issue/Feature.
You are reasonably confident these changes will not impact any other components of this package or any dependent packages.
You have provided details below around the validation steps performed to gain confidence in these changes.

See internal ticket for validation.

Standard Updates

Please acknowledge that your PR contains the following standard updates:

Package versioning has been appropriately indexed in the following locations:
- indexed within dbt_project.yml
- indexed within integration_tests/dbt_project.yml
CHANGELOG has individual entries for each respective change in this PR
README updates have been applied (if applicable)
~~DECISIONLOG updates have been updated (if applicable)~~
Appropriate yml documentation has been added (if applicable)

dbt Docs

Please acknowledge that after the above were all completed the below were applied to your branch:

docs were regenerated (unless this PR does not include any code or yml updates)

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

🆔

models/netsuite2/intermediate/int_netsuite2__tran_with_converted_amounts.sql

models/netsuite2/netsuite2__balance_sheet.sql

fivetran-joemarkiewicz

@fivetran-catfritz these changes look great. This is a HUGE enhancement to the package by making it more accurate and robust to handle changing Netsuite2 environments. Thanks for the due diligence in updating these models and also documenting your validation efforts. I was also able to confirm that for every period and for every account in our internal data that the totals match between the current version of the package and your changes for IS and BS.

I do have just a few minor and final requests and suggestions. Would you be able to address these. Then this should be good to go for approval!

fivetran-joemarkiewicz · 2023-09-29T15:35:03Z

CHANGELOG.md

+  - netsuite2__balance_sheet
+  - netsuite2__income_statement
+  - netsuite2__transaction_details
+- This change was implemented because `type_name` was previously utilized to categorize records, which was causing issues for users that customized the `type_name` values. Utilizing the unique identifier `account_type_id` instead produces more accurate results in the final models.


Can we just clarify here that we are referencing the above bullet and that this is only for Netsuite2

I recall Jamie found a Netsuite doc that showed this mapping. Would we be able to share that in the CHANGELOG so customers are aware of how we were informed around the proper pairings for these changes?

Found the source Jamie identified

https://blog.prolecto.com/2013/09/10/netsuite-searchfilter-internal-account-type-codes/

Thanks! Can you also include the link somewhere just for reference so customers know where we found the mapping.

models/netsuite2/intermediate/int_netsuite2__tran_with_converted_amounts.sql

fivetran-joemarkiewicz · 2023-09-29T16:18:16Z

models/netsuite2/netsuite2__transaction_details.sql

-    lower(accounts.type_name) like 'accounts payable%' as is_accounts_payable,
-    lower(accounts.type_name) like 'accounts receivable%' as is_accounts_receivable,
+    lower(accounts.account_type_id) = 'acctpay' as is_accounts_payable,
+    lower(accounts.account_type_id) = 'acctrec' as is_accounts_receivable,
    lower(accounts.name) like '%intercompany%' as is_account_intercompany,


It looks like we didn't update this account name. I agree with you that it seems this is the correct move as there is not a lookup key for these fields. But would you be able to just add some clarification in this thread for posterity.

This is a good point. I hadn't really considered it, but I looked into it, and there is a flag in a column called eliminate in the accounts source table that is meant to flag an account as intercompany. See the netsuite docs for field eliminate. See also this doc (and search for "eliminate").

I added this to the source and tried this out, however unlike my other changes, this does produce different results from our current model. For example our data has some accounts marked as eliminate = T, but the account name does not contain intercompany. Thinking this needs to be a new issue for further investigation?

Thanks for investigating! This is interesting, however I would prefer we do not make the update here as you can see internally that the changes without the eliminate update tie out, but with the eliminate update they do not. I don't feel there is a need for a new issue as there currently is not an existing issue around this.

Let's discuss at standup what our internally procedures may to flag something like this. I don't feel it is necessary to be an issue, but we also don't have enough information to create a FR.

From discussion, we will leave this flag as-is for now, but will come back to it.

fivetran-joemarkiewicz

@fivetran-catfritz these changes look good! I just have two very minor updates to request before we kick off this for release review.

fivetran-joemarkiewicz · 2023-10-02T15:52:51Z

CHANGELOG.md

+  - netsuite2__balance_sheet
+  - netsuite2__income_statement
+  - netsuite2__transaction_details
+- The above change was implemented for Netsuite2 because `type_name` was previously utilized to categorize records, which was causing issues for users that customized the `type_name` values. Utilizing the unique identifier `account_type_id` instead produces more accurate results in the final models. Below is a list of the account type name and type id pairings used:


Can you add the link here for where the pairings came from.

fivetran-joemarkiewicz · 2023-10-02T15:53:44Z

models/netsuite2/netsuite2__transaction_details.sql

    lower(accounts.name) like '%intercompany%' as is_account_intercompany,
    coalesce(parent_account.name, accounts.name) as parent_account_name,
-    lower(accounts.type_name) like '%expense' as is_expense_account, -- includes deferred expense
-    lower(accounts.type_name) like '%income' as is_income_account,
+    lower(accounts.account_type_id) in ('expense', 'othexpense', 'deferexpense') as is_expense_account, -- includes deferred expense


We can probably remove the comment here as it is a bit obvious with these updates.

fivetran-catfritz added 4 commits September 25, 2023 11:43

bug/account-type-id

7f0d4f2

updates

84d7584

updates

8475846

updates

4c5d5d1

fivetran-catfritz linked an issue Sep 28, 2023 that may be closed by this pull request

[Bug] Income Statement model not accounting for renamed account types #83

Closed

4 tasks

updates

0ed92e7

fivetran-catfritz commented Sep 28, 2023

View reviewed changes

models/netsuite2/intermediate/int_netsuite2__tran_with_converted_amounts.sql Show resolved Hide resolved

fivetran-catfritz commented Sep 28, 2023

View reviewed changes

models/netsuite2/netsuite2__balance_sheet.sql Show resolved Hide resolved

fivetran-catfritz added 3 commits September 28, 2023 10:38

update ymls

51d8622

update changelog & regen docs

772a6ee

update ymls

e65bbb8

fivetran-catfritz self-assigned this Sep 28, 2023

update readme

f08b13e

fivetran-catfritz requested a review from fivetran-joemarkiewicz September 28, 2023 22:31

fivetran-catfritz added 2 commits September 28, 2023 17:43

updates

67719f5

updates

08c09dd

fivetran-joemarkiewicz requested changes Sep 29, 2023

View reviewed changes

fivetran-catfritz requested a review from fivetran-joemarkiewicz September 29, 2023 21:02

fivetran-catfritz added 2 commits September 29, 2023 16:07

updates

c1d16df

updates

ed75203

fivetran-joemarkiewicz approved these changes Oct 2, 2023

View reviewed changes

fivetran-catfritz added 4 commits October 2, 2023 11:24

update comments

4777984

update changelog and decisionlog

2380b6d

refen docs

fd9fd7d

Update CHANGELOG.md

1eb1d51

fivetran-catfritz merged commit 06e4923 into main Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug/account-type-id #84

bug/account-type-id #84

fivetran-catfritz commented Sep 25, 2023 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Sep 29, 2023

fivetran-joemarkiewicz Sep 29, 2023

fivetran-joemarkiewicz Sep 29, 2023

fivetran-catfritz Sep 29, 2023

fivetran-joemarkiewicz Oct 2, 2023

fivetran-catfritz Oct 2, 2023

fivetran-joemarkiewicz Sep 29, 2023

fivetran-catfritz Sep 29, 2023

fivetran-joemarkiewicz Oct 2, 2023

fivetran-catfritz Oct 2, 2023

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Oct 2, 2023

fivetran-catfritz Oct 2, 2023

fivetran-joemarkiewicz Oct 2, 2023

fivetran-catfritz Oct 2, 2023

bug/account-type-id #84

bug/account-type-id #84

Conversation

fivetran-catfritz commented Sep 25, 2023 • edited Loading

PR Overview

PR Checklist

Basic Validation

Detailed Validation

Standard Updates

dbt Docs

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

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-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-catfritz commented Sep 25, 2023 •

edited

Loading