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

source-intercom-native: add tickets stream and reduce logging #2281

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

Alex-Bair
Copy link
Member

@Alex-Bair Alex-Bair commented Jan 16, 2025

Description:

This PR's scope includes:

  • Adding the tickets stream.
    • The tickets stream is extremely similar to the conversations stream. Their /{resource}/search endpoints are queried and behave in the same way, so a good chunk of this PR was copy + paste of conversations, then renaming & refactoring.
  • Updating config.yaml to not use /companies/scroll for snapshot testing to avoid possible intermittent failures when running tests.
  • Reducing frequency of repetitive logs like "Processing page 1 of 1` when streams are caught up to the present.

Snapshot changes are expected due to the addition of the tickets stream.

Our production Intercom OAuth app will need updated to request the "Read and List tickets" permission since the connector needs it to successfully make calls to /tickets/search. After the OAuth app is updated, any existing tasks will need to be reauthenticated if they want to use the tickets stream.

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

Documentation should be updated to reflect the added stream. We may also want to list what scopes are required if users authenticate with access tokens. Based on Intercom's docs, it sounds like scope/permissions are only applicable to OAuth apps, not access tokens.

Notes for reviewers:

Tested on a local stack. Confirmed:

  • the tickets stream captures data & behaves like the conversations stream.
  • the logs around page numbers are only logged when a stream is backfilling / has a not small amount of data to process.

This change is Reviewable

The `tickets` stream is extremely similar to the `conversations` stream;
their search endpoints are queried and behave in the same way, so most
of this commit was copy + paste of `conversations`, then renaming &
refactoring.
Only a single "scroll" can be ongoing at a given time, so it's possible
(albeit unlikely) that the capture snapshot test could fail if two
applications were running the capture snapshot test.
The connector was logging out `Processing page 1 of 1` frequently when
streams were caught up to the present & capturing incrementally. This
connector has had sufficient usage that I feel comfortable removing
these logs when a stream is caught up, so I limited the page number
logging to only happen when a stream is backfilling (i.e. has multiple
pages to process or has more than an hour of results to page through).
@Alex-Bair Alex-Bair force-pushed the bair/source-intercom-native-tickets-stream branch from 373abc3 to 43a7e18 Compare January 17, 2025 18:46
@Alex-Bair Alex-Bair changed the title source-intercom-native: add tickets stream source-intercom-native: add tickets stream and reduce logging Jan 17, 2025
@Alex-Bair Alex-Bair marked this pull request as ready for review January 17, 2025 18:55
@Alex-Bair Alex-Bair added the change:unplanned Unplanned change, useful for things like doc updates label Jan 17, 2025
Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

LGTM

source-intercom-native/source_intercom_native/api.py Outdated Show resolved Hide resolved
@Alex-Bair Alex-Bair force-pushed the bair/source-intercom-native-tickets-stream branch from 4662836 to 09bf899 Compare January 17, 2025 20:45
@Alex-Bair
Copy link
Member Author

I'm holding off on merging until our production Intercom OAuth app has been updated to request the permissions needed for the tickets stream.

@Alex-Bair
Copy link
Member Author

The "Read and List Tickets" permission has been requested for our OAuth app, so I'm going to merge this PR now. If the permission isn't granted until after Intercom's review is complete, we can disable the tickets stream for captures until then.

@Alex-Bair Alex-Bair merged commit ff89100 into main Jan 22, 2025
75 of 82 checks passed
@Alex-Bair Alex-Bair deleted the bair/source-intercom-native-tickets-stream branch January 22, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:unplanned Unplanned change, useful for things like doc updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants