[Components] ninjaone - WIP #14892
[Components] ninjaone - WIP #14892
Conversation
jcortes
added
action
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
WalkthroughThis pull request introduces multiple new modules and enhancements to the NinjaOne application. Key additions include actions for creating support tickets and updating device custom fields, as well as new event sources for monitoring device online status and remote session initiation. A constants module is also added to centralize configuration values. Furthermore, utility functions for string and JSON parsing are introduced. The Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
components/ninjaone/sources/common/webhook.mjs (1)
60-65: Clarify purpose of methods throwingConfigurationErrorThe methods
getResourcesFnandgetResourcesFnArgsthrowConfigurationErrorby default. If these methods are intended to be abstract and meant for overriding in subclasses, consider adding documentation or comments to make this clear.components/ninjaone/ninjaone.app.mjs (2)
8-112: Refactor pagination logic into a shared utility functionThe
optionsmethods fororganizationId(lines 12-35) anddeviceId(lines 87-111) share similar pagination logic. To improve maintainability and reduce code duplication, consider refactoring this logic into a shared utility function that can be reused across these methods.
124-133: Handle potential errors in_makeRequestmethodThe
_makeRequestmethod does not include error handling for API request failures. To enhance robustness, consider adding error handling to manage cases where the API might return errors or fail to respond.components/ninjaone/sources/new-ticket-instant/new-ticket-instant.mjs (1)
17-23: Improve resource metadata generation.The current implementation uses a generic "New Resource" message. Consider making it more specific to tickets for better clarity.
generateMeta(resource) { return { id: resource.id, - summary: `New Resource: ${resource.name}`, + summary: `New Ticket: ${resource.name || resource.title}`, ts: Date.parse(resource.created_at), }; }components/ninjaone/sources/new-device-online-instant/new-device-online-instant.mjs (1)
1-25: Consider creating a base source factory to reduce code duplication.The implementation is nearly identical to
new-ticket-instant, differing only in key, name, and description. Consider creating a factory function to generate these source components.Example implementation:
// components/ninjaone/sources/base/sourceFactory.mjs export function createSource({ key, name, description, }) { return { ...common, key, name, description, version: "0.0.1", type: "source", dedupe: "unique", methods: { ...common.methods, getEventName() { return events.DEFAULT; }, generateMeta(resource) { return { id: resource.id, summary: `${name}: ${resource.name}`, ts: Date.parse(resource.created_at), }; }, }, }; }This would reduce duplication and make it easier to maintain consistent behavior across sources.
components/ninjaone/sources/new-remote-session-instant/new-remote-session-instant.mjs (3)
7-8: Enhance component description with filtering capabilitiesThe description should mention the optional filters for session type or technician as specified in the PR objectives.
- description: "Emit new event when a remote access session is initiated. [See the documentation](https://app.ninjarmm.com/apidocs/?links.active=core#/webhooks/configureWebhook).", + description: "Emit new event when a remote access session is initiated. Optionally filter by session type or technician. [See the documentation](https://app.ninjarmm.com/apidocs/?links.active=core#/webhooks/configureWebhook).",
14-16: Consider using a more specific event nameUsing the DEFAULT event name might be too generic. Consider defining a specific event name for remote sessions in the events module.
- return events.DEFAULT; + return events.REMOTE_SESSION_STARTED;
17-23: Enhance metadata summary with session detailsThe summary could be more descriptive by including session type or technician information if available.
- summary: `New Resource: ${resource.name}`, + summary: `New Remote Session: ${resource.type || ''} by ${resource.technician || 'Unknown'}`,components/ninjaone/actions/update-device-custom-fields/update-device-custom-fields.mjs (1)
47-48: Enhance success message with update detailsThe success message could be more informative by including the device ID and number of fields updated.
- $.export("$summary", "Successfully updated device custom fields."); + $.export("$summary", `Successfully updated ${Object.keys(utils.parse(data)).length} custom fields for device ${deviceId}.`);components/ninjaone/actions/create-ticket/create-ticket.mjs (1)
124-124: Enhance success message with ticket detailsInclude the ticket ID and subject in the success message for better traceability.
- $.export("$summary", "Successfully created ticket."); + $.export("$summary", `Successfully created ticket #${response.id}: ${subject}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
components/ninjaone/actions/create-ticket/create-ticket.mjs(1 hunks)components/ninjaone/actions/update-device-custom-fields/update-device-custom-fields.mjs(1 hunks)components/ninjaone/common/constants.mjs(1 hunks)components/ninjaone/common/utils.mjs(1 hunks)components/ninjaone/ninjaone.app.mjs(1 hunks)components/ninjaone/package.json(2 hunks)components/ninjaone/sources/common/webhook.mjs(1 hunks)components/ninjaone/sources/new-device-online-instant/new-device-online-instant.mjs(1 hunks)components/ninjaone/sources/new-remote-session-instant/new-remote-session-instant.mjs(1 hunks)components/ninjaone/sources/new-ticket-instant/new-ticket-instant.mjs(1 hunks)
🔇 Additional comments (2)
components/ninjaone/common/constants.mjs (1)
1-15: Constants are well-defined and improve code maintainability
The defined constants provide a central place for configuration values, which enhances code readability and maintainability.
components/ninjaone/package.json (1)
16-17: Verify that @pipedream/platform dependency is up-to-date and secure
Ensure that the version "3.0.3" of @pipedream/platform is the latest stable release and that there are no known security vulnerabilities associated with it.
Run the following script to check for the latest version:
✅ Verification successful
@pipedream/platform dependency is up-to-date
The package is using the latest stable version (3.0.3) available on npm registry.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the latest version of @pipedream/platform on npm
# Fetch the latest version from the npm registry
curl -s https://registry.npmjs.org/@pipedream/platform | jq -r '."dist-tags".latest'
Length of output: 92
| generateMeta() { | ||
| throw new ConfigurationError("generateMeta is not implemented"); | ||
| }, |
There was a problem hiding this comment.
Ensure generateMeta is implemented in subclasses to prevent runtime errors
The generateMeta method currently throws a ConfigurationError. If not overridden in subclasses, this will cause runtime errors when processResource is called in the run method. Please ensure that all subclasses implement this method appropriately.
| getEventName() { | ||
| throw new ConfigurationError("getEventName is not implemented"); | ||
| }, |
There was a problem hiding this comment.
Ensure getEventName is implemented in subclasses to avoid errors during webhook activation
The getEventName method throws a ConfigurationError by default. This method is called in the activate hook when creating a webhook. If not implemented in subclasses, it will result in runtime errors during activation. Please ensure this method is properly implemented in all subclasses.
| function parse(value) { | ||
| const valueToParse = emptyStrToUndefined(value); | ||
| if (typeof(valueToParse) === "object" || valueToParse === undefined) { | ||
| return valueToParse; | ||
| } | ||
| try { | ||
| return JSON.parse(valueToParse); | ||
| } catch (e) { | ||
| throw new ConfigurationError("Make sure the custom expression contains a valid object."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add documentation.
The parse function would benefit from:
- More descriptive error messages
- JSDoc documentation
- Additional type checking
Here's the suggested implementation:
+/**
+ * Parses a value into an object, handling empty strings and JSON parsing
+ * @param {any} value - The value to parse
+ * @returns {object|undefined} The parsed object or undefined
+ * @throws {ConfigurationError} If the value is an invalid JSON string
+ */
function parse(value) {
const valueToParse = emptyStrToUndefined(value);
if (typeof(valueToParse) === "object" || valueToParse === undefined) {
return valueToParse;
}
try {
return JSON.parse(valueToParse);
} catch (e) {
- throw new ConfigurationError("Make sure the custom expression contains a valid object.");
+ throw new ConfigurationError(
+ `Invalid JSON format. Expected a valid JSON object, received: ${typeof valueToParse}`
+ );
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parse(value) { | |
| const valueToParse = emptyStrToUndefined(value); | |
| if (typeof(valueToParse) === "object" || valueToParse === undefined) { | |
| return valueToParse; | |
| } | |
| try { | |
| return JSON.parse(valueToParse); | |
| } catch (e) { | |
| throw new ConfigurationError("Make sure the custom expression contains a valid object."); | |
| } | |
| } | |
| /** | |
| * Parses a value into an object, handling empty strings and JSON parsing | |
| * @param {any} value - The value to parse | |
| * @returns {object|undefined} The parsed object or undefined | |
| * @throws {ConfigurationError} If the value is an invalid JSON string | |
| */ | |
| function parse(value) { | |
| const valueToParse = emptyStrToUndefined(value); | |
| if (typeof(valueToParse) === "object" || valueToParse === undefined) { | |
| return valueToParse; | |
| } | |
| try { | |
| return JSON.parse(valueToParse); | |
| } catch (e) { | |
| throw new ConfigurationError( | |
| `Invalid JSON format. Expected a valid JSON object, received: ${typeof valueToParse}` | |
| ); | |
| } | |
| } |
| function emptyStrToUndefined(value) { | ||
| const trimmed = typeof(value) === "string" && value.trim(); | ||
| return trimmed === "" | ||
| ? undefined | ||
| : value; | ||
| } |
There was a problem hiding this comment.
Fix type checking and string handling logic.
The current implementation has several issues:
- Incorrect
typeofoperator syntax (should betypeof value) - The trimmed value calculation is not properly utilized
- No handling for
nullvalues
Here's the corrected implementation:
function emptyStrToUndefined(value) {
- const trimmed = typeof(value) === "string" && value.trim();
- return trimmed === ""
- ? undefined
- : value;
+ if (value === null || value === undefined) return undefined;
+ if (typeof value !== "string") return value;
+ const trimmed = value.trim();
+ return trimmed === "" ? undefined : value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function emptyStrToUndefined(value) { | |
| const trimmed = typeof(value) === "string" && value.trim(); | |
| return trimmed === "" | |
| ? undefined | |
| : value; | |
| } | |
| function emptyStrToUndefined(value) { | |
| if (value === null || value === undefined) return undefined; | |
| if (typeof value !== "string") return value; | |
| const trimmed = value.trim(); | |
| return trimmed === "" ? undefined : value; | |
| } |
| export default { | ||
| ...common, | ||
| key: "ninjaone-new-device-online-instant", | ||
| name: "New Device Online (Instant)", | ||
| description: "Emit new event when a monitored device comes online. [See the documentation](https://app.ninjarmm.com/apidocs/?links.active=core#/webhooks/configureWebhook).", | ||
| version: "0.0.1", | ||
| type: "source", | ||
| dedupe: "unique", | ||
| methods: { | ||
| ...common.methods, | ||
| getEventName() { | ||
| return events.DEFAULT; | ||
| }, | ||
| generateMeta(resource) { | ||
| return { | ||
| id: resource.id, | ||
| summary: `New Resource: ${resource.name}`, | ||
| ts: Date.parse(resource.created_at), | ||
| }; | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the webhook common implementation I can now provide the final response:
Optional filters for device groups and types are missing from the implementation
The current webhook implementation only provides basic webhook functionality without the mentioned filter options. The common webhook props only include:
- app connection
- database interface
- HTTP webhook interface
Required changes:
- Add props for device group filter
- Add props for device type filter
- Update webhook creation to include these filters in the request payload
🔗 Analysis chain
Verify webhook configuration options against requirements.
The PR objectives mention optional filters for device groups and types, but the current implementation doesn't show how these filters are configured.
Let me try a different search strategy to verify the webhook configuration and filter implementation.
Let me try one more search to locate the webhook implementation and filter configuration.
Let me check the webhook common implementation since we found its location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for filter configuration in common webhook implementation
echo "Searching for filter configuration in webhook implementation..."
rg -l "filter" --type js components/ninjaone/common/
# Check if props are defined for filters
echo "Checking for filter-related props..."
ast-grep --pattern 'props: {
$$$
}' components/ninjaone/sources/
Length of output: 341
Script:
#!/bin/bash
# Search for webhook configuration in common implementation
echo "Checking common webhook implementation..."
rg -A 10 "webhook" components/ninjaone/common/
# Search for props or configuration in the source file
echo "Checking source implementation..."
cat components/ninjaone/sources/new-device-online-instant/new-device-online-instant.mjs
# Check common.js content
echo "Checking common.js content..."
cat components/ninjaone/common/webhook.mjs
Length of output: 1765
Script:
#!/bin/bash
# Find the webhook common file
echo "Looking for webhook common file..."
fd webhook.mjs components/ninjaone/
# Check events implementation
echo "Checking events implementation..."
fd events.mjs components/ninjaone/ --exec cat {}
# Check for any prop definitions in source files
echo "Checking for prop definitions in source files..."
rg "props: {" --type js components/ninjaone/ -A 10
Length of output: 533
Script:
#!/bin/bash
# Check webhook common implementation
echo "Checking webhook common implementation..."
cat components/ninjaone/sources/common/webhook.mjs
# Fix the props search with proper escaping
echo "Checking for props definitions..."
rg "props:\s*{" --type js components/ninjaone/ -A 10
Length of output: 2344
| data: { | ||
| type: "object", | ||
| label: "Custom Fields", | ||
| description: "Additional custom fields to update.", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and examples for custom fields
The data property could benefit from:
- Schema validation for custom fields
- Example in the description
data: {
type: "object",
label: "Custom Fields",
- description: "Additional custom fields to update.",
+ description: "Additional custom fields to update. Example: `{\"customField1\": \"value1\", \"customField2\": \"value2\"}`",
+ additionalProperties: {
+ type: ["string", "number", "boolean"],
+ },
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data: { | |
| type: "object", | |
| label: "Custom Fields", | |
| description: "Additional custom fields to update.", | |
| }, | |
| }, | |
| data: { | |
| type: "object", | |
| label: "Custom Fields", | |
| description: "Additional custom fields to update. Example: `{\"customField1\": \"value1\", \"customField2\": \"value2\"}`", | |
| additionalProperties: { | |
| type: ["string", "number", "boolean"], | |
| }, | |
| }, | |
| }, |
| type, | ||
| assignedAppUserId, | ||
| description, | ||
| // isDescriptionPublic, |
There was a problem hiding this comment.
Remove or implement commented isDescriptionPublic field
The isDescriptionPublic field is commented out in both the destructuring and the request data. Either implement it or remove the comments.
- // isDescriptionPublic,
+ isDescriptionPublic,And:
- // isDescriptionPublic,
+ isDescriptionPublic,Also applies to: 118-118
| const response = await createTicket({ | ||
| $, | ||
| data: { | ||
| clientId, | ||
| ticketFormId, | ||
| subject, | ||
| status, | ||
| type, | ||
| assignedAppUserId, | ||
| description, | ||
| // isDescriptionPublic, | ||
| parentTicketId, | ||
| priority, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation
The ticket creation could benefit from:
- Error handling for API responses
- Validation for required fields before making the request
+ // Validate required fields
+ if (!subject || !description) {
+ throw new Error("Subject and description are required fields");
+ }
+
const response = await createTicket({
$,
data: {
clientId,
ticketFormId,
subject,
status,
type,
assignedAppUserId,
description,
parentTicketId,
priority,
},
});
+
+ if (!response?.id) {
+ throw new Error("Failed to create ticket: Invalid response from API");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await createTicket({ | |
| $, | |
| data: { | |
| clientId, | |
| ticketFormId, | |
| subject, | |
| status, | |
| type, | |
| assignedAppUserId, | |
| description, | |
| // isDescriptionPublic, | |
| parentTicketId, | |
| priority, | |
| }, | |
| }); | |
| // Validate required fields | |
| if (!subject || !description) { | |
| throw new Error("Subject and description are required fields"); | |
| } | |
| const response = await createTicket({ | |
| $, | |
| data: { | |
| clientId, | |
| ticketFormId, | |
| subject, | |
| status, | |
| type, | |
| assignedAppUserId, | |
| description, | |
| // isDescriptionPublic, | |
| parentTicketId, | |
| priority, | |
| }, | |
| }); | |
| if (!response?.id) { | |
| throw new Error("Failed to create ticket: Invalid response from API"); | |
| } |
WHY
Resolves #14845
Summary by CodeRabbit
Release Notes
New Features
Improvements
Version Update