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

fixing bug for salesforce_user passthrough columns #32

Conversation

calder-holt
Copy link
Contributor

@calder-holt calder-holt commented Aug 10, 2022

Pull Request
Are you a current Fivetran customer?

Holt Calder, Data Engineering Manager, Greenhouse Software

What change(s) does this PR introduce?

Resolving bug in salesforce_contact_enhanced for user passthrough columns.

Did you update the CHANGELOG?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)
    Existing functionality was not working.

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)

  • Yes

Is this PR in response to a previously created Bug or Feature Request

How did you test the PR changes?

  • CircleCi
  • Local (please provide additional testing details below)

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

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 on our existing dbt packages or what you'd like to see next.

@fivetran-joemarkiewicz fivetran-joemarkiewicz added the bug Something isn't working label Aug 10, 2022
@calder-holt
Copy link
Contributor Author

@fivetran-joemarkiewicz any feedback on this PR? Would love to get it merged, we are having a recurring failure for this bug that is causing some unnecessary noise.

@fivetran-reneeli
Copy link
Contributor

Hey @calder-holt thank you so much for raising this. We're working on making these changes this current sprint!

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

@calder-holt Thank you so much for opening this PR! We really appreciate you taking the proactive steps to creating this.

For that one change you made and we suggested changes for, would you be able to make the same changes for the other passthrough variables in the final models so that the proper logic applies to all additional Salesforce fields?

Let me know if you have any questions or feedback!

CHANGELOG.md Show resolved Hide resolved
models/salesforce__contact_enhanced.sql Outdated Show resolved Hide resolved
@fivetran-avinash
Copy link
Contributor

Hey @calder-holt! Hope you had a great weekend. If you have the time, we'd love if you could apply the recommended changes--otherwise we can try and commit additional code up later in the week. Also happy to chat through it if you have any thoughts or questions. Cheers!

calder-holt and others added 2 commits August 23, 2022 10:23
Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>
@calder-holt
Copy link
Contributor Author

@fivetran-avinash I have committed the suggested changes, thanks!

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

Hi @calder-holt! Thanks for applying these changes!

Apologies for not being clearer--the change I suggested has to be applied to all the Salesforce models or they will also experience the same issues.

So for salesforce__contact_enhanced, you'd want to add those variable names in front of the other fields where {{ var('contact_pass_through_columns')}} exists, similar to what you did with salesforce_user.

Likewise, you'll want to do the same on salesforce__opportunity_line_item_enhanced.

Let me know if that makes sense! Happy to clarify further.

@@ -65,7 +65,7 @@ select
{% endif %}

{% if var('user_pass_through_columns',[]) != [] %}
, {{ var('user_pass_through_columns') | join (", user.")}}
, salesforce_user.{{ var('user_pass_through_columns') | join (", salesforce_user.")}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work with the changes!

Sorry for the confusion. To make sure we're accounting for all additional potential issues, you will also want to make the same changes for the other joins in line 58 (adding contact. in front of {{ var('contact_pass_through_columns')) and line 63 (adding account. in front of {{ var('contact_pass_through_columns')).

You will want to replicate these changes for salesforce__opportunity_line_item_enhanced model as well on the oli (on line 61) and product_2 (on line 66) joins as well!

@fivetran-avinash
Copy link
Contributor

Hey @calder-holt ! Just wanted to see if you were planning to work on the suggested changes today!

If time doesn't permit for you, we will apply them from our end later this afternoon. Thanks again for all your contributions!

@fivetran-avinash fivetran-avinash changed the base branch from main to customer/passthrough_fix August 24, 2022 21:53
Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

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

Thanks so much @calder-holt! I'll be merging your code into our PR where we'll make the final changes, and you should see the new release by this time tomorrow!

@fivetran-avinash fivetran-avinash merged commit 30565fa into fivetran:customer/passthrough_fix Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants