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

Gainsight NXT new components #14453

Merged
merged 12 commits into from
Nov 6, 2024
Merged

Gainsight NXT new components #14453

merged 12 commits into from
Nov 6, 2024

Conversation

GTFalcao
Copy link
Collaborator

@GTFalcao GTFalcao commented Oct 28, 2024

Closes #14373

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced modules for creating or updating company, custom object, and person records in Gainsight NXT.
    • Enhanced API interaction capabilities with new methods for managing company and custom object data.
  • Bug Fixes

    • Improved error handling for create/update operations to ensure seamless transitions.
  • Documentation

    • Updated package version to 0.1.0 and added dependencies for better functionality.
  • Chores

    • Added utility functions for improved JSON parsing and object entry transformation.

Copy link

vercel bot commented Oct 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2024 4:11am
pipedream-docs ⬜️ Ignored (Inspect) Nov 6, 2024 4:11am
pipedream-docs-redirect-do-not-edit ⬜️ Ignored (Inspect) Nov 6, 2024 4:11am

Copy link
Contributor

coderabbitai bot commented Oct 28, 2024

Walkthrough

This pull request introduces multiple modules for the Gainsight NXT application, enabling the creation and updating of company, custom object, and person records. Each module defines an action with relevant properties and an asynchronous run method that attempts to update existing records or create new ones if updates fail. Additionally, enhancements to the main application file include new properties and methods to facilitate API interactions. The package version is updated, and a new dependency is added.

Changes

File Path Change Summary
components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs New module for creating or updating company records with an asynchronous run method.
components/gainsight_nxt/actions/create-or-update-custom-object/create-or-update-custom-object.mjs New module for creating or updating custom object records with an asynchronous run method.
components/gainsight_nxt/actions/create-or-update-person/create-or-update-person.mjs New module for creating or updating person records with an asynchronous run method.
components/gainsight_nxt/gainsight_nxt.app.mjs Enhanced functionality with new properties and methods for API interactions, including updateCompany and others.
components/gainsight_nxt/package.json Updated version from "0.0.1" to "0.1.0" and added new dependency on @pipedream/platform.
components/gainsight_nxt/common/utils.mjs New utility functions for JSON parsing and object entry transformation.

Assessment against linked issues

Objective Addressed Explanation
Create or update a company record (#14373)
Create or modify a custom object (#14373)
Add or update a person's record (#14373)

Possibly related PRs

  • Salesforce usability improvements #12697: The main PR introduces a module for creating or updating company records, which is related to the changes in the Salesforce integration that also involve updating records.
  • New Components - specific #12926: The new components introduced in this PR for creating or updating records in Gainsight NXT are conceptually similar to the actions for creating or updating custom objects in the retrieved PR.
  • [Components] unthread #13686 #13852: The changes in this PR for creating or updating person records in Gainsight NXT relate to the new module for creating or updating a person's record in the main PR.
  • New Components - burstyai #14224: The new components for triggering workflows in the BurstyAI platform may relate to the structured approach of managing records in the main PR, as both involve handling data in a defined manner.
  • New Components - doppler_marketing_automation #14243: The new components for managing subscribers in the Doppler marketing automation service may relate to the structured handling of company records in the main PR, as both involve managing entities with specific properties.

Suggested labels

action

Suggested reviewers

  • michelle0927

Poem

In the Gainsight fields, we hop and play,
New records bloom, brightening the day.
With actions to create and update with glee,
Our data grows strong, as strong as can be!
So let’s celebrate this code we share,
A joyful leap, with records to spare! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 361a527 and d840820.

📒 Files selected for processing (1)
  • components/gainsight_nxt/actions/create-or-update-custom-object/create-or-update-custom-object.mjs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • components/gainsight_nxt/actions/create-or-update-custom-object/create-or-update-custom-object.mjs

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (6)
components/gainsight_nxt/actions/create-or-update-custom-object/create-or-update-custom-object.mjs (2)

3-8: Consider enhancing component metadata.

  1. Consider starting with version "1.0.0" since this is a new, complete component implementation.
  2. The description could be more explicit about the update-then-create behavior to set proper expectations.
-  version: "0.0.1",
+  version: "1.0.0",
-  description: "Create or update a custom object record. [See the documentation](https://support.gainsight.com/gainsight_nxt/API_and_Developer_Docs/Custom_Object_API/Gainsight_Custom_Object_API_Documentation#Insert_API)",
+  description: "Creates or updates a custom object record. First attempts to update using provided key field(s); if no matching record exists, creates a new one. [See the documentation](https://support.gainsight.com/gainsight_nxt/API_and_Developer_Docs/Custom_Object_API/Gainsight_Custom_Object_API_Documentation#Insert_API)",

28-67: Consider implementing additional safeguards and monitoring.

For production reliability, consider implementing:

  1. Rate limiting or throttling mechanism to prevent API abuse
  2. Structured logging for better debugging and monitoring
  3. Schema validation for the fieldValues object to catch data issues early

Would you like assistance in implementing any of these improvements?

components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (1)

1-145: Consider architectural improvements for maintainability and security.

As this component handles sensitive business data and is part of a larger integration:

  1. Consider extracting common validation logic into shared utilities for reuse across company, person, and custom object components
  2. Add logging for audit trail of data modifications
  3. Consider implementing rate limiting to prevent API abuse

Would you like assistance in implementing these architectural improvements?

components/gainsight_nxt/gainsight_nxt.app.mjs (3)

36-38: Consider removing or configuring the hardcoded accept-language header

The accept-language header is hardcoded to specific language preferences. Unless required, it might be better to remove it or make it configurable to accommodate different users' locales.

You can modify the headers as follows:

            headers: {
              "content-type": "application/json",
              "accept": "application/json, text/plain, */*",
-             "accept-language": "en-GB,en-US;q=0.9,en;q=0.8",
              "accesskey": `${this.$auth.access_key}`,
              ...headers,
            },

68-72: Handle potential errors in listCustomObjects

If the API call fails in listCustomObjects, the method may return undefined or throw an error. Consider adding error handling or returning a default value to prevent issues downstream.


27-27: Review the use of default parameter $ = this

In the _makeRequest method, the default parameter $ = this might lead to confusion or unintended behavior. Ensure that this is necessary and won't conflict with the context where _makeRequest is called.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7f501a2 and 9b34452.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (1 hunks)
  • components/gainsight_nxt/actions/create-or-update-custom-object/create-or-update-custom-object.mjs (1 hunks)
  • components/gainsight_nxt/actions/create-or-update-person/create-or-update-person.mjs (1 hunks)
  • components/gainsight_nxt/gainsight_nxt.app.mjs (1 hunks)
  • components/gainsight_nxt/package.json (2 hunks)
🔇 Additional comments (10)
components/gainsight_nxt/package.json (3)

3-3: Version bump follows semver correctly.

The version increment from 0.0.1 to 0.1.0 appropriately reflects the addition of new features (Gainsight NXT components) while maintaining backward compatibility.


14-14: LGTM: JSON formatting fix.

The proper closing brace improves the JSON structure.


15-17: Dependencies section is properly configured.

The @pipedream/platform dependency is correctly specified with a caret range (^3.0.3), allowing for compatible updates while preventing breaking changes.

Let's verify this is the latest compatible version:

✅ Verification successful

Based on the search results, I can see that @pipedream/platform versions range from ^0.9.0 to ^3.0.3 across different components. The gainsight_nxt component is using version ^3.0.3, which is the latest version being used across the codebase.

The dependency version is up-to-date.

The @pipedream/platform dependency in gainsight_nxt is using version ^3.0.3, which matches the latest version being used across other recently updated components in the codebase. No update is needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if there's a newer compatible version of @pipedream/platform
# that we should consider using

# Search for other components using @pipedream/platform to compare versions
rg -g "package.json" '"@pipedream/platform":\s*"\^[0-9]+\.[0-9]+\.[0-9]+"' components/

Length of output: 89239

components/gainsight_nxt/actions/create-or-update-person/create-or-update-person.mjs (2)

1-8: LGTM! Well-structured metadata with documentation.

The action metadata is well-defined with appropriate versioning and includes helpful documentation links.


1-1: Verify integration with gainsight_nxt.app.mjs.

Let's confirm the imported app module provides the required functionality.

components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (1)

1-8: LGTM! Component metadata follows Pipedream best practices.

The component is well-structured with appropriate naming, versioning, and documentation links.

components/gainsight_nxt/gainsight_nxt.app.mjs (4)

39-39: Verify the inclusion of the access key in headers

Including the accesskey directly from this.$auth.access_key in the request headers may expose sensitive information. Please confirm that this is the recommended method for authentication with the Gainsight NXT API.

If not, consider using a more secure authentication approach as per the API documentation.


23-25: Ensure the base URL is correctly formatted

Verify that this.$auth.customer_domain includes the protocol (e.g., https://). If not, the constructed URL may be incorrect. Consider adding the protocol if it's missing.

Adjust the _baseUrl method if necessary:

      _baseUrl() {
-       return `${this.$auth.customer_domain}/v1`;
+       return `https://${this.$auth.customer_domain}/v1`;
      },

73-84: Validate objectName to prevent injection attacks

When using objectName in the URL path, ensure it is properly validated to prevent potential injection vulnerabilities. Since objectName may come from user input, adding validation enhances security.

[security]

You can add validation like this:

      async updateCustomObject({
        objectName, ...args
      }) {
+       if (!/^[a-zA-Z0-9_-]+$/.test(objectName)) {
+         throw new Error("Invalid objectName parameter.");
+       }
        return this._makeRequest({
          path: `/data/objects/${objectName}`,
          method: "PUT",
          ...args,
        });
      },

Apply similar validation in createCustomObject.


61-67: Confirm the HTTP method for createOrUpdatePerson

Verify that using the PUT method with the /peoplemgmt/v1.0/people endpoint is appropriate for both creating and updating a person. If the API differentiates between creation and updates, you might need to handle them separately.

description: "The name of the custom object.",
async options() {
const { data } = await this.listCustomObjects();
return data/*?.filter?.((obj) => obj.objectType === "CUSTOM")*/.map(( {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Uncomment the filter to retrieve only custom objects

In the options method for objectName, the filter to select only custom objects is commented out. If you intend to display only custom objects in the options, please uncomment the filter.

Apply this diff to fix the issue:

-          return data/*?.filter?.((obj) => obj.objectType === "CUSTOM")*/.map(( {
+          return data?.filter?.((obj) => obj.objectType === "CUSTOM")?.map(( {
📝 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.

Suggested change
return data/*?.filter?.((obj) => obj.objectType === "CUSTOM")*/.map(( {
return data?.filter?.((obj) => obj.objectType === "CUSTOM")?.map(( {

},
arr: {
type: "string",
label: "Anual Recurring Revenue (ARR)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with coderabbit, this looks like a typo.

Suggested change
label: "Anual Recurring Revenue (ARR)",
label: "Annual Recurring Revenue (ARR)",

Comment on lines 40 to 51
originalContractDate: {
type: "string",
label: "Original Contract Date",
description: "The date the engagement with the client started.",
optional: true,
},
renewalDate: {
type: "string",
label: "Renewal Date",
description: "The upcoming renewal date of the contract.",
optional: true,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with coderabbit. The date props should include the date format in the description.

Suggested change
originalContractDate: {
type: "string",
label: "Original Contract Date",
description: "The date the engagement with the client started.",
optional: true,
},
renewalDate: {
type: "string",
label: "Renewal Date",
description: "The upcoming renewal date of the contract.",
optional: true,
},
originalContractDate: {
type: "string",
label: "Original Contract Date",
description: "The date the engagement with the client started. Format: YYYY-MM-DD",
optional: true,
},
renewalDate: {
type: "string",
label: "Renewal Date",
description: "The upcoming renewal date of the contract. Format: YYYY-MM-DD",
optional: true,
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call - thanks!

description: "The name of the custom object.",
async options() {
const { data } = await this.listCustomObjects();
return data/*?.filter?.((obj) => obj.objectType === "CUSTOM")*/.map(( {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the filter meant to be commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, I commented it out for testing and forgot to remove the block afterwards - doing it now, thanks.

For the record, it seems several custom object types are not actually classified explicitly as "custom", hence why I removed this filter (it was returning no objects whatsoever with it)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (3)
components/gainsight_nxt/gainsight_nxt.app.mjs (1)

61-67: Add JSDoc documentation for createOrUpdatePerson method.

The method would benefit from documentation describing the expected payload structure and required fields.

Add documentation:

+    /**
+     * Creates or updates a person record
+     * @param {Object} args - The person data
+     * @param {Object} args.data - The person properties
+     * @param {string} args.data.emailAddress - Primary email (required)
+     * @returns {Promise<Object>} The API response
+     */
     async createOrUpdatePerson(args) {
components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (2)

22-27: Add pattern validation for ARR field.

Consider adding pattern validation to ensure proper currency format input.

 arr: {
   type: "string",
   label: "Annual Recurring Revenue (ARR)",
   description: "The annual recurring revenue of the company, as a currency value.",
+  pattern: "^\\$?\\d+(\\.\\d{2})?$",
+  placeholder: "$1000.00",
   optional: true,
 },

77-88: Clarify CSM field descriptions.

The descriptions for CSM fields still use the unexplained abbreviation "POC". Consider updating for clarity.

 csmFirstName: {
   type: "string",
   label: "CSM First Name",
-  description: "The first name of the POC of the company.",
+  description: "The first name of the Customer Success Manager (CSM) of the company.",
   optional: true,
 },
 csmLastName: {
   type: "string",
   label: "CSM Last Name",
-  description: "The last name of the POC of the company.",
+  description: "The last name of the Customer Success Manager (CSM) of the company.",
   optional: true,
 },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9b34452 and c5a99d4.

📒 Files selected for processing (2)
  • components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (1 hunks)
  • components/gainsight_nxt/gainsight_nxt.app.mjs (1 hunks)
🔇 Additional comments (5)
components/gainsight_nxt/gainsight_nxt.app.mjs (2)

1-6: LGTM! Standard app setup.

The import and app definition follow Pipedream's conventions.


44-60: Refer to previous review comment about company methods.

A previous review already suggested extracting the common path to avoid duplication.

components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (3)

1-8: LGTM! Component metadata is well-structured.

The component metadata follows best practices with:

  • Descriptive name and key
  • Documentation link in description
  • Appropriate version numbering

1-1: Verify Gainsight NXT API integration.

Let's verify that the app methods match the API documentation.

Also applies to: 119-122, 129-132

✅ Verification successful

API integration is properly implemented and matches documentation

The verification confirms:

  • Both createCompany and updateCompany methods are correctly implemented using appropriate HTTP methods (POST and PUT)
  • API endpoints use the correct path /data/objects/Company
  • Authentication is properly configured using customer domain and access key
  • Content type and accept headers are set correctly for JSON payloads
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify app methods and API integration

# Check if app methods are implemented
echo "Checking app implementation..."
rg -A 5 "updateCompany|createCompany" "components/gainsight_nxt/gainsight_nxt.app.mjs"

# Check for API endpoint configuration
echo "Checking API configuration..."
rg -A 5 "baseUrl|auth" "components/gainsight_nxt/gainsight_nxt.app.mjs"

Length of output: 1125


96-144: ⚠️ Potential issue

Enhance input validation and error handling.

The run method would benefit from additional validation and improved error handling:

  1. Validate required fields before API calls
  2. Validate date formats if provided
  3. Improve error handling and logging
  4. Ensure consistent result handling
 async run({ $ }) {
+  // Validate required fields
+  if (!this.name?.trim()) {
+    throw new Error("Company name is required");
+  }
+
+  // Validate date formats if provided
+  const dateFields = {
+    originalContractDate: this.originalContractDate,
+    renewalDate: this.renewalDate,
+  };
+  for (const [field, value] of Object.entries(dateFields)) {
+    if (value && !/^\d{4}-\d{2}-\d{2}$/.test(value)) {
+      throw new Error(`Invalid date format for ${field}. Expected: YYYY-MM-DD`);
+    }
+  }

   const data = {
     records: [
       {
         Name: this.name,
         Industry: this.industry,
         ARR: this.arr,
         Employees: this.employees,
         LifecycleInWeeks: this.lifecycleInWeeks,
         OriginalContractDate: this.originalContractDate,
         RenewalDate: this.renewalDate,
         Stage: this.stage,
         Status: this.status,
         CSMFirstName: this.csmFirstName,
         CSMLastName: this.csmLastName,
         ParentCompanyName: this.parentCompanyName,
       },
     ],
   };

   let summary = "";
   let result;
   try {
     const updateReq = await this.app.updateCompany({
       $,
       data,
     });
-    result = updateReq;
-    summary = updateReq.result === true
+    if (!updateReq?.result) {
+      throw new Error("Update failed: Company not found");
+    }
+    result = updateReq;
+    summary = updateReq.result === true
       ? `Successfully updated company '${this.name}'`
       : `Error updating company '${this.name}'`;
   }
   catch (err) {
+    // Log the update error for debugging
+    console.log("Update failed:", err.message);
+
     const createReq = await this.app.createCompany({
       $,
       data,
     });
+    if (!createReq?.result) {
+      throw new Error("Failed to create company");
+    }
     result = createReq;
     summary = createReq.result === true
       ? `Successfully created company '${this.name}'`
       : `Error creating company '${this.name}'`;
   }

   $.export(
     "$summary",
     summary,
   );
   return result;
 },

Likely invalid or redundant comment.

components/gainsight_nxt/gainsight_nxt.app.mjs Outdated Show resolved Hide resolved
michelle0927
michelle0927 previously approved these changes Oct 29, 2024
Copy link
Collaborator

@michelle0927 michelle0927 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
components/gainsight_nxt/common/utils.mjs (1)

1-22: Consider adding comprehensive input validation and documentation.

As these utilities are critical for API data transformation:

  1. Add JSDoc documentation describing expected input/output formats
  2. Consider adding input schema validation (e.g., using Zod or Joi)
  3. Add unit tests covering various edge cases

Would you like me to:

  1. Generate JSDoc documentation for these functions?
  2. Provide example schema validation implementation?
  3. Create unit test cases covering edge cases?
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c5a99d4 and 3c548be.

📒 Files selected for processing (2)
  • components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (1 hunks)
  • components/gainsight_nxt/common/utils.mjs (1 hunks)
🔇 Additional comments (2)
components/gainsight_nxt/actions/create-or-update-company/create-or-update-company.mjs (2)

86-132: Well-implemented 'run' method for creating or updating companies

The run method efficiently handles the logic for updating an existing company and gracefully falls back to creating a new one if the update fails. The control flow and error handling ensure that the action behaves reliably.


78-84: Flexible addition of extra parameters via 'additionalOptions'

The additionalOptions prop allows users to pass extra parameters dynamically, enhancing the flexibility and extensibility of the action.

Comment on lines +1 to +7
function optionalParseAsJSON(value) {
try {
return JSON.parse(value);
} catch (e) {
return value;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing error handling and type safety.

While the function provides basic JSON parsing with fallback, consider these improvements for better robustness:

  1. Add input validation
  2. Handle undefined/null values
  3. Add error logging for debugging
-function optionalParseAsJSON(value) {
+function optionalParseAsJSON(value) {
+  if (value == null) return value;
+  if (typeof value !== 'string') return value;
   try {
     return JSON.parse(value);
   } catch (e) {
+    console.debug(`Failed to parse JSON: ${e.message}`);
     return 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.

Suggested change
function optionalParseAsJSON(value) {
try {
return JSON.parse(value);
} catch (e) {
return value;
}
}
function optionalParseAsJSON(value) {
if (value == null) return value;
if (typeof value !== 'string') return value;
try {
return JSON.parse(value);
} catch (e) {
console.debug(`Failed to parse JSON: ${e.message}`);
return value;
}
}

Comment on lines +9 to +22
export function parseObjectEntries(value) {
const obj = typeof value === "string"
? JSON.parse(value)
: value;
return Object.fromEntries(
Object.entries(obj).map(([
key,
value,
]) => [
key,
optionalParseAsJSON(value),
]),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add defensive programming measures.

The function needs better error handling and input validation to prevent runtime errors:

  1. Direct JSON.parse call could throw on invalid JSON
  2. No null/undefined checks before accessing Object methods
  3. No validation of object structure

Apply these improvements:

 export function parseObjectEntries(value) {
+  if (value == null) {
+    throw new Error('Input value cannot be null or undefined');
+  }
+
   const obj = typeof value === "string"
-    ? JSON.parse(value)
+    ? optionalParseAsJSON(value)
     : value;
+
+  if (typeof obj !== 'object' || Array.isArray(obj)) {
+    throw new Error('Input must be an object or valid JSON object string');
+  }
+
   return Object.fromEntries(
     Object.entries(obj).map(([
       key,
       value,
     ]) => [
       key,
       optionalParseAsJSON(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.

Suggested change
export function parseObjectEntries(value) {
const obj = typeof value === "string"
? JSON.parse(value)
: value;
return Object.fromEntries(
Object.entries(obj).map(([
key,
value,
]) => [
key,
optionalParseAsJSON(value),
]),
);
}
export function parseObjectEntries(value) {
if (value == null) {
throw new Error('Input value cannot be null or undefined');
}
const obj = typeof value === "string"
? optionalParseAsJSON(value)
: value;
if (typeof obj !== 'object' || Array.isArray(obj)) {
throw new Error('Input must be an object or valid JSON object string');
}
return Object.fromEntries(
Object.entries(obj).map(([
key,
value,
]) => [
key,
optionalParseAsJSON(value),
]),
);
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
components/gainsight_nxt/gainsight_nxt.app.mjs (1)

61-67: Standardize API version and add error handling for person creation

The person management endpoint uses a different path pattern (/peoplemgmt/v1.0/) compared to other endpoints. Consider:

  1. Standardizing API version across all endpoints
  2. Adding specific error handling for common failure cases

Consider implementing:

  1. API version constant
  2. Specific error handling for duplicate records
+    static API_VERSION = 'v1.0';
+
     async createOrUpdatePerson(args) {
+      try {
         return this._makeRequest({
-          path: "/peoplemgmt/v1.0/people",
+          path: `/peoplemgmt/${this.constructor.API_VERSION}/people`,
           method: "PUT",
           ...args,
         });
+      } catch (error) {
+        if (error.response?.status === 409) {
+          // Handle duplicate person
+          throw new Error('Person already exists with different identifier');
+        }
+        throw error;
+      }
     },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3c548be and 361a527.

📒 Files selected for processing (1)
  • components/gainsight_nxt/gainsight_nxt.app.mjs (1 hunks)

Comment on lines +44 to +60
async updateCompany(args) {
return this._makeRequest({
path: "/data/objects/Company",
method: "PUT",
params: {
keys: "Name",
},
...args,
});
},
async createCompany(args) {
return this._makeRequest({
path: "/data/objects/Company",
method: "POST",
...args,
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add retry logic for company operations

The company creation/update methods could benefit from retry logic with exponential backoff for transient failures.

Consider implementing a retry wrapper:

+    async _withRetry(operation, maxRetries = 3) {
+      for (let i = 0; i < maxRetries; i++) {
+        try {
+          return await operation();
+        } catch (error) {
+          if (i === maxRetries - 1 || !this._isRetryable(error)) throw error;
+          await new Promise(r => setTimeout(r, Math.pow(2, i) * 1000));
+        }
+      }
+    },
+
     async updateCompany(args) {
-      return this._makeRequest({
+      return this._withRetry(() => this._makeRequest({
         path: "/data/objects/Company",
         method: "PUT",
         params: {
           keys: "Name",
         },
         ...args,
-      });
+      }));
     },

Committable suggestion was skipped due to low confidence.

Comment on lines +26 to +43
async _makeRequest({
$ = this,
path,
headers = {},
...otherOpts
} = {}) {
return axios($, {
...otherOpts,
url: this._baseUrl() + path,
headers: {
"content-type": "application/json",
"accept": "application/json, text/plain, */*",
"accept-language": "en-GB,en-US;q=0.9,en;q=0.8",
"accesskey": `${this.$auth.access_key}`,
...headers,
},
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add path parameter validation in _makeRequest

The path parameter is directly concatenated with the base URL, which could lead to path traversal issues. Consider adding validation to ensure the path:

  1. Starts with a forward slash
  2. Contains no relative path segments (../)
  3. Contains only allowed characters

Add path validation:

     async _makeRequest({
       $ = this,
       path,
       headers = {},
       ...otherOpts
     } = {}) {
+      if (!path || typeof path !== 'string' || !path.startsWith('/') || 
+          path.includes('../') || !/^[\w\-./]+$/.test(path)) {
+        throw new Error('Invalid path parameter');
+      }
       return axios($, {
         ...otherOpts,
         url: this._baseUrl() + path,
📝 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.

Suggested change
async _makeRequest({
$ = this,
path,
headers = {},
...otherOpts
} = {}) {
return axios($, {
...otherOpts,
url: this._baseUrl() + path,
headers: {
"content-type": "application/json",
"accept": "application/json, text/plain, */*",
"accept-language": "en-GB,en-US;q=0.9,en;q=0.8",
"accesskey": `${this.$auth.access_key}`,
...headers,
},
});
},
async _makeRequest({
$ = this,
path,
headers = {},
...otherOpts
} = {}) {
if (!path || typeof path !== 'string' || !path.startsWith('/') ||
path.includes('../') || !/^[\w\-./]+$/.test(path)) {
throw new Error('Invalid path parameter');
}
return axios($, {
...otherOpts,
url: this._baseUrl() + path,
headers: {
"content-type": "application/json",
"accept": "application/json, text/plain, */*",
"accept-language": "en-GB,en-US;q=0.9,en;q=0.8",
"accesskey": `${this.$auth.access_key}`,
...headers,
},
});
},

Comment on lines +13 to +18
return data?.filter?.((obj) => obj.objectType === "CUSTOM").map(( {
label, objectName,
}) => ({
label,
value: objectName,
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the code analysis, I can now provide a final response:

Remove the objectType filter to support all valid object types

The current implementation filters objects by objectType === "CUSTOM", but the API endpoints show that the integration handles various object types including built-in ones like "Company". The filter should be removed as it may exclude valid objects that can be operated on through the /data/objects/{objectName} endpoints.

  • Remove the filter at line 13: data?.filter?.((obj) => obj.objectType === "CUSTOM")
  • Keep only the mapping part to return all available objects: data?.map(({label, objectName}) => ({label, value: objectName}))
🔗 Analysis chain

Reconsider the custom object type filter

Based on the previous discussion, several custom object types may not be explicitly classified as "CUSTOM", which could result in missing valid objects. Consider either:

  1. Removing the filter to show all available objects, or
  2. Validating the complete list of object types to ensure no valid objects are excluded

Let's verify the object types:


Let me search for more specific information about Gainsight NXT's object types.


Let me check the API implementation and any test files that might reveal the expected object types.


Let me fix the regex and try again.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the available object types in the API response
rg -A 5 "objectType.*=.*" .

Length of output: 17301


Script:

#!/bin/bash
# Search for any references to object types or API responses in Gainsight NXT files
rg -A 5 "objectType|getObjects|listObjects" components/gainsight_nxt/

Length of output: 534


Script:

#!/bin/bash
# Search for API calls and test files related to object types in Gainsight NXT
rg -g "*.{mjs,js,test.js}" -B 3 -A 5 "\/objects|\/custom-objects|objectTypes" components/gainsight_nxt/

Length of output: 210


Script:

#!/bin/bash
# Search for API calls and test files related to object types in Gainsight NXT
rg -g "*.{mjs,js,test.js}" -B 3 -A 5 "(objects|custom-objects|objectTypes)" components/gainsight_nxt/

# Also search for any API-related code that might show object type handling
ast-grep --pattern 'async $FUNC($$$) {
  $$$
  const $_ = await this.$_.$_($$$)
  $$$
}' components/gainsight_nxt/

Length of output: 3201

@GTFalcao
Copy link
Collaborator Author

GTFalcao commented Nov 6, 2024

/approve

@GTFalcao GTFalcao merged commit 2989ea2 into master Nov 6, 2024
12 checks passed
@GTFalcao GTFalcao deleted the issue-14373 branch November 6, 2024 17:00
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.

[Components] gainsight_nxt
2 participants