-
Notifications
You must be signed in to change notification settings - Fork 95
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
Tdl 24434 custom object support #242
Conversation
154943a
to
595abe6
Compare
tap_hubspot/__init__.py
Outdated
|
||
if not data.get(more_key): | ||
break | ||
params['after'] = data.get(more_key).get('next').get('after') |
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.
Chains of .get()
calls are not as safe as you think.
Consider the following scenarios. Which ones print and which ones error?
A.
data = {}
more_key = "paging"
print(data.get(more_key).get('next').get('after'))
B.
data = {"paging": {}}
more_key = "paging"
print(data.get(more_key).get('next').get('after'))
C.
data = {"paging": {"next": {}}}
more_key = "paging"
print(data.get(more_key).get('next').get('after'))
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.
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.
Attribute error for options A and B. Option C gives us the default value as None.
In this case, I think I can modify as below -
params['after'] = data.get(more_key, {}).get('next', {}).get('after', None)
if params['after'] is None:
break
8efb496
to
28ba295
Compare
tap_hubspot/__init__.py
Outdated
if params['after'] is None: | ||
break | ||
|
||
def sync_records(stream_id, primary_key, bookmark_key, catalog, STATE, params, is_custom_object=False): |
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.
Can we rename this to be a little more obvious that this pertains to custom objects? Looks like all the other sync_... functions specify the stream for clarity, and this reads right now as it handles syncing all records regardless of stream but is only called in sync_custom_object_records
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.
I am planning to create one more refactoring branch in which sync_records will be used by tickets stream as well. Therefore, thought of keeping the generic name.
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.
I think that until that gets done, we should keep the name more explicit. We can always change it back when the refactor happens and then keep the function obvious in the event that the refactor gets delayed or deprioritized
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.
done
…bjects with solid analytical info.
* initial commit to add support for custom objects * modify the endpoint for custom records * functionality for the discover mode of custom streams * add custom_objects schema * add sync functionality for custom_object records * create shared folder for the re-use of schema * Log a warning for custom object scopes, handling exceptions. * add doc string to definitions * remove custom_object schema stream, as it just the schema of custom objects with solid analytical info. * modifying naming conventions * include shared folder in the package data * include shared json file names * modified package data in setup.py * update manifest file with the shared folder * include custom objects stream for testing all the scenarios * correct the misspelled * make pylint happy * fix the errors * take the recent startdate, as the custom objects are created 2 days before. * remove `custom_` prefix from the name of custom object stream * remove unused code * Update start dates due to the absence of custom object creation in between. * update test client with pagination logic * fix all fields test for deals stream * update STATE assignment post custom object sync * correct variable name
6526a67
to
0be3b79
Compare
Description of change
Enhancing tap with support for custom CRM objects. Here's a summary of the key developments and considerations:
Custom Object Handling
Tables for custom objects will be prefixed with "custom_" to distinguish them from standard objects in the backend. For example, if a customer has a custom object named "cars," the corresponding table in Stitch UI will be named "custom_cars."To prevent confusion for customers, the custom object "cars" will be displayed simply as "cars" in the Stitch UI.
Field Appearance
Stitch will support both default and custom properties. To streamline user selection, all properties will be raised to the top level. For instance, if a custom object "cars" includes a custom property "model," it will be displayed as "property_model."
Associations
Expanded support for associations of custom objects with various objects, including 'emails, meetings, notes, tasks, calls, conversations, contacts, companies, deals, tickets.' Additionally, a support ticket has been raised with HubSpot to explore the feasibility of fetching associations between two custom objects.
Manual QA steps
Risks
Rollback steps