Skip to content

Conversation

@Srinath0916
Copy link
Contributor

@Srinath0916 Srinath0916 commented Dec 2, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #1158.

Description of Changes
Added two new query parameters to the Device list API endpoint:

?location= - filters devices by their associated location
?floorplan= - filters devices by their associated floorplan
The filters work through the DeviceLocation relationship. Added test cases for both filters.

Screenshot

Screenshot 2025-12-02 at 12 32 31 PM Screenshot 2025-12-02 at 12 36 53 PM Screenshot 2025-12-02 at 1 46 01 PM Screenshot 2025-12-02 at 1 46 21 PM

@stktyagi
Copy link
Member

stktyagi commented Dec 4, 2025

Hey @Srinath0916 thank you for the PR. Looking at this PR there are some issues that need to be resolved. For starters kindly make sure you read Openwisp Contributing Guidelines as QA checks are failing in the CI build.

Also it looks like the tests are failing because of missing indoor coordinates. Since you are assigning a floorplan in test_filter_devices_by_floorplan, the DeviceLocation model requires the indoor field to be set.

Kindly make sure to run manual tests too before pushing, it would let you know for any inconsistencies.

@Srinath0916
Copy link
Contributor Author

Srinath0916 commented Dec 6, 2025

Hi, thank you for the review,
I've added the required indoor field to both device location creations in the test_filter_devices_by_floorplan test.
Could you please check if this resolves the issue? Let me know if there's anything else that needs to be fixed.

Thank you.

@Srinath0916
Copy link
Contributor Author

Hi, @stktyagi , thank you for the review,

I've added the required indoor field to both device location creations in the test_filter_devices_by_floorplan test.

I can see that 12 out of 13 CI checks have passed successfully. The only failing check is Python 3.9 with Django 4.2.0, which appears to be failing due to a dependency issue.
This seems to be unrelated to my changes since all other Python versions (3.10, 3.11, 3.12, 3.13) are passing. Could you please let me know if this is a known issue or if there's anything I should do on my end?
Looking forward for your feedback on this so i can work on it.
Thank you.

@Srinath0916
Copy link
Contributor Author

Hi @stktyagi , PR #1174 has been merged which drops Python 3.9 support. Could you re-run the checks on this PR? All tests should pass now. Thanks!

@pandafy pandafy moved this from To do (general) to Needs review in OpenWISP Contributor's Board Dec 10, 2025
@Srinath0916 Srinath0916 force-pushed the issues/device-location-floorplan-filter branch from 9dd8f90 to 33185a8 Compare December 10, 2025 10:55
@coveralls
Copy link

coveralls commented Dec 10, 2025

Coverage Status

coverage: 98.641%. remained the same
when pulling 40a024f on Srinath0916:issues/device-location-floorplan-filter
into cbdb3c6 on openwisp:master.

@Srinath0916
Copy link
Contributor Author

Srinath0916 commented Dec 10, 2025

Hi @pandafy , @stktyagi , all checks are passing now, This PR is ready for review.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

BTW @Srinath0916 the screenshots in the PR description are useless, next time please send screenshots or screencast of the application UI, in this case you could have shown the browsable API. The rest looks good, I am going to do a quick round of manual testing.

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

The changes introduce new geographic filtering capabilities for devices in the API. Filters for device location name, location UUID, and floorplan UUID are now available, with labels added for clarity. The codebase was updated to inject these fields into the device filter set and deprecated a custom label-setting function in favor of direct attribute labels. Two tests were added to verify device filtering by location and floorplan, ensuring the correct behavior for various filter query combinations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding location and floorplan filters to the device list API.
Description check ✅ Passed The description covers the main changes, includes issue reference, test coverage confirmation, and screenshots; documentation was intentionally not updated.
Linked Issues check ✅ Passed All requirements from issue #1158 are met: location filter [#1158] and floorplan filter [#1158] both implemented with proper DeviceLocation relationship integration.
Out of Scope Changes check ✅ Passed All changes are directly related to the scope of adding location and floorplan filters; no unrelated modifications were introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9ab51dc and 40a024f.

📒 Files selected for processing (2)
  • openwisp_controller/geo/api/filters.py
  • openwisp_controller/geo/tests/test_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • openwisp_controller/geo/tests/test_api.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

  • openwisp_controller/geo/api/filters.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

  • openwisp_controller/geo/api/filters.py
🪛 Ruff (0.14.14)
openwisp_controller/geo/api/filters.py

29-29: Unused method argument: name

(ARG002)


35-40: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.12 | django~=4.2.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (3)
openwisp_controller/geo/api/filters.py (3)

12-27: LGTM!

The new filter definitions are well-structured: UUID filters for location and floorplan correctly traverse the devicelocation relationship, and the location__name filter appropriately uses icontains for case-insensitive partial matching. Labels are properly internationalized with gettext_lazy.


29-31: Static analysis false positive for unused name parameter.

The name parameter is required by django-filters' custom method signature (self, queryset, name, value). The Ruff ARG002 warning can be safely ignored here.


41-43: [Rewritten comment text]
[Classification tag]

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nemesifier nemesifier changed the title Added location and floorplan filters to device list API [feature[ Added location and floorplan filters to device list API Jan 27, 2026
@nemesifier nemesifier changed the title [feature[ Added location and floorplan filters to device list API [feature] Added location and floorplan filters to device list API Jan 27, 2026
nemesifier
nemesifier previously approved these changes Jan 27, 2026
@github-project-automation github-project-automation bot moved this from Needs review to In progress in OpenWISP Contributor's Board Jan 27, 2026
@nemesifier
Copy link
Member

nemesifier commented Jan 27, 2026

I did some minor changes in fc1374f and 40a024f, waiting for the CI build to pass and then will merge. Thanks @Srinath0916 🙏

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 27, 2026
@nemesifier nemesifier dismissed stale reviews from coderabbitai[bot] and themself via 40a024f January 27, 2026 20:19
@nemesifier nemesifier force-pushed the issues/device-location-floorplan-filter branch from 9ab51dc to 40a024f Compare January 27, 2026 20:19
@nemesifier nemesifier merged commit 6c08f7f into openwisp:master Jan 27, 2026
14 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in OpenWISP Contributor's Board Jan 27, 2026
@Srinath0916
Copy link
Contributor Author

@nemesifier , Thank you for merging, Looking forward to contributing more!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

[feature] REST API: allow filtering devices by location ID and floorplan ID

4 participants