Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Add Pipedrive Organizations Export #2556

Merged
merged 29 commits into from
Dec 3, 2021

Conversation

aelafifi
Copy link
Contributor

@aelafifi aelafifi commented Nov 18, 2021

Change description

  • Added Pipedrive export organizations destination (with needed tests for it).
  • Refactored the file structure and made plugin functionality more reusable.

Checklists

Development

  • Application changes have been tested appropriately

Impact

  • Code follows company security practices and guidelines
  • Security impact of change has been considered
  • Performance impact of change has been considered
  • Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
  • Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

Ahmed El-Afifi added 2 commits November 18, 2021 18:43
- Generalize common API calls.
- Add API calls for organizations.
Examples of code changes depending on that:
- `client.deletePerson()` -> `client.persons.delete()`
- `client.getAllPersonFields()` -> `client.persons.fields.getAll()`
- `client.getAllPersonFilters()` -> `client.persons.filters.getAll()`
Copy link
Member

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

Good start!

The next step would be to add tests for exporting Orginizations. We should re-orginize the test suite to have an export record test for both Profiles and Organizations. We use nock to actually record the the HTTP requests locally so we can ensure that the API is being used the way we expect.

Also a suggestion for the linter - if you are using VSCode, install the prettier plugin and your files will be auto-formatter on save.

Comment on lines 3 to 15
type Entity = 'persons' | 'organizations';
type EntityField = 'personFields' | 'organizationFields';
type FilterType = 'people' | 'org';

const EntityFieldMapping: Record<Entity, EntityField> = {
persons: "personFields",
organizations: "organizationFields",
};

const FilterTypeMapping: Record<Entity, FilterType> = {
persons: "people",
organizations: "org",
};
Copy link
Member

Choose a reason for hiding this comment

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

👍 really good typing!

Copy link
Member

@bleonard bleonard left a comment

Choose a reason for hiding this comment

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

Looking like a good start of re-use.

Copy link
Member

@bleonard bleonard 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!

plugins/@grouparoo/pipedrive/src/initializers/plugin.ts Outdated Show resolved Hide resolved
daterange: "date",

// TODO
time: null, // time in the format of "00:00:00"
Copy link
Member

Choose a reason for hiding this comment

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

@pauloouriques is mapping many of these to string. I think we can do that here too for time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By looking to this function

function formatVar(value) {
  if (value === undefined) {
    return null;
  }

  if (value instanceof Date) {
    return value.toISOString();
  }

  return value;
}

Mapping dates are applicable because there is a type Date, in our case here, how to detect time values??

Copy link
Member

Choose a reason for hiding this comment

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

We don;t have the type of "time" but making this a string will allow the user to come up with their own string property in the right format and be able to set the remote field.

Copy link
Member

@bleonard bleonard 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. interested in any refactorings that we should do.

plugins/@grouparoo/pipedrive/src/initializers/plugin.ts Outdated Show resolved Hide resolved
this.request.interceptors.response.use(
(response) => response,
(error) => {
if (error.code === "ECONNRESET") {
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why this was happening? is it different than rate limiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens when you have local network issues and trying to send many requests in a few amount of time!

@aelafifi aelafifi marked this pull request as ready for review November 30, 2021 22:58
Copy link
Member

@bleonard bleonard left a comment

Choose a reason for hiding this comment

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

maybe more complex but it's DRY'd up. Few notes on caching in particular. Some likely issues if those keys are shared across types.

@bleonard bleonard self-requested a review December 2, 2021 19:17
@bleonard bleonard added the enhancement New feature or request label Dec 2, 2021
@bleonard bleonard merged commit ce20980 into main Dec 3, 2021
@bleonard bleonard deleted the feature/pipedrive-organizations-export branch December 3, 2021 17:19
@bleonard bleonard mentioned this pull request Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants