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

Add account_display_full_name and accounting_period_full_name #107

Conversation

jmongerlyra
Copy link
Contributor

@jmongerlyra jmongerlyra commented Feb 7, 2024

Please provide your name and company
Jared Monger, Lyra Health

Link the issue/feature request which this PR is meant to address

#106

Detail what changes this PR introduces and how this addresses the issue/feature request linked above.
The PR creates hierarchical strings for GL accounts and accounting periods and then presents those new fields in the main models. It also adds the commonly used NetSuite fields below into the models.

  • transaction_accounting_lines.exchange_rate (exchange rate on accounting line)
  • subsidiaries.full_name (full hierarchical subsidiary name)
  • transaction_lines.is_eliminate (boolean to indicate automatic elimination)
  • departments.full_name (full hierarchical department name)
  • subsidiaries_currencies.symbol (base currency of subsidiary)
  • transaction_lines.netamount (net amount of transaction line)

How did you validate the changes introduced within this PR?
Lyra maintains a fork of this repository. These changes were tested and validated against production NetSuite reports.

Which warehouse did you use to develop these changes?
Snowflake

Did you update the CHANGELOG?

  • Yes

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

  • Yes

Provide an emoji that best describes your current mood

🚀

Feedback

We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.

PR Template

rwang-lyra and others added 30 commits October 5, 2023 14:23
using ssh git url so fivetran could auth using ssh key
* Add fields to netsuite2__transaction_details

* Use exchange_rate from transaction_accounting_lines

* Add subsidiary_currency_symbol to netsuite2__transaction_details

* Add fields to balance_sheet

* Add fields to income_statement

* Remove transaction_line_amount from passthrough

* subsidiary currency to currency_id

* Add type_based_document_number

* Add type_based_document_number to models

Add type_based_document_number to balance sheet and income statement
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.

Comments for fivetran use.

Comment on lines +35 to +37
from accounts
join account_hierarchy
on accounts.parent_id = account_hierarchy.account_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Fivetran: Double check self-join syntax works for non-Snowflake

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jmongerlyra! Unfortunately the self-join logic did not work in other warehouses, so I recommend using the below code for this model that turns the second union query into a separate CTE.

Updated codes from lines 15 and on. Let me know if this looks good!


account_hierarchy as (

    select
        account_id,
        parent_id,
        1 as level,
        account_number || ' - ' || display_name as display_full_name
    
    from accounts
    where parent_id is null
),

unioned as (

    select * 
    from account_hierarchy

    union all

    select
        accounts.account_id,
        accounts.parent_id,
        account_hierarchy.level + 1,
        account_hierarchy.display_full_name || ' : ' || accounts.account_number || ' - ' || accounts.display_name as display_full_name
    
    from accounts
    join account_hierarchy
        on accounts.parent_id = account_hierarchy.account_id 
),

joined as (

    select 
        accounts.*,
        unioned.display_full_name,
        account_types.type_name,
        account_types.is_balancesheet,
        account_types.is_leftside

    from accounts
    left join account_types
        on accounts.account_type_id = account_types.account_type_id
    left join unioned
        on accounts.account_id = unioned.account_id
)

select *
from joined

Comment on lines +34 to +35
join accounting_period_hierarchy
on accounting_periods.parent_id = accounting_period_hierarchy.accounting_period_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Fivetran: Double check self-join syntax works for non-Snowflake

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jmongerlyra! Unfortunately the self-join logic did not work in other warehouses, so I recommend using the below code for this model that turns the second union query into a separate CTE.

Updated codes from lines 15 and on. Let me know if this looks good!

accounting_period_hierarchy as (

    select
        accounting_period_id,
        parent_id,
        1 as level,
        name as full_name
    from accounting_periods
    where parent_id is null
),

unioned as (

    select *
    from accounting_period_hierarchy

    union all 

    select
        accounting_periods.accounting_period_id,
        accounting_periods.parent_id,
        accounting_period_hierarchy.level + 1,
        accounting_period_hierarchy.full_name || ' : ' || accounting_periods.name as full_name
    from accounting_periods
    join accounting_period_hierarchy
        on accounting_periods.parent_id = accounting_period_hierarchy.accounting_period_id
),

joined as (

    select 
        accounting_periods.*,
        accounting_period_fiscal_calendars.fiscal_calendar_id,
        unioned.full_name

    from accounting_periods
    left join accounting_period_fiscal_calendars
        on accounting_periods.accounting_period_id = accounting_period_fiscal_calendars.accounting_period_id
    left join unioned
        on accounting_periods.accounting_period_id = unioned.accounting_period_id
)

select *
from joined

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-avinash Feel free to merge my PR into a non-main Fivetran branch and make the changes. I can clone and test locally. Does that sound good? If the solutions are equivalent sounds fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmongerlyra Sounds good! I will merge these changes in now and let you know when they are ready to clone and test locally.

@@ -12,17 +12,45 @@ account_types as (
from {{ var('netsuite2_account_types') }}
),

account_hierarchy as (
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz Feb 28, 2024

Choose a reason for hiding this comment

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

Fivetran: We'll also want to consider if we want to limit this to instances where the customer is using subsidiaries/multibook.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seemed to work.

@@ -188,7 +200,10 @@ transaction_details as (
on accounting_periods.accounting_period_id = transactions.accounting_period_id

left join customers
on customers.customer_id = coalesce(transaction_lines.entity_id, transactions.entity_id)
on case when lower(transactions.transaction_type) in ('custinvc', 'custcred')
then customers.customer_id = transactions.entity_id
Copy link
Contributor

Choose a reason for hiding this comment

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

@jmongerlyra are there any cases where the transaction type is 'custinvc', 'custcred', 'vendbill', or 'vendcred' but the transaction entity_id is null?

essentially i am wondering if there is a need for a coalesce(transactions.entity_id, transaction_lines.entity_id) here

Copy link
Contributor Author

@jmongerlyra jmongerlyra Mar 9, 2024

Choose a reason for hiding this comment

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

@fivetran-jamie transactions.entity_id is on header record of those transaction types. The master table being referenced by transactions.entity_id is contingent on the transaction type. For custinvc & custcred it's customers.customer_id. Similar logic for bills/vendors.

coalesce(transactions.entity_id, transaction_lines.entity_id) is needed because on journal entries and other transaction types, the customer/vendor ID could be on transaction_lines.entity_id. It can also be NULL.

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-jamie This patch is slower than we would like, so I have rebased it out of my fork. The PR is back where it was originally. I'll submit a separate PR for issue 109. No further changes will be made here.

Copy link
Contributor

@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.

Hi @jmongerlyra ! Was able to run and compile your code successfully. Had some suggestions to update the code base so it'll work across all non-Snowflake warehouses, and then I'll complete final validations next week.

One quick update is that we will likely not release this immediately, as we have discovered other potential issues we might need to address (independent of this issue), and we don't want to have multiple releases that are breaking. So we will likely fold this PR into a release branch before actually setting this live.

It seems like you have been okay with using your other branches for using Netsuite, so we hope this line of development is satisfactory! Let us know if there's anything else we can do to assist you.

Comment on lines +34 to +35
join accounting_period_hierarchy
on accounting_periods.parent_id = accounting_period_hierarchy.accounting_period_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jmongerlyra! Unfortunately the self-join logic did not work in other warehouses, so I recommend using the below code for this model that turns the second union query into a separate CTE.

Updated codes from lines 15 and on. Let me know if this looks good!

accounting_period_hierarchy as (

    select
        accounting_period_id,
        parent_id,
        1 as level,
        name as full_name
    from accounting_periods
    where parent_id is null
),

unioned as (

    select *
    from accounting_period_hierarchy

    union all 

    select
        accounting_periods.accounting_period_id,
        accounting_periods.parent_id,
        accounting_period_hierarchy.level + 1,
        accounting_period_hierarchy.full_name || ' : ' || accounting_periods.name as full_name
    from accounting_periods
    join accounting_period_hierarchy
        on accounting_periods.parent_id = accounting_period_hierarchy.accounting_period_id
),

joined as (

    select 
        accounting_periods.*,
        accounting_period_fiscal_calendars.fiscal_calendar_id,
        unioned.full_name

    from accounting_periods
    left join accounting_period_fiscal_calendars
        on accounting_periods.accounting_period_id = accounting_period_fiscal_calendars.accounting_period_id
    left join unioned
        on accounting_periods.accounting_period_id = unioned.accounting_period_id
)

select *
from joined

packages.yml Outdated
packages:
- package: fivetran/netsuite_source
version: [">=0.9.0", "<0.10.0"]
packages:
Copy link
Contributor

Choose a reason for hiding this comment

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

@jmongerlyra I used this to test your source branch, feel free to modify this back around, didn't mean to push this to your branch.

@@ -12,17 +12,45 @@ account_types as (
from {{ var('netsuite2_account_types') }}
),

account_hierarchy as (
Copy link
Contributor

Choose a reason for hiding this comment

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

Seemed to work.

Comment on lines +35 to +37
from accounts
join account_hierarchy
on accounts.parent_id = account_hierarchy.account_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jmongerlyra! Unfortunately the self-join logic did not work in other warehouses, so I recommend using the below code for this model that turns the second union query into a separate CTE.

Updated codes from lines 15 and on. Let me know if this looks good!


account_hierarchy as (

    select
        account_id,
        parent_id,
        1 as level,
        account_number || ' - ' || display_name as display_full_name
    
    from accounts
    where parent_id is null
),

unioned as (

    select * 
    from account_hierarchy

    union all

    select
        accounts.account_id,
        accounts.parent_id,
        account_hierarchy.level + 1,
        account_hierarchy.display_full_name || ' : ' || accounts.account_number || ' - ' || accounts.display_name as display_full_name
    
    from accounts
    join account_hierarchy
        on accounts.parent_id = account_hierarchy.account_id 
),

joined as (

    select 
        accounts.*,
        unioned.display_full_name,
        account_types.type_name,
        account_types.is_balancesheet,
        account_types.is_leftside

    from accounts
    left join account_types
        on accounts.account_type_id = account_types.account_type_id
    left join unioned
        on accounts.account_id = unioned.account_id
)

select *
from joined

@jmongerlyra jmongerlyra force-pushed the lyra-v0.13.0 branch 2 times, most recently from a46904c to 0f67395 Compare March 18, 2024 15:31
@fivetran-avinash fivetran-avinash changed the base branch from main to feature/add-hierarchy-for-account-period-name March 18, 2024 16:23
@fivetran-avinash fivetran-avinash merged commit 935853e into fivetran:feature/add-hierarchy-for-account-period-name Mar 18, 2024
@jmongerlyra jmongerlyra deleted the lyra-v0.13.0 branch August 9, 2024 13:18
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.

5 participants