[change] Add mobile 5g rssi signal data#782
Conversation
📝 WalkthroughWalkthroughThe changes extend OpenWISP monitoring to support 5G mobile signal metrics. The JSON schema for mobile signal validation was updated to relax strict field requirements, replacing all-fields-required lists with Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
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 808-857: The current test_5g_mobile_properties only exercises the
full signal.5g payload; add a minimal-payload case to assert the new "at least
one metric" validation path succeeds: after creating org/device, call _post_data
with a DeviceMonitoring payload where interfaces[0].mobile.signal contains "5g"
with a single metric (e.g., {"rsrp": -75}) and assert response.status_code ==
200, then load DeviceData(pk=device.pk).data and check that
mobile.signal["5g"]["rsrp"] is stored as float and that other metrics are absent
or not required; use the existing helpers (_create_org, _create_device,
_post_data, DeviceData, device.id/device.key/device.pk) and either add this as a
new test function or a subTest inside test_5g_mobile_properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7071a885-5f75-45cf-aaa7-24123cb75415
📒 Files selected for processing (4)
openwisp_monitoring/device/schema.pyopenwisp_monitoring/device/templates/admin/monitoring/device/change_form.htmlopenwisp_monitoring/device/tests/test_admin.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). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/schema.pyopenwisp_monitoring/device/tests/test_admin.py
🧠 Learnings (6)
📚 Learning: 2026-03-24T21:28:33.379Z
Learnt from: CR
Repo: openwisp/openwisp-monitoring PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:28:33.379Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_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.pyopenwisp_monitoring/device/tests/test_admin.py
📚 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/tests/test_api.pyopenwisp_monitoring/device/schema.pyopenwisp_monitoring/device/tests/test_admin.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.pyopenwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/schema.pyopenwisp_monitoring/device/tests/test_admin.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/device/schema.pyopenwisp_monitoring/device/tests/test_admin.py
🔇 Additional comments (6)
openwisp_monitoring/device/tests/test_admin.py (1)
504-542: Looks good.This covers the new 5G admin rendering path end-to-end and matches the existing LTE-style assertions.
openwisp_monitoring/device/templates/admin/monitoring/device/change_form.html (1)
327-352: Looks good.The 5G block is consistent with the existing LTE rendering and is gated correctly on the signal object.
openwisp_monitoring/device/schema.py (1)
335-363: Looks good.The relaxed
anyOfvalidation matches the new one-metric requirement for both LTE and 5G without changing the surrounding schema shape.openwisp_monitoring/device/tests/test_api.py (3)
918-919: Good update to the negative validation case.Switching this payload to
unknown_metrickeeps the regression test invalid now that partiallte/5gpayloads are accepted.
1004-1043: Looks good.This covers the 5G-only chart path and verifies the new access-tech mapping behaves as expected.
1044-1085: Looks good.The mixed LTE/5G case is a good regression check for 5G-NSA precedence and chart summaries.
| def test_5g_mobile_properties(self): | ||
| org = self._create_org() | ||
| device = self._create_device(organization=org) | ||
| data = { | ||
| "type": "DeviceMonitoring", | ||
| "interfaces": [ | ||
| { | ||
| "name": "mobile0", | ||
| "mac": "00:00:00:00:00:00", | ||
| "mtu": 1900, | ||
| "multicast": True, | ||
| "txqueuelen": 1000, | ||
| "type": "modem-manager", | ||
| "up": True, | ||
| "mobile": { | ||
| "connection_status": "connected", | ||
| "imei": "300000001234567", | ||
| "manufacturer": "Sierra Wireless, Incorporated", | ||
| "model": "MC7430", | ||
| "operator_code": "50502", | ||
| "operator_name": "YES OPTUS", | ||
| "power_status": "on", | ||
| "signal": { | ||
| "5g": {"rsrp": -75, "rsrq": -8, "rssi": -52.00, "snr": 13} | ||
| }, | ||
| }, | ||
| } | ||
| ], | ||
| } | ||
| response = self._post_data(device.id, device.key, data) | ||
| self.assertEqual(response.status_code, 200) | ||
| mobile_data = DeviceData(pk=device.pk).data["interfaces"][0]["mobile"] | ||
| with self.subTest("check mobile interface static properties"): | ||
| self.assertEqual(mobile_data["connection_status"], "connected") | ||
| self.assertEqual(mobile_data["imei"], "300000001234567") | ||
| self.assertEqual(mobile_data["model"], "MC7430") | ||
| self.assertEqual(mobile_data["operator_code"], "50502") | ||
| self.assertEqual(mobile_data["operator_name"], "YES OPTUS") | ||
| self.assertEqual(mobile_data["power_status"], "on") | ||
| with self.subTest("ensure signal data is converted to float"): | ||
| # ensure numbers are stored as floats, | ||
| # lua cannot be forced to send floats so we need to force it | ||
| self.assertIsInstance(mobile_data["signal"]["5g"]["rsrp"], float) | ||
| self.assertIsInstance(mobile_data["signal"]["5g"]["rsrq"], float) | ||
| self.assertIsInstance(mobile_data["signal"]["5g"]["rssi"], float) | ||
| self.assertIsInstance(mobile_data["signal"]["5g"]["snr"], float) | ||
| self.assertDictEqual( | ||
| mobile_data["signal"], | ||
| {"5g": {"rsrp": -75.00, "rsrq": -8.00, "rssi": -52.00, "snr": 13.00}}, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cover the new relaxed 5G validation with a minimal payload.
This test still posts a fully populated signal.5g object, so it doesn't prove the new "at least one metric" schema path that this PR introduces.
♻️ Suggested addition
with self.subTest("ensure signal data is converted to float"):
# ensure numbers are stored as floats,
# lua cannot be forced to send floats so we need to force it
self.assertIsInstance(mobile_data["signal"]["5g"]["rsrp"], float)
self.assertIsInstance(mobile_data["signal"]["5g"]["rsrq"], float)
self.assertIsInstance(mobile_data["signal"]["5g"]["rssi"], float)
self.assertIsInstance(mobile_data["signal"]["5g"]["snr"], float)
self.assertDictEqual(
mobile_data["signal"],
{"5g": {"rsrp": -75.00, "rsrq": -8.00, "rssi": -52.00, "snr": 13.00}},
)
+
+ with self.subTest("accept a minimal 5G signal payload"):
+ minimal_data = deepcopy(data)
+ minimal_data["interfaces"][0]["mobile"]["signal"] = {"5g": {"rssi": -52}}
+ response = self._post_data(device.id, device.key, minimal_data)
+ self.assertEqual(response.status_code, 200)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/tests/test_api.py` around lines 808 - 857, The
current test_5g_mobile_properties only exercises the full signal.5g payload; add
a minimal-payload case to assert the new "at least one metric" validation path
succeeds: after creating org/device, call _post_data with a DeviceMonitoring
payload where interfaces[0].mobile.signal contains "5g" with a single metric
(e.g., {"rsrp": -75}) and assert response.status_code == 200, then load
DeviceData(pk=device.pk).data and check that mobile.signal["5g"]["rsrp"] is
stored as float and that other metrics are absent or not required; use the
existing helpers (_create_org, _create_device, _post_data, DeviceData,
device.id/device.key/device.pk) and either add this as a new test function or a
subTest inside test_5g_mobile_properties.
There was a problem hiding this comment.
if we want to check with this level of detail, wont we need to check each of the metrics one by one ?
There was a problem hiding this comment.
@asmodehn — Not necessarily. The goal of the suggested minimal-payload subTest is narrower than full per-metric coverage: it just needs to confirm that sending a single metric no longer triggers a 400 (i.e., the anyOf gate in the schema is working at all). One metric is sufficient to prove that path.
That said, if you did want to be thorough, you could do something compact like:
with self.subTest("accept each single 5G metric on its own"):
for metric, value in [("rssi", -52), ("rsrp", -75), ("rsrq", -8), ("snr", 13)]:
minimal_data = deepcopy(data)
minimal_data["interfaces"][0]["mobile"]["signal"] = {"5g": {metric: value}}
response = self._post_data(device.id, device.key, minimal_data)
self.assertEqual(response.status_code, 200, f"Failed for single metric: {metric}")But for this PR's scope, a single-metric check (especially for rssi, which is the new addition) is enough to cover the relaxed-validation path without requiring four separate checks.
🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-monitoring PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:28:33.379Z
Learning: Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Learnt from: CR
Repo: openwisp/openwisp-monitoring PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-24T21:28:33.379Z
Learning: Features: Tests must be added and coverage must not decrease significantly; if the feature impacts UI, Selenium browser tests are preferred
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.
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.
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.
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Multiple Test Failures DetectedHello @asmodehn, There are multiple test failures in the CI logs. Please address the following:
|
asmodehn
changed the title
There was a problem hiding this comment.
Findings
- Low: admin renders empty metric rows for valid partial signal payloads
openwisp_monitoring/device/templates/admin/monitoring/device/change_form.html:327
The schema now accepts 5g payloads with any one of rssi, rsrp, rsrq, or snr, but the admin block renders all four rows whenever interface.mobile.signal.5g exists. For valid inputs like {"5g": {"rsrp": -70}}, the admin will show blank Signal Strength, Signal Quality, and Signal to noise ratio values with units. The same behavioral issue applies to LTE after relaxing its schema in schema.py:335. Consider guarding each metric row individually, or only displaying fields present in the payload.
2 participants
Checklist
Reference to Existing Issue
Address parts of #779
Description of Changes
This adds rssi to 5g metrics. Also requires one metric only for lte and 5g signal data