[fix] Make wireless SSID optional in monitoring schema #729#752
Open
sargunn20 wants to merge 1 commit intoopenwisp:masterfrom
Open
[fix] Make wireless SSID optional in monitoring schema #729#752sargunn20 wants to merge 1 commit intoopenwisp:masterfrom
sargunn20 wants to merge 1 commit intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughRemoved the "ssid" property from the wireless interface schema's required list so wireless entries no longer fail validation when SSID is absent. Added a regression test Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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}
sargunn20
force-pushed
the
fix/729-ssid-not-required
branch
from
589f6d6 to
c2e5913
Compare
Removed ssid from the required fields in the wireless interface schema. When a wireless access point is disabled, the monitoring agent does not report an SSID, which caused schema validation to fail with: 'ssid' is a required property. The ssid property definition is retained, it is simply no longer mandatory. Added a regression test to verify wireless data without an SSID passes validation. Closes openwisp#729
sargunn20
force-pushed
the
fix/729-ssid-not-required
branch
from
c2e5913 to
d490518
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_models.py`:
- Line 460: Replace the fragile deletion of the SSID key (currently done with
del data["interfaces"][0]["wireless"]["ssid"]) with a safe removal using
dict.pop with a default so the test won't KeyError if the fixture changes;
locate the mutation of the sample fixture around the interfaces->wireless SSID
and change the deletion to use pop("ssid", None) on that wireless dict to
preserve intent and make the test robust.
- Around line 452-463: The test test_wireless_ssid_not_required only calls
dd.validate_data(); extend it to exercise the full ingestion path by invoking
dd.save_data() as well (after dd.validate_data()) so the code paths in
DeviceData.save_data() are exercised; locate the test using the
_create_device_data() helper, the dd variable, and the _sample_data mutation to
add the save_data() call.
In `@openwisp_monitoring/tests/test_selenium.py`:
- Around line 763-769: Insert a short animation-settle pause before the
WebDriverWaits that execute the ECharts inspection lambda (the lambda calling
d.execute_script to read window._owGeoMap.echarts and find the series data for
"Test-Location"); specifically add a brief sleep (e.g., sleep(0.3)) immediately
before the WebDriverWait that wraps that lambda at the shown location and the
sibling WebDriverWait around lines 780-786 so the test reads ECharts state after
animations settle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f394d849-378b-43bc-b283-1827b6b311aa
📒 Files selected for processing (3)
openwisp_monitoring/device/schema.pyopenwisp_monitoring/device/tests/test_models.pyopenwisp_monitoring/tests/test_selenium.py
💤 Files with no reviewable changes (1)
- openwisp_monitoring/device/schema.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.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.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.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 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_models.pyopenwisp_monitoring/tests/test_selenium.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_models.pyopenwisp_monitoring/tests/test_selenium.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_models.pyopenwisp_monitoring/tests/test_selenium.py
🔇 Additional comments (1)
openwisp_monitoring/tests/test_selenium.py (1)
836-844: JS execution string rewrites look behavior-preserving.These changes keep the same query logic and return shape for the map-series assertions.
Also applies to: 870-878, 903-911
Comment on lines
+452
to
+463
| def test_wireless_ssid_not_required(self): | ||
| """Regression test for https://github.com/openwisp/openwisp-monitoring/issues/729 | ||
|
|
||
| Verifies that wireless interface data without an SSID | ||
| (e.g. when an AP is disabled) passes schema validation. | ||
| """ | ||
| dd = self._create_device_data() | ||
| data = deepcopy(self._sample_data) | ||
| del data["interfaces"][0]["wireless"]["ssid"] | ||
| dd.data = data | ||
| dd.validate_data() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Exercise save_data() in this regression, not only validate_data().
Line 462 verifies schema acceptance, but this change also affects data that can reach persistence/session logic. Extend this test to call dd.save_data() so the full ingestion path is covered.
Proposed test strengthening
def test_wireless_ssid_not_required(self):
"""Regression test for https://github.com/openwisp/openwisp-monitoring/issues/729
Verifies that wireless interface data without an SSID
(e.g. when an AP is disabled) passes schema validation.
"""
dd = self._create_device_data()
data = deepcopy(self._sample_data)
del data["interfaces"][0]["wireless"]["ssid"]
dd.data = data
- dd.validate_data()
+ dd.validate_data()
+ dd.save_data()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_wireless_ssid_not_required(self): | |
| """Regression test for https://github.com/openwisp/openwisp-monitoring/issues/729 | |
| Verifies that wireless interface data without an SSID | |
| (e.g. when an AP is disabled) passes schema validation. | |
| """ | |
| dd = self._create_device_data() | |
| data = deepcopy(self._sample_data) | |
| del data["interfaces"][0]["wireless"]["ssid"] | |
| dd.data = data | |
| dd.validate_data() | |
| def test_wireless_ssid_not_required(self): | |
| """Regression test for https://github.com/openwisp/openwisp-monitoring/issues/729 | |
| Verifies that wireless interface data without an SSID | |
| (e.g. when an AP is disabled) passes schema validation. | |
| """ | |
| dd = self._create_device_data() | |
| data = deepcopy(self._sample_data) | |
| del data["interfaces"][0]["wireless"]["ssid"] | |
| dd.data = data | |
| dd.validate_data() | |
| dd.save_data() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/tests/test_models.py` around lines 452 - 463, The
test test_wireless_ssid_not_required only calls dd.validate_data(); extend it to
exercise the full ingestion path by invoking dd.save_data() as well (after
dd.validate_data()) so the code paths in DeviceData.save_data() are exercised;
locate the test using the _create_device_data() helper, the dd variable, and the
_sample_data mutation to add the save_data() call.
Contributor
There was a problem hiding this comment.
This seems like an interesting suggestion.
There was a problem hiding this comment.
@asmodehn, glad you find it useful! The addition is minimal but meaningful — validate_data() only runs the JSON schema check, whereas save_data() exercises the write path through the timeseries DB and session logic. Since SSID-less wireless data could flow through that path in production (e.g. when an AP is disabled), having the regression test cover it end-to-end gives stronger confidence.
Here's the proposed change:
- Add
dd.save_data()call afterdd.validate_data()intest_wireless_ssid_not_required
🧠 Learnings used
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.
Failed to handle agent chat message. Please try again.
| """ | ||
| dd = self._create_device_data() | ||
| data = deepcopy(self._sample_data) | ||
| del data["interfaces"][0]["wireless"]["ssid"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer pop("ssid", None) over del for fixture mutation robustness.
Line 460 can raise KeyError if the sample fixture changes. pop(..., None) keeps the intent while reducing brittleness.
Proposed small refactor
- del data["interfaces"][0]["wireless"]["ssid"]
+ data["interfaces"][0]["wireless"].pop("ssid", None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| del data["interfaces"][0]["wireless"]["ssid"] | |
| data["interfaces"][0]["wireless"].pop("ssid", None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/tests/test_models.py` at line 460, Replace the
fragile deletion of the SSID key (currently done with del
data["interfaces"][0]["wireless"]["ssid"]) with a safe removal using dict.pop
with a default so the test won't KeyError if the fixture changes; locate the
mutation of the sample fixture around the interfaces->wireless SSID and change
the deletion to use pop("ssid", None) on that wireless dict to preserve intent
and make the test robust.
Comment on lines
+763
to
+769
| lambda d: d.execute_script(""" | ||
| const options = window._owGeoMap.echarts.getOption(); | ||
| const series = options.series.find( | ||
| (s) => s.type === "scatter" || s.type === "effectScatter", | ||
| ); | ||
| return series.data.find(d => d.name === "Test-Location").value; | ||
| """ | ||
| ) | ||
| """) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add the same animation settle delay used in sibling subtests.
These waits read ECharts state immediately after geometry updates. For consistency with the other map-update subtests, add a short pause before each WebDriverWait here to reduce flakiness.
Proposed reliability tweak
location.geometry = Point(12.513124, 41.898903)
location.full_clean()
location.save()
+ sleep(0.3) # Wait for JS animation
series_value = WebDriverWait(self.web_driver, 5).until(
lambda d: d.execute_script("""
const options = window._owGeoMap.echarts.getOption();
const series = options.series.find(
(s) => s.type === "scatter" || s.type === "effectScatter",
);
return series.data.find(d => d.name === "Test-Location").value;
""")
)
@@
location.geometry = Point(12.511124, 41.898903)
location.full_clean()
location.save()
+ sleep(0.3) # Wait for JS animation
series_value = WebDriverWait(self.web_driver, 5).until(
lambda d: d.execute_script("""
const options = window._owGeoMap.echarts.getOption();
const series = options.series.find(
(s) => s.type === "scatter" || s.type === "effectScatter",
);
return series.data.find(d => d.name === "Test-Location").value;
""")
)Based on learnings: In Selenium tests for JS-driven dashboard/map updates, insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to reduce animation-related flakiness.
Also applies to: 780-786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/tests/test_selenium.py` around lines 763 - 769, Insert a
short animation-settle pause before the WebDriverWaits that execute the ECharts
inspection lambda (the lambda calling d.execute_script to read
window._owGeoMap.echarts and find the series data for "Test-Location");
specifically add a brief sleep (e.g., sleep(0.3)) immediately before the
WebDriverWait that wraps that lambda at the shown location and the sibling
WebDriverWait around lines 780-786 so the test reads ECharts state after
animations settle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removed
ssidfrom the required fields in the wireless interface schema. When a wireless access point is disabled, the monitoring agent does not report an SSID, which caused schema validation to fail with:'ssid' is a required property.The
ssidproperty definition is retained, it is simply no longer mandatory. Added a regression test to verify wireless data without an SSID passes validation.Checklist
Reference to Existing Issue
Closes #729
Description of Changes
ssidfrom therequiredarray in the wireless interface schema (device/schema.py)test_wireless_ssid_not_requiredintest_models.py