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

Pipedrive oAuth support #7968

Merged
merged 8 commits into from
Nov 15, 2021
Merged

Pipedrive oAuth support #7968

merged 8 commits into from
Nov 15, 2021

Conversation

avida
Copy link
Contributor

@avida avida commented Nov 15, 2021

Resolves #7512

Also reverts 0c41542 commit that was merged by mistake with #7325 and causes 422 response for oAuth requests (Chris working on another backend implementation).

@github-actions github-actions bot added area/connectors Connector related issues area/frontend area/platform issues related to the platform labels Nov 15, 2021
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Nov 15, 2021
@avida avida changed the title Drezchykov/pipedrive oauth Pipedrive oAuth support Nov 15, 2021
@avida avida requested review from eliziario, sherifnada, jamakase and ChristopheDuong and removed request for eliziario and sherifnada November 15, 2021 09:17
@avida avida temporarily deployed to more-secrets November 15, 2021 09:18 Inactive
Comment on lines 49 to 50
.put("airbyte/source-quickbooks", new QuickbooksOAuthFlow(configRepository, httpClient))
.put("airbyte/source-pipedrive", new PipeDriveOAuthFlow(configRepository, httpClient))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.put("airbyte/source-quickbooks", new QuickbooksOAuthFlow(configRepository, httpClient))
.put("airbyte/source-pipedrive", new PipeDriveOAuthFlow(configRepository, httpClient))
.put("airbyte/source-pipedrive", new PipeDriveOAuthFlow(configRepository, httpClient))
.put("airbyte/source-quickbooks", new QuickbooksOAuthFlow(configRepository, httpClient))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@avida avida temporarily deployed to more-secrets November 15, 2021 09:34 Inactive
@jamakase
Copy link
Contributor

@avida I have merged this not accidentally. We supposed that it will be much easier to support both formats and remove legacy one later.

Are you aware what needs to be fixed to get it working with those PR?

@avida
Copy link
Contributor Author

avida commented Nov 15, 2021

@avida I have merged this not accidentally. We supposed that it will be much easier to support both formats and remove legacy one later.

That was PR for temporary backend solution that is already abandoned. @ChristopheDuong is working on final solution so there is no sense of fixing it on backend side.

@@ -86,5 +81,13 @@
}
},
"supportsIncremental": true,
"supported_destination_sync_modes": ["append"]
"supported_destination_sync_modes": ["append"],
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is a source, is that normal to set destination modes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, missed that, will remove

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

public class PipeDriveOAuthFlowTest {
Copy link
Contributor

@ChristopheDuong ChristopheDuong Nov 15, 2021

Choose a reason for hiding this comment

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

Sorry, could you update your branch and make the unit test extends from BaseOAuthTestFlow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, nice refactoring, I love it

@avida
Copy link
Contributor Author

avida commented Nov 15, 2021

/publish connector=connectors/source-pipedrive

🕑 connectors/source-pipedrive https://github.com/airbytehq/airbyte/actions/runs/1462504917
❌ connectors/source-pipedrive https://github.com/airbytehq/airbyte/actions/runs/1462504917

@avida avida temporarily deployed to more-secrets November 15, 2021 14:04 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets November 15, 2021 14:05 Inactive
@avida
Copy link
Contributor Author

avida commented Nov 15, 2021

/publish connector=connectors/source-pipedrive

🕑 connectors/source-pipedrive https://github.com/airbytehq/airbyte/actions/runs/1462665179
✅ connectors/source-pipedrive https://github.com/airbytehq/airbyte/actions/runs/1462665179

@jrhizor jrhizor temporarily deployed to more-secrets November 15, 2021 14:43 Inactive
@avida avida merged commit 495202e into master Nov 15, 2021
@avida avida deleted the drezchykov/pipedrive-oauth branch November 15, 2021 14:51
@avida avida temporarily deployed to more-secrets November 15, 2021 14:52 Inactive
@lmossman
Copy link
Contributor

@avida It looks like this PR broke the platform build, as it removed lodash.pick from package.json, but did not remove it from package-lock.json, so now the build is failing with diffs in the package-lock.json file: https://github.com/airbytehq/airbyte/runs/4213123200?check_suite_focus=true#step:12:71

Could you submit a PR to fix the lock file please?

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 area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oAuth backend for pipedrive source
5 participants