fivetran / dbt_netsuite_source

Data models for Fivetran's Netsuite source package, built using dbt.
https://fivetran.github.io/dbt_netsuite_source/
Apache License 2.0
14 stars 20 forks source link

Bugfix/consolidate exch accounting book #36

Closed fivetran-reneeli closed 1 year ago

fivetran-reneeli commented 1 year ago

PR Overview

This PR will address the following Issue/Feature: https://github.com/fivetran/dbt_netsuite/issues/72

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

Non breaking as these changes are happening under the hood and will not impact downstream fields that customers see.

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR: Brought in accounting_book_id (accountingbook) to the stg_netsuite2__consolidated_exchange_rates model.

Details: This is responding to a customer issue seeing duplicate records were showing in end models transaction_details (and potentially balance_sheet too...).

The int_netsuite2__consolidated_exchange_rates model was the root cause for duplicates because the join:

    from consolidated_exchange_rates
    left join accounting_book_subsidiaries
        on consolidated_exchange_rates.to_subsidiary_id = accounting_book_subsidiaries.subsidiary_id

was resulting in multiple accounting_book_id 's. We should add accounting_book_id to the join. But --

We can remove int_netsuite2__consolidated_exchange_rates entirely now since that was to bring accounting_book_id in, and now we have that via the consolidated_exchange_rates staging model.

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 acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":

Standard Updates

Please acknowledge that your PR contains the following standard updates:

dbt Docs

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

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

:dancer:
fivetran-reneeli commented 1 year ago

regen docs after approve

fivetran-reneeli commented 1 year ago

Your changes look mostly good, I just had a suggestion to make some small changes in the CHANGELOG which look to be artifacts of the merge conflicts.

Additionally, I wanted to do a quick test to run this package version with the current live version of the dbt_netsuite package to make sure this field addition wouldn't cause any failures.... and surprisingly it does! image

This is because in the downstream intermediate model when we reference the accounting_book_id we do not specify the source table and therefore we get this ambiguous column error 😱. Therefore, we cannot make this be a patch upgrade. Instead we will want to make this a breaking change as well.

Can you make the appropriate breaking change version bump updates (don't forget to update the README version as well) and then let me know once those are applied for re-review.

@fivetran-joemarkiewicz Ahh.. Thanks for noticing that, definitely needs to be a breaking change then. Updated the versions, readme, and made a note in the changelog. I'll make the downstream updates as well.