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

feature/account-eliminate-field-add #39

Closed fivetran-joemarkiewicz closed 10 months ago

fivetran-joemarkiewicz commented 11 months ago

PR Overview

This PR will address the following Issue/Feature: dbt_netstuite Issue #87

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

This will result in a breaking change because if users are leveraging the accounts_pass_through_columns variable and passing through the eliminate field then it will fail due to a duplicate column unexpectedly. Therefore, we should make this a breaking change to account for these users.

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

This PR includes the addition of the eliminate field within the stg_netsuite2__accounts model. This new field is necessary to properly categories inter-company accounts in downstream calculations.

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":

I was able to confirm the eliminate field was being pulled through to the stg_netsuite__accounts model as is_eliminate and was showing as a boolean that may be leveraged in downstream transformations.

image

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?

🏎️
fivetran-catfritz commented 10 months ago

@fivetran-joemarkiewicz I had one question before I finish up, would it make sense to add the netsuite column is_included_in_elimination while we're making netsuite2 updates? I don't think it necessarily needs to be pulled into the transform, but I was wondering if we want to include it if we ever need it?

fivetran-joemarkiewicz commented 10 months ago

@fivetran-catfritz good question, we have recently only been supporting Nestuite2 in updates but I feel it would be worthwhile to sync as a team and align on if we should continue to support Netsuite1. For the time being I will keep it excluded from the staging model as it is not essential in the downstream logic for Netsuite1. However, we can reconsider this following further discussions tomorrow.

fivetran-joemarkiewicz commented 10 months ago

@fivetran-catfritz I just merged in and made some additional changes from PR #40 so they may be incorporated alongside these updates. Would you be able to give another review on these additions? If all looks good I will move forward with the release process.