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

Include site category data in Readings schema #3884

Merged
merged 2 commits into from
Nov 24, 2024
Merged

Conversation

Baalmart
Copy link
Contributor

@Baalmart Baalmart commented Nov 24, 2024

Description

Include site category data in Readings schema

Changes Made

  • Include site category data in Readings schema
  • Created new bulk Scripts and also updated one old one.

Testing

  • Tested locally
  • Tested against staging environment
  • Relevant tests passed: [List test names]

Affected Services

  • Which services were modified:
    • Device Registry

Endpoints Ready for Testing

  • New endpoints ready for testing:
    • Get Readings

API Documentation Updated?

  • Yes, API documentation was updated
  • No, API documentation does not need updating

Additional Notes

Include site category data in Readings schema

Summary by CodeRabbit

  • New Features

    • Introduced a new schema for site categories, enhancing site detail categorization.
    • Added functionality to transform API response data for compatibility with a new API format.
    • Updated batch processing to support a new API version, including modified request URLs and data structures.
  • Bug Fixes

    • Improved error handling during API response transformations and data updates.
  • Documentation

    • Updated documentation to reflect changes in API interactions and data structures.

Sorry, something went wrong.

Copy link
Contributor

coderabbitai bot commented Nov 24, 2024

📝 Walkthrough

Walkthrough

The pull request introduces a new schema, categorySchema, which defines site categories with various fields and integrates it into the existing SiteDetailsSchema by adding a site_category field. Additionally, a new utility script, api-response-transformer.js, is created to transform API response data, enhancing data handling for site categories. The bulk-script.js file is updated to accommodate a transition to a new API version, modifying request URLs and data structures accordingly. These changes collectively enhance the data model and API interactions without altering existing functionalities.

Changes

File Path Change Summary
src/device-registry/models/Reading.js - Added const categorySchema defining site categories.
- Integrated site_category: { type: categorySchema } into SiteDetailsSchema.
src/device-registry/utils/scripts/api-response-transformer.js - Introduced transformApiResponse(inputData) for API response transformation.
- Added createRequestBody(apiResponse) for handling requests.
- Exported both functions.
src/device-registry/utils/scripts/bulk-script.js - Added function extractBeforeV2IncludingSlash(url) for URL processing.
- Updated GET and PUT request URLs to new API version.
- Modified data structure in PUT request to include api_code.

Possibly related PRs

Suggested reviewers

  • NicholasTurner23
  • BenjaminSsempala

Poem

In the realm of code where schemas grow,
A new categorySchema starts to glow.
With transformations and APIs anew,
Our data dances, fresh and true.
From v1 to v2, we boldly stride,
Enhancing our model, with pride we ride! 🌟


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

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 11.75%. Comparing base (b480324) to head (f60c184).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           staging    #3884   +/-   ##
========================================
  Coverage    11.74%   11.75%           
========================================
  Files          114      114           
  Lines        15254    15255    +1     
  Branches       274      274           
========================================
+ Hits          1792     1793    +1     
  Misses       13462    13462           
Files with missing lines Coverage Δ
src/device-registry/models/Reading.js 17.51% <100.00%> (+0.46%) ⬆️
---- 🚨 Try these New Features:

Copy link
Contributor

Device registry changes in this PR available for preview here

@Baalmart Baalmart merged commit 3916a3b into staging Nov 24, 2024
52 checks passed
@Baalmart Baalmart deleted the en-site-metadata branch November 24, 2024 14:42
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: 6

🧹 Outside diff range and nitpick comments (6)
src/device-registry/utils/scripts/api-response-transformer.js (3)

7-7: Remove unused dependency to streamline the code.

The is-empty package is imported but not utilized in the code. Consider removing it to reduce unnecessary dependencies and potential confusion.


33-36: Enhance 'extractValue' function to handle unexpected line formats gracefully.

The extractValue function assumes each line contains ': ' for splitting. If a line doesn't match this pattern, split(': ') could result in undefined values, leading to runtime errors. It's advisable to add a check to handle such cases.

