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

[WIP] feat: add crm/contracts for pipedrive #372

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mladibejn
Copy link
Contributor

@mladibejn mladibejn commented Mar 15, 2023

Description

This PR adds Pipedrive provider and impl. of use cases for crm/contracts profile

  • Search
  • Create (with Org create if not existant)
  • TBD - Update

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING document.
  • I haven't repeated the code. (DRY)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Contributor

@tmladek tmladek left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for the update on the progress!

I mostly have just nitpicks - and one issue that's a bit more important.

superface.run({
useCase: 'Create',
input: {
email: '[email protected]',
Copy link
Contributor

Choose a reason for hiding this comment

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

It's best practice to use example.com or example.org for such purposes; that way, you can also avoid some potential spam on your domain 😅


return {
title = "Unknown error"
detail = `Unknown error occurred. Status: ${statusCode}. IPData provider error info: ${detail}.`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's IPData?

}
}

operation GetOrCreateOrganizatio {
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
operation GetOrCreateOrganizatio {
operation GetOrCreateOrganization {


operation GetOrCreateOrganizatio {

data = call GetOrganizationByName(input = args.input)
Copy link
Contributor

Choose a reason for hiding this comment

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

This way of calling ends up silently throwing away the error if the operation call fails, see:

https://superface.ai/docs/comlink/reference/map#calling-operation

This may be useful in some cases - but here it may result in e.g. creating duplicate organizations, in case the operation falls intermittently, or due to some other reason.

It'd be best to use the "Calling with a handler" syntax (documented in the link above)

@@ -57,3 +57,5 @@ superface/grid

# Superface compiled capabilities
grid/**/*.ast.json

.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably shouldn't be a part of this PR? :)

(Also, I assume you're using WebStorm for the TypeScript part?)

body = {
name: [input.firstName, input.lastName].join(" "),
email: [{
label: "",
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
label: "",

Please note that only value is required.

Probably best to remove, if there's no reason to keep it


}],
phone: [{
label: "",
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
label: "",

Ditto

}
}

organization = call GetOrCreateOrganizatio(input = input)
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
organization = call GetOrCreateOrganizatio(input = input)
organization = call GetOrCreateOrganization(input = input)

Copy link
Contributor

Choose a reason for hiding this comment

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

This ends up silently throwing away the error, see https://superface.ai/docs/comlink/reference/map#calling-operation

}
}

// https://developers.pipedrive.com/docs/api/v1/Persons#searchPersons
Copy link
Contributor

Choose a reason for hiding this comment

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

Once again, ❤️ for linking the docs :)


request "application/json" {
query {
// operator does not exist
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
// operator does not exist
// PipeDrive doesn't support search operators

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.

2 participants