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

API implementation for Discord module to allow multiple guilds. #8

Open
wants to merge 15 commits into
base: staging
Choose a base branch
from

Conversation

benrobson
Copy link
Member

@benrobson benrobson commented Jan 14, 2025

Depends on ModularSoftAU/DevoteMe-API#13

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a new configuration command for server administrators to manage channel settings
    • Implemented dynamic tenant handling for devotion and VOTD messages
    • Added interactive buttons to mark devotion and VOTD as completed
  • Improvements

    • Enhanced error handling across multiple commands and controllers
    • Updated dependencies and Node.js version
    • Improved logging and API integration
  • Bug Fixes

    • Fixed import path issues by adding .js extensions
  • Chores

    • Updated project dependencies
    • Migrated to Node.js version 20.0.0

Copy link

coderabbitai bot commented Jan 14, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces several enhancements to a Discord bot's functionality, focusing on tenant management, configuration, and improved error handling. The changes include adding new commands for server configuration, updating controllers to support multi-tenant operations, implementing dynamic channel selection, and introducing interactive message buttons. The project has also been upgraded to Node.js version 20 and updated various dependencies to their latest versions.

Changes

File Change Summary
.env.example Added new APIKEY environment variable
.gitignore Added .env, removed config.json
commands/configure.mjs New ConfigureCommand for channel configuration
commands/devotion.mjs Updated import statements, added error handling
commands/votd.mjs Updated import path with .js extension
config.json.example Deleted configuration template
controller/devotionController.js Added tenant-specific devotion sending, button interaction
controller/tenantController.js Added getTenants and getTenantConfiguration functions
controller/votdController.js Implemented multi-tenant VOTD sending, added button interaction
cron/daily.js Updated import paths, changed cron job times to 6:00 AM
listeners/guildCreate.js New listener for handling guild creation and tenant registration
package.json Updated Node.js version to 20, upgraded dependencies

Sequence Diagram

sequenceDiagram
    participant User
    participant DiscordBot
    participant TenantController
    participant APIServer

    User->>DiscordBot: Trigger Configure Command
    DiscordBot->>TenantController: Get Tenant Configuration
    TenantController->>APIServer: Fetch Tenant Details
    APIServer-->>TenantController: Return Tenant Data
    TenantController-->>DiscordBot: Provide Configuration
    DiscordBot->>User: Display/Update Channel Settings
Loading

Poem

🐰 A Rabbit's Ode to Bot Enhancement 🤖

With tenants and channels, our bot grows wise,
Buttons that sparkle, commands that rise!
From config to cron, we've danced with glee,
Upgrading our code, setting features free!
Hop, hop, hurray for version twenty! 🎉


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@benrobson
Copy link
Member Author

@coderabbitai review

Copy link

coderabbitai bot commented Jan 14, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@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: 12

🧹 Nitpick comments (12)
controller/devotionController.js (2)

68-68: Remove unnecessary await keyword

The get method on Collection is synchronous and does not return a promise. Therefore, await is not necessary and should be removed to prevent confusion.

Apply this diff to remove the await:

-const guild = await client.guilds.cache.get(tenant.tenantId);
+const guild = client.guilds.cache.get(tenant.tenantId);

165-167: Use consistent async/await syntax

Mixing await with .then() can lead to confusion. Use async/await consistently for better readability and error handling.

Apply this diff to refactor the code:

-await client.users.fetch(process.env.ADMIN_USER_ID).then((admin) => {
-  admin.send({ embeds: [errorEmbed] });
-});
+const admin = await client.users.fetch(process.env.ADMIN_USER_ID);
+await admin.send({ embeds: [errorEmbed] });
controller/votdController.js (2)

62-62: Remove unnecessary await keyword

The get method on Collection is synchronous. Remove the await to prevent misunderstanding.

Apply this diff to remove the await:

-const guild = await client.guilds.cache.get(tenant.tenantId);
+const guild = client.guilds.cache.get(tenant.tenantId);

166-168: Use consistent async/await syntax

Maintain consistency by using async/await syntax instead of mixing with .then().

Apply this diff to refactor the code:

