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.

Sign up for GitHub

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

Jump to bottom

just adding the query parameters for site and device online status #3489

Merged
merged 2 commits into from
Sep 23, 2024
Merged

just adding the query parameters for site and device online status #3489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d17bfd0
just adding the query parameters for site and device online status
Baalmart Sep 23, 2024
84871e1
correcting the typos and improving the error messaging
Baalmart Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions src/device-registry/routes/v2/devices.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,53 @@ router.get(
.optional()
.notEmpty()
.trim(),
query("online_status")
.optional()
.notEmpty()
.withMessage("the online_status should not be empty if provided")
.bail()
.trim()
.toLowerCase()
.isIn(["online", "offline"])
.withMessage(
"the online_status value is not among the expected ones which include: online, offline"
),
query("last_active_before")
.optional()
.notEmpty()
.withMessage("last_active_before date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_before date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ).."
)
.bail()
.toDate(),
query("last_active_after")
.optional()
.notEmpty()
.withMessage("last_active_after date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_after date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active")
.optional()
.notEmpty()
.withMessage("last_active date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
],
]),
deviceController.list
Expand Down Expand Up @@ -455,6 +502,53 @@ router.get(
.optional()
.notEmpty()
.trim(),
query("online_status")
.optional()
.notEmpty()
.withMessage("the online_status should not be empty if provided")
.bail()
.trim()
.toLowerCase()
.isIn(["online", "offline"])
.withMessage(
"the online_status value is not among the expected ones which include: online, offline"
),
query("last_active_before")
.optional()
.notEmpty()
.withMessage("last_active_before date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_before date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active_after")
.optional()
.notEmpty()
.withMessage("last_active_after date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_after date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active")
.optional()
.notEmpty()
.withMessage("last_active date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
],
]),
deviceController.listSummary
Expand Down
150 changes: 150 additions & 0 deletions src/device-registry/routes/v2/sites.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,81 @@
.isIn(constants.NETWORKS)
.withMessage("the tenant value is not among the expected ones"),
]),
oneOf([
[
query("id")
.optional()
.notEmpty()
.trim()
.isMongoId()
.withMessage("id must be an object ID")
.bail()
.customSanitizer((value) => {
return ObjectId(value);

Check warning on line 126 in src/device-registry/routes/v2/sites.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/sites.js#L125-L126 

Added lines #L125 - L126 were not covered by tests
}),
Comment on lines +125 to +127
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Add Unit Tests for Custom Sanitizers Converting to ObjectId

The custom sanitizers for id and site_id convert input values to ObjectId. Currently, these lines are not covered by tests. To ensure the sanitization logic works as intended and to prevent potential issues, please consider adding unit tests for these sanitizers.

Also applies to: 135-137, 206-208, 216-218

Tools
GitHub Check: codecov/patch

[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please create a new Github issue for this suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

@Baalmart, I've successfully created the GitHub issue for this suggestion. You can find it here: #3490

Analysis chain

@Baalmart, sure, I'll open a new GitHub issue for this suggestion.

((\
( ^.^)
o_(")(")

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a new GitHub issue for adding unit tests for custom sanitizers
gh issue create \
  --title "Add Unit Tests for Custom Sanitizers in 'sites.js'" \
  --body "The custom sanitizers for \`id\` and \`site_id\` in \`sites.js\` convert input values to \`ObjectId\`. These lines are not currently covered by tests. To ensure the sanitization logic works as intended and to prevent potential issues, please consider adding unit tests for these sanitizers.

PR: https://github.com/airqo-platform/AirQo-api/pull/3489
Comment: https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771370131

Requested by: @Baalmart" \
  --assignee "Baalmart"

Length of output: 622

query("site_id")
.optional()
.notEmpty()
.trim()
.isMongoId()
.withMessage("site_id must be an object ID")
.bail()
.customSanitizer((value) => {
return ObjectId(value);

Check warning on line 136 in src/device-registry/routes/v2/sites.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/sites.js#L135-L136 

Added lines #L135 - L136 were not covered by tests
}),
query("name")
.optional()
.notEmpty()
.trim(),
query("online_status")
.optional()
.notEmpty()
.withMessage("the online_status should not be empty if provided")
.bail()
.trim()
.toLowerCase()
.isIn(["online", "offline"])
.withMessage(
"the online_status value is not among the expected ones which include: online, offline"
),
query("last_active_before")
.optional()
.notEmpty()
.withMessage("last_active_before date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_before date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active_after")
.optional()
.notEmpty()
.withMessage("last_active_after date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_after date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active")
.optional()
.notEmpty()
.withMessage("last_active date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
],
]),
Comment on lines +116 to +190
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Missing Test Coverage for Custom Sanitizers

No test files were found for src/device-registry/routes/v2/sites.js, indicating that the customSanitizer functions for id and site_id are not currently covered by unit tests. It's crucial to add appropriate tests to ensure these sanitizers behave as expected and maintain the integrity of the API.

Analysis chain

Comprehensive query parameter validation implemented

The addition of new query parameters (id, site_id, name, online_status, last_active_before, last_active_after, last_active) with their respective validation logic enhances the API's capability to filter sites based on various criteria. The validation is thorough and well-structured.

However, there are a few points to consider:

  1. Error Handling: Consider implementing a custom error handler to provide more user-friendly error messages. This could improve the API's usability.

  2. Code Organization: The validation logic for date-related parameters (last_active_before, last_active_after, last_active) is quite similar. You might want to consider extracting this into a reusable function to reduce code duplication.

  3. Test Coverage: The static analysis hints indicate that the custom sanitizers for 'id' and 'site_id' (lines 125-126 and 135-136) are not covered by tests. It's important to add unit tests for these to ensure they behave as expected.

To improve code organization, consider extracting the date validation logic into a reusable function:

function validateDateParameter(field) {
  return query(field)
    .optional()
    .notEmpty()
    .withMessage(`${field} date cannot be empty IF provided`)
    .bail()
    .trim()
    .isISO8601({ strict: true, strictSeparator: true })
    .withMessage(
      `${field} date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ).`
    )
    .bail()
    .toDate();
}

// Usage
validateDateParameter('last_active_before'),
validateDateParameter('last_active_after'),
validateDateParameter('last_active'),

To address the test coverage issue, we can use the following script to locate the relevant test file:

This script will help identify if there are any existing tests for the custom sanitizers and where to add them if they're missing.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find the test file for src/device-registry/routes/v2/sites.js
test_file=$(find test -name "*sites.test.js" -o -name "*sites.spec.js")
if [ -n "$test_file" ]; then
  echo "Test file found: $test_file"
  echo "Checking for sanitizer tests:"
  grep -n "customSanitizer" "$test_file" || echo "No sanitizer tests found."
else
  echo "No test file found for sites.js"
fi

Length of output: 203

Script:

#!/bin/bash
# Search for test files matching *sites.test.js or *sites.spec.js across the entire repository
test_files=$(find . -type f \( -name '*sites.test.js' -o -name '*sites.spec.js' \))

if [ -n "$test_files" ]; then
  echo "Test files found:"
  echo "$test_files"
  echo "Checking for sanitizer tests:"
  grep -n "customSanitizer" $test_files || echo "No sanitizer tests found."
else
  echo "No test files found for sites.js"
fi

Length of output: 179

Tools
GitHub Check: codecov/patch

[warning] 125-126: src/device-registry/routes/v2/sites.js#L125-L126
Added lines #L125 - L126 were not covered by tests

[warning] 135-136: src/device-registry/routes/v2/sites.js#L135-L136
Added lines #L135 - L136 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please create a new Github issue for this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Baalmart, the GitHub issue has been created to track this suggestion: Issue #3494.

Analysis chain

@Baalmart, I'll create a new GitHub issue to track this suggestion.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
gh issue create \
  --title "Add unit tests for custom sanitizers in src/device-registry/routes/v2/sites.js" \
  --body $'The custom sanitizers for `id` and `site_id` in `src/device-registry/routes/v2/sites.js` are not currently covered by unit tests. It\'s crucial to add appropriate tests to ensure these sanitizers behave as expected and maintain the integrity of the API.\n\nOriginally suggested in [PR #3489](https://github.com/airqo-platform/AirQo-api/pull/3489) by @Baalmart.\n\nSee the [comment](https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708562) for more details.' \
  --assignee Baalmart

Length of output: 667

siteController.list
);
router.get(
Expand All @@ -128,6 +203,81 @@
.isIn(constants.NETWORKS)
.withMessage("the tenant value is not among the expected ones"),
]),
oneOf([
[
query("id")
.optional()
.notEmpty()
.trim()
.isMongoId()
.withMessage("id must be an object ID")
.bail()
.customSanitizer((value) => {
return ObjectId(value);

Check warning on line 216 in src/device-registry/routes/v2/sites.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/sites.js#L215-L216 

Added lines #L215 - L216 were not covered by tests
}),
query("site_id")
.optional()
.notEmpty()
.trim()
.isMongoId()
.withMessage("site_id must be an object ID")
.bail()
.customSanitizer((value) => {
return ObjectId(value);

Check warning on line 226 in src/device-registry/routes/v2/sites.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/routes/v2/sites.js#L225-L226 

Added lines #L225 - L226 were not covered by tests
}),
query("name")
.optional()
.notEmpty()
.trim(),
query("online_status")
.optional()
.notEmpty()
.withMessage("the online_status should not be empty if provided")
.bail()
.trim()
.toLowerCase()
.isIn(["online", "offline"])
.withMessage(
"the online_status value is not among the expected ones which include: online, offline"
),
query("last_active_before")
.optional()
.notEmpty()
.withMessage("last_active_before date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_before date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active_after")
.optional()
.notEmpty()
.withMessage("last_active_after date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active_after date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
query("last_active")
.optional()
.notEmpty()
.withMessage("last_active date cannot be empty IF provided")
.bail()
.trim()
.isISO8601({ strict: true, strictSeparator: true })
.withMessage(
"last_active date must be a valid ISO8601 datetime (YYYY-MM-DDTHH:mm:ss.sssZ)."
)
.bail()
.toDate(),
],
]),
siteController.listSummary
);
router.get("/weather", siteController.listWeatherStations);
Expand Down
60 changes: 60 additions & 0 deletions src/device-registry/utils/generate-filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,10 @@
visibility,
deviceName,
status,
online_status,
last_active,
last_active_before,
last_active_after,
} = { ...req.query, ...req.params };

const filter = {};
Expand All @@ -925,6 +929,32 @@
filter.device_number = parseInt(channel);
}