Here's how you might modify the function:

 const extractValue = (line) => {
+  if (!line.includes(': ')) {
+    return { key: '', value: '' };
+  }
   const [key, value] = line.trim().split(': ');
   return { key: key.trim(), value: value.trim() };
 };

100-100: Clean up commented-out code to maintain clarity.

The line // console.log("the site _id", site._id); is commented out. Removing unnecessary commented code helps keep the codebase clean and improves readability.

src/device-registry/models/Reading.js (3)

23-44: Consider adding validation rules for critical fields

The categorySchema structure is well-organized, but could benefit from additional validation rules for critical fields:

Consider applying these validations:

 const categorySchema = new Schema(
   {
     area_name: { type: String },
     category: { type: String },
     highway: { type: String },
     landuse: { type: String },
-    latitude: { type: Number },
-    longitude: { type: Number },
+    latitude: { 
+      type: Number,
+      min: -90,
+      max: 90,
+      required: true
+    },
+    longitude: { 
+      type: Number,
+      min: -180,
+      max: 180,
+      required: true
+    },
     natural: { type: String },
-    search_radius: { type: Number },
+    search_radius: { 
+      type: Number,
+      min: 0,
+      required: true
+    },
     waterway: { type: String },
     tags: [
       {
         type: String,
         trim: true,
       },
     ],
   },
   {
     _id: false,
   }
 );

Line range hint 5-24: Consider adding indexes for frequently queried fields

The schema involves complex aggregation pipelines, particularly in the analytics functions. Adding strategic indexes could significantly improve query performance.

Consider adding these indexes to support the aggregation pipelines:

 ReadingsSchema.index({ device_id: 1, time: 1 }, { unique: true });
 ReadingsSchema.index({ device: 1, time: 1 }, { unique: true });
 ReadingsSchema.index({ site_id: 1, time: 1 }, { unique: true });
+// Add indexes for analytics queries
+ReadingsSchema.index({ "pm2_5.value": 1 });
+ReadingsSchema.index({ "pm10.value": 1 });
+ReadingsSchema.index({ "no2.value": 1 });

Line range hint 447-450: Add null checks and error handling for edge cases

The percentage difference calculations could potentially cause division by zero errors.

