-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New Components - picqer #17044
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
base: master
Are you sure you want to change the base?
New Components - picqer #17044
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
""" WalkthroughA comprehensive Picqer integration was implemented, introducing a fully functional app module, multiple action modules for orders, returns, customers, and picklists, as well as webhook source modules for event handling. Utilities and constants were added to support the integration, and package metadata was updated to reflect new dependencies and versioning. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action
participant PicqerApp
participant PicqerAPI
User->>Action: Trigger (e.g., Create Order)
Action->>PicqerApp: Call method (e.g., createOrder)
PicqerApp->>PicqerAPI: HTTP request (e.g., POST /orders)
PicqerAPI-->>PicqerApp: API response
PicqerApp-->>Action: Return data
Action-->>User: Output result/summary
sequenceDiagram
participant PicqerAPI
participant WebhookSource
participant User
PicqerAPI->>WebhookSource: Send webhook event (with signature)
WebhookSource->>WebhookSource: Verify signature
alt Valid signature
WebhookSource->>User: Emit event (with summary)
else Invalid signature
WebhookSource->>PicqerAPI: Respond 401 Unauthorized
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
components/offorte/offorte.app.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/arpoone/arpoone.app.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/picqer/picqer.app.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Sources - New Event (Instant) Actions - Add Order Comment - Add Return Comment - Create Order - Get Customer - Get Order - Get Picklist - Get Status Per Order Line - Search Orders - Update Order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
🧹 Nitpick comments (10)
components/picqer/package.json (1)
15-17
: Ensure dependency list is deterministicConsider alphabetising the keys in
"dependencies"
(e.g.crypto-js
before@pipedream/platform
) or adding a lint rule that enforces order. A consistent order avoids unnecessary diff-noise on future bumps.components/picqer/common/utils.mjs (1)
4-23
: Optional: deep-parse plain objectsAt the moment only top-level strings / arrays are parsed. If callers pass an object whose property values are JSON strings (common with dynamic fields), they stay unparsed. A recursive walk would make
parseObject
more generally reusable:if (typeof obj === "string") { try { return JSON.parse(obj); } catch (e) { return obj; } } - return obj; + if (typeof obj === "object") { + return Object.fromEntries( + Object.entries(obj).map(([k, v]) => [k, parseObject(v)]), + ); + } + return obj;components/picqer/actions/get-picklist/get-picklist.mjs (1)
7-8
: Component version mismatchAction version is
0.0.1
while the package was bumped to0.1.0
. Keeping them aligned helps consumers understand compatibility at a glance.- version: "0.0.1", + version: "0.1.0",components/picqer/actions/add-order-comment/add-order-comment.mjs (1)
7-8
: Align action version with packageSame as for Get Picklist – bump to
0.1.0
for consistency.components/picqer/actions/search-orders/search-orders.mjs (1)
30-41
: Consider stronger typing or helper UI for date inputs
sinceDate
anduntilDate
are free-form strings. Wrongly-formatted timestamps will silently propagate to the API and are hard to debug.
Pipedream supports the"datetime"
prop-type which renders a date-time picker in the UI.- sinceDate: { - type: "string", + sinceDate: { + type: "datetime",Same for
untilDate
.
This gives users immediate feedback and prevents malformed requests.components/picqer/actions/get-customer/get-customer.mjs (1)
18-26
: Handle 404 / empty responses gracefully
getCustomer
might returnnull
or throw when the ID is unknown. Emit a meaningful summary instead of “Successfully retrieved customer X”.try { const response = await this.picqer.getCustomer({ $, customerId: this.customerId }); - $.export("$summary", `Successfully retrieved customer ${this.customerId}`); - return response; + if (!response) $.export("$summary", `No customer found with ID ${this.customerId}`); + else $.export("$summary", `Customer ${this.customerId} retrieved`); + return response; } catch (err) { this.picqer.logError(err, $); throw err; }Prevents misleading success messages.
components/picqer/actions/update-order/update-order.mjs (1)
234-272
: Consider extracting address field visibility logic.The repetitive pattern of hiding/showing address fields could be extracted to reduce duplication and improve maintainability.
Consider refactoring to:
const addressFields = [ 'deliveryName', 'deliveryContactName', 'deliveryAddress', 'deliveryAddress2', 'deliveryZipcode', 'deliveryCity', 'deliveryRegion', 'deliveryCountry', 'invoiceName', 'invoiceContactName', 'invoiceAddress', 'invoiceAddress2', 'invoiceZipcode', 'invoiceCity', 'invoiceRegion', 'invoiceCountry' ]; addressFields.forEach(field => { fixedProps[field].hidden = hasCustomerId; });components/picqer/actions/create-order/create-order.mjs (1)
310-315
: Consider more descriptive error messages.The error messages could be enhanced to guide users better.
Apply this diff for clearer error messages:
- throw new ConfigurationError("Delivery Name is required if **Customer Id** is not provided"); + throw new ConfigurationError("Either Customer ID or Delivery Name must be provided. Please select a customer or enter delivery details."); } if (!customerId && !deliveryCountry) { - throw new ConfigurationError("Delivery Country is required if **Customer Id** is not provided"); + throw new ConfigurationError("Delivery Country is required when creating an order without a Customer ID. Please provide a valid ISO 3166 2-character country code.");components/picqer/picqer.app.mjs (2)
194-238
: Consider adding country code validation.The delivery and invoice country fields require ISO 3166 2-char codes, but there's no validation or helper to ensure correct format.
Consider adding validation or providing a dropdown with country codes:
deliveryCountry: { type: "string", label: "Delivery Country", description: "Country of delivery address (needs to be ISO 3166 2-char code). Required if **Customer Id** is not provided.", + options: async () => { + // Return common country codes or fetch from an API + return [ + { label: "United States", value: "US" }, + { label: "Canada", value: "CA" }, + { label: "United Kingdom", value: "GB" }, + // ... more countries + ]; + }, },Alternatively, add validation in the action components that use these fields.
301-452
: Consider adding JSDoc comments for better developer experience.The methods section would benefit from JSDoc comments to document parameters, return values, and potential errors. This would improve the developer experience when using this app component.
Example for one method:
/** * Create a new order in Picqer * @param {Object} opts - Request options * @param {Object} opts.$ - Pipedream context object * @param {Object} opts.data - Order data * @returns {Promise<Object>} Created order object * @throws {Error} If the API request fails */ createOrder(opts = {}) { return this._makeRequest({ method: "POST", path: "/orders", ...opts, }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
components/picqer/actions/add-order-comment/add-order-comment.mjs
(1 hunks)components/picqer/actions/add-return-comment/add-return-comment.mjs
(1 hunks)components/picqer/actions/create-order/create-order.mjs
(1 hunks)components/picqer/actions/get-customer/get-customer.mjs
(1 hunks)components/picqer/actions/get-order/get-order.mjs
(1 hunks)components/picqer/actions/get-picklist/get-picklist.mjs
(1 hunks)components/picqer/actions/get-status-per-order-line/get-status-per-order-line.mjs
(1 hunks)components/picqer/actions/search-orders/search-orders.mjs
(1 hunks)components/picqer/actions/update-order/update-order.mjs
(1 hunks)components/picqer/common/constants.mjs
(1 hunks)components/picqer/common/utils.mjs
(1 hunks)components/picqer/package.json
(2 hunks)components/picqer/picqer.app.mjs
(1 hunks)components/picqer/sources/common/base.mjs
(1 hunks)components/picqer/sources/new-event-instant/new-event-instant.mjs
(1 hunks)components/picqer/sources/new-event-instant/test-event.mjs
(1 hunks)
🔇 Additional comments (8)
components/picqer/package.json (1)
15-17
: Re-evaluate version rangesUsing caret ranges (
^3.1.0
,^4.2.0
) automatically picks up new minor / patch versions at deploy time. If Picqer components are sensitive to upstream changes (especiallycrypto-js
for signature verification), consider pinning exact versions or adding an automated lockfile-refresh step to surface breaking changes early.components/picqer/sources/new-event-instant/test-event.mjs (1)
1-43
: Looks good – representative sample eventThe static payload is well-structured and mirrors the Picqer docs. No issues found.
components/picqer/actions/get-picklist/get-picklist.mjs (1)
18-24
: Verify underlying method existsAssuming
picqer.app.mjs
exposesgetPicklist
, but this isn’t guaranteed at compile-time. Please confirm the method name and signature match; a typo will only surface at runtime.components/picqer/actions/get-status-per-order-line/get-status-per-order-line.mjs (1)
19-26
: Pagination / large-response advisory
getStatusPerOrderLine
can return many lines for big orders. Ensure the underlyingpicqer
helper streams or paginates; otherwise responses might exceed Pipedream’s 6 MB limit.components/picqer/actions/get-order/get-order.mjs (1)
1-27
: LGTM!Clean and straightforward implementation of the get order action. Good use of propDefinition for consistency.
components/picqer/actions/update-order/update-order.mjs (1)
309-355
: Well-implemented data transformation logic.Good handling of:
- Float parsing for discount
- Array joining for multi-value custom fields
- Proper field name mapping to API format
components/picqer/common/constants.mjs (1)
1-230
: Well-organized constants.Good structure with clear option objects and comprehensive event types coverage.
components/picqer/actions/create-order/create-order.mjs (1)
253-272
: Good handling of required custom fields.Nice improvement over the update action by including the
required
property for custom order fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There's just this one operator that should be swapped, unless there's a very unlikely reason not to.
I'm applying the suggestion then approving the PR; let me know if this is not correct.
Resolves #16959.
Summary by CodeRabbit