[Bug] Customer & Vendor joins in transaction_details

jmongerlyra commented 6 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the issue

There is a bug in netsuite2__transaction_details model that causes company_name and vendor_name to be NULL on certain lines.

The issue is in the joins to customer and vendor master tables. Certain transaction types can contain both transaction_lines.entity_id (line level) and transactions.entity_id (header level) values. The current join preferences the former in all cases.

However, in the example of a vendor bill with a billable customer referenced at the line level, this logic causes the customer ID to be joined to the vendor master table, resulting in a NULL. Instead, the join should be contingent on transaction type.

We will finalize a patch and submit a PR.

Relevant error log or model output

n/a

Expected behavior

Customer should always be populated on invoices and vendors always populated on bills.

dbt Project configurations

config-version: 2
name: 'netsuite'
version: '0.12.0'
require-dbt-version: [">=1.3.0", "<2.0.0"]

models:
  netsuite:
    +materialized: table
    +schema: netsuite
    netsuite:
      intermediate:
        +materialized: ephemeral
    netsuite2:
      intermediate:
        +materialized: ephemeral

vars:
  netsuite:
    ## Netsuite staging models
    netsuite_accounting_books: "{{ ref('stg_netsuite__accounting_books') }}"
    netsuite_accounting_periods: "{{ ref('stg_netsuite__accounting_periods') }}"
    netsuite_accounts: "{{ ref('stg_netsuite__accounts') }}"
    netsuite_classes: "{{ ref('stg_netsuite__classes') }}"
    netsuite_consolidated_exchange_rates: "{{ ref('stg_netsuite__consolidated_exchange_rates') }}"
    netsuite_currencies: "{{ ref('stg_netsuite__currencies') }}"
    netsuite_customers: "{{ ref('stg_netsuite__customers') }}"
    netsuite_departments: "{{ ref('stg_netsuite__departments') }}"
    netsuite_expense_accounts: "{{ ref('stg_netsuite__expense_accounts') }}"
    netsuite_income_accounts: "{{ ref('stg_netsuite__income_accounts') }}"
    netsuite_items: "{{ ref('stg_netsuite__items') }}"
    netsuite_locations: "{{ ref('stg_netsuite__locations') }}"
    netsuite_subsidiaries: "{{ ref('stg_netsuite__subsidiaries') }}"
    netsuite_transaction_lines: "{{ ref('stg_netsuite__transaction_lines') }}"
    netsuite_transactions: "{{ ref('stg_netsuite__transactions') }}"
    netsuite_vendor_types: "{{ ref('stg_netsuite__vendor_types') }}"
    netsuite_vendors: "{{ ref('stg_netsuite__vendors') }}"
    netsuite2_account_types: "{{ ref('stg_netsuite2__account_types') }}"
    netsuite2_accounting_book_subsidiaries: "{{ ref('stg_netsuite2__accounting_book_subsidiaries') }}"
    netsuite2_accounting_books: "{{ ref('stg_netsuite2__accounting_books') }}"
    netsuite2_accounting_period_fiscal_calendars: "{{ ref('stg_netsuite2__accounting_period_fiscal_cal') }}"
    netsuite2_accounting_periods: "{{ ref('stg_netsuite2__accounting_periods') }}"
    netsuite2_accounts: "{{ ref('stg_netsuite2__accounts') }}"
    netsuite2_classes: "{{ ref('stg_netsuite2__classes') }}"
    netsuite2_consolidated_exchange_rates: "{{ ref('stg_netsuite2__consolidated_exchange_rates') }}"
    netsuite2_currencies: "{{ ref('stg_netsuite2__currencies') }}"
    netsuite2_customers: "{{ ref('stg_netsuite2__customers') }}"
    netsuite2_departments: "{{ ref('stg_netsuite2__departments') }}"
    netsuite2_entities: "{{ ref('stg_netsuite2__entities') }}"
    netsuite2_entity_address: "{{ ref('stg_netsuite2__entity_address') }}"
    netsuite2_items: "{{ ref('stg_netsuite2__items') }}"
    netsuite2_jobs: "{{ ref('stg_netsuite2__jobs') }}"
    netsuite2_location_main_address: "{{ ref('stg_netsuite2__location_main_address') }}"
    netsuite2_locations: "{{ ref('stg_netsuite2__locations') }}"
    netsuite2_subsidiaries: "{{ ref('stg_netsuite2__subsidiaries') }}"
    netsuite2_transaction_accounting_lines: "{{ ref('stg_netsuite2__transaction_accounting_lines') }}"
    netsuite2_transaction_lines: "{{ ref('stg_netsuite2__transaction_lines') }}"
    netsuite2_transactions: "{{ ref('stg_netsuite2__transactions') }}"
    netsuite2_vendor_categories: "{{ ref('stg_netsuite2__vendor_categories') }}"
    netsuite2_vendors: "{{ ref('stg_netsuite2__vendors') }}"
    accounts_pass_through_columns: []
    classes_pass_through_columns: []
    departments_pass_through_columns: []
    transactions_pass_through_columns: []
    transaction_lines_pass_through_columns: []
    balance_sheet_transaction_detail_columns: []
    income_statement_transaction_detail_columns: []