if (last_active) {
filter.lastActive = {};
const start = new Date(last_active);
filter["lastActive"]["$gte"] = start;

Check warning on line 935 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L933-L935 

Added lines #L933 - L935 were not covered by tests
}

if (last_active_after) {
filter.lastActive = {};
const start = new Date(last_active_after);
filter["lastActive"]["$gte"] = start;

Check warning on line 941 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L939-L941 

Added lines #L939 - L941 were not covered by tests
}
Comment on lines +932 to +942
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Refactor suggestion for last_active and last_active_after filters

The implementation of last_active and last_active_after is correct, but they currently set the same filter condition. To improve clarity and maintainability, consider combining these conditions:

if (last_active || last_active_after) {
  filter.lastActive = {};
  const start = new Date(last_active || last_active_after);
  filter["lastActive"]["$gte"] = start;
}

This refactoring reduces code duplication and makes the intent clearer.

Additionally, it's important to note that this new code is not covered by tests. To ensure the reliability of these new filters, please add unit tests that cover various scenarios for these parameters.

Would you like assistance in generating unit tests for these new filters?

Tools
GitHub Check: codecov/patch

[warning] 933-935: src/device-registry/utils/generate-filter.js#L933-L935
Added lines #L933 - L935 were not covered by tests

[warning] 939-941: src/device-registry/utils/generate-filter.js#L939-L941
Added lines #L939 - L941 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please create a new Github issue for this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Baalmart, I've created a GitHub issue for this suggestion as requested: #3492.