Consider adding safeguards:

 const calculatePercentageDiff = (current, previous) => {
-  if (previous === 0) return 0;
+  if (!current || !previous || previous === 0) return 0;
   return ((current - previous) / previous) * 100;
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b480324 and f60c184.

📒 Files selected for processing (3)
  • src/device-registry/models/Reading.js (2 hunks)
  • src/device-registry/utils/scripts/api-response-transformer.js (1 hunks)
  • src/device-registry/utils/scripts/bulk-script.js (2 hunks)
🔇 Additional comments (1)
src/device-registry/models/Reading.js (1)

68-68: Integration of site_category looks good

The site_category field is correctly integrated into the SiteDetailsSchema using the newly defined categorySchema.

Comment on lines +85 to +89
const url = "http://localhost:3000/api/v2/devices/sites/summary";
const config = {
headers: {
Authorization: "",
},
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

Use environment variables for configuration to enhance security and flexibility.

Hardcoding URLs and the Authorization header can lead to security risks and make the code less adaptable across different environments. Using environment variables or a configuration file allows for better security practices and easier configuration management.

Comment on lines +96 to +139
for (let i = 0; i < response.data.sites.length; i += 10) {
const batch = response.data.sites.slice(i, i + 10);
// Process batch of 10 items
batch.forEach(async (site) => {
// console.log("the site _id", site._id);
const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`;

//***MAKE THE GET REQUEST */
axios
.get(url, config)
.then((response) => {
// console.log("PUT response:", response.data);
const apiResponse = response.data;
const requestBody = createRequestBody(apiResponse);
// console.log("requestBody", requestBody);
const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`;
const data = {
site_category: requestBody,
};

// Make a PUT request
axios
.put(updateURL, data, config)
.then((response) => {
console.log("PUT response:", response.data);
})
.catch((error) => {
if (error.response) {
console.log(error.response.status);
console.log(error.response.data);
} else {
console.log(error.message);
}
});
})
.catch((error) => {
if (error.response) {
console.log(error.response.status);
console.log(error.response.data);
} else {
console.log(error.message);
}
});
});
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

Refactor asynchronous code to use async/await consistently for improved readability.

In the batch processing loop, you're mixing async functions with .then() and .catch() promise handling. For better readability and error handling, consider refactoring the code to use async/await throughout.

Here's how you could refactor the code:

 // Process batch of items
-batch.forEach(async (site) => {
+for (const site of batch) {
+  try {
+    const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`;
+
+    // Make the GET request
+    const response = await axios.get(url, config);
+    const apiResponse = response.data;
+    const requestBody = createRequestBody(apiResponse);
+
+    const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`;
+    const data = {
+      site_category: requestBody,
+    };
+
+    // Make the PUT request
+    const putResponse = await axios.put(updateURL, data, config);
+    console.log('PUT response:', putResponse.data);
+  } catch (error) {
+    if (error.response) {
+      console.log(error.response.status);
+      console.log(error.response.data);
+    } else {
+      console.log(error.message);
+    }
+  }
 }
📝 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 (let i = 0; i < response.data.sites.length; i += 10) {
const batch = response.data.sites.slice(i, i + 10);
// Process batch of 10 items
batch.forEach(async (site) => {
// console.log("the site _id", site._id);
const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`;
//***MAKE THE GET REQUEST */
axios
.get(url, config)
.then((response) => {
// console.log("PUT response:", response.data);
const apiResponse = response.data;
const requestBody = createRequestBody(apiResponse);
// console.log("requestBody", requestBody);
const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`;
const data = {
site_category: requestBody,
};
// Make a PUT request
axios
.put(updateURL, data, config)
.then((response) => {
console.log("PUT response:", response.data);
})
.catch((error) => {
if (error.response) {
console.log(error.response.status);
console.log(error.response.data);
} else {
console.log(error.message);
}
});
})
.catch((error) => {
if (error.response) {
console.log(error.response.status);
console.log(error.response.data);
} else {
console.log(error.message);
}
});
});
for (let i = 0; i < response.data.sites.length; i += 10) {
const batch = response.data.sites.slice(i, i + 10);
// Process batch of items
for (const site of batch) {
try {
const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`;
// Make the GET request
const response = await axios.get(url, config);
const apiResponse = response.data;
const requestBody = createRequestBody(apiResponse);
const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`;
const data = {
site_category: requestBody,
};
// Make the PUT request
const putResponse = await axios.put(updateURL, data, config);
console.log('PUT response:', putResponse.data);
} catch (error) {
if (error.response) {
console.log(error.response.status);
console.log(error.response.data);
} else {
console.log(error.message);
}
}
}

Comment on lines +37 to +41
const url = `http://localhost:3000/api/v2/devices/soft?id=${device._id}`;
const new_api_code = extractBeforeV2IncludingSlash(device.api_code);

const data = {
network: "airqo",
api_code: new_api_code,
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

Improve batch processing reliability and error handling

The current implementation has several potential issues:

  1. Using forEach with async operations doesn't guarantee sequential processing
  2. No rate limiting could overwhelm the API
  3. No progress tracking for large datasets

Consider this more robust approach:

- batch.forEach(async (device) => {
+ for (const device of batch) {
+   try {
      const url = `http://localhost:3000/api/v2/devices/soft?id=${device._id}`;
      const new_api_code = extractBeforeV2IncludingSlash(device.api_code);
+     if (!new_api_code) {
+       console.warn(`Skipping device ${device._id}: Invalid api_code`);
+       continue;
+     }
      const data = {
        api_code: new_api_code,
      };
+     await new Promise(resolve => setTimeout(resolve, 100)); // Rate limiting
+     const response = await axios.put(url, data, config);
+     console.log(`Updated device ${device._id}`);
+   } catch (error) {
+     console.error(`Failed to update device ${device._id}:`, error.message);
+   }
- });
+ }

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

/**
* Update each device to have the default network of VISIBLE_DEVICES_ONLY
*/

// Make a GET request
const url = "http://localhost:3000/api/v1/devices";
const url = "http://localhost:3000/api/v2/devices/summary";
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

Use environment configuration for API endpoints

Hardcoding the localhost URL makes the script environment-dependent and could lead to issues in different deployment environments.

Consider using environment variables:

- const url = "http://localhost:3000/api/v2/devices/summary";
+ const BASE_URL = process.env.API_BASE_URL || "http://localhost:3000";
+ const url = `${BASE_URL}/api/v2/devices/summary`;
📝 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 url = "http://localhost:3000/api/v2/devices/summary";
const BASE_URL = process.env.API_BASE_URL || "http://localhost:3000";
const url = `${BASE_URL}/api/v2/devices/summary`;

💡 Codebase verification

Based on the results from our investigation, I can now provide a conclusive response about the API version migration.

Migration from v1 to v2 endpoints is incomplete

  • Found commented out v1 route in src/device-registry/bin/server.js: // app.use("/api/v1/devices", require("@routes/v1"));
  • The v1 routes directory still exists and contains active code
  • The v2 endpoint being used in bulk-script.js is correct, but the overall migration is not complete
🔗 Analysis chain

Verify complete migration from v1 to v2 endpoints

Let's ensure all v1 endpoints have been properly updated to v2 and that we're using all required v2 parameters.

Also applies to: 37-37


Let me gather more information about the device-registry endpoints to ensure a complete verification.

The previous results show v1 references in device-registry files, and we need to verify if there are any device-specific v1 endpoints still in use. This will help us confirm if the migration is complete and consistent.


Let me check the device-registry routes to verify the complete migration status.

This will help us verify if there are any v1 routes still in use and confirm the migration status of the endpoints.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining v1 API references
echo "Checking for v1 API references..."
rg -l "api/v1/" --type js

# Search for v2 endpoint usage patterns to verify consistency
echo "Checking v2 endpoint patterns..."
rg "api/v2/devices/(summary|soft)" --type js -A 2

Length of output: 3023


Script:

#!/bin/bash
# Check for any v1 device endpoints specifically
echo "Checking for v1 device endpoints..."
rg "api/v1/devices" --type js -A 2

# Check the device-registry config and routes for endpoint definitions
echo "Checking device-registry endpoint configurations..."
fd -e js -e ts . src/device-registry/routes --exec cat {}

Length of output: 102549


Script:

#!/bin/bash
# Check device-registry routes for v1 and v2 endpoint definitions
echo "Checking device-registry routes for API versions..."
fd -e js . src/device-registry/routes/v1 --exec cat {}
fd -e js . src/device-registry/routes/v2 --exec cat {}

# Check for any v1 device endpoints in use
echo "Checking for v1 device endpoint usage..."
rg "api/v1/devices" --type js -A 2 src/device-registry/

Length of output: 155385

Comment on lines +4 to +14
function extractBeforeV2IncludingSlash(url) {
const v2Index = url.indexOf("v2/");

if (v2Index !== -1) {
// Found "v2/" in the string
return url.substring(0, v2Index + 3); // Include "v2/"
} else {
// "v2/" not found, return the original string
return url;
}
}
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 input validation and edge case handling to the utility function

The function needs to be more robust to handle various edge cases and invalid inputs.

Consider this improved implementation:

 function extractBeforeV2IncludingSlash(url) {
+  if (!url || typeof url !== 'string') {
+    return '';
+  }
+
   const v2Index = url.indexOf("v2/");
 
   if (v2Index !== -1) {
-    // Found "v2/" in the string
     return url.substring(0, v2Index + 3); // Include "v2/"
   } else {
-    // "v2/" not found, return the original string
     return url;
   }
 }
📝 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 extractBeforeV2IncludingSlash(url) {
const v2Index = url.indexOf("v2/");
if (v2Index !== -1) {
// Found "v2/" in the string
return url.substring(0, v2Index + 3); // Include "v2/"
} else {
// "v2/" not found, return the original string
return url;
}
}
function extractBeforeV2IncludingSlash(url) {
if (!url || typeof url !== 'string') {
return '';
}
const v2Index = url.indexOf("v2/");
if (v2Index !== -1) {
return url.substring(0, v2Index + 3); // Include "v2/"
} else {
return url;
}
}

@Baalmart Baalmart mentioned this pull request Nov 24, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant