automated update of default preferences (selected-sites) #3644
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the functionality of the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #3644 +/- ##
===========================================
- Coverage 27.05% 27.04% -0.01%
===========================================
Files 146 146
Lines 21338 21339 +1
Branches 274 274
===========================================
Hits 5772 5772
- Misses 15566 15567 +1
|
Baalmart
changed the title
|
Auth-service 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: 41
🧹 Outside diff range and nitpick comments (28)
src/auth-service/bin/jobs/test/ut_preferences-update-job.js (1)
1-9: Excellent setup for the test suite!The import statements and initial test structure are well-organized. The use of
module-aliassuggests a thoughtful approach to module resolution.A minor suggestion to enhance readability:
Consider grouping related imports together. For example:
require("module-alias/register"); const chai = require("chai"); const expect = chai.expect; const sinon = require("sinon"); const sinonChai = require("sinon-chai"); chai.use(sinonChai); // Add this line to set up sinonChaiThis grouping makes it easier to distinguish between core imports and testing library imports.
src/auth-service/bin/jobs/test/ut_token-expiration-job.js (4)
1-6: Excellent choice of testing libraries!The setup looks good, utilizing Sinon for mocking and Chai for assertions. The use of module-alias suggests a well-organized project structure.
A small suggestion to enhance code clarity:
Consider adding a comment explaining the purpose of
module-alias/registerfor developers who might not be familiar with it. For example:// Enable custom module resolution paths require("module-alias/register");This helps newcomers to the project understand the purpose of this import more quickly.
7-44: Well-structured test setup and teardown!The beforeEach and afterEach hooks are properly utilized to set up and clean up mocks. This ensures each test runs in isolation, which is crucial for reliable unit testing.
A small suggestion to improve error handling:
Consider adding a try-catch block in the afterEach hook to ensure all mocks are restored even if an error occurs. This can prevent potential issues in subsequent tests. For example:
afterEach(() => { try { AccessTokenModel.restore(); console.info.restore(); console.error.restore(); } catch (error) { console.warn('Error during test cleanup:', error); } });This addition ensures that even if one restore operation fails, the others will still be attempted, and any issues will be logged for debugging.
46-88: Comprehensive test coverage for the happy path!This test thoroughly verifies the main functionality of
sendAlertsForExpiringTokens. The assertions are well-structured, checking both the method calls and their arguments.To enhance readability and maintainability:
Consider extracting the mock user data into a constant at the top of the describe block. This would make it easier to modify the test data and ensure consistency. For example:
describe("successful execution", () => { const mockUsers = [ { email: "[email protected]", firstName: "John", lastName: "Doe" }, { email: "[email protected]", firstName: "Jane", lastName: "Smith" } ]; beforeEach(() => { // ... existing setup ... AccessTokenModel.prototype.getExpiringTokens.returns( Promise.resolve({ success: true, data: mockUsers.map(user => ({ user })) }) ); }); it("should fetch expiring tokens and send emails", async () => { // ... existing test ... mockUsers.forEach(user => { expect(mailer.expandingToken).to.have.been.calledWithMatch(user); }); }); });This approach makes the test more maintainable and easier to extend with additional test cases.
90-118: Excellent coverage of edge cases and error scenarios!The additional tests for handling no expiring tokens and error conditions are crucial for ensuring robust functionality. It's great to see these scenarios being explicitly tested.
A suggestion for consistency in error logging:
The error message in the last test includes emojis (🐛🐛). While this can make logs more visually distinct, it might not align with standard logging practices. Consider standardizing the error message format across the application. For example:
expect(console.error).to.have.been.calledWith( `[ERROR] Internal Server Error: Test error` );This format maintains readability while potentially aligning better with any log parsing or analysis tools you might use in production.
Additionally, consider adding a test case for partial failures, where some email sends succeed and others fail. This would further enhance the robustness of your test suite.
src/auth-service/bin/jobs/test/ut_preferences-log-job.js (4)
1-9: Imports and initial setup look good!The necessary modules are imported, and Chai is set up with Sinon-Chai. This follows best practices for setting up a test environment.
Consider adding a comment explaining the purpose of
require("module-alias/register")for better code readability and maintainability.
10-44: Well-structured setup and teardown!The
beforeEachandafterEachhooks effectively set up and clean up the test environment, ensuring isolation between tests. The use of Sinon's mocking and stubbing capabilities is appropriate for unit testing.Consider extracting the mock data (user and preference objects) into separate constants at the top of the file. This would improve readability and make it easier to maintain or modify the test data.
46-69: Comprehensive test cases for main scenarios!The "successful execution" and "no users found" test cases effectively cover the primary scenarios for the
logUserPreferencesfunction. The assertions are well-chosen to verify the expected behavior.In the "successful execution" test, consider adding an assertion to check the specific percentage value logged. This would provide a more precise verification of the function's output.
83-120: Thorough coverage of edge cases!The "empty selected sites" and "undefined selected sites" test cases effectively cover important edge cases. These tests ensure that the
logUserPreferencesfunction correctly handles scenarios where users have empty or undefined selected sites.Consider adding a test case for a mixed scenario where some users have selected sites and others don't. This would provide a more comprehensive test of the function's behavior with varied user data.
src/auth-service/bin/jobs/test/ut_incomplete-profile-job.js (3)
7-43: Impressive test setup and mocking!Your approach to mocking and stubbing is thorough and well-structured. The use of
beforeEachensures a clean slate for each test, which is crucial for test isolation. The mock data forUserModel.findcovers various user scenarios, providing a solid foundation for comprehensive testing.A small suggestion for enhancement:
Consider using a factory function or fixture to generate mock user data. This could make it easier to maintain and extend your test cases in the future.
For example:
const createMockUser = (id, firstName, email, isActive) => ({ _id: id, firstName, email, isActive }); // Usage in your stub: sinon.stub(UserModel.prototype, "find").resolves([ createMockUser("user1", "Unknown", "[email protected]", false), createMockUser("user2", "Unknown", "[email protected]", true), createMockUser("user3", "John", "[email protected]", false) ]);This approach can make your test data more flexible and easier to read.
53-61: Well-crafted test for the happy path scenario!Your test case for successful execution is thorough and checks all the essential aspects: correct number of calls to
UserModel.findandmailer.updateProfileReminder, and absence of error logging.To further enhance this test:
Consider adding assertions to verify the content of the emails sent. This could involve checking the arguments passed to
mailer.updateProfileReminder. For example:expect(mailer.updateProfileReminder).to.have.been.calledWith({ email: "[email protected]", // other expected arguments });This would ensure not just that the correct number of emails are sent, but also that they're sent to the right users with the correct content.
73-155: Comprehensive test coverage across various scenarios!Your test cases for email sending failure, internal server error, unknown firstName, and active users are well-structured and cover important aspects of the system's behavior. The use of stubs to simulate different conditions is particularly noteworthy.
To further enhance your test suite:
Consider adding a test case for partial failure scenarios. For example, what happens if some emails are sent successfully while others fail? This could involve stubbing
mailer.updateProfileReminderto succeed for some calls and fail for others within the same test.Example:
it("should handle partial email sending failure", async () => { let callCount = 0; sinon.stub(mailer.updateProfileReminder).callsFake(() => { callCount++; if (callCount === 2) { return Promise.reject(new Error("Test error")); } return Promise.resolve({ success: true, data: {} }); }); await checkStatus(); expect(console.error).to.have.been.calledOnce; expect(mailer.updateProfileReminder).to.have.been.calledThrice; });This would ensure your system can gracefully handle partial failures, which can occur in real-world scenarios.
src/auth-service/routes/v2/test/ut_preferences.js (1)
1-39: Excellent test setup and organization!The initial setup demonstrates a well-structured approach to unit testing. The use of Jest for mocking and the comprehensive setup in the
beforeEachhook are commendable. This ensures that each test runs in a clean, isolated environment.A small suggestion to enhance maintainability:
Consider extracting the mock implementation into a separate file. This can help keep the test file cleaner and allow for easier reuse of mocks across different test files if needed.
// In a new file, e.g., 'preferenceControllerMock.js' module.exports = { create: jest.fn(), list: jest.fn(), delete: jest.fn(), addSelectedSites: jest.fn(), updateSelectedSite: jest.fn(), deleteSelectedSite: jest.fn(), }; // In your test file jest.mock("@controllers/create-preference", () => require("./preferenceControllerMock"));This approach can make your tests more modular and easier to maintain as the codebase grows.
src/auth-service/bin/jobs/test/ut_active-status-job.js (6)
1-6: Excellent choice of testing libraries!The use of Sinon for mocking and Chai for assertions is a solid foundation for your unit tests. The inclusion of module-alias suggests a well-organized project structure.
Consider adding a brief comment explaining the purpose of
module-alias/registerfor better code readability and to help other developers understand its importance in the project setup.
10-53: Well-structured test setup and teardown!Your beforeEach and afterEach hooks provide a clean and isolated environment for each test. Mocking UserModel, console.error, and stringify.default ensures consistent behavior across tests.
Consider extracting the mock data (user objects) into a separate constant or fixture file. This would improve readability and make it easier to maintain or extend the test data in the future.
55-66: Comprehensive test for successful execution!This test effectively verifies the core functionality of the
checkStatusfunction. It ensures that inactive users are correctly identified and updated.Consider adding an assertion to verify the number of users marked as inactive, which would provide an additional layer of validation.
91-122: Excellent test for inactive threshold logic!By mocking the current timestamp, this test effectively verifies the inactive threshold logic. It's a crucial test for ensuring the core functionality of the
checkStatusfunction.Consider adding a test case for a user right at the threshold boundary to ensure the logic handles edge cases correctly.
136-162: Thorough test for handling already inactive users!This test verifies that the function correctly skips users who are already marked as inactive, which is an important optimization.
Consider adding an assertion to verify that the
isActivestatus of the already inactive user (user1) remains unchanged after the function execution.
1-162: Excellent test suite for the checkStatus function!This test file demonstrates a thorough and well-structured approach to unit testing. The coverage of various scenarios, including edge cases and error handling, provides robust validation of the
checkStatusfunction's behavior. The use of descriptive test names, proper setup and teardown procedures, and effective mocking techniques all contribute to the high quality of these tests.To further enhance this test suite:
- Consider extracting mock data into a separate fixture file for improved maintainability.
- Add tests for boundary conditions, such as users exactly at the inactivity threshold.
- Implement parameterized tests for scenarios that could benefit from testing multiple similar cases.
These enhancements would elevate an already strong test suite to an exemplary level of quality and comprehensiveness.
src/auth-service/models/test/ut_selected_site.js (1)
1-7: Consider grouping and organizing imports for better readability.The import statements are correct, but we can enhance the code's readability by grouping related imports together. Here's a suggested organization:
require("module-alias/register"); // Testing libraries const chai = require("chai"); const expect = chai.expect; const sinon = require("sinon"); const sinonChai = require("sinon-chai"); // Core dependencies const mongoose = require("mongoose"); // Project-specific imports const constants = require("@config/constants");This grouping makes it easier to distinguish between testing libraries, core dependencies, and project-specific imports at a glance.
src/auth-service/utils/generate-filter.js (1)
336-358: Excellent addition of theselected_sitesmethod!The new
selected_sitesmethod is well-structured and consistent with the existing codebase. It efficiently extracts thesite_idparameter and creates an appropriate filter object. The error handling is robust, following the established pattern in the file.As a minor suggestion for future-proofing:
Consider allowing for more complex queries by accepting additional parameters. This could enhance the flexibility of the method without impacting its current functionality. For example:
let { site_id, status, created_at } = { ...req.body, ...req.query, ...req.params, }; let filter = {}; if (site_id) filter["site_id"] = site_id; if (status) filter["status"] = status; if (created_at) filter["created_at"] = { $gte: new Date(created_at) };This approach would maintain backwards compatibility while allowing for more sophisticated queries in the future.
src/auth-service/bin/jobs/preferences-update-job.js (1)
Line range hint
97-141: Optimize Database Operations with Bulk WritePerforming individual create and update operations inside a loop can lead to a significant number of database calls, which may affect performance. Utilizing bulk write operations can enhance efficiency by batching these operations.
Here's how you could implement bulk operations:
const bulkOperations = []; for (const user of users) { const userIdStr = user._id.toString(); const preference = preferencesMap.get(userIdStr); if (!preference) { // Prepare to create a new preference bulkOperations.push({ insertOne: { document: { ...defaultPreference, user_id: user._id, selected_sites: selectedSites, }, }, }); } else if (isEmpty(preference.selected_sites)) { // Prepare to update the existing preference bulkOperations.push({ updateOne: { filter: { _id: preference._id }, update: { $set: { ...defaultPreference, selected_sites: selectedSites, }, }, }, }); } } if (bulkOperations.length > 0) { await PreferenceModel("airqo") .bulkWrite(bulkOperations) .catch((error) => { logger.error( `🐛🐛 Failed to perform bulk operations: ${stringify(error)}` ); }); }This approach reduces the number of database interactions and can improve overall performance.
src/auth-service/models/SelectedSite.js (1)
195-195: Error message refers to 'User' instead of 'Selected Site'The error message in the
removemethod references "Provided User" which may confuse users of the API. Updating the message to "Provided selected site" will enhance clarity.Suggested change:
- message: "Provided User does not exist, please crosscheck", + message: "Provided selected site does not exist, please crosscheck",src/auth-service/utils/create-preference.js (2)
399-400: Clarify retrieval of 'tenant' and 'site_id' from requestIn the
updateSelectedSitemethod, you're mergingqueryandparamsto extracttenantandsite_id. For better clarity and maintainability, it's advisable to explicitly destructure these values from their respective locations in therequestobject.Consider updating the code as follows:
const { query, params, body } = request; - const { tenant, site_id } = { ...query, ...params }; + const { tenant } = query; + const { site_id } = params;
423-430: Provide context for service unavailabilityThe immediate return of a "Service Temporarily Unavailable" response in the
deleteSelectedSitemethod suggests that this functionality might be under development or intentionally disabled. Adding a comment to explain this can help other developers understand the current state of this method.Consider adding a comment:
return { success: false, message: "Service Temporarily Unavailable", errors: { message: "Service Temporarily Unavailable", }, status: httpStatus.SERVICE_UNAVAILABLE, }; + // TODO: Implement delete functionality or remove if deprecated.src/auth-service/controllers/create-preference.js (2)
433-433: Correct the log message to reflect the correct operationThe log message indicates "deleting preference," but this method deletes a selected site. Updating the log message will improve clarity in the logs.
Suggested change:
- logText("deleting preference.........."); + logText("Deleting selected site..........");
490-491: Update log messages to accurately represent the actionThe log messages refer to listing preferences, but the method lists selected sites. For better clarity, please update the log messages.
Suggested changes:
- logText("....................................."); - logText("list all preferences by query params provided"); + logText("Listing all selected sites with provided query parameters");src/auth-service/routes/v2/preferences.js (1)
1085-1153: Ensure consistent validation and error messages across new routesThe new routes for managing selected sites (
GET,POST,PUT,DELETEat/selected-sites) introduce validation logic that should be consistent with existing patterns in the application. Ensure that:
- All parameter and body validations use consistent message formats.
- The
tenantquery parameter validation is uniform across all routes.- Sanitization and transformation of input data are handled consistently.
This enhances the API's reliability and makes it easier for clients to interact with it correctly.
Consider reviewing the validation chains and updating the messages or validation logic to align with the rest of the application.
Also applies to: 1155-1191, 1193-1230, 1232-1264
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- src/auth-service/bin/jobs/preferences-update-job.js (7 hunks)
- src/auth-service/bin/jobs/test/ut_active-status-job.js (1 hunks)
- src/auth-service/bin/jobs/test/ut_incomplete-profile-job.js (1 hunks)
- src/auth-service/bin/jobs/test/ut_preferences-log-job.js (1 hunks)
- src/auth-service/bin/jobs/test/ut_preferences-update-job.js (1 hunks)
- src/auth-service/bin/jobs/test/ut_token-expiration-job.js (1 hunks)
- src/auth-service/bin/server.js (1 hunks)
- src/auth-service/controllers/create-preference.js (1 hunks)
- src/auth-service/controllers/test/ut_create-preference.js (1 hunks)
- src/auth-service/models/SelectedSite.js (1 hunks)
- src/auth-service/models/test/ut_selected_site.js (1 hunks)
- src/auth-service/routes/v2/preferences.js (2 hunks)
- src/auth-service/routes/v2/test/ut_preferences.js (1 hunks)
- src/auth-service/utils/create-preference.js (2 hunks)
- src/auth-service/utils/generate-filter.js (1 hunks)
- src/auth-service/utils/test/ut_create-preference.js (2 hunks)
🧰 Additional context used
🪛 Biome
src/auth-service/models/SelectedSite.js
[error] 85-85: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 89-89: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/auth-service/routes/v2/preferences.js
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth-service/utils/create-preference.js
[error] 431-444: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (13)
src/auth-service/bin/jobs/test/ut_preferences-update-job.js (1)
1-79: Excellent test suite for the updatePreferences function!Overall, this is a well-structured and comprehensive set of unit tests. The coverage of successful execution, error handling, and edge cases demonstrates a thorough approach to testing the
updatePreferencesfunction.Key strengths:
- Clear organization using describe blocks
- Effective use of mocking and stubbing with Sinon
- Good coverage of main scenarios and edge cases
Suggestions for further enhancement:
- Add error handling in the beforeEach setup
- Increase assertion specificity in the successful execution test
- Expand error handling tests to cover all database interactions
- Enhance edge case tests with more specific assertions
These minor improvements will further increase the robustness and reliability of your test suite. Great job on creating a solid foundation for ensuring the quality of the
updatePreferencesfunction!src/auth-service/bin/jobs/test/ut_preferences-log-job.js (2)
71-81: Excellent error handling test!The "error handling" test case effectively simulates an error condition and verifies that the
logUserPreferencesfunction properly catches and logs the error. This is crucial for ensuring the robustness of the function in real-world scenarios.
1-120: Overall, an excellent and comprehensive test suite!This test file for the
logUserPreferencesfunction demonstrates a high-quality approach to unit testing. It covers all major scenarios, including successful execution, error handling, and important edge cases. The use of Mocha and Sinon for testing and mocking is appropriate and well-implemented.The structure of the tests is clear and follows best practices, with proper setup and teardown procedures. The assertions effectively verify the expected behavior of the function under various conditions.
While the test suite is already robust, consider implementing the minor suggestions provided in previous comments to further enhance its readability and coverage.
Great job on creating a thorough and reliable set of unit tests!
src/auth-service/bin/jobs/test/ut_incomplete-profile-job.js (4)
1-6: Excellent choice of testing libraries!The setup looks well-structured with the appropriate imports for Mocha, Chai, and Sinon. The use of
module-aliassuggests a thoughtful approach to module resolution in your project structure.
45-51: Excellent cleanup in the afterEach hook!Your diligence in restoring all mocks and stubs after each test is commendable. This practice is crucial for maintaining test isolation and preventing any unintended side effects between tests. Keep up the good work!
63-71: Excellent edge case coverage!Your test case for the "no users found" scenario is well-thought-out. It's crucial to verify that the system behaves correctly when there's no data to process, and your test does exactly that. The assertion checking for the absence of error logging is spot-on.
This kind of thorough testing, including edge cases, significantly contributes to the robustness of your system. Keep up the great work!
1-156: Impressive and comprehensive test suite!Your unit tests for the
checkStatusfunction are thorough, well-structured, and cover a wide range of scenarios. The use of mocking and stubbing is exemplary, and your attention to both happy paths and error cases is commendable.The test suite demonstrates a strong testing methodology, which will greatly contribute to the reliability and maintainability of your codebase. The suggestions provided earlier for using factory functions for mock data and adding a partial failure test case are minor enhancements to an already robust set of tests.
Overall, this is a high-quality test file that sets a great standard for the rest of the project. Keep up the excellent work!
src/auth-service/routes/v2/test/ut_preferences.js (1)
1-144: Excellent foundation for preference controller tests. Let's take it to the next level!Your unit tests for the preference controller provide a solid foundation for ensuring the reliability of your API endpoints. The structure and organization of your tests are commendable, and you've covered the basic scenarios for each endpoint.
To elevate your test suite to production-grade quality, consider implementing the following improvements:
Expand test coverage: Add more test cases for each endpoint to cover edge cases, error scenarios, and various input combinations.
Enhance assertion specificity: Instead of just checking if responses are defined, verify the exact structure and content of the responses.
Mock external dependencies consistently: Ensure all calls to the controller methods are properly mocked and their responses are controlled within the tests.
Test error handling: Include tests for various error scenarios to ensure your API gracefully handles and reports errors.
Validate request parsing: Add tests to ensure that your endpoints correctly parse and validate incoming request data.
Check authorization and authentication: If applicable, include tests to verify that your endpoints properly handle authorization and authentication.
By implementing these suggestions, you'll create a robust test suite that not only verifies the current functionality but also serves as a safety net for future changes and refactoring.
Great job on laying the groundwork for a solid test suite! With these enhancements, you'll have a comprehensive set of tests that significantly increase confidence in your code's reliability and correctness.
src/auth-service/bin/jobs/test/ut_active-status-job.js (2)
68-89: Good edge case coverage for no inactive users!This test ensures that the function behaves correctly when all users are active, which is an important scenario to consider.
124-134: Proper error handling test!This test ensures that the function gracefully handles and logs internal server errors, which is crucial for robust error management.
src/auth-service/bin/server.js (1)
26-26: Excellent addition of the preferences update job!The new import for the preferences update job is well-placed and follows the existing pattern for job imports. This addition aligns perfectly with the PR objectives of implementing an automated system for updating default user preferences.
To ensure the smooth integration of this new job, let's verify its implementation and potential interactions with existing jobs:
This script will help us confirm the presence of the new job file and identify any potential conflicts or overlaps with existing jobs.
✅ Verification successful
Verification Successful: No Conflicts Detected
The new
preferences-update-job.jshas been successfully verified. The job file exists along with its corresponding test file, and no conflicts or overlaps with existing jobs were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new preferences-update-job and its interactions with existing jobs. # Test 1: Check if the preferences-update-job file exists echo "Checking if preferences-update-job.js exists:" fd -t f "preferences-update-job.js" src/auth-service/bin/jobs # Test 2: Look for any potential conflicts or overlaps with existing jobs echo "\nChecking for potential conflicts or overlaps with existing jobs:" rg -i "preferences|update" src/auth-service/bin/jobs -A 5 -B 5Length of output: 44097
src/auth-service/models/test/ut_selected_site.js (1)
9-20: Well-structured test suite setup.The test suite is set up correctly with proper use of
beforeEachandafterEachhooks. Mocking theSelectedSiteModelbefore each test and restoring it afterward ensures a clean slate for each test case, preventing inter-test dependencies.src/auth-service/utils/test/ut_create-preference.js (1)
321-324: Ensure stubs are configured correctlyWhen stubbing
insertMany, ensure that the stub returns the expected resolved value. IfSelectedSiteModelis an object with stubs, you should configureinsertManydirectly.Please verify that
SelectedSiteModel.insertManyis correctly stubbed and that the method under test calls it as expected.
| beforeEach(() => { | ||
| // Set up mocks | ||
| sinon.stub(PreferenceModel.prototype, "find").resolves([]); | ||
| sinon.stub(PreferenceModel.prototype, "create").resolves({}); | ||
| sinon.stub(PreferenceModel.prototype, "findOneAndUpdate").resolves({}); | ||
|
|
||
| sinon | ||
| .stub(SelectedSiteModel.prototype, "find") | ||
| .resolves([{ site_id: "site1" }, { site_id: "site2" }]); | ||
|
|
||
| sinon | ||
| .stub(UserModel.prototype, "find") | ||
| .resolves([{ _id: "user1" }, { _id: "user2" }]); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| // Restore mocks | ||
| sinon.restore(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-structured test setup and teardown!
The beforeEach and afterEach hooks are implemented correctly, ensuring a clean slate for each test. The mocking of database models is comprehensive and will help isolate the updatePreferences function during testing.
A suggestion to improve robustness:
Consider adding error handling to the mock setups. This can help identify issues in the test setup itself:
beforeEach(() => {
try {
// Existing mock setup code
} catch (error) {
console.error("Error in test setup:", error);
throw error;
}
});This addition will make debugging easier if there are issues with the mock setup.
| describe("successful execution", () => { | ||
| it("should update preferences for users", async () => { | ||
| await updatePreferences(); | ||
|
|
||
| expect(PreferenceModel.prototype.create).to.have.been.calledTwice; | ||
| expect(PreferenceModel.prototype.findOneAndUpdate).to.have.been | ||
| .calledOnce; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good test for successful execution!
This test effectively verifies that the updatePreferences function is calling the expected methods on the PreferenceModel. The use of Sinon's call counting is appropriate for this scenario.
To make the test more robust:
Consider adding assertions to verify the content of the calls to create and findOneAndUpdate. This will ensure that not only are the methods called the correct number of times, but also with the expected arguments. For example:
it("should update preferences for users", async () => {
await updatePreferences();
expect(PreferenceModel.prototype.create).to.have.been.calledTwice;
expect(PreferenceModel.prototype.findOneAndUpdate).to.have.been.calledOnce;
// Add these assertions
expect(PreferenceModel.prototype.create.getCall(0).args[0]).to.deep.equal(/* expected first create call args */);
expect(PreferenceModel.prototype.create.getCall(1).args[0]).to.deep.equal(/* expected second create call args */);
expect(PreferenceModel.prototype.findOneAndUpdate.getCall(0).args).to.deep.equal(/* expected findOneAndUpdate call args */);
});This addition will provide a more comprehensive verification of the updatePreferences function's behavior.
| describe("error handling", () => { | ||
| it("should log errors when creating preferences fails", async () => { | ||
| const errorMock = new Error("Test error"); | ||
| sinon.stub(PreferenceModel.prototype, "create").rejects(errorMock); | ||
|
|
||
| await updatePreferences(); | ||
|
|
||
| expect(logObject).to.have.been.calledWith("error", errorMock); | ||
| }); | ||
|
|
||
| it("should log errors when updating preferences fails", async () => { | ||
| const errorMock = new Error("Test error"); | ||
| sinon | ||
| .stub(PreferenceModel.prototype, "findOneAndUpdate") | ||
| .rejects(errorMock); | ||
|
|
||
| await updatePreferences(); | ||
|
|
||
| expect(logObject).to.have.been.calledWith("error", errorMock); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thorough error handling tests!
These tests effectively verify that errors are properly logged when creating or updating preferences fails. The use of Sinon to simulate errors by rejecting promises is a good approach.
For consistency and completeness:
Consider adding a test for error handling in the SelectedSiteModel.find and UserModel.find methods. This will ensure all potential error scenarios are covered. For example:
it("should log errors when fetching selected sites fails", async () => {
const errorMock = new Error("Test error");
SelectedSiteModel.prototype.find.rejects(errorMock);
await updatePreferences();
expect(logObject).to.have.been.calledWith("error", errorMock);
});
it("should log errors when fetching users fails", async () => {
const errorMock = new Error("Test error");
UserModel.prototype.find.rejects(errorMock);
await updatePreferences();
expect(logObject).to.have.been.calledWith("error", errorMock);
});These additional tests will provide complete coverage of all database interaction error scenarios.
| describe("edge cases", () => { | ||
| it("should handle empty selected sites", async () => { | ||
| sinon.stub(SelectedSiteModel.prototype, "find").resolves([]); | ||
|
|
||
| await updatePreferences(); | ||
|
|
||
| expect(PreferenceModel.prototype.create).to.not.have.been.called; | ||
| }); | ||
|
|
||
| it("should handle no users found", async () => { | ||
| sinon.stub(UserModel.prototype, "find").resolves([]); | ||
|
|
||
| await updatePreferences(); | ||
|
|
||
| expect(PreferenceModel.prototype.create).to.not.have.been.called; | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-considered edge case tests!
These tests effectively cover important edge cases: empty selected sites and no users found. It's good practice to verify that unnecessary database operations are not performed in these scenarios.
To make these tests more robust:
Consider adding more specific assertions about what should happen in these edge cases. For example:
it("should handle empty selected sites", async () => {
SelectedSiteModel.prototype.find.resolves([]);
await updatePreferences();
expect(PreferenceModel.prototype.create).to.not.have.been.called;
expect(logObject).to.have.been.calledWith("info", "No selected sites found. Skipping preference update.");
});
it("should handle no users found", async () => {
UserModel.prototype.find.resolves([]);
await updatePreferences();
expect(PreferenceModel.prototype.create).to.not.have.been.called;
expect(logObject).to.have.been.calledWith("info", "No users found. Skipping preference update.");
});These additions will ensure that the function not only avoids unnecessary database operations but also logs appropriate messages in these edge cases.
| describe("POST /api/preferences", () => { | ||
| it("should validate required fields", async () => { | ||
| const response = await request(app) | ||
| .post("/api/preferences") | ||
| .send({ | ||
| user_id: "1234567890abcdef1234567890abcdef", | ||
| chartTitle: "Test Chart Title", | ||
| period: {}, | ||
| site_ids: ["1234567890abcdef1234567890abcdef"], | ||
| device_ids: ["1234567890abcdef1234567890abcdef"], | ||
| selected_sites: [], | ||
| }) | ||
| .expect(200); | ||
|
|
||
| expect(response.body).toBeDefined(); | ||
| }); | ||
|
|
||
| it("should fail validation for missing required fields", async () => { | ||
| await request(app).post("/api/preferences").send({}).expect(400); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good start on POST /api/preferences tests. Let's enhance coverage!
Your tests cover the basic scenarios for creating preferences. To make these tests more robust, consider adding the following test cases:
- Test with partial valid data to ensure proper handling of optional fields.
- Test with invalid data types for each field to ensure proper validation.
- Test the response structure to ensure it matches the expected format.
Here's an example of how you might implement these additional tests:
it("should handle partial valid data", async () => {
const response = await request(app)
.post("/api/preferences")
.send({
user_id: "1234567890abcdef1234567890abcdef",
chartTitle: "Partial Data Test"
})
.expect(200);
expect(response.body).toBeDefined();
// Add more specific assertions based on your API's behavior with partial data
});
it("should reject invalid data types", async () => {
const response = await request(app)
.post("/api/preferences")
.send({
user_id: 12345, // Should be a string
chartTitle: ["Invalid Title"], // Should be a string
period: "Invalid Period" // Should be an object
})
.expect(400);
expect(response.body).toHaveProperty('error');
// Add more specific assertions based on your error response structure
});
it("should return the correct response structure", async () => {
const response = await request(app)
.post("/api/preferences")
.send({
// ... valid data ...
})
.expect(200);
expect(response.body).toHaveProperty('id');
expect(response.body).toHaveProperty('user_id');
// Add more assertions to check all expected properties
});These additional tests will significantly improve your test coverage and help catch potential edge cases.
| res.status(status).json({ | ||
| success: true, | ||
| message: result.message, | ||
| seletected_sites: result.data, |
There was a problem hiding this comment.
Fix typo in 'seletected_sites' key in response JSON
The key 'seletected_sites' in the JSON response is misspelled. It should be 'selected_sites' to ensure consistency and correct access in the client code.
Apply this diff to fix the typo:
348,357c348,357
- seletected_sites: result.data,
+ selected_sites: result.data,Also applies to: 357-357
| addSelectedSites: async (req, res, next) => { | ||
| try { | ||
| const errors = extractErrorsFromRequest(req); | ||
| if (errors) { | ||
| next( | ||
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | ||
| ); | ||
| return; | ||
| } | ||
| const request = req; | ||
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | ||
| request.query.tenant = isEmpty(req.query.tenant) | ||
| ? defaultTenant | ||
| : req.query.tenant; | ||
|
|
||
| const result = await createPreferenceUtil.addSelectedSites(request, next); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| if (result.success === true) { | ||
| const status = result.status ? result.status : httpStatus.OK; | ||
| res.status(status).json({ | ||
| success: true, | ||
| message: result.message, | ||
| seletected_sites: result.data, | ||
| }); | ||
| } else if (result.success === false) { | ||
| const status = result.status | ||
| ? result.status | ||
| : httpStatus.INTERNAL_SERVER_ERROR; | ||
| res.status(status).json({ | ||
| success: false, | ||
| message: result.message, | ||
| seletected_sites: result.data, | ||
| errors: result.errors | ||
| ? result.errors | ||
| : { message: "Internal Server Error" }, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code to improve maintainability
The methods addSelectedSites, updateSelectedSite, deleteSelectedSite, and listSelectedSites contain repetitive code for error extraction, tenant setting, and response handling. Consider refactoring this common logic into helper functions or middleware to adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability.
Also applies to: 375-430, 431-486, 488-543
There was a problem hiding this comment.
Create a Github issue for this please
There was a problem hiding this comment.
@Baalmart, I've created a GitHub issue to track this refactoring suggestion: #3645
🔗 Analysis chain
@Baalmart, I'll create a GitHub issue to track this refactoring suggestion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
gh issue create \
--title "Refactor duplicated code in create-preference controller" \
--body "The methods \`addSelectedSites\`, \`updateSelectedSite\`, \`deleteSelectedSite\`, and \`listSelectedSites\` in \`src/auth-service/controllers/create-preference.js\` contain repetitive code for error extraction, tenant setting, and response handling. Refactoring this common logic into helper functions or middleware would adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability.
PR URL: https://github.com/airqo-platform/AirQo-api/pull/3644
Comment URL: https://github.com/airqo-platform/AirQo-api/pull/3644#discussion_r1798373378
Requested by: Baalmart" \
--assignee Baalmart
Length of output: 744
| function validateDefaultSelectedSitesField(value) { | ||
| const requiredFields = ["site_id", "search_name", "name"]; | ||
| // Check if all required fields exist | ||
| if (!requiredFields.every((field) => field in value)) { | ||
| return false; | ||
| } | ||
|
|
||
| function validateNumericFields(fields) { | ||
| let isValid = true; | ||
|
|
||
| fields.forEach((field) => { | ||
| if (!(field in value)) { | ||
| isValid = false; | ||
| return; | ||
| } | ||
| const numValue = parseFloat(value[field]); | ||
| if (Number.isNaN(numValue)) { | ||
| isValid = false; | ||
| return; | ||
| } else if ( | ||
| field === "latitude" || | ||
| field === "longitude" || | ||
| field === "approximate_latitude" || | ||
| field === "approximate_longitude" | ||
| ) { | ||
| if (Math.abs(numValue) > 90) { | ||
| isValid = false; | ||
| return; | ||
| } | ||
| } else if (field === "search_radius") { | ||
| if (numValue <= 0) { | ||
| isValid = false; | ||
| return; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return isValid; | ||
| } | ||
|
|
||
| function validateStringFields(fields) { | ||
| let isValid = true; | ||
| fields.forEach((field) => { | ||
| if (typeof value[field] !== "string" || value[field].trim() === "") { | ||
| isValid = false; | ||
| return; | ||
| } | ||
| }); | ||
| return isValid; | ||
| } | ||
|
|
||
| function validateTags(tags) { | ||
| if (isEmpty(tags)) { | ||
| return true; | ||
| } else if (!Array.isArray(tags)) { | ||
| return false; | ||
| } else { | ||
| return tags.every((tag) => typeof tag === "string"); | ||
| } | ||
| } | ||
|
|
||
| const numericValid = validateNumericFields([ | ||
| "latitude", | ||
| "longitude", | ||
| "approximate_latitude", | ||
| "approximate_longitude", | ||
| ]); | ||
|
|
||
| const stringValid = validateStringFields(["name", "search_name"]); | ||
|
|
||
| const tags = value && value.site_tags; | ||
| const tagValid = validateTags(tags); | ||
|
|
||
| return numericValid && stringValid && tagValid; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate duplicate code in validation functions
The validateDefaultSelectedSitesField function appears to have significant overlap with the existing validateSelectedSitesField function. Both functions perform similar validations with minor differences in the required fields. Maintaining two nearly identical functions can lead to code duplication and increase the risk of inconsistencies when updates are made.
Consider refactoring these functions to create a single, more flexible validation function that accepts parameters to handle the adjustable parts, such as the list of required fields. This approach enhances maintainability and reduces code redundancy.
Here is an example of how you might refactor the functions:
function createValidateSelectedSitesField(requiredFields) {
return function (value) {
// Check if all required fields exist
if (!requiredFields.every((field) => field in value)) {
return false;
}
// ... rest of the validation logic remains the same
};
}
// Then create specific validators
const validateSelectedSitesField = createValidateSelectedSitesField([
"_id",
"search_name",
"name",
]);
const validateDefaultSelectedSitesField = createValidateSelectedSitesField([
"site_id",
"search_name",
"name",
]);🧰 Tools
🪛 Biome
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Potential logical error in validation functions due to 'return' inside 'forEach' loops
In the validateDefaultSelectedSitesField function, the validateNumericFields and validateStringFields helper functions use forEach loops with return statements to exit early upon encountering invalid data. However, using return inside a forEach loop only exits the current iteration, not the parent function. This means that the validation may not stop as intended when an invalid field is found, potentially allowing invalid data to pass through.
Consider using a standard for loop or the Array.some method to properly exit the helper functions upon validation failure.
Here is a suggested fix using a for loop:
function validateNumericFields(fields) {
- let isValid = true;
-
- fields.forEach((field) => {
- if (!(field in value)) {
- isValid = false;
- return;
- }
- const numValue = parseFloat(value[field]);
- if (Number.isNaN(numValue)) {
- isValid = false;
- return;
- } else if (
- field === "latitude" ||
- field === "longitude" ||
- field === "approximate_latitude" ||
- field === "approximate_longitude"
- ) {
- if (Math.abs(numValue) > 90) {
- isValid = false;
- return;
- }
- } else if (field === "search_radius") {
- if (numValue <= 0) {
- isValid = false;
- return;
- }
- }
- });
-
- return isValid;
+ for (const field of fields) {
+ if (!(field in value)) {
+ return false;
+ }
+ const numValue = parseFloat(value[field]);
+ if (Number.isNaN(numValue)) {
+ return false;
+ } else if (
+ field === "latitude" ||
+ field === "longitude" ||
+ field === "approximate_latitude" ||
+ field === "approximate_longitude"
+ ) {
+ if (Math.abs(numValue) > 90) {
+ return false;
+ }
+ } else if (field === "search_radius") {
+ if (numValue <= 0) {
+ return false;
+ }
+ }
+ }
+ return true;
}
function validateStringFields(fields) {
- let isValid = true;
- fields.forEach((field) => {
- if (typeof value[field] !== "string" || value[field].trim() === "") {
- isValid = false;
- return;
- }
- });
- return isValid;
+ for (const field of fields) {
+ if (typeof value[field] !== "string" || value[field].trim() === "") {
+ return false;
+ }
+ }
+ return 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.
| function validateDefaultSelectedSitesField(value) { | |
| const requiredFields = ["site_id", "search_name", "name"]; | |
| // Check if all required fields exist | |
| if (!requiredFields.every((field) => field in value)) { | |
| return false; | |
| } | |
| function validateNumericFields(fields) { | |
| let isValid = true; | |
| fields.forEach((field) => { | |
| if (!(field in value)) { | |
| isValid = false; | |
| return; | |
| } | |
| const numValue = parseFloat(value[field]); | |
| if (Number.isNaN(numValue)) { | |
| isValid = false; | |
| return; | |
| } else if ( | |
| field === "latitude" || | |
| field === "longitude" || | |
| field === "approximate_latitude" || | |
| field === "approximate_longitude" | |
| ) { | |
| if (Math.abs(numValue) > 90) { | |
| isValid = false; | |
| return; | |
| } | |
| } else if (field === "search_radius") { | |
| if (numValue <= 0) { | |
| isValid = false; | |
| return; | |
| } | |
| } | |
| }); | |
| return isValid; | |
| } | |
| function validateStringFields(fields) { | |
| let isValid = true; | |
| fields.forEach((field) => { | |
| if (typeof value[field] !== "string" || value[field].trim() === "") { | |
| isValid = false; | |
| return; | |
| } | |
| }); | |
| return isValid; | |
| } | |
| function validateTags(tags) { | |
| if (isEmpty(tags)) { | |
| return true; | |
| } else if (!Array.isArray(tags)) { | |
| return false; | |
| } else { | |
| return tags.every((tag) => typeof tag === "string"); | |
| } | |
| } | |
| const numericValid = validateNumericFields([ | |
| "latitude", | |
| "longitude", | |
| "approximate_latitude", | |
| "approximate_longitude", | |
| ]); | |
| const stringValid = validateStringFields(["name", "search_name"]); | |
| const tags = value && value.site_tags; | |
| const tagValid = validateTags(tags); | |
| return numericValid && stringValid && tagValid; | |
| } | |
| function validateDefaultSelectedSitesField(value) { | |
| const requiredFields = ["site_id", "search_name", "name"]; | |
| // Check if all required fields exist | |
| if (!requiredFields.every((field) => field in value)) { | |
| return false; | |
| } | |
| function validateNumericFields(fields) { | |
| for (const field of fields) { | |
| if (!(field in value)) { | |
| return false; | |
| } | |
| const numValue = parseFloat(value[field]); | |
| if (Number.isNaN(numValue)) { | |
| return false; | |
| } else if ( | |
| field === "latitude" || | |
| field === "longitude" || | |
| field === "approximate_latitude" || | |
| field === "approximate_longitude" | |
| ) { | |
| if (Math.abs(numValue) > 90) { | |
| return false; | |
| } | |
| } else if (field === "search_radius") { | |
| if (numValue <= 0) { | |
| return false; | |
| } | |
| } | |
| } | |
| return true; | |
| } | |
| function validateStringFields(fields) { | |
| for (const field of fields) { | |
| if (typeof value[field] !== "string" || value[field].trim() === "") { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| function validateTags(tags) { | |
| if (isEmpty(tags)) { | |
| return true; | |
| } else if (!Array.isArray(tags)) { | |
| return false; | |
| } else { | |
| return tags.every((tag) => typeof tag === "string"); | |
| } | |
| } | |
| const numericValid = validateNumericFields([ | |
| "latitude", | |
| "longitude", | |
| "approximate_latitude", | |
| "approximate_longitude", | |
| ]); | |
| const stringValid = validateStringFields(["name", "search_name"]); | |
| const tags = value && value.site_tags; | |
| const tagValid = validateTags(tags); | |
| return numericValid && stringValid && tagValid; | |
| } |
🧰 Tools
🪛 Biome
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const tags = value && value.site_tags; | ||
| const tagValid = validateTags(tags); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify property access using optional chaining
You can enhance readability and avoid potential errors by using optional chaining when accessing site_tags from value. This change ensures that if value is null or undefined, tags will be undefined instead of causing an error.
Here is the suggested modification:
- const tags = value && value.site_tags;
+ const tags = value?.site_tags;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tags = value && value.site_tags; | |
| const tagValid = validateTags(tags); | |
| const tags = value?.site_tags; | |
| const tagValid = validateTags(tags); | |
🧰 Tools
🪛 Biome
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| param("site_id") | ||
| .exists() | ||
| .withMessage("the site_id parameter is required") | ||
| .bail() | ||
| .isMongoId() | ||
| .withMessage("site_id must be a valid MongoDB ObjectId") | ||
| .bail() | ||
| .customSanitizer((value) => { | ||
| return ObjectId(value); | ||
| }), | ||
| body() | ||
| .custom(validateDefaultSelectedSitesField) | ||
| .withMessage( | ||
| "Invalid selected site data. Verify required fields and data types." | ||
| ), |
There was a problem hiding this comment.
Validate entire request body appropriately in PUT '/selected-sites/:site_id' route
In the route handler for updating a selected site, the validation chain includes body().custom(validateDefaultSelectedSitesField), which applies the custom validator to the entire request body. If the request body contains additional fields beyond the selected site data, the validation might not work as intended.
Consider specifying the exact property to validate or adjusting the validation function to handle the structure of the request body correctly.
You can modify the validation to target a specific field:
- body()
+ body("selected_site")And ensure that the request handler expects the selected site data under the selected_site key.
Alternatively, if the entire body represents the selected site data, ensure that the validation function can handle any additional fields appropriately.
Committable suggestion was skipped due to low confidence.
|
Auth-service changes in this PR available for preview here |
Description
automated update of default preferences
Changes Made
selected_sitesunder their saved preferences.Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation