Skip to content

[tests] Fixed hardcoded config_app_label and app_label in device test_admin #748

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.

Sign up for GitHub

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

Merged
nemesifier merged 2 commits into from
May 8, 2026
Merged

[tests] Fixed hardcoded config_app_label and app_label in device test_admin #748

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d923e30
[change] Add config_app_label and app_label into device test_admin
Feb 26, 2026
fb6da62
[change] Cleanup redundant app_label and use Device._meta to retrieve it
Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 32 additions & 22 deletions openwisp_monitoring/device/tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ class TestAdmin(
):
"""Test the additions of openwisp-monitoring to DeviceAdmin"""

# Since TestImportExportMixin expects app_label for config app
app_label = Device._meta.app_label
config_app_label = Device._meta.app_label
Comment on lines +46 to +48

@coderabbitai coderabbitai Bot Mar 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -150

Repository: 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 -n

Repository: 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}_changelist
  • admin:{app_label}_{model_name}_add
  • admin:{app_label}_{model_name}_change (expects args=(object_id,))
  • admin:{app_label}_{model_name}_delete (expects args=(object_id,))
  • admin:{app_label}_{model_name}_history (expects args=(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 -n

Repository: 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 -30

Repository: 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 -n

Repository: 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.


resources_fields = TestImportExportMixin.resource_fields
resources_fields.append("monitoring_status")
app_label = "config"
Expand Down Expand Up @@ -118,7 +122,7 @@ def test_device_admin(self):
content_object=dd,
params={},
)
url = reverse("admin:config_device_change", args=[dd.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[dd.pk])
response = self.client.get(url)
self.assertContains(response, "<h2>Status</h2>")
self.assertContains(response, "<h2>Charts</h2>")
Expand Down Expand Up @@ -189,7 +193,7 @@ def test_status_data(self):
)
data["interfaces"][2]["wireless"]["mode"] = "station"
self._post_data(d.id, d.key, data)
url = reverse("admin:config_device_change", args=[d.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[d.pk])
r = self.client.get(url)
with self.subTest("DHCP lease MAC is shown"):
self.assertContains(r, "f2:f1:3e:56:d2:77")
Expand Down Expand Up @@ -229,7 +233,7 @@ def test_status_data(self):
def test_status_data_contains_wifi_version(self):
data = self._data()
d = self._create_device(organization=self._create_org())
url = reverse("admin:config_device_change", args=[d.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[d.pk])
self._post_data(d.id, d.key, data)
response = self.client.get(url)
self.assertContains(
Expand All @@ -254,7 +258,7 @@ def test_status_data_contains_wifi_version(self):
def test_status_data_contains_wifi_client_he_vht_ht_unknown(self):
data = deepcopy(self._data())
d = self._create_device(organization=self._create_org())
url = reverse("admin:config_device_change", args=[d.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[d.pk])
wireless_interface = data["interfaces"][0]["wireless"]
client = data["interfaces"][0]["wireless"]["clients"][0]

Expand Down Expand Up @@ -329,13 +333,13 @@ def test_status_data_contains_wifi_client_he_vht_ht_unknown(self):

def test_no_device_data(self):
d = self._create_device(organization=self._create_org())
url = reverse("admin:config_device_change", args=[d.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[d.pk])
r = self.client.get(url)
self.assertNotContains(r, "<h2>Status</h2>")
self.assertNotContains(r, "AlertSettings")

def test_device_add_view(self):
url = reverse("admin:config_device_add")
url = reverse(f"admin:{self.config_app_label}_device_add")
r = self.client.get(url)
self.assertNotContains(r, "AlertSettings")
self.assertContains(
Expand All @@ -356,7 +360,7 @@ def test_device_disabled_organization_admin(self):
org = device.organization
org.is_active = False
org.save()
url = reverse("admin:config_device_change", args=[device.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[device.pk])
response = self.client.get(url)
self.assertContains(response, "<h2>Status</h2>")
self.assertContains(response, "<h2>Charts</h2>")
Expand All @@ -375,19 +379,19 @@ def test_remove_invalid_interface(self):
d.key,
{"type": "DeviceMonitoring", "interfaces": [{"name": "br-lan"}]},
)
url = reverse("admin:config_device_change", args=[dd.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[dd.pk])
self.client.get(url)

def test_wifi_clients_admin(self):
dd = self.create_test_data(no_resources=True)
url = reverse("admin:config_device_change", args=[dd.id])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[dd.id])
r1 = self.client.get(url, follow=True)
self.assertEqual(r1.status_code, 200)
self.assertContains(r1, "00:ee:ad:34:f5:3b")

def test_interface_properties_admin(self):
dd = self.create_test_data(no_resources=True)
url = reverse("admin:config_device_change", args=[dd.id])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[dd.id])
r1 = self.client.get(url, follow=True)
self.assertEqual(r1.status_code, 200)
self.assertContains(r1, "44:d1:fa:4b:38:44")
Expand Down Expand Up @@ -416,7 +420,7 @@ def test_interface_bridge_admin(self):
],
},
)
url = reverse("admin:config_device_change", args=[dd.id])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[dd.id])
r1 = self.client.get(url, follow=True)
self.assertEqual(r1.status_code, 200)
self.assertContains(r1, "Bridge Members")
Expand Down Expand Up @@ -484,7 +488,7 @@ def test_interface_mobile_admin(self):
],
},
)
url = reverse("admin:config_device_change", args=[d.id])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[d.id])
r1 = self.client.get(url, follow=True)
self.assertEqual(r1.status_code, 200)
self.assertContains(r1, "Signal Strength (LTE)")
Expand All @@ -495,7 +499,7 @@ def test_interface_mobile_admin(self):
def test_uuid_bug(self):
dd = self.create_test_data(no_resources=True)
uuid = str(dd.pk).replace("-", "")
url = reverse("admin:config_device_change", args=[uuid])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[uuid])
r = self.client.get(url)
self.assertContains(r, "<h2>Status</h2>")