-await client.users.fetch(process.env.ADMIN_USER_ID).then((admin) => {
-  admin.send({ embeds: [errorEmbed] });
-});
+const admin = await client.users.fetch(process.env.ADMIN_USER_ID);
+await admin.send({ embeds: [errorEmbed] });
commands/configure.mjs (2)

62-68: Ensure the bot has the necessary permissions

The check for administrator permissions is good, but consider verifying that the bot also has the necessary permissions to send messages and manage channels, to prevent unexpected errors during execution.


135-161: Improve error handling in updateChannelConfig

Include more detailed error responses from the API to provide better feedback when the configuration update fails. Additionally, handle network errors that may occur during the fetch request.

Update the error handling as follows:

 async updateChannelConfig(tenantId, channelType, channelId) {
   try {
     const fetchURL = `${process.env.APIURL}/api/tenant/update`;
     const response = await fetch(fetchURL, {
       method: "POST",
       headers: {
         "x-access-token": process.env.APIKEY,
         "Content-Type": "application/json",
       },
       body: JSON.stringify({
         tenantId: tenantId,
         [channelType]: channelId,
       }),
     });

     if (!response.ok) {
+      const errorData = await response.json();
+      throw new Error(`Failed to update ${channelType}: ${errorData.message || response.statusText}`);
     }

     const data = await response.json();
     if (!data.success) {
       throw new Error(`API response: ${data.message}`);
     }
   } catch (error) {
     console.error(`Error updating channel configuration: ${error.message}`);
     throw new Error("Failed to update channel configuration.");
   }
 }
cron/daily.js (2)

6-6: Document the schedule change impact

The cron job schedule has been changed from 7:00 AM to 6:00 AM. Please update the documentation to reflect this change and its potential impact on users.


Line range hint 9-24: Add timezone validation

