-
-
Notifications
You must be signed in to change notification settings - Fork 253
[feature] Added location and floorplan filters to device list API #1169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] Added location and floorplan filters to device list API #1169
Conversation
|
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. |
|
Hi, thank you for the review, Thank you. |
|
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. |
9dd8f90 to
33185a8
Compare
nemesifier
left a comment
There was a problem hiding this 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.
📝 WalkthroughWalkthroughThe 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🪛 Ruff (0.14.14)openwisp_controller/geo/api/filters.py29-29: Unused method argument: (ARG002) 35-40: Mutable class attributes should be annotated with (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)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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. Comment |
|
I did some minor changes in fc1374f and 40a024f, waiting for the CI build to pass and then will merge. Thanks @Srinath0916 🙏 |
40a024f
9ab51dc to
40a024f
Compare
|
@nemesifier , Thank you for merging, Looking forward to contributing more! |
Checklist
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