Analysis chain

@Baalmart, I'll create a GitHub issue for this suggestion.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Open a new GitHub issue for the suggestion
gh issue create --title "Refactor filters for 'last_active' and 'last_active_after' in generateFilter utility" --body "The implementation of \`last_active\` and \`last_active_after\` in the \`generateFilter\` utility sets the same filter condition. To improve clarity and maintainability, consider combining these conditions to reduce code duplication and enhance readability.

**Pull Request**: https://github.com/airqo-platform/AirQo-api/pull/3489

**Comment**: https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708574

**Requested by**: @Baalmart" --assignee Baalmart

Length of output: 654


if (last_active_before) {
filter.lastActive = {};
const start = new Date(last_active_before);
filter["lastActive"]["$lte"] = start;

Check warning on line 947 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L945-L947 

Added lines #L945 - L947 were not covered by tests
}
Comment on lines +944 to +948
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Correct implementation of last_active_before filter

The implementation of the last_active_before filter is correct. It properly uses the $lte operator to find devices active before the specified date.

However, as with the previous filters, this new code is not covered by tests. To ensure the reliability of this filter, please add unit tests that cover various scenarios for the last_active_before parameter.

Consider adding tests that check:

  1. Devices active before a specific date are included
  2. Devices active after the specified date are excluded
  3. Edge cases, such as devices active exactly on the specified date

