[change] Add config_app_label and app_label into device test_admin#748
[change] Add config_app_label and app_label into device test_admin#748asmodehn wants to merge 2 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThis change updates tests in openwisp_monitoring/device/tests/test_admin.py to remove hard-coded admin URL reversals and use per-class attributes for app labels. TestAdmin now defines public attributes Sequence Diagram(s)(Skipped — changes are test-only URL construction updates and do not introduce a new multi-component control flow.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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_admin.py`:
- Around line 46-52: Remove the duplicate assignment to the module-level
variable app_label (it’s assigned twice to "config"); keep a single app_label =
"config" (the existing one is sufficient), and leave config_app_label = "config"
and the modification to resources_fields
(TestImportExportMixin.resource_fields.append("monitoring_status")) unchanged;
update the block that currently contains two app_label = "config" lines to only
include one.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/tests/test_admin.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.11 | django~=5.1.0
- GitHub Check: Python==3.10 | 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.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.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | 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_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_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_admin.py
🔇 Additional comments (2)
openwisp_monitoring/device/tests/test_admin.py (2)
125-125: LGTM!The dynamic URL construction using
self.config_app_labelenables test extensibility for derived apps while maintaining the same default behavior.
915-916: LGTM!Adding
config_app_labeltoTestWifiSessionAdmincorrectly enables dynamic URL construction for this test class. Since it doesn't inherit fromTestImportExportMixin, omittingapp_labelhere is appropriate.
asmodehn
force-pushed
the
config_app_label_in_monitoring_device_tests
branch
from
8986308 to
d923e30
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_monitoring/device/tests/test_admin.py (1)
46-52:⚠️ Potential issue | 🟠 MajorRemove the hardcoded
app_labeloverride.Line 52 (
app_label = "config") overrides the dynamic assignment at Line 47, which defeats the goal of making this test class derivable across apps.🔧 Proposed fix
class TestAdmin( TestWifiClientSessionMixin, TestImportExportMixin, DeviceMonitoringTestCase ): @@ # Since TestImportExportMixin expects app_label for config app app_label = Device._meta.app_label config_app_label = Device._meta.app_label @@ resources_fields = TestImportExportMixin.resource_fields resources_fields.append("monitoring_status") - app_label = "config" _device_params = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/tests/test_admin.py` around lines 46 - 52, The test currently overrides the dynamically set app_label by hardcoding app_label = "config"; remove that hardcoded assignment so app_label remains derived from Device._meta.app_label; ensure TestImportExportMixin.resource_fields is still appended with "monitoring_status" (reference: Device, TestImportExportMixin.resource_fields, monitoring_status, app_label) so the test stays derivable across apps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_monitoring/device/tests/test_admin.py`:
- Around line 46-52: The test currently overrides the dynamically set app_label
by hardcoding app_label = "config"; remove that hardcoded assignment so
app_label remains derived from Device._meta.app_label; ensure
TestImportExportMixin.resource_fields is still appended with "monitoring_status"
(reference: Device, TestImportExportMixin.resource_fields, monitoring_status,
app_label) so the test stays derivable across apps.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_monitoring/device/tests/test_admin.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.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.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_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_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_admin.py
🔇 Additional comments (2)
openwisp_monitoring/device/tests/test_admin.py (2)
125-125: Good migration to dynamic admin URL construction.Using
self.config_app_labelin these reversals removes hard-coded app namespace coupling and makes test reuse across derived apps much cleaner.Also applies to: 196-196, 236-236, 261-261, 336-336, 342-342, 363-363, 382-382, 387-387, 394-394, 423-423, 491-491, 502-502, 538-538, 567-567, 612-612, 755-755, 870-877, 1060-1062, 1087-1089
915-915:config_app_labelonTestWifiSessionAdminis a good extensibility hook.Adding this class attribute makes downstream overrides straightforward where device admin namespace differs.
| TestWifiClientSessionMixin, | ||
| TestCase, | ||
| ): | ||
| config_app_label = "config" |
There was a problem hiding this comment.
I think this is missing right?
| config_app_label = "config" | |
| config_app_label = Device._meta.app_label |
There was a problem hiding this comment.
correct. I ll fix it asap.
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_admin.py`:
- Around line 46-48: Tests currently hardcode "device" in admin URL names and
paths; add a dynamic config_model_name = Device._meta.model_name and replace all
hardcoded "device" usages (e.g., admin:{self.config_app_label}_device_change,
device_changelist, and /admin/{self.config_app_label}/device/recover/) with the
model-name variable (use
admin:{self.config_app_label}_{self.config_model_name}_change,
{self.config_app_label}_{self.config_model_name}_changelist, and
/admin/{self.config_app_label}/{self.config_model_name}/recover/). Update every
occurrence noted in the review (including the blocks around lines 124-125, 341,
869-876, 914-915, 1059-1061, 1086-1088) to use config_model_name so the tests
work when a concrete Device model has a different name, following the pattern
used for WifiSession._meta.model_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d37804cc-09eb-468b-9c3b-a84db7d91a12
📒 Files selected for processing (1)
openwisp_monitoring/device/tests/test_admin.py
📜 Review details
🧰 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_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_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_admin.py
| # Since TestImportExportMixin expects app_label for config app | ||
| app_label = Device._meta.app_label | ||
| config_app_label = Device._meta.app_label |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the test file to understand the current implementation
cat -n openwisp_monitoring/device/tests/test_admin.py | head -150Repository: openwisp/openwisp-monitoring
Length of output: 7299
🏁 Script executed:
# Check lines around the mentioned segments
sed -n '40,130p' openwisp_monitoring/device/tests/test_admin.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 4271
🌐 Web query:
Django admin URL names app_label model_name convention
💡 Result:
Django admin URL names for a model follow this convention (in the admin URL namespace):
admin:{app_label}_{model_name}_changelistadmin:{app_label}_{model_name}_addadmin:{app_label}_{model_name}_change(expectsargs=(object_id,))admin:{app_label}_{model_name}_delete(expectsargs=(object_id,))admin:{app_label}_{model_name}_history(expectsargs=(object_id,))
These are generated from ModelAdmin.get_urls() using info = (self.opts.app_label, self.opts.model_name) (note: model_name is the lowercased _meta.model_name). [1]
Example:
from django.urls import reverse
reverse("admin:auth_user_changelist")
reverse("admin:polls_choice_change", args=(obj.pk,))If you have a custom AdminSite(name="custom"), pass current_app="custom" to reverse the URL for that specific admin instance. [2]
Related: the per-app index is admin:app_list with kwargs={"app_label": "auth"}. [3]
Sources: Django admin source/docs for ModelAdmin.get_urls() and reversing admin URLs. [1][2][3]
🏁 Script executed:
# Check all the mentioned line ranges
sed -n '120,130p' openwisp_monitoring/device/tests/test_admin.py | cat -n
echo "---"
sed -n '335,345p' openwisp_monitoring/device/tests/test_admin.py | cat -n
echo "---"
sed -n '865,880p' openwisp_monitoring/device/tests/test_admin.py | cat -n
echo "---"
sed -n '910,920p' openwisp_monitoring/device/tests/test_admin.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 2339
🏁 Script executed:
# Check how check_model_name and metric_model_name are used
rg -n "check_model_name|metric_model_name" openwisp_monitoring/device/tests/test_admin.py | head -30Repository: openwisp/openwisp-monitoring
Length of output: 2836
🏁 Script executed:
# Search for TestWifiSessionAdmin class
sed -n '900,950p' openwisp_monitoring/device/tests/test_admin.py | cat -nRepository: openwisp/openwisp-monitoring
Length of output: 2539
Add dynamic model name for Device admin URLs.
Django admin URLs require both app_label and model_name. While the code makes app_label dynamic with config_app_label = Device._meta.app_label, it hardcodes "device" in all reversals and path expectations: admin:{self.config_app_label}_device_change, device_changelist, and /admin/{self.config_app_label}/device/recover/. If a derived app swaps config.Device with a differently named concrete model, these will break. Add config_model_name = Device._meta.model_name and use it consistently, following the same pattern already demonstrated in TestWifiSessionAdmin for WifiSession._meta.model_name (lines 917, 924-925).
Also applies to: 124-125, 341-341, 869-876, 914-915, 1059-1061, 1086-1088
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/tests/test_admin.py` around lines 46 - 48, Tests
currently hardcode "device" in admin URL names and paths; add a dynamic
config_model_name = Device._meta.model_name and replace all hardcoded "device"
usages (e.g., admin:{self.config_app_label}_device_change, device_changelist,
and /admin/{self.config_app_label}/device/recover/) with the model-name variable
(use admin:{self.config_app_label}_{self.config_model_name}_change,
{self.config_app_label}_{self.config_model_name}_changelist, and
/admin/{self.config_app_label}/{self.config_model_name}/recover/). Update every
occurrence noted in the review (including the blocks around lines 124-125, 341,
869-876, 914-915, 1059-1061, 1086-1088) to use config_model_name so the tests
work when a concrete Device model has a different name, following the pattern
used for WifiSession._meta.model_name.
2 participants
Checklist
Description of Changes
This adds config_app_label attrs in device test_admin tests to make them derivable in other django apps.
I currently also need to set app_label, since the derived TestImportExportMixin requires it... we could probably devise a slightly nicer way of dealing with it, like f.i. renaming the variable config_app_label (in controller) in TestImportExportMixin to make it more explicit which app it is referring to...