Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal stage updates #37

Merged
merged 16 commits into from
Apr 9, 2021
Merged

Deal stage updates #37

merged 16 commits into from
Apr 9, 2021

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

This PR includes the following updates:

  • Addition of the hubspot__deal_stage final model as requested within Issue Feature Request - Deal_stage modeling #30
  • Added README documentation around pass through fields and addition of the hubspot_deal_company_enabled variable
  • Postgres compatibility
  • Github pages docs update
  • Version upgrade to v0.3.1

README.md Outdated Show resolved Hide resolved
fivetran-joemarkiewicz and others added 2 commits April 2, 2021 12:26
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
description: The unique deal stage identifier.
tests:
- not_null
- unique
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this might not always pass since we're pulling in deal_company, which has a 1:n relationship

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we shouldn't bring the company into the deal_stages model, which is what we decided with hubspot__deals (and users can just join with deal_company and company using the staging models)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I forgot about that issue. That is a great point. I will remove the deal_company and refer the customer that they can simply join company in if they would like. Thanks!

Copy link

@coisnepe coisnepe Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @fivetran-jamie and @fivetran-joemarkiewicz,
I dug through the documentation of dbt_hubspot as well as dbt_hubspot_source's but saw nothing about "simply joining company" as per the last comment here. How would you go about including the company_id in hubspot_deals without adding a custom field on Hubspot or having to build a child table only to join deal_company?
Thanks for your help, and thanks for this incredible package!

Edit:
This is what I ended up doing! Works like a charm. Is this the kind of thing you had in mind when you wrote about "refering the customer" ? I feel like it could be a great addition to the README, and would be happy to open a PR if need be.

-- models/marts/facts/hubspot__deal_company.sql
{{ config(schema='hubspot') }}

SELECT * FROM {{ source('hubspot', 'deal_company') }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @coisnepe the deal_company model does exist within the source package as a staging model (stg_hubspot__deal_comapny). @fivetran-jamie was referring to using this staging model to join in the company information to the deal_stages model. If you leverage this, I believe would you achieve the same results.

Let me know if this works!

Copy link
Collaborator

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks mostly good! left a comment regarding the hubspot__deal_stages PK

fivetran-joemarkiewicz and others added 3 commits April 2, 2021 13:03
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
Copy link
Collaborator

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Contributor

@kristin-bagnall kristin-bagnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for you regarding modeling. All other files look good.

@@ -0,0 +1,63 @@
{{ config(enabled=fivetran_utils.enabled_vars(['hubspot_sales_enabled','hubspot_deal_enabled'])) }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought here ---

Why did you choose this approach, rather than joining the new deal_stage model to the final hubspot__deals model? Seems like we're doing a lot of duplicative work in this model, but I may be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right this is duplicative work. The only difference is the inclusion of the pipeline.is_active which is not in the hubspot__deals model. I will add that field to the hubspot__deals model to make this a cleaner model and remove duplicative code. Thanks!

@fivetran-joemarkiewicz
Copy link
Contributor Author

When updating the hubspot__deal_stages model to pull from hubspot__deals I remembered the reason for the previous modeling choice. I did not want to bring in the metrics within hubspot__deals as that would add confusion with the grain of husbpot__deal_stages being each stage within a deal. Additionally, explicitly selecting only relevant fields from the hubspot__deals model is not possible as this can vary for each user due to our pass through fields.

As such, to accommodate for this and produce cleaner code I decided to create an intermediate model int_hubspot__deals_enhanced which performed the duplicate deal joining, then used this model within hubspot__deals and hubspot__deal_stages.

Copy link
Contributor

@kristin-bagnall kristin-bagnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit b36cac5 into master Apr 9, 2021
@fivetran-joemarkiewicz fivetran-joemarkiewicz deleted the deal_stage-updates branch April 9, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants