fix(hubspot): simplify integration to only create contacts#7147
fix(hubspot): simplify integration to only create contacts#7147
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7147 +/- ##
========================================
Coverage 98.37% 98.38%
========================================
Files 1366 1366
Lines 51215 51081 -134
========================================
- Hits 50384 50257 -127
+ Misses 831 824 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
So I get the overall intent of this PR. Although there is a side-effect that feels critical to me -> We are breaking the synchronization between flagsmith and hubspot (even in read-only) and we might be unable to update the subscription to Hubspot.
My understanding with this code is that hubspot will be completely in control of creating the organisation AND the association of the contacts. That makes sense.
Right now, _get_organisation_hubspot_id is in charge of finding the organisation and creating the HubspotOrganisation record. It became dead code and we are never storing the information on our side.
It becomes a problem in update_company_active_subscription that is now a no-op (we don't have hubspot_id) and I feel using the domain or the name is too fragile.
My proposal
The fix should keep _get_organisation_hubspot_id in create_lead and drop the name= overwrite and keep writing flagsmith_organisation_id to HubSpot.
Optionally we can remove associate_contact_to_company to make our intent 100% clear and remove the associated tests (that just confirms it's never called right?).
Let me know what you think and i'll re-review with your thinking in mind.
If aligned, here is a final assist 😄
For Claude
1. In _get_organisation_hubspot_id, remove the name=organisation.name parameter from the self.client.update_company(...) call but keep hubspot_company_id and flagsmith_organisation_id.
2. In create_lead, keep the call to _get_organisation_hubspot_id but remove associate_contact_to_company and the early returns tied to it.
3. Update/remove tests that assert associate_contact_to_company behaviour.
4. Remove any now-unused imports (e.g. re2/re if domain filtering stays removed).
Holmus
changed the title
|
Thanks for the review @Zaimwa9 - good catch on update_company_active_subscription becoming a no-op. After thinking through it more, I went further than your proposal: I removed update_company_active_subscription entirely rather than fixing it. Why: We have a ChargeBee -> HubSpot sync that runs daily and writes subscription data (plan, MRR, billing status, contract dates) to HubSpot companies. It's the billing source of truth. Having the Flagsmith integration also write subscription data was: Redundant - ChargeBee sync already handles this The new commit removes update_company_active_subscription, its task, the AFTER_SAVE hook, and associated dead code (_get_organisation_hubspot_id, _get_hubspot_company_by_domain). |
Visual Regression16 screenshots compared. See report for details. |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Sorry to be annoying. Couple of NITs that we should take care of now:
- Code removal
- Unused variables
Then i'll be happy to approve assuming all ✅ on the functionality
|
|
||
| domain = user.email_domain | ||
|
|
||
| if settings.HUBSPOT_IGNORE_DOMAINS_REGEX and re.match( |
There was a problem hiding this comment.
Let's clean those HUBSPOT_XXX from the settings please
|
@Holmus do you want me to take over this one ? |
The HubSpot lead tracking integration was creating and updating companies from Flagsmith org data, overwriting enriched company names with user-entered org names. HubSpot already handles company creation and contact association automatically from email domains. Simplify create_lead to only create the contact. Remove domain filtering from should_track since HubSpot handles that natively. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The Flagsmith integration writing subscription/plan data to HubSpot was redundant (ChargeBee sync handles this daily) and caused confusion - trial subscriptions were being treated as paid accounts, overwriting correct data. - Remove update_company_active_subscription method and task - Remove AFTER_SAVE hook on Subscription.plan - Remove _get_organisation_hubspot_id (wrote org name + plan to HubSpot) - Remove _get_hubspot_company_by_domain (only used by above) - Remove associated tests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
for more information, see https://pre-commit.ci
Clean up unused HUBSPOT_IGNORE_* settings, remove the dead associate_contact_to_company client method and its test, and rename the existing-org test to reflect that associations are no longer created. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Holmus
force-pushed
the
fix/hubspot-let-hubspot-handle-companies
branch
from
60856fb to
d61fa0d
Compare
2 participants
The HubSpot lead tracking integration was doing three things:
Problems with (2): Flagsmith org names like "ew", "Test", "Manejo-Org" overwrote enriched company names in HubSpot.
Problems with (3): This was redundant with our ChargeBee -> HubSpot sync (which runs daily and is the billing source of truth). Worse, it caused data confusion - trial subscriptions written by the integration were treated as paid accounts, triggering workflows that incorrectly set company type to Customer.
Changes
create_leadto only create the contact - HubSpot handles company creation and association from email domainsshould_trackto just check if the feature is enabledupdate_company_active_subscriptionand its AFTER_SAVE hook - ChargeBee sync is the source of truth for subscription data_get_organisation_hubspot_idand_get_hubspot_company_by_domain(dead code after above)What still works
HubspotOrganisationmodel is unchanged (existing records preserved)Review effort: 1/5 (almost entirely deletions)