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

Hubspot 'main' PR #10

Merged
merged 116 commits into from
Jul 6, 2023
Merged

Hubspot 'main' PR #10

merged 116 commits into from
Jul 6, 2023

Conversation

NeilGorman104
Copy link
Collaborator

@NeilGorman104 NeilGorman104 commented Jun 15, 2023

Hubspot main PR:

  1. Adds 22 Hubspot streams & inline schemas
  2. Adds Hubspot Auth methods
  3. Updates to Pagination method
  4. Updates to Readme
  5. Updates connector naming convention to remove "-sdk"
  6. Following checklist made in standards for building a new connector

Update: We have added the following ‘objects’ streams which previously were covered only in the properties stream:

  • CompanyStream
  • DealStream
  • FeedbackSubmissionsStream
  • LineItemStream
  • ProductStream
  • TicketStream
  • QuoteStream
  • GoalStream
  • CallStream
  • CommunicationStream
  • EmailStream
  • MeetingStream
  • NoteStream
  • PostalMailStream
  • TaskStream

Note: Pytests are not passing for the following reasons:

  1. Our limited Hubspot test account can't access the permissions required to access the following endpoints:
  • Feedback_submissions
  • Quotes
  • Goals
  1. We are getting the following error: AssertionError: No records returned in stream for each of the streams where we have no data in our Hubspot test account

Copy link
Member

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

Hey folks!

Can you sync the poetry.lock file? That'll save some time not resolving dependencies in CI.

Copy link
Member

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

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

I don't wanna leave a comment for each stream class 😅, but I think we can replace parse_response with records_jsonpath.

@NeilGorman104
Copy link
Collaborator Author

Hey @edgarrmondragon, we have removed the parse_response function in favor of records_jsonpath, and have also added the Poetry.lock file in repo so tests finish quickly.

@edgarrmondragon
Copy link
Member

Thanks for your patience @NeilGorman104, this LGTM!

Copy link
Collaborator

@pnadolny13 pnadolny13 left a comment

Choose a reason for hiding this comment

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

@NeilGorman104 NeilGorman104 merged commit 0c1805f into main Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants