Adding site creation among activities to log and store for future reference #3946
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3946 +/- ##
========================================
Coverage 11.73% 11.73%
========================================
Files 114 114
Lines 15318 15326 +8
Branches 318 318
========================================
+ Hits 1797 1799 +2
- Misses 13521 13527 +6
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
src/device-registry/models/Activity.js (3)
34-34: Consider adding constraints to the device_id fieldWhile adding the
device_idfield is a good addition for tracking device-related activities, consider whether this field should have additional constraints:
- Should it be required for certain activity types?
- Should it have a reference to the actual device collection?
- device_id: { type: ObjectId }, + device_id: { + type: ObjectId, + ref: 'devices', + required: function() { + return ['device_creation', 'device_update', 'device_maintenance'].includes(this.activityType); + } + },
Line range hint
67-86: Add device_id to toJSON methodThe new
device_idfield is not included in the toJSON method, which means it won't be visible in API responses.toJSON() { return { _id: this._id, device: this.device, + device_id: this.device_id, network: this.network, group: this.group,
Line range hint
236-248: Update remove method projection to include device_idThe remove method's projection should include the new
device_idfield to maintain consistency with the schema update.let options = { projection: { _id: 1, device: 1, + device_id: 1, site_id: 1, host_id: 1, user_id: 1,src/device-registry/utils/create-activity.js (4)
Line range hint
71-308: Consider refactoring the deploy function for better maintainabilityThe deploy function is handling multiple responsibilities and contains deeply nested conditionals. Consider breaking it down into smaller, focused functions:
- Device validation
- Site validation
- Activity creation
- Device update
- Kafka notification
Here's a suggested approach for the initial validation:
+ async function validateDeploymentPrerequisites(tenant, deviceName, site_id) { + const [deviceExists, siteExists] = await Promise.all([ + DeviceModel(tenant).exists({ name: deviceName }), + SiteModel(tenant).exists({ _id: ObjectId(site_id) }) + ]); + + if (!deviceExists || !siteExists) { + return { + success: false, + message: "Device or Site not found", + status: httpStatus.BAD_REQUEST, + errors: { + message: `Invalid request, Device ${deviceName} or Site ${site_id} not found` + } + }; + } + return { success: true }; + } deploy: async (request, next) => { try { const { body, query } = request; const { tenant, deviceName } = query; const { site_id } = body; + const validationResult = await validateDeploymentPrerequisites(tenant, deviceName, site_id); + if (!validationResult.success) { + return validationResult; + }
Line range hint
309-324: Enhance error handling with more descriptive messagesThe error handling pattern is consistent but could be more informative. Consider:
- Adding error codes for better error tracking
- Including more context in error messages
- Implementing error recovery strategies where applicable
} catch (error) { logger.error(`🐛🐛 Internal Server Error ${error.message}`); + const errorCode = 'DEPLOY_ERROR_001'; + const errorContext = { + operation: 'device_deployment', + deviceName, + site_id, + error: error.message + }; next( new HttpError( "Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, - { message: error.message } + { + message: error.message, + code: errorCode, + context: errorContext + } ) ); }
Line range hint
271-291: Improve Kafka integration reliabilityThe current Kafka integration could be enhanced for better reliability and maintainability:
- Extract Kafka producer configuration to a shared utility
- Add retry mechanism for failed messages
- Implement proper error recovery
+ const MAX_RETRIES = 3; + const RETRY_DELAY = 1000; // ms + + async function publishToKafka(topic, message, retries = MAX_RETRIES) { + const producer = kafka.producer({ + groupId: constants.UNIQUE_PRODUCER_GROUP, + }); + try { + await producer.connect(); + await producer.send({ + topic, + messages: [message], + }); + } catch (error) { + if (retries > 0) { + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return publishToKafka(topic, message, retries - 1); + } + throw error; + } finally { + await producer.disconnect(); + } + } - try { - const kafkaProducer = kafka.producer({ - groupId: constants.UNIQUE_PRODUCER_GROUP, - }); - await kafkaProducer.connect(); - await kafkaProducer.send({ - topic: "activities-topic", - messages: [ - { - action: "create", - value: JSON.stringify(data), - }, - ], - }); - await kafkaProducer.disconnect(); - } catch (error) { - logger.error(`internal server error -- ${error.message}`); - } + try { + await publishToKafka("activities-topic", { + action: "create", + value: JSON.stringify(data), + }); + } catch (error) { + logger.error(`Failed to publish to Kafka: ${error.message}`); + // Consider implementing a dead letter queue or alternative notification mechanism + }
Line range hint
644-744: Enhance batch deployment with better control and monitoringThe batch deployment implementation could benefit from:
- Controlled concurrency to prevent overwhelming the system
- Transaction handling for atomic operations
- Progress tracking for long-running deployments
+ const BATCH_SIZE = 5; // Process 5 deployments at a time + + async function processBatch(deployments, tenant, next) { + const results = { + successful: [], + failed: [], + existing: [] + }; + + for (let i = 0; i < deployments.length; i += BATCH_SIZE) { + const batch = deployments.slice(i, i + BATCH_SIZE); + const batchPromises = batch.map(deployment => + processDeployment(deployment, tenant, next) + .then(result => { + if (result.existing) results.existing.push(result.data); + else if (result.success) results.successful.push(result.data); + else results.failed.push(result.data); + }) + ); + await Promise.all(batchPromises); + + // Report progress + const progress = Math.min(100, (i + BATCH_SIZE) / deployments.length * 100); + logger.info(`Batch deployment progress: ${progress.toFixed(1)}%`); + } + + return results; + } batchDeployWithCoordinates: async (request, next) => { try { const { body, query } = request; const { tenant } = query; - const successful_deployments = []; - const failed_deployments = []; - const existing_sites = []; - - const deploymentPromises = body.map(async (deployment) => { + const results = await processBatch(body, tenant, next);src/device-registry/utils/generate-filter.js (1)
1562-1564: LGTM! Consider adding input validation.The change improves the handling of activity tags by properly converting the comma-separated string into an array for MongoDB's
$inoperator. This enhancement aligns well with the PR's objective of improving activity logging.Consider adding input validation to handle edge cases:
if (activity_tags) { + // Ensure activity_tags is a string and trim whitespace + const tagsString = activity_tags.toString().trim(); + if (tagsString) { - const activityTagsArray = activity_tags.toString().split(","); + const activityTagsArray = tagsString.split(",").map(tag => tag.trim()).filter(Boolean); filter["tags"] = {}; filter["tags"]["$in"] = activityTagsArray; + } }This refinement:
- Trims whitespace from the input string
- Trims individual tags
- Filters out empty tags
- Guards against empty input
src/device-registry/utils/create-site.js (2)
404-417: Conditional Property Inclusion Might Skip Valid ValuesUsing the
&&operator may omit properties with falsy values like0or empty strings. To include all non-null values, consider checking fornullorundefinedinstead.Apply this change to ensure properties with valid falsy values are included:
const siteActivityBody = { date: new Date(), description: "site created", activityType: "site-creation", site_id: createdSite._id, - ...(network && { network }), - ...(userName && { userName }), - ...(lastName && { lastName }), - ...(firstName && { firstName }), - ...(user_id && { user_id }), - ...(group && { group }), + ...(network != null && { network }), + ...(userName != null && { userName }), + ...(lastName != null && { lastName }), + ...(firstName != null && { firstName }), + ...(user_id != null && { user_id }), + ...(group != null && { group }), };
430-431: Consider Removing Emojis from Error Logs for ClarityIncluding emojis like
🐛🐛in error logs might reduce readability and clutter log files. Consider removing them to maintain professional and clear logging.Also applies to: 450-450
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/device-registry/models/Activity.js(1 hunks)src/device-registry/models/test/ut_activity.js(1 hunks)src/device-registry/utils/create-activity.js(1 hunks)src/device-registry/utils/create-site.js(5 hunks)src/device-registry/utils/generate-filter.js(1 hunks)
🔇 Additional comments (8)
src/device-registry/models/test/ut_activity.js (2)
6-6: LGTM! Import path updated correctly.
The import path change from @models/SiteActivity to @models/Activity aligns with the model restructuring mentioned in the PR.
Line range hint 1-70: Essential test coverage is missing for new features.
While the test structure is well-organized, several critical test cases are missing:
- No tests for the new
device_idfield that was added to the schema - No tests for site creation activities, which is the main focus of this PR
- Empty test sections for instance methods, static methods, and CRUD operations
Consider adding these essential test cases:
describe("Schema Definition", () => {
// ... existing tests ...
+
+ it("Should have the 'device_id' field", () => {
+ const deviceIdField = Activity.schema.obj.device_id;
+ expect(deviceIdField).to.exist;
+ expect(deviceIdField.type).to.equal(String);
+ });
+
+ it("Should validate site creation activity type", () => {
+ const activity = new Activity({
+ email: "[email protected]",
+ activity_type: "site_creation",
+ device_id: "device123",
+ description: "Created new site"
+ });
+ expect(activity.validateSync()).to.be.undefined;
+ });
});
describe("Static Methods", () => {
- // Add your static method tests here
+ describe("createSiteActivity", () => {
+ it("Should create a new site activity record", async () => {
+ const activityData = {
+ email: "[email protected]",
+ activity_type: "site_creation",
+ device_id: "device123",
+ description: "Created new site"
+ };
+ const activity = await Activity.createSiteActivity(activityData);
+ expect(activity).to.have.property('_id');
+ expect(activity.activity_type).to.equal("site_creation");
+ });
+ });
});Let's verify if there are any other activity-related test files that might need similar updates:
src/device-registry/models/Activity.js (2)
Line range hint 284-297: LGTM: Model creation logic is robust
The tenant-based model creation logic is well-implemented and handles the singleton pattern correctly. No changes needed here.
Line range hint 47-51: Verify activity codes for site creation
Let's ensure that the activity_codes array supports site creation tracking as per the PR objectives.
✅ Verification successful
Let me gather more information about activity types and their usage.
Activity codes schema supports site creation tracking
The verification confirms that the schema changes align with the PR objectives. Evidence found:
activityType: "site-creation"is used insrc/device-registry/utils/create-site.js- Supporting constants and projections exist for site activities:
SITE_ACTIVITIES_INCLUSION_PROJECTIONSITE_ACTIVITIES_EXCLUSION_PROJECTION
The activity_codes array in the schema is appropriately structured to accommodate site creation tracking activities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for activity type constants and verify site creation is included
rg -A 5 "ACTIVITY_TYPE|ACTIVITY_CODES" src/device-registry/config/
# Search for site creation related activities in existing data
ast-grep --pattern 'activityType: { $$ "site" $$ }'
Length of output: 692
Script:
#!/bin/bash
# Search for activity type definitions or usage in the codebase
rg -g '!*.test.*' -g '!*.spec.*' "activityType.*site" -B 2 -A 2
# Look for site-related constants or enums
rg -g '!*.test.*' -g '!*.spec.*' "SITE_" -B 2 -A 2 src/device-registry/
# Check for any site activity related code
ast-grep --pattern 'const $$ = { $$ site $$ }'
Length of output: 4318
src/device-registry/utils/create-activity.js (1)
1-1: Verify the impact of Activity model migration
The change from SiteActivity to Activity model suggests a consolidation of activity tracking. Let's ensure this change maintains backward compatibility.
✅ Verification successful
Activity model change is safe and maintains compatibility
The change from SiteActivity to Activity model is a safe refactoring that maintains backward compatibility. The Activity model schema includes all necessary site-related fields (site_id, network, etc.) and the codebase consistently uses the new model name. The schema also includes comprehensive activity tracking capabilities with proper validation and error handling.
Key evidence:
- Activity schema includes site-specific fields:
site_id,network,group - Model maintains existing functionality through
register,list,modify, andremovemethods - Constants reference
SITE_ACTIVITIES_INCLUSION_PROJECTIONshowing site activity support - Logger name includes "site-activities-model" indicating maintained site functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to SiteActivity model
rg -l "SiteActivity" --type js
# Check if the Activity model includes all fields from SiteActivity
ast-grep --pattern 'const $schema = new Schema({$$$})'
Length of output: 87
Script:
#!/bin/bash
# Let's try a broader search for model definitions and activity-related files
# Search for Activity model definition
rg -l "Activity.*Model" --type js
# Search for schema definitions related to activities
rg -A 10 "new Schema\(" --type js
# Look for files that might contain activity-related code
fd -e js -E node_modules | rg -l "activity|Activity"
# Check imports in the device registry directory
fd -e js . src/device-registry | xargs rg "require.*Activity"
Length of output: 74106
Script:
#!/bin/bash
# Let's check the Activity model schema and its usage
rg -A 10 "const activitySchema = new Schema" src/device-registry/models/Activity.js
# Check if there are any references to SiteActivity in git history
git log -p | rg "SiteActivity"
# Look for activity-related function implementations
ast-grep --pattern 'function $FUNC_NAME($$$) {
$$$
ActivityModel.$$$
$$$
}'
# Check test files for activity-related tests
rg -A 5 "describe.*Activity" src/device-registry/models/test/ut_activity.js
Length of output: 723
Script:
#!/bin/bash
# Let's examine the Activity model's full schema and any site-related fields
cat src/device-registry/models/Activity.js
# Check for any activity creation functions and their usage
rg -A 10 "createActivity" src/device-registry/utils/create-activity.js
# Look for any site-related fields or methods in activity files
rg -l "site" src/device-registry/models/Activity.js src/device-registry/utils/create-activity.js
# Check test cases for activity creation
rg -A 10 "test.*create.*activity" src/device-registry/models/test/ut_activity.js
Length of output: 9591
src/device-registry/utils/create-site.js (3)
2-2: Appropriate Import of ActivityModel for Activity Logging
Importing ActivityModel is necessary for logging activities related to site creation.
19-19: Importing stringify Utility for Enhanced Logging
Including the stringify utility enhances the logging functionality by allowing structured data to be logged effectively.
324-335: Destructuring Additional User Information from Request Body
Extracting user-related parameters from body improves the detail of activity logs. Ensure that any required fields are validated before use.
| logger.error( | ||
| `Unable to Store the Site Activity for this operation ${stringify( | ||
| siteActivityBody | ||
| )} ` | ||
| ); |
There was a problem hiding this comment.
Avoid Logging Sensitive User Information
When logging errors, avoid including potentially sensitive user data like user_id or userName to protect user privacy.
Consider revising the log statement:
logger.error(
- `Unable to Store the Site Activity for this operation ${stringify(
- siteActivityBody
- )} `
+ "Unable to store the site activity for this operation."
);📝 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.
| logger.error( | |
| `Unable to Store the Site Activity for this operation ${stringify( | |
| siteActivityBody | |
| )} ` | |
| ); | |
| logger.error( | |
| "Unable to store the site activity for this operation." | |
| ); |
|
Device registry changes in this PR available for preview here |
Description
I am just adding site creation among activities to retrieve and know who created the Site
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
I am just adding site creation among activities to retrieve and know who created the Site
Summary by CodeRabbit
Release Notes
New Features
device_idfield in activity logging to associate activities with specific devices.Bug Fixes
Refactor
ActivityModelto streamline activity-related operations across the application.Documentation