[fix] Fixed unused fields in device monitoring API docs#750
[fix] Fixed unused fields in device monitoring API docs#750sargunn20 wants to merge 2 commits intoopenwisp:masterfrom
Conversation
Removed the 'schema' attribute from DeviceMetricView which was accidentally shadowing DRF's APIView.schema attribute used by drf-yasg for Swagger documentation generation. This caused the browsable API to show misleading 'example' and 'Model' tabs with empty content. Also changed serializer_class from the empty serializers.Serializer to MonitoringDeviceDetailSerializer so that drf-yasg can properly auto-generate the response schema documentation. Closes openwisp#716
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDeviceMetricView in openwisp_monitoring/device/api/views.py now sets serializer_class = MonitoringDeviceDetailSerializer (replacing the previous generic serializers.Serializer) and the view-level schema attribute was removed. The import of the serializers module from that file was removed. A regression test test_device_metric_view_schema_not_empty was added in openwisp_monitoring/device/tests/test_api.py to assert the view uses the concrete serializer, that get_fields() returns real fields (including id, name, monitoring), and that DeviceMetricView.schema is not a plain dict if present. Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/device/api/views.py (1)
102-127: 🧹 Nitpick | 🔵 TrivialAdd a regression test for API docs output on this view.
This fix is documentation-facing; add a test that asserts
/monitoring/device/{id}/no longer exposes empty/misleading “example”/“Model” artifacts, so the shadowing issue does not regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/api/views.py` around lines 102 - 127, Add a regression test that requests the API schema/docs (OpenAPI/Swagger JSON) for the DeviceMetricView endpoint and asserts the /monitoring/device/{id}/ operation does not contain empty or misleading "example"/"Model" artifacts; locate the endpoint by referencing the DeviceMetricView view name or URL pattern for the device detail route and use the MonitoringDeviceDetailSerializer to confirm the documented request/response schema matches the serializer fields (and contains no empty example/model entries). Implement the test by generating the schema (using DRF/OpenAPI schema generator or the project test client hitting the docs JSON endpoint), finding the path item for /monitoring/device/{id}/, and failing the test if any schema objects for that operation include empty "example" or placeholder "Model" strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_monitoring/device/api/views.py`:
- Around line 102-127: Add a regression test that requests the API schema/docs
(OpenAPI/Swagger JSON) for the DeviceMetricView endpoint and asserts the
/monitoring/device/{id}/ operation does not contain empty or misleading
"example"/"Model" artifacts; locate the endpoint by referencing the
DeviceMetricView view name or URL pattern for the device detail route and use
the MonitoringDeviceDetailSerializer to confirm the documented request/response
schema matches the serializer fields (and contains no empty example/model
entries). Implement the test by generating the schema (using DRF/OpenAPI schema
generator or the project test client hitting the docs JSON endpoint), finding
the path item for /monitoring/device/{id}/, and failing the test if any schema
objects for that operation include empty "example" or placeholder "Model"
strings.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/api/views.py
📜 Review details
⏰ 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). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/api/views.py
🧬 Code graph analysis (1)
openwisp_monitoring/device/api/views.py (1)
openwisp_monitoring/device/api/serializers.py (1)
MonitoringDeviceDetailSerializer(118-119)
🔇 Additional comments (1)
openwisp_monitoring/device/api/views.py (1)
126-126: Good serializer binding for schema generation.Using
MonitoringDeviceDetailSerializerhere is the right fix to expose meaningful response schema metadata for this endpoint.
|
Please run the following in order: And then (Make sure all QA checks show Also the commit message (according to this PR) should follow this structure: Please make sure to read more here |
sargunn20
force-pushed
the
fix/716-device-monitoring-api-schema
branch
2 times, most recently
from
8b3d895 to
fa3a0d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/device/tests/test_api.py`:
- Around line 1453-1458: The current loop over schema_dict paths uses a loose
substring check on path_name which can select nested endpoints (e.g.,
/monitoring/device/{id}/nearby/); tighten the matching in the loop that assigns
path_item by changing the condition to only accept exact device-detail endpoints
by matching path_name against a precise pattern (for example, use a regex to
match ^/monitoring/device/(\{id\}|\{uuid\})/?$) instead of the current "in"
checks; update the condition that references path_name, path_obj and sets
path_item so only the intended detail endpoint is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fa75fcb-635b-48ba-8ee9-cc0ba17fe04b
📒 Files selected for processing (2)
openwisp_monitoring/device/api/views.pyopenwisp_monitoring/device/tests/test_api.py
📜 Review details
⏰ 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). (8)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/device/api/views.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_api.py
🔇 Additional comments (2)
openwisp_monitoring/device/api/views.py (2)
16-16: Import cleanup is correct.Dropping the unused
serializersimport here is consistent with the updated view implementation.
126-126: Good fix for API schema generation.Using
MonitoringDeviceDetailSerializeron Line 126 is the right change for accurate DRF/drf-yasg response documentation.
sargunn20
force-pushed
the
fix/716-device-monitoring-api-schema
branch
from
fa3a0d5 to
2619adc
Compare
|
9 out of 11 CI jobs pass. The 2 failures (Python 3.12 + Django 5.2 and Python 3.13 + Django 5.2) are a pre-existing upstream issue, the same jobs fail on other PRs as well (e.g., CI Run #2122 on a different branch) |
| from ..api.serializers import MonitoringDeviceDetailSerializer | ||
| from ..api.views import DeviceMetricView |
| """ | ||
| Regression test for https://github.com/openwisp/openwisp-monitoring/issues/716. | ||
| Ensures DeviceMetricView uses a proper serializer (not blank) | ||
| and does not shadow DRF's schema attribute with a plain dict. | ||
| """ |
There was a problem hiding this comment.
can you remove the first line and keep only bottom 2
| """ | ||
| from ..api.serializers import MonitoringDeviceDetailSerializer | ||
| from ..api.views import DeviceMetricView | ||
|
|
There was a problem hiding this comment.
Thanks for the review! I've made the 3 requested changes (moved imports to the top, shortened the docstring, and removed the trailing blank line). Everything is pushed.
Removed empty schema from DeviceMetricView that shadowed DRF auto-generated schema. Fixes openwisp#716
sargunn20
force-pushed
the
fix/716-device-monitoring-api-schema
branch
from
2619adc to
09bfed4
Compare
3 participants
Removed the 'schema' attribute from DeviceMetricView which was accidentally shadowing DRF's APIView.schema attribute used by drf-yasg for Swagger documentation generation. This caused the browsable API to show misleading 'example' and 'Model' tabs with empty content.
Also changed serializer_class from the empty serializers.Serializer to MonitoringDeviceDetailSerializer so that drf-yasg can properly auto-generate the response schema documentation.
Checklist
Reference to Existing Issue
Closes #716
Description of Changes
schema = schemafrom DeviceMetricView — this NetJSON validation dict was shadowing DRF'sAPIView.schemaattribute, confusing drf-yasgserializer_classfrom emptyserializers.Serializerto MonitoringDeviceDetailSerializerfrom ..schema import schemaimportNote: The data validation schema on the model (
AbstractDeviceData.schema) is unaffected — only the view attribute was removed.