If you need assistance in writing these tests, please let me know, and I'd be happy to help.

Tools
GitHub Check: codecov/patch

[warning] 945-947: src/device-registry/utils/generate-filter.js#L945-L947
Added lines #L945 - L947 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please create a new Github issue for this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Baalmart, I've successfully updated the GitHub issue with the correct title, including the last_active_before filter. You can view the updated issue here: #3491. Let me know if there's anything else I can assist with.

(_/)
(^.^)
(")_(")

Analysis chain

@Baalmart, I'll create a GitHub issue for this suggestion.

@Baalmart, it appears there was an error in the script due to unescaped backticks, which caused the issue title to be incorrect. I've corrected the script by escaping the backticks to prevent shell interpretation:

This script will update the existing issue with the correct title. You can check the updated issue here: #3491.

Let me know if there's anything else I can assist with.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a new GitHub issue for adding unit tests for the `last_active_before` filter
gh issue create --title "Add unit tests for `last_active_before` filter in generate-filter.js" \
--body "In PR #3489 (https://github.com/airqo-platform/AirQo-api/pull/3489), new code was added implementing the \`last_active_before\` filter in \`src/device-registry/utils/generate-filter.js\`. This new code is not currently covered by unit tests. To ensure its reliability, unit tests should be added covering various scenarios for the \`last_active_before\` parameter.

Original comment: https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708575

Requester: @Baalmart" \
--assignee Baalmart

Length of output: 746

Script:

#!/bin/bash
# Update the title of the existing issue to include `last_active_before`
gh issue edit 3491 --title "Add unit tests for \\\`last_active_before\\\` filter in generate-filter.js"

Length of output: 157

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


if (online_status) {
if (online_status.toLowerCase() === "online") {
filter["isOnline"] = true;

Check warning on line 952 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L952 

Added line #L952 was not covered by tests
} else if (online_status.toLowerCase() === "offline") {
filter["isOnline"] = false;

Check warning on line 954 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L954 

Added line #L954 was not covered by tests
}
}
Comment on lines +950 to +956
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Enhance robustness of online_status filter

The implementation of the online_status filter is fundamentally correct, but it could be made more robust. Consider the following improvements:

  1. Handle unexpected values:
if (online_status) {
  const status = online_status.toLowerCase();
  if (status === "online") {
    filter["isOnline"] = true;
  } else if (status === "offline") {
    filter["isOnline"] = false;
  } else {
    // Handle unexpected value
    console.warn(`Unexpected online_status value: ${online_status}`);
    // Optionally, you could throw an error or set a default value
  }
}
  1. Add test coverage for this new filter. Include tests for:
    • Setting filter to true for "online" status
    • Setting filter to false for "offline" status
    • Handling of unexpected values
    • Case insensitivity of the input

These enhancements will improve the reliability and maintainability of the online_status filter.

Would you like assistance in implementing these improvements or writing the corresponding unit tests?

Tools
GitHub Check: codecov/patch

[warning] 952-952: src/device-registry/utils/generate-filter.js#L952
Added line #L952 was not covered by tests

[warning] 954-954: src/device-registry/utils/generate-filter.js#L954
Added line #L954 was not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please create a new Github issue for this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Baalmart, I've created a new GitHub issue for this suggestion: #3493.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a new GitHub issue for the suggested improvements
gh issue create \
  --title "Enhance robustness of 'online_status' filter" \
  --body "There is a suggestion to enhance the robustness of the \`online_status\` filter by handling unexpected values and adding test coverage.

[PR URL](https://github.com/airqo-platform/AirQo-api/pull/3489)
[Comment URL](https://github.com/airqo-platform/AirQo-api/pull/3489#discussion_r1771708577)

Requested by @Baalmart" \
  --assignee Baalmart

Length of output: 478


if (category) {
filter.category = category;
}
Expand Down Expand Up @@ -1035,6 +1065,10 @@
network,
group,
google_place_id,
online_status,
last_active,
last_active_before,
last_active_after,
} = { ...req.query, ...req.params, ...req.body };
const filter = {};
logText("we are generating the filter man!");
Expand Down Expand Up @@ -1078,6 +1112,32 @@
filter["visibility"] = true;
}

if (last_active) {
filter.lastActive = {};
const start = new Date(last_active);
filter["lastActive"]["$gte"] = start;

Check warning on line 1118 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L1116-L1118 

Added lines #L1116 - L1118 were not covered by tests
Comment on lines +1116 to +1118
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Test Coverage for New last_active Filters

The new code handling the last_active, last_active_before, and last_active_after parameters in the sites filter is not covered by tests. Including tests for these cases will help verify that the filters work correctly and improve the reliability of the API.

Also applies to: 1122-1124, 1128-1130

Tools
GitHub Check: codecov/patch

[warning] 1116-1118: src/device-registry/utils/generate-filter.js#L1116-L1118
Added lines #L1116 - L1118 were not covered by tests

}

if (last_active_after) {
filter.lastActive = {};
const start = new Date(last_active_after);
filter["lastActive"]["$gte"] = start;

Check warning on line 1124 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L1122-L1124 

Added lines #L1122 - L1124 were not covered by tests
}

if (last_active_before) {
filter.lastActive = {};
const start = new Date(last_active_before);
filter["lastActive"]["$lte"] = start;

Check warning on line 1130 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L1128-L1130 

Added lines #L1128 - L1130 were not covered by tests
}

if (online_status) {
if (online_status.toLowerCase() === "online") {
filter["isOnline"] = true;

Check warning on line 1135 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L1135 

Added line #L1135 was not covered by tests
} else if (online_status.toLowerCase() === "offline") {
filter["isOnline"] = false;

Check warning on line 1137 in src/device-registry/utils/generate-filter.js

View check run for this annotation

Codecov / codecov/patch

src/device-registry/utils/generate-filter.js#L1137 

Added line #L1137 was not covered by tests
}
}

Comment on lines +1133 to +1140
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle Unexpected Values for online_status

In the sites filter, when online_status is neither "online" nor "offline", the filter remains unchanged. To enhance user experience and prevent potential confusion, consider handling unexpected values by adding an else clause that returns an informative error message.

Suggested addition:

        if (online_status) {
          if (online_status.toLowerCase() === "online") {
            filter["isOnline"] = true;
          } else if (online_status.toLowerCase() === "offline") {
            filter["isOnline"] = false;
+         } else {
+           // Handle unexpected value
+           next(new HttpError("Invalid online_status value", httpStatus.BAD_REQUEST));
          }
        }

Committable suggestion was skipped due to low confidence.

Tools
GitHub Check: codecov/patch

[warning] 1135-1135: src/device-registry/utils/generate-filter.js#L1135
Added line #L1135 was not covered by tests

[warning] 1137-1137: src/device-registry/utils/generate-filter.js#L1137
Added line #L1137 was not covered by tests

if (site_codes) {
const siteCodesArray = site_codes.toString().split(",");
filter["site_codes"] = { $in: siteCodesArray };
Expand Down
Loading