[Feature] Add employees model, timestamp fixes, add common fields #57

fivetran-avinash · 2024-09-16T09:29:31Z

PR Overview

This PR will address the following Issue/Feature: [#55]

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

We changed the data type from timestamp to date for some fields to fix some timezone conversion issues.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Breaking Changes

Casted specific timestamp fields across all staging models as dates where the Netsuite UI does not perform timezone conversion. Keeping these fields as timestamp causes issues in reporting tools that perform automatic timezone conversion.
As this will change the datatype of the underlying fields, this will require a --full-refresh.

Feature Updates

We introduced the stg_netsuite2__employees model to bring in data from the employee source table. This was brought in to leverage fields like first_name, last_name and supervisor in downstream models in the dbt_netsuite transformation package.
Adds additional commonly used fields to accounts, subsidiaries, transaction_lines, transactions, transaction_accounting_lines, customers, and vendors within the Netsuite2 staging models.

Under the Hood

Created new seed data in integration_tests to support the new stg_netsuite2__employees model, as well as the new fields introduced into the new Netsuite2 staging models.

Contributors

@jmongerlyra (PR #54)

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
[NA] dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

The new staging employees model is successfully being passed through and new data types are being applied for timestamp->date conversions. (validated this with integration_tests data).

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

🩺

Add employees model, fix timestamps, and add common fields

fivetran-joemarkiewicz

@fivetran-avinash thanks for coordinating with @jmongerlyra to push these changes forward!

I was able to perform and initial review and have a few comments and callouts included in my review below. One major note that jumped out at me is to confirm the usage of the employee table. I want to make sure we aren't adding a table to be required if we don't see all customers syncing this table. Should we consider adding a variable? I haven't done too much of a deep dive here, but I would like to evaluate if it makes sense to require for all customers or if we should offer more flexibility.

CHANGELOG.md

README.md

fivetran-joemarkiewicz · 2024-09-16T20:41:01Z

docs/run_results.json

Let's remove this file since it's not needed.

models/netsuite2/stg_netsuite2__accounts.sql

models/netsuite2/stg_netsuite2__accounting_periods.sql

models/netsuite2/stg_netsuite2__transactions.sql

fivetran-avinash

@fivetran-joemarkiewicz Thanks from some help from @jmongerlyra, this PR is ready for re-review.

fivetran-joemarkiewicz

@fivetran-avinash thanks for working on these updates and @jmongerlyra for chiming in and sharing your insights!

Overall these changes look good! I just have a few final notes to address before this should be good to go! Also, be sure to add the employee disable/enable details in the respective downstream PR #138.

fivetran-joemarkiewicz · 2024-09-17T22:11:12Z

CHANGELOG.md

+
+## Breaking Changes 
+- Casted specific timestamp fields across all staging models as dates where the Netsuite UI does not perform timezone conversion. Keeping these fields as type timestamp causes issues in reporting tools that perform automatic timezone conversion. 
+  - As this will change the datatype of the underlying fields, this will require a  `--full-refresh`.


This really won't require a full refresh for the staging models. However, it would be for the downstream ones. Let's clarify that.

Suggested change

- As this will change the datatype of the underlying fields, this will require a `--full-refresh`.

- As this will change the datatype of the underlying fields, this will require a `--full-refresh` for downstream incremental models.

fivetran-joemarkiewicz · 2024-09-17T22:12:30Z

CHANGELOG.md

+
+## Feature Updates
+- Introduced the `stg_netsuite2__employees` model to bring in data from the `employee` source table. This was brought in to leverage fields like `first_name`, `last_name` and `supervisor` in downstream models in the `dbt_netsuite` transformation package. 
+  - Since this model is only used by a subset of customers, we've introduced the variable `netsuite2__using_employees` to allow users who don't utilize the `employee` table in Netsuite2 the ability to disable that functionality within your `dbt_project.yml`.


Can you link to the README section to show users how to disable if they are interested.

It actually looks like this wasn't documented in the README. Would you be able to include this in the same section as the other disable/enable variables.

Didn't save the file the first time so it didn't commit. It's in now.

fivetran-avinash

@fivetran-joemarkiewicz This is ready for re-review.

@jmongerlyra I did notice that it was mentioned in the feature that employees fields will be used in downstream models eventually. However, when reviewing the current PR and the other PR you've created, I didn't see any references to employees being brought in yet.

For now, I removed the references from the CHANGELOG, but was curious how would you eventually bring employees fields into the dbt_netsuite transformation package? Would love to learn more!

jmongerlyra · 2024-09-18T00:55:03Z

@fivetran-avinash We do the joins in a production layer downstream of these models to the transaction dataset.

fivetran-avinash · 2024-09-18T00:57:26Z

Thanks @jmongerlyra ! So there are no plans to bring the employees data into dbt_netsuite, just leverage it from the source, and join that information in off the end models independent of the package?

jmongerlyra · 2024-09-18T01:02:01Z

@fivetran-avinash No immediate plans, but we could since you all have already parameterized the usage.

fivetran-joemarkiewicz

Thanks for making these changes @fivetran-avinash and helping address my questions along the way @jmongerlyra! This PR is looking to be in a good position! I have one last required change before this will be good to merge, but it should be a quick one.

Additionally, since the employee model is not used at all downstream at the moment I am a bit cautious/curious if it will impact Quickstart at all (that being the manifest believes it's an end model since it has no downstream dependencies). @fivetran-avinash let's confirm this with our internal team if this will be an issue. If we know it won't then this should be good to go! If it will be an issue, then we can likely avoid it by simply having the default be false until we decide to use it downstream. Hopefully the addition of the downstream public_models config ensures this won't be an issue.

fivetran-joemarkiewicz · 2024-09-18T13:12:28Z

models/netsuite2/src_netsuite2.yml

+        description: "{{ doc('employee_table') }}"
+        columns:


Apologies I didn't catch this after the addition of the netsuite2__using_employees variable, but we need to add a config here to allow the disablement of the employee source if it isn't being used.

Suggested change

description: "{{ doc('employee_table') }}"

columns:

description: "{{ doc('employee_table') }}"

config:

enabled: "{{ var('netsuite2__using_employees', true) }}"

columns:

fivetran-avinash

@fivetran-joemarkiewicz Review comments addressed. Based on Maxwell's response hopefully we are good to go!

fivetran-joemarkiewicz

LGTM!

CHANGELOG.md

fivetran-avinash

@fivetran-jamie Release notes addressed!

fivetran-jamie

lgtm!

jmongerlyra and others added 6 commits August 18, 2024 06:36

Add employees model, fix timestamps, and add common fields

57529fe

Merge pull request #54 from jmongerlyra/main

d978e95

Add employees model, fix timestamps, and add common fields

update seeds

b882944

fix identifier

8a5dab7

fix identifier

cd904f5

Documentation, seeds, CHANGELOG, REAADME

7aa94c8

fivetran-avinash self-assigned this Sep 16, 2024

fivetran-avinash changed the title ~~Feature/add employees model timestamp fixes common fields~~ [Feature] Add employees model, timestamp fixes, add common fields Sep 16, 2024

fivetran-joemarkiewicz self-requested a review September 16, 2024 20:23

fivetran-joemarkiewicz requested changes Sep 16, 2024

View reviewed changes

fivetran-avinash added 3 commits September 17, 2024 01:58

PR fixes

f532ec8

PR fixes

7652a17

bump schema

951f0c8

fivetran-avinash commented Sep 17, 2024

View reviewed changes

fivetran-joemarkiewicz requested changes Sep 17, 2024

View reviewed changes

PR re-review

fb3d4fd

fivetran-avinash commented Sep 18, 2024

View reviewed changes

fivetran-joemarkiewicz requested changes Sep 18, 2024

View reviewed changes

fivetran-avinash added 2 commits September 18, 2024 08:13

Add config

c609675

update run_models.sh

b78c8c1

fivetran-avinash commented Sep 18, 2024

View reviewed changes

fivetran-joemarkiewicz approved these changes Sep 18, 2024

View reviewed changes

fivetran-jamie reviewed Sep 19, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

release review

8f8a863

fivetran-avinash commented Sep 19, 2024

View reviewed changes

fivetran-jamie approved these changes Sep 23, 2024

View reviewed changes

fivetran-avinash merged commit bd22af7 into main Sep 25, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add employees model, timestamp fixes, add common fields #57

[Feature] Add employees model, timestamp fixes, add common fields #57

fivetran-avinash commented Sep 16, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Sep 16, 2024

fivetran-avinash Sep 17, 2024 •

edited

Loading

fivetran-avinash left a comment •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Sep 17, 2024

fivetran-avinash Sep 18, 2024

fivetran-joemarkiewicz Sep 17, 2024

fivetran-joemarkiewicz Sep 17, 2024

fivetran-avinash Sep 18, 2024 •

edited

Loading

fivetran-avinash left a comment

jmongerlyra commented Sep 18, 2024

fivetran-avinash commented Sep 18, 2024 •

edited

Loading

jmongerlyra commented Sep 18, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Sep 18, 2024

fivetran-avinash left a comment

fivetran-joemarkiewicz left a comment

fivetran-avinash left a comment

fivetran-jamie left a comment

	- As this will change the datatype of the underlying fields, this will require a `--full-refresh`.
	- As this will change the datatype of the underlying fields, this will require a `--full-refresh` for downstream incremental models.

[Feature] Add employees model, timestamp fixes, add common fields #57

[Feature] Add employees model, timestamp fixes, add common fields #57

Conversation

fivetran-avinash commented Sep 16, 2024 • edited Loading

PR Overview

Breaking Changes

Feature Updates

Under the Hood

Contributors

PR Checklist

Basic Validation

Detailed Validation

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

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Sep 16, 2024

Choose a reason for hiding this comment

fivetran-avinash Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

fivetran-avinash left a comment • edited Loading

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Sep 17, 2024

Choose a reason for hiding this comment

fivetran-avinash Sep 18, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Sep 17, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Sep 17, 2024

Choose a reason for hiding this comment

fivetran-avinash Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

jmongerlyra commented Sep 18, 2024

fivetran-avinash commented Sep 18, 2024 • edited Loading

jmongerlyra commented Sep 18, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Sep 18, 2024

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-jamie left a comment

Choose a reason for hiding this comment

fivetran-avinash commented Sep 16, 2024 •

edited

Loading

fivetran-avinash Sep 17, 2024 •

edited

Loading

fivetran-avinash left a comment •

edited

Loading

fivetran-avinash Sep 18, 2024 •

edited

Loading

fivetran-avinash commented Sep 18, 2024 •

edited

Loading