refactor: Make Pipedrive api-module v1 ready #56
Conversation
- Created a mock for updating activities in Pipedrive. - Implemented a mock API class to handle various Pipedrive API calls. - Added mock data for listing deals with detailed user and organization information. - Developed tests for the Pipedrive API class, covering user profiles, deals, activities, and error handling for authentication.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the Pipedrive API module from the legacy ModuleManager pattern to the v1 module definition pattern, removing all Mongoose-based model code and legacy manager implementations.
Key Changes:
- Removed all legacy code including ModuleManager, Mongoose models (Credential, Entity), and associated test files
- Implemented new v1 module pattern with
definition.jscontaining OAuth2 authentication methods - Updated API client with enhanced error handling, domain validation, and v2 endpoint support
Reviewed Changes
Copilot reviewed 11 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/v1-ready/pipedrive/index.js | New simplified module export with Api and Definition only |
| packages/v1-ready/pipedrive/definition.js | New v1 module definition with OAuth2 auth methods and entity/credential logic |
| packages/v1-ready/pipedrive/api.js | New API client with v2 endpoints, enhanced validation, and comprehensive documentation |
| packages/v1-ready/pipedrive/CHANGELOG.md | Updated changelog documenting v1.0.0 and v1.1.0 releases with migration guides |
| packages/needs-updating/pipedrive/* | Deleted legacy manager, models, and test files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/v1-ready/pipedrive/api.js
Outdated
|
|
||
| if (this.companyDomain) { | ||
| this.baseUrl = `${this.companyDomain}/api`; | ||
| } |
There was a problem hiding this comment.
The baseUrl is only set if companyDomain is provided in the constructor, but it's required for all API calls. This creates an inconsistent state where API methods will fail with undefined baseUrl. Consider either requiring companyDomain in the constructor or setting a placeholder that triggers a clear error when API methods are called.
| if (this.companyDomain) { | |
| this.baseUrl = `${this.companyDomain}/api`; | |
| } | |
| if (!this.companyDomain) { | |
| throw new Error('companyDomain is required for Pipedrive API'); | |
| } | |
| this.baseUrl = `${this.companyDomain}/api`; |
packages/v1-ready/pipedrive/api.js
Outdated
| const dealId = get(params, 'dealId', null); | ||
| const subject = get(params, 'subject'); | ||
| const type = get(params, 'type'); |
There was a problem hiding this comment.
Variables dealId, subject, and type are extracted but never used in the function. These should either be removed or utilized if they were intended for validation.
| const dealId = get(params, 'dealId', null); | |
| const subject = get(params, 'subject'); | |
| const type = get(params, 'type'); |
|
|
||
| --- | ||
|
|
||
| # v1.0.0 (Mon Oct 21 2024) |
There was a problem hiding this comment.
Both v1.1.0 (line 1) and v1.0.0 (line 60) are dated 'Mon Oct 21 2024', which is impossible for sequential releases. The dates should be different to reflect when each version was actually released.
| # v1.0.0 (Mon Oct 21 2024) | |
| # v1.0.0 (Mon Sep 16 2024) |
…itions - Converted all files to TypeScript and added comprehensive type definitions for all Pipedrive API v2 endpoints. - Updated field naming in ActivityParams to align with Pipedrive API conventions. - Enhanced request parameter types with additional fields and improved type safety. - Added nested object types for complex structures. - Updated testing framework from Jest to Vitest, including necessary test adjustments. - Added detailed documentation and inline comments for better clarity. - Refactored code organization for improved maintainability.
… in package.json and package-lock.json
… TypeScript version - Deleted old JavaScript API implementation in `api.js`, `definition.js`, and `index.js`. - Introduced new TypeScript API implementation in `src/api.ts`, `src/definition.ts`, and `src/defaultConfig.json`. - Updated authentication methods and user details retrieval in the new implementation. - Added new test setup and teardown files for Vitest. - Created a mock API utility for testing purposes. - Updated package dependencies and configurations for TypeScript and Vitest.
seanspeaks
left a comment
seanspeaks
left a comment
There was a problem hiding this comment.
Cool, we'll get the live test harness going so this is simpler in the future
|
🚀 PR was released in |
This pull request removes the legacy Pipedrive v1 module implementation, including all code and tests based on the old
ModuleManagerpattern and Mongoose models. The codebase is being refactored to use the new v1 module definition pattern, as described in the updated changelog.Legacy code removal and migration to v1 module pattern:
api.jsfile, which previously contained the Pipedrive API client implementation with v1 endpoints and business logic.manager.js, which held the legacyModuleManagerclass and associated entity/credential logic.models/credential.jsandmodels/entity.js, which defined custom credential/entity schemas for Pipedrive. [1] [2]manager.test.jsandtest/Manager.test.js, which tested the old manager and API code. [1] [2]index.js, removing all references to legacy classes and models.Documentation and migration guide updates:
CHANGELOG.mdto reflect the migration to the v1 module pattern, breaking changes, enhancements, and migration instructions for users upgrading from previous versions.📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via:
npm install @friggframework/api-module-attio@1.0.1-canary.56.5db6eda.0 npm install @friggframework/api-module-pipedrive@2.0.1-canary.56.5db6eda.0 # or yarn add @friggframework/api-module-attio@1.0.1-canary.56.5db6eda.0 yarn add @friggframework/api-module-pipedrive@2.0.1-canary.56.5db6eda.0Version
Published prerelease version:
@friggframework/api-module-attio@1.1.0-next.1@friggframework/api-module-pipedrive@2.1.0-next.0Changelog
🚀 Enhancement
@friggframework/api-module-attio,@friggframework/api-module-pipedrive🐛 Bug Fix
@friggframework/api-module-microsoft-teams,@friggframework/api-module-slack,@friggframework/api-module-42matters,@friggframework/api-module-asana,@friggframework/api-module-attio,@friggframework/api-module-connectwise,@friggframework/api-module-contentful,@friggframework/api-module-contentstack,@friggframework/api-module-crossbeam,@friggframework/api-module-deel,@friggframework/api-module-frigg-scale-test,@friggframework/api-module-frontify,@friggframework/api-module-google-calendar,@friggframework/api-module-google-drive,@friggframework/api-module-helpscout,@friggframework/api-module-hubspot,@friggframework/api-module-ironclad,@friggframework/api-module-linear,@friggframework/api-module-salesforce,@friggframework/api-module-stripe,@friggframework/api-module-unbabel-projects,@friggframework/api-module-unbabel,@friggframework/api-module-zoho-crm,@friggframework/api-module-zoomAuthors: 1