Expand Down Expand Up @@ -531,7 +535,7 @@ def test_check_inline_formset(self):

def test_health_checks_list(self):
dd = self.create_test_data()
url = reverse("admin:config_device_change", args=[dd.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[dd.pk])
r = self.client.get(url)
self.assertNotContains(r, "<label>Health checks:</label>")
m = Metric.objects.filter(configuration="disk").first()
Expand Down Expand Up @@ -560,7 +564,7 @@ def test_health_checks_list(self):

def test_wifisession_inline(self):
device = self._create_device()
path = reverse("admin:config_device_change", args=[device.id])
path = reverse(f"admin:{self.config_app_label}_device_change", args=[device.id])

with self.subTest("Test inline absent when no WiFiSession is present"):
response = self.client.get(path)
Expand Down Expand Up @@ -605,7 +609,7 @@ def test_check_alertsetting_inline(self):
)
ping_check.full_clean()
ping_check.save()
url = reverse("admin:config_device_change", args=[device.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[device.pk])
metric = self._create_general_metric(
name="", content_object=device, configuration="ping"
)
Expand Down Expand Up @@ -748,7 +752,7 @@ def test_alert_settings_inline_post(self):
metric = self._create_general_metric(
name="", content_object=device, configuration="iperf3"
)
url = reverse("admin:config_device_change", args=[device.pk])
url = reverse(f"admin:{self.config_app_label}_device_change", args=[device.pk])
alertsettings = self._create_alert_settings(metric=metric)
test_inline_params = {
"name": device.name,
Expand Down Expand Up @@ -863,13 +867,14 @@ def _assert_alertsettings_inline(response, operator, threshold, tolerance):

def test_device_admin_recover_button_visibility(self):
self._create_device(organization=self._create_org())
url = reverse("admin:config_device_changelist")
url = reverse(f"admin:{self.config_app_label}_device_changelist")
response = self.client.get(url)
self.assertContains(
response,
"""
f"""
<li>
<a href="/admin/config/device/recover/" class="recoverlink">Recover deleted Devices</a>
<a href="/admin/{self.config_app_label}/device/recover/" """
"""class="recoverlink">Recover deleted Devices</a>
</li>
""",
html=True,
Expand Down Expand Up @@ -907,6 +912,7 @@ class TestWifiSessionAdmin(
TestWifiClientSessionMixin,
TestCase,
):
config_app_label = "config"

@nemesifier nemesifier Mar 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is missing right?

Suggested change
config_app_label = "config"
config_app_label = Device._meta.app_label

@asmodehn asmodehn Mar 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. I ll fix it asap.

wifi_session_app_label = WifiSession._meta.app_label
wifi_session_model_name = WifiSession._meta.model_name

Expand Down Expand Up @@ -1051,7 +1057,9 @@ def test_deleting_device_with_wifisessions(self):
device_data = self._save_device_data()
device = Device.objects.first()
device.deactivate()
path = reverse("admin:config_device_delete", args=[device_data.pk])
path = reverse(
f"admin:{self.config_app_label}_device_delete", args=[device_data.pk]
)
response = self.client.post(path, {"post": "yes"}, follow=True)
self.assertEqual(response.status_code, 200)
self.assertEqual(Device.objects.count(), 0)
Expand All @@ -1076,7 +1084,9 @@ def test_wifi_client_he_vht_ht_unknown(self):
device = Device.objects.all().first()

with self.subTest("Test device wifi session inline"):
url = reverse("admin:config_device_change", args=[device.id])
url = reverse(
f"admin:{self.config_app_label}_device_change", args=[device.id]
)
response = self.client.get(url)
self.assertContains(
response,
Expand Down
Loading