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: extend Contacts schema with opted_out_subscription_types #22095

Merged
merged 6 commits into from
Mar 15, 2023

Conversation

KayakinKoder
Copy link
Contributor

@KayakinKoder KayakinKoder commented Jan 30, 2023

What

Resolves #22043

How

  • adds opted_out_subscription_types property to Contacts

Recommended reading order

NA

🚨 User Impact 🚨

Since this adds a new property, should not affect existing usages

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

⚠️ an airbyter should run and verify test results; they require a specific Intercom test account setup which I did not have.

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@CLAassistant
Copy link

CLAassistant commented Jan 30, 2023

CLA assistant check
All committers have signed the CLA.

| 0.1.30 | 2023-01-27 | [22010](https://github.com/airbytehq/airbyte/pull/22010) | Set `AvailabilityStrategy` for streams explicitly to `None` |
| 0.1.29 | 2022-10-31 | [18681](https://github.com/airbytehq/airbyte/pull/18681) | Define correct version for airbyte-cdk~=0.2 |
|:--------|:-----------| :------------------------------------------------------- |:----------------------------------------------------------------------------------------------|
| 0.1.31 | 2023-01-30 | [22095](https://github.com/airbytehq/airbyte/pull/22095) | Extended `Contacts` scehma adding `opted_out_subscription_types` property |
Copy link
Contributor

Choose a reason for hiding this comment

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

Hint: I just found a typo, should be: schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, all set

Copy link
Contributor

@juweins juweins left a comment

Choose a reason for hiding this comment

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

@KayakinKoder Thank you for your improvement!

Looking at the relevant Intercom documentation this addition LGTM. 👍🏼

I found a typo in the changelog. Can you please correct that in your PR?

Edit:
As per your comment in #22095 , the mandatory tests should be run. @sajarin

@KayakinKoder
Copy link
Contributor Author

unit_tests

Unit tests pass. @sajarin anything else I can do/any chance you could take a look at this? I'd like to update the Intercom api version, add the remaining missing props, and add tests in some other PRs after this one is merged. Thank you

@YowanR
Copy link
Contributor

YowanR commented Feb 16, 2023

@bazarnov Could you take a look at this, please?

Copy link
Collaborator

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

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

@YowanR I'm approving this from a technical perspective it looks good and safe as for GA connector, but I cannot test this change out using our Integration test account, since these properties are probably part of some kind of paid plan or so, not sure, but we don't have this fields populated for us.

@@ -270,6 +270,40 @@
"type": ["null", "boolean"]
}
}
},
"opted_out_subscription_types": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make the same thing but for "opted_in_subscription_types" as well, once we add the opt_out we should add the opt_in, especially if it has the same structure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, but can't do that until we update to Intercom API version 2.6 when that prop was added: https://developers.intercom.com/intercom-api-reference/v2.6/reference/changelog

Currently we're at 2.5, and I would not recommend trying to update version in this PR. In a near future PR I'll update to the latest version, 2.8, but there's one potential breaking change that Intercom notes in 2.7

Along with updating to 2.8 in that next PR, I'll attempt to add all the additional props + test coverage for them. Just wanted to get this PR through because 1. is my first time getting to know the codebase and 2. having this specific prop will really help my team :)

@bazarnov
Copy link
Collaborator

@juweins @marcosmarxm Are you able to assist with further actions here?

@marcosmarxm
Copy link
Member

@KayakinKoder right now Airbyte is in code freeze because of the monorepo split project to move platform code to another repo. After the period ends I'll publish your contribution and merge the code. Thanks for the contribution

@KayakinKoder KayakinKoder force-pushed the source-intercom-subscript branch from a29302a to 4843719 Compare February 27, 2023 22:48
@sh4sh sh4sh mentioned this pull request Mar 6, 2023
@sajarin
Copy link
Contributor

sajarin commented Mar 7, 2023

hey @KayakinKoder thanks for being patient, I'll take a look at this today and provide an update either today or tomorrow at the latest. Thanks for being patient, we just have to test the connector.

@KayakinKoder
Copy link
Contributor Author

@sajarin anything I can do? Our marketing team keeps pinging me about this, sorry to keep pinging you in return

@marcosmarxm marcosmarxm added the contributor-program PRs submitted through the contributor program. label Mar 15, 2023
@marcosmarxm marcosmarxm merged commit 6bc96b5 into airbytehq:master Mar 15, 2023
adriennevermorel pushed a commit to adriennevermorel/airbyte that referenced this pull request Mar 17, 2023
…n_types` (airbytehq#22095)

* new contacts prop, opted_out_subscription_types

* bump intercom version

* update changelog

* fix typo in changelog

* update connectors.md

---------

Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
erohmensing pushed a commit that referenced this pull request Mar 22, 2023
…n_types` (#22095)

* new contacts prop, opted_out_subscription_types

* bump intercom version

* update changelog

* fix typo in changelog

* update connectors.md

---------

Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
erohmensing pushed a commit that referenced this pull request Mar 22, 2023
…n_types` (#22095)

* new contacts prop, opted_out_subscription_types

* bump intercom version

* update changelog

* fix typo in changelog

* update connectors.md

---------

Co-authored-by: sh4sh <6833405+sh4sh@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation bounty community connectors/source/intercom contributor-program PRs submitted through the contributor program.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Intercom: extend Contacts schema with opted_out_subscription_types
9 participants