The cron jobs rely on the TZ environment variable but there's no validation to ensure it's set correctly. Consider adding validation to prevent runtime issues.

 export default async function dailyCron(client) {
+    if (!process.env.TZ) {
+        console.error('TZ environment variable is not set. Defaulting to UTC.');
+        process.env.TZ = 'UTC';
+    }
+
     const sendDevotionTask = cron.schedule('0 6 * * *', async function () {
listeners/guildCreate.js (1)

26-36: Enhance error handling and logging

The error handling could be more informative while being security-conscious:

  1. Add error codes/types
  2. Avoid logging potentially sensitive response data
       if (response.ok && responseData.success) {
         console.log(
-          `Successfully created tenant for guild: ${guild.name} (ID: ${guild.id}). Response: `,
-          responseData
+          `Successfully created tenant for guild: ${guild.name} (ID: ${guild.id})`
         );
       } else {
         console.error(
-          `Failed to create tenant for guild: ${guild.name} (ID: ${guild.id}). Response status: ${response.status}, Response: `,
-          responseData
+          `Failed to create tenant for guild: ${guild.name} (ID: ${guild.id}). Status: ${response.status}`,
+          responseData.error || 'Unknown error'
         );
commands/devotion.mjs (1)

29-48: Enhance error handling with specific messages and monitoring

While the error handling is good, consider:

  1. Categorizing errors to provide more specific user feedback
  2. Adding error tracking for monitoring
     try {
       const devotionData = await getDevotion();
       const embed = await compileDevotionMessage(devotionData);
       await interaction.reply({ embeds: [embed] });
     } catch (error) {
-      console.error(error);
+      // Add error tracking
+      const errorId = Date.now().toString(36);
+      console.error(`Error ID: ${errorId}`, error);
+
+      const errorMessage = error.code === 'API_UNAVAILABLE'
+        ? 'The devotion service is temporarily unavailable.'
+        : 'An error occurred while processing the devotion.';
 
       const errorEmbed = new MessageEmbed()
         .setColor("RED")
         .setTitle("Error")
-        .setDescription(
-          "An error occurred while processing the devotion. Please try again later."
-        );
+        .setDescription(`${errorMessage} Please try again later.`)
+        .setFooter({ text: `Error ID: ${errorId}` });
 
       await interaction.reply({ embeds: [errorEmbed], ephemeral: true });
     }
.env.example (1)

3-3: Document the APIKEY's purpose and format

While adding the APIKEY environment variable is good, its purpose and format should be documented for other developers.

Add a comment above the APIKEY line:

 APIURL=http://example.com
+# API authentication key for tenant management
 APIKEY=KEY
package.json (1)

22-25: Consider upgrading discord.js to v14

While the minor version updates are good, discord.js v14 offers significant improvements and better TypeScript support. However, this would require additional changes due to breaking changes in v14.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b00a26 and 448d18c.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (12)
  • .env.example (1 hunks)
  • .gitignore (1 hunks)
  • commands/configure.mjs (1 hunks)
  • commands/devotion.mjs (2 hunks)
  • commands/votd.mjs (1 hunks)
  • config.json.example (0 hunks)
  • controller/devotionController.js (2 hunks)
  • controller/tenantController.js (1 hunks)
  • controller/votdController.js (2 hunks)
  • cron/daily.js (2 hunks)
  • listeners/guildCreate.js (1 hunks)
  • package.json (1 hunks)
💤 Files with no reviewable changes (1)
  • config.json.example
✅ Files skipped from review due to trivial changes (1)
  • commands/votd.mjs
🔇 Additional comments (5)
controller/devotionController.js (1)

75-75: Avoid logging sensitive information

Logging tenantConfig may expose sensitive configuration details. Consider removing this log statement or ensure that no sensitive data is being logged.

[security_issue]

Apply this diff to remove the log statement:

-console.log(tenantConfig);
controller/votdController.js (1)

69-69: Avoid logging sensitive information

Refrain from logging the entire tenantConfig object to prevent potential exposure of sensitive data.

[security_issue]

Apply this diff to remove or sanitize the log statement:

-console.log(tenantConfig);
.gitignore (2)

2-2: LGTM! Good security practice.

Adding .env to .gitignore ensures sensitive configuration data like API keys remain secure.


1-2: Verify the removal of config.json from .gitignore

Since config.json is no longer ignored, please ensure it doesn't contain any sensitive information and is properly templated for multi-tenant support.

package.json (1)

15-15: Verify Node.js 20 compatibility

The upgrade from Node.js 16 to 20 is a significant change that might introduce breaking changes. Please ensure:

  1. All dependencies are compatible with Node.js 20
  2. All ES modules syntax is updated for Node.js 20
  3. The deployment environment supports Node.js 20
✅ Verification successful

Node.js 20 compatibility verified

The codebase is fully compatible with Node.js 20:

  • All files use modern ES modules syntax
  • Dependencies are compatible with Node.js 20
  • No deprecated Node.js features are in use
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential compatibility issues
echo "Checking package.json for potential Node.js 20 compatibility issues:"
# Look for deprecated Node.js features or syntax
rg -l "require\(" . --type js
# Check for ES modules usage
rg -l "import |export " . --type js

Length of output: 388


Script:

#!/bin/bash
# Check import/export syntax and package.json dependencies
echo "Checking import/export syntax:"
rg "^(import|export)" -A 1 --type js

echo -e "\nChecking dependencies:"
cat package.json | jq '.dependencies'

Length of output: 3118

Comment on lines 65 to 67
for (const tenant of tenants) {
console.log(tenant.tenantId);

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check for null or undefined tenant in the loop

Before accessing tenant.tenantId, ensure that tenant is not null or undefined to prevent runtime errors.

Apply this diff to add a null check:

 for (const tenant of tenants) {
+  if (!tenant) {
+    console.log("Tenant is null or undefined.");
+    continue;
+  }
   console.log(tenant.tenantId);
📝 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
for (const tenant of tenants) {
console.log(tenant.tenantId);
for (const tenant of tenants) {
if (!tenant) {
console.log("Tenant is null or undefined.");
continue;
}
console.log(tenant.tenantId);

Comment on lines 60 to 63
let tenants = await getTenants();
if (!Array.isArray(tenants)) {
tenants = [tenants];
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential null or undefined tenants

Ensure that the tenants variable is not null or undefined before processing. If getTenants() returns null or undefined, the current check may not handle it properly, potentially leading to errors when iterating over tenants.

Apply this diff to enhance the null check:

 let tenants = await getTenants();
-if (!Array.isArray(tenants)) {
+if (!tenants) {
+  console.log("No tenants found.");
+  return;
+} else if (!Array.isArray(tenants)) {
   tenants = [tenants];
 }
📝 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
let tenants = await getTenants();
if (!Array.isArray(tenants)) {
tenants = [tenants];
}
let tenants = await getTenants();
if (!tenants) {
console.log("No tenants found.");
return;
} else if (!Array.isArray(tenants)) {
tenants = [tenants];
}

Comment on lines 74 to 77
const tenantConfig = await getTenantConfiguration(tenant.tenantId);
console.log(tenantConfig);

const devotionChannel = guild.channels.cache.get(
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle possible null or undefined tenantConfig

After fetching the tenant configuration, add a check to ensure tenantConfig exists before accessing its properties to prevent errors if the configuration is missing.

Apply this diff to add the null check:

 const tenantConfig = await getTenantConfiguration(tenant.tenantId);
+if (!tenantConfig) {
+  console.log("Tenant configuration not found.");
+  continue;
+}
📝 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
const tenantConfig = await getTenantConfiguration(tenant.tenantId);
console.log(tenantConfig);
const devotionChannel = guild.channels.cache.get(
const tenantConfig = await getTenantConfiguration(tenant.tenantId);
if (!tenantConfig) {
console.log("Tenant configuration not found.");
continue;
}
console.log(tenantConfig);
const devotionChannel = guild.channels.cache.get(

Comment on lines 56 to 60
let tenants = await getTenants();
if (!Array.isArray(tenants)) {
tenants = [tenants];
}

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential null or undefined tenants

Ensure that the tenants variable is not null or undefined before processing. This prevents errors during the iteration when getTenants() returns null or undefined.

Apply this diff to enhance the null check:

 let tenants = await getTenants();
-if (!Array.isArray(tenants)) {
+if (!tenants) {
+  console.log("No tenants found.");
+  return;
+} else if (!Array.isArray(tenants)) {
   tenants = [tenants];
 }
📝 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
let tenants = await getTenants();
if (!Array.isArray(tenants)) {
tenants = [tenants];
}
let tenants = await getTenants();
if (!tenants) {
console.log("No tenants found.");
return;
} else if (!Array.isArray(tenants)) {
tenants = [tenants];
}

Comment on lines +61 to +66
for (const tenant of tenants) {
const guild = await client.guilds.cache.get(tenant.tenantId);
if (!guild) {
console.log("Guild not found.");
continue;
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check for null or undefined tenant in the loop

Add a null check for each tenant before accessing its properties to avoid runtime errors.

Apply this diff to add the null check:

 for (const tenant of tenants) {
+  if (!tenant) {
+    console.log("Tenant is null or undefined.");
+    continue;
+  }
   const guild = client.guilds.cache.get(tenant.tenantId);
📝 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
for (const tenant of tenants) {
const guild = await client.guilds.cache.get(tenant.tenantId);
if (!guild) {
console.log("Guild not found.");
continue;
}
for (const tenant of tenants) {
if (!tenant) {
console.log("Tenant is null or undefined.");
continue;
}
const guild = await client.guilds.cache.get(tenant.tenantId);
if (!guild) {
console.log("Guild not found.");
continue;
}

Comment on lines +126 to +132
console.error(error);
const embed = new MessageEmbed()
.setColor("RED")
.setTitle("Error")
.setDescription("There was an error while configuring the channels.");
return interaction.reply({ embeds: [embed], ephemeral: true });
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure error replies are only sent once

When an error occurs, check if an interaction response has already been sent before attempting to reply again. This prevents potential exceptions due to multiple replies.

Apply this diff to check the interaction status:

 try {
   // ... existing code ...
 } catch (error) {
   console.error(error);
   const embed = new MessageEmbed()
     .setColor("RED")
     .setTitle("Error")
     .setDescription("There was an error while configuring the channels.");
-  return interaction.reply({ embeds: [embed], ephemeral: true });
+  if (interaction.replied || interaction.deferred) {
+    return interaction.followUp({ embeds: [embed], ephemeral: true });
+  } else {
+    return interaction.reply({ embeds: [embed], ephemeral: true });
+  }
 }
📝 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
console.error(error);
const embed = new MessageEmbed()
.setColor("RED")
.setTitle("Error")
.setDescription("There was an error while configuring the channels.");
return interaction.reply({ embeds: [embed], ephemeral: true });
}
console.error(error);
const embed = new MessageEmbed()
.setColor("RED")
.setTitle("Error")
.setDescription("There was an error while configuring the channels.");
if (interaction.replied || interaction.deferred) {
return interaction.followUp({ embeds: [embed], ephemeral: true });
} else {
return interaction.reply({ embeds: [embed], ephemeral: true });
}
}

Comment on lines +8 to +10
const fetchURL = `${process.env.APIURL}/api/tenant/create`;
console.log(`Making API request to: ${fetchURL}`);

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid logging sensitive URLs

The API URL being logged could expose sensitive information about your infrastructure. Consider removing or redacting sensitive parts of the URL in logs.

-    console.log(`Making API request to: ${fetchURL}`);
+    console.log('Making API request to create tenant');
📝 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
const fetchURL = `${process.env.APIURL}/api/tenant/create`;
console.log(`Making API request to: ${fetchURL}`);
const fetchURL = `${process.env.APIURL}/api/tenant/create`;
console.log('Making API request to create tenant');

Comment on lines +11 to +22
try {
const response = await fetch(fetchURL, {
method: "POST",
headers: {
"x-access-token": process.env.APIKEY,
"Content-Type": "application/json",
},
body: JSON.stringify({
tenantId: guild.id,
tenantName: guild.name,
}),
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add environment variable validation and retry mechanism

The code assumes environment variables are set and the API is always available. Consider:

  1. Validating environment variables before use
  2. Implementing a retry mechanism for failed API calls
+    if (!process.env.APIURL || !process.env.APIKEY) {
+        throw new Error('Required environment variables APIURL and APIKEY must be set');
+    }
+
+    const MAX_RETRIES = 3;
+    const RETRY_DELAY = 1000;
+    
+    async function makeRequestWithRetry(url, options, retries = 0) {
+        try {
+            return await fetch(url, options);
+        } catch (error) {
+            if (retries < MAX_RETRIES) {
+                await new Promise(resolve => setTimeout(resolve, RETRY_DELAY));
+                return makeRequestWithRetry(url, options, retries + 1);
+            }
+            throw error;
+        }
+    }
+
     try {
-      const response = await fetch(fetchURL, {
+      const response = await makeRequestWithRetry(fetchURL, {

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +27 to +51
export async function getTenantConfiguration(tenantId) {
try {
const fetchURL = `${process.env.APIURL}/api/tenant/configuration/get?id=${tenantId}`;
const response = await fetch(fetchURL, {
method: "GET",
headers: {
"x-access-token": process.env.APIKEY,
},
});

if (!response.ok) {
throw new Error("Failed to fetch current configuration.");
}

const data = await response.json();
if (!data.success) {
throw new Error(`API response: ${data.message}`);
}

return data.data[0];
} catch (error) {
console.error(`Error fetching configuration: ${error.message}`);
throw new Error("Failed to fetch current configuration.");
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation and improve error handling

The getTenantConfiguration function needs:

  1. Input validation for tenantId
  2. Reuse of API request utility
  3. Safe array access
 export async function getTenantConfiguration(tenantId) {
+    if (!tenantId) {
+        throw new Error('Tenant ID is required');
+    }
+
   try {
     const fetchURL = `${process.env.APIURL}/api/tenant/configuration/get?id=${tenantId}`;
-    const response = await fetch(fetchURL, {
-      method: "GET",
-      headers: {
-        "x-access-token": process.env.APIKEY,
-      },
-    });
-
-    if (!response.ok) {
-      throw new Error("Failed to fetch current configuration.");
-    }
-
-    const data = await response.json();
-    if (!data.success) {
-      throw new Error(`API response: ${data.message}`);
-    }
+    const data = await makeApiRequest(fetchURL, { method: "GET" });
 
-    return data.data[0];
+    if (!data.data?.length) {
+        throw new Error(`No configuration found for tenant ${tenantId}`);
+    }
+    return data.data[0];
   } catch (error) {
     console.error(`Error fetching configuration: ${error.message}`);
-    throw new Error("Failed to fetch current configuration.");
+    throw new Error(`Failed to fetch configuration for tenant ${tenantId}`);
   }
 }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 1 to 25
export async function getTenants() {
try {
const fetchURL = `${process.env.APIURL}/api/tenant/get`;
const response = await fetch(fetchURL, {
method: "GET",
headers: {
"x-access-token": process.env.APIKEY,
},
});

if (!response.ok) {
throw new Error("Failed to fetch tenants.");
}

const data = await response.json();
if (!data.success) {
throw new Error(`API response: ${data.message}`);
}

return data.data[0];
} catch (error) {
console.error(`Error fetching configuration: ${error.message}`);
throw new Error("Failed to fetch tenants.");
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor tenant data access and validation

The getTenants function has several issues:

  1. Unsafe array access without validation
  2. Generic error messages
  3. Duplicate API call pattern
+const makeApiRequest = async (url, options) => {
+    const response = await fetch(url, {
+        ...options,
+        headers: {
+            "x-access-token": process.env.APIKEY,
+            ...options?.headers,
+        },
+    });
+
+    if (!response.ok) {
+        throw new Error(`API request failed with status ${response.status}`);
+    }
+
+    const data = await response.json();
+    if (!data.success) {
+        throw new Error(data.message || 'API request failed');
+    }
+
+    return data;
+};
+
 export async function getTenants() {
   try {
     const fetchURL = `${process.env.APIURL}/api/tenant/get`;
-    const response = await fetch(fetchURL, {
-      method: "GET",
-      headers: {
-        "x-access-token": process.env.APIKEY,
-      },
-    });
-
-    if (!response.ok) {
-      throw new Error("Failed to fetch tenants.");
-    }
-
-    const data = await response.json();
-    if (!data.success) {
-      throw new Error(`API response: ${data.message}`);
-    }
+    const data = await makeApiRequest(fetchURL, { method: "GET" });
 
-    return data.data[0];
+    if (!data.data?.length) {
+        throw new Error('No tenants found');
+    }
+    return data.data[0];
   } catch (error) {
     console.error(`Error fetching configuration: ${error.message}`);
     throw new Error("Failed to fetch tenants.");
   }
 }
📝 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 async function getTenants() {
try {
const fetchURL = `${process.env.APIURL}/api/tenant/get`;
const response = await fetch(fetchURL, {
method: "GET",
headers: {
"x-access-token": process.env.APIKEY,
},
});
if (!response.ok) {
throw new Error("Failed to fetch tenants.");
}
const data = await response.json();
if (!data.success) {
throw new Error(`API response: ${data.message}`);
}
return data.data[0];
} catch (error) {
console.error(`Error fetching configuration: ${error.message}`);
throw new Error("Failed to fetch tenants.");
}
}
const makeApiRequest = async (url, options) => {
const response = await fetch(url, {
...options,
headers: {
"x-access-token": process.env.APIKEY,
...options?.headers,
},
});
if (!response.ok) {
throw new Error(`API request failed with status ${response.status}`);
}
const data = await response.json();
if (!data.success) {
throw new Error(data.message || 'API request failed');
}
return data;
};
export async function getTenants() {
try {
const fetchURL = `${process.env.APIURL}/api/tenant/get`;
const data = await makeApiRequest(fetchURL, { method: "GET" });
if (!data.data?.length) {
throw new Error('No tenants found');
}
return data.data[0];
} catch (error) {
console.error(`Error fetching configuration: ${error.message}`);
throw new Error("Failed to fetch tenants.");
}
}

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.

1 participant