Closed jmongerlyra closed 10 months ago
@jmongerlyra thanks for responding to all my comments. I am going to do a more thorough review on Monday.
I did want to share that we have made progress with the CTA and historical rates updates and are planning to roll those out next week. I will review your changes with your comments and consider if we can also fold some of these updates into the ones planned for next week Wednesday. However, I don't know if all the suggested changes here will be ready for release on Wednesday as we will want to do further testing of them on our own data.
Thanks again and I'll share more of my thoughts here next week! 😄
@jmongerlyra these changes are looking good on my end, but I do want to take some more time to validate them with our own internal finance team to ensure everything is tying out as expected.
As mentioned previously, we do have the existing PR #92 that addresses the intercompany, translation rate, and cta issues you previously mentioned and address in this PR. In order to get those updates out within our sprint timeline, I am going to push those forward for release. However, we will spend our upcoming sprint validating the changes from this PR with our own data to confirm they are appropriate for being integrated in the next release.
I appreciate you working with us on these changes. You are both really helping bring to light many nuances of Netsuite that we were not previously aware of and are helping make this package better for ourselves and the community!
@fivetran-joemarkiewicz Thanks for the update, and let me know if I can answer additional questions about the proposed changes.
@fivetran-joemarkiewicz Since this PR didn't make the last release, I added another bug fix. PR verbiage above has been updated. I am open to discussing the details and meeting in person.
New patch adds the to_subsidiary
dimension to all models which allows for consolidated reporting for all roll-ups rather than the root parent alone.
@fivetran-joemarkiewicz Since this PR didn't make the last release, I added another bug fix. PR verbiage above has been updated. I am open to discussing the details and meeting in person.
New patch adds the
to_subsidiary
dimension to all models which allows for consolidated reporting for all roll-ups rather than the root parent alone.
Thanks for sharing @jmongerlyra! Someone from my team will be picking up this initial bug and your PR in the current sprint (starting today). They will review your changes and will reach out if we have any questions on your changes and it is really helpful that you are open to meeting in person to help go through them as well. Thank you again for working so diligently with us. Your contributions have been invaluable!
@jmongerlyra Thanks again for walking us through all the changes. It was very very helpful and appreciated! What I'll do now is merge your branch into a new branch on our repo, so I can make those small edits without disturbing your branch. I'll also keep you posted for when we plan to release!
@fivetran-catfritz Great! Let me know if you need anything else from me.
Please provide your name and company Jared Monger, Lyra Health
Link the issue/feature request which this PR is meant to address https://github.com/fivetran/dbt_netsuite/issues/89 https://github.com/fivetran/dbt_netsuite/issues/93
Dependent on two related PRs in
dbt_netsuite_source
.Detail what changes this PR introduces and how this addresses the issue/feature request linked above. This PR addresses three issues.
Equity
rather thanCumulative Translation Adjustment
andRetained Earnings
respectively innetsuite2__balance_sheet.sql
to_subsidiary
) missing for all but root subsidiary on all models.The first issue is addressed by adding base book transactions onto the related adjustment book in
int_netsuite2__transaction_lines.sql
ifnetsuite2__multibook_accounting_enabled
is true. It then includesaccounting_book_id
in subsequent joins.The second issue is addressed by leveraging the field
sspecacct
. This field is not null for system generated accounts and is null for manually created accounts. CTA accounts containCTA-E
andCumulTransAdj
. Retained Earnings account containsRetEarnings
.The third issue is addressed by removing the filter on
consolidated_exchange_rates
and addingto_subsidiary
fields to all models. This allows consolidated financial reporting for eachto_subsidiary
in the appropriate functional currency.How did you validate the changes introduced within this PR? Lyra forked the repo and implemented/validated the changes against NetSuite reporting.
Which warehouse did you use to develop these changes? Snowflake
Did you update the CHANGELOG?
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)
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](https://www.surveymonkey.com/r/DQ7K7WW) on our existing dbt packages or what you'd like to see next. **PR Template** - [Community Pull Request Template](?expand=1&template=pull_request_template.md) (default) - [Maintainer Pull Request Template](?expand=1&template=maintainer_pull_request_template.md) (to be used by maintainers)