Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Merged deals #135
Merged deals #135
Changes from 9 commits
029f7eb
c90839d
3afe3fb
406d4a3
24bff3e
5be9c9c
d4b39a0
3450a24
46cfed1
0bc1eb8
cdfef06
2ba80e6
568bcae
6f70dd1
6cff115
5a9f70a
d5b611c
c0bd2e6
6b229d7
8e6721b
1a1fbeb
3c930e8
d13442f
80edb49
5a39f63
603204d
b157c08
5be67aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, following the conversation from the source PR we may want to add a conditional to the merged deal components if we do decide to leverage a variable to enable/disable these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included
hubspot_merged_deal_enabled
configThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? I don't imagine the
merged_deals
table will contain alldeal_id
s and then if it has no merged deals then it will benull
for themerged_deal_id
column. I would assume this table is only populated with deals that have been merged. Therefore, if a deal has not been merged I would assume it would not be present in this table.If the above is the case then this logic would not be accurate for filtering out merged deals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking at the underlying data I see that there do in fact seem to be more deals in the
deal
table than exist in themerged_deal
table. Given this, I believe this logic needs to be adjusted to accurately filter out merged deals.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is filtering out the
null
records the correct way to filter out merged accounts? Wouldn't we want to cross reference which deals are present in themerged_deal_id
column and then when there are any matches that is what we filter out?Let me know if I am missing a component, but I am not sure if filtering where the merged_deal_id is null is the right way to achieve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh goodness yes I confused how I was looking at the table-- right not all deals exist in the
merged_deals
. Therefore to correctly filter out deals in the final models that have been merged, we would need to do a check for ifdeal_id
is amerged_deal_id
.I updated it to the following:
where deals.deal_id not in (select merged_deal_id from merged_deals)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to update before merge