Package versions

packages:
  - package: fivetran/netsuite_source
    version: [">=0.9.0", "<0.10.0"]

What database are you using dbt with?

snowflake

dbt Version

1.7.3

Additional Context

No response

Are you willing to open a PR to help address this issue?

[X] Yes.
[ ] Yes, but I will need assistance and will schedule time during our office hours for guidance
[ ] No.

fivetran-jamie commented 6 months ago

hey @jmongerlyra thanks for opening this and adding the fix to your PR!

left one question for you in the PR here -- we'll be reviewing your PR this sprint FYI 🤠

fivetran-avinash commented 6 months ago

Thanks for addressing this issue in the existing PR @jmongerlyra ! I should hopefully pick up work on validating that all is well on this PR you've opened in this coming sprint.

jmongerlyra commented 6 months ago

I removed the patch from the existing PR. Needs some optimization. We will submit a separate PR once a new patch is available.

fivetran-avinash commented 5 months ago

No worries @jmongerlyra ! Looking forward for that PR.

I completed a first review of your PR, and we have a review open for your code with some requested changes.

Please go ahead and take a look. We're getting closer to having this ready to fold this into a future release of the package!

fivetran-avinash commented 2 weeks ago

Hi @jmongerlyra , hope all is well, just wanted to circle back here.

1) I believe you were going to submit a new PR after the initial run was too slow. How is the status of that PR at the moment? 2) Any chance you were able to figure out how to handle multiple self joins within dbt? I think that is a blocker for this current solution as well.

jmongerlyra commented 2 weeks ago

@fivetran-avinash Thanks for the follow up. Here's the current status of our fork against Fivetran main. https://github.com/fivetran/dbt_netsuite/compare/main...jmongerlyra:dbt_netsuite:main

1)https://github.com/fivetran/dbt_netsuite/pull/107#discussion_r1518580302 after the initial run was too slow. How is the status of that PR at the moment?

This is addressed in our fork. The issue is vendor and customer data lives in various fields depending on the transaction type. See here for customers.

2) Any chance you were able to figure out how to handle https://github.com/fivetran/dbt_netsuite/pull/107#discussion_r1525433814? I think that is a blocker for this current solution as well.

We are still using the recursive CTE since that works in Snowflake. I can submit a new PR and you can strip out the full_name fields that depend on these joins? That would at least address the customer and vendor issue.

We also have some minor bugs fixes in our fork for account numbers and is leftside values on Retained Earnings and CTA.

jmongerlyra commented 2 weeks ago

@fivetran-avinash I went ahead and removed the recursive CTEs. See PR here to address this bug. https://github.com/fivetran/dbt_netsuite/pull/131

fivetran-avinash commented 1 week ago

Hey @jmongerlyra, thanks for raising this new PR last week! I can see you put a lot of thought and care into creating this out.

This is quite a hefty PR to validate in its current state. Would it be possible for you to break up this PR into two separate PRs so we can better review and validate the changes for each:

First PR: The customer/vendor join updates in transaction details that were mentioned in this issue. We can also include the income statement update since it's quite small. We can move it into an upcoming sprint and prioritize this for release.
Second PR: All other changes, primarily on balance sheet, since there are some complexities we'd like to address and it might take a little longer for us to confirm that your updates are the right approach to take.

Particularly around the second PR, we'd love if you could open up a new issue detailing the balance sheet updates, particularly regarding

the account_number fixes for Retained Earnings and CTA
the is_account_leftside fixes

Right now, it's not quite clear in this issue why these changes are being introduced. An issue with supporting details for why these changes need to be made will make it easier for us to accelerate development. Then this will help us validate that your solution is the right one for all customers to use in a faster and more timely fashion.

If you can open up that issue and separate out the PRs, then I think we will be ready to roll out the first PR ASAP and then make sure the second PR is addressed in a very near sprint.

Hope this approach makes sense! Let me know if you have any questions, and thanks for your patience here.

jmongerlyra commented 1 week ago

@fivetran-avinash Definitely. I have removed updates from the branch that were unrelated to the customer & vendor join issue. This should make it easier to review. Let me know if you have any questions.

fivetran-avinash commented 3 days ago

Thanks so much @jmongerlyra for splitting everything out!

I have some comments in the other issue you created, but I believe the plan is to bring this issue into a coming sprint so we can close it out very shortly! We will be in touch on this issue when we begin working on validating this bug fix.

fivetran / dbt_netsuite