-
Notifications
You must be signed in to change notification settings - Fork 16
Update hubspot users and owners configurations #1091
Conversation
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.
This addition of user endpoints look good. But we are missing some things here:
- We need to add assertions to verify results of both user access and erasure endpoints. Please take a look at assertions of other endpoints in file test_hubspot_task.py
- We need to create some test data for user erasure endpoint, please have a look at function hubspot_erasure_data in hubspot_fixtures.py file and observe contact creation for our hubspot_erasure_identity_email
Let me know if you need any clarification/ help in understanding this.
Thanks, that all makes sense! I had alot of difficulties with the integration tests because I had created the new private app key in a different account than the current api key. It's unfortunate because I was under the impression it was the same account so it was really confusing. Should be able to get this working now! |
@HamzaWaseemOnBench I made sure integration tests cover the new cases now |
There's still some things we need for this PR to be merged. If we want to use a new Hubspot account then we need to do the following to get integration tests to work:
other todos:
|
@galvana @HamzaWaseemOnBench This should be ready for review. I ended up making the erasure request delete users as there was no data to mask. Let me know your thoughts! |
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.
Everything is working as expected! I linked two other issues that are resolved by these changes. The requested changes are more around testing preferences that have been established after this connector was developed.
@galvana I removed #992 from the fixes as I think there is still some timing issues where these tests can fail. It's hard to reproduce but I think the reason is that the search endpoints are eventually consistent. Even though we use |
The changes Hamza requested have been addressed
"updatedAt", | ||
"firstName", | ||
"lastName", | ||
} |
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.
Since we're adding new assertions here, please consider asserting them in this style rather than looking for an exact match — hopefully we'd see less random failures moving forward.
Purpose
Changes
Checklist
CHANGELOG.md
fileCHANGELOG.md
file is being appended toUnreleased
section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checks
label has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #901 #361 #878