Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
the change form.

We utilize a distinct form element (id="act_deact_device_form")
specifically for these actions. The form attribute of the submit buttons (Acivate/Deactivate)
specifically for these actions. The form attribute of the submit buttons (Activate/Deactivate)
within the submit-row div references this form. By doing so, we ensure that
these actions can be submitted independently without causing any
disruption to the device form.
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/config/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -1317,7 +1317,7 @@ def test_variable_usage(self):

def test_preview_device_config_empty_id(self):
path = reverse(f"admin:{self.app_label}_device_preview")
config = json.dumps({"general": {"descripion": "id: {{ id }}"}})
config = json.dumps({"general": {"description": "id: {{ id }}"}})
data = {
"id": "",
"name": "test-empty-id",
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/config/tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def test_backend_openwrt_different_versions(self):
self.assertIsInstance(c.backend_instance, OpenWrt)
self.assertEqual(c.backend_instance.dsa, True)

with self.subTest("DSA disabed OpenWrt Firmware"):
with self.subTest("DSA disabled OpenWrt Firmware"):
c = Config(
backend="netjsonconfig.OpenWrt",
device=Device(name="test", os="OpenWrt 19.02.2 r16495-bf0c965af0"),
Expand Down
15 changes: 12 additions & 3 deletions openwisp_controller/config/tests/test_selenium.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ def test_force_delete_device_with_deactivating_config(self):
By.CSS_SELECTOR, "#deactivating-warning .messagelist .warning p"
)
self.find_element(by=By.CSS_SELECTOR, value="#warning-ack").click()
# After accepting the warning, wee need to wait for the animation
# After accepting the warning, we need to wait for the animation
# to complete before trying to interact with the button,
# otherwise the test may fail due to the button not being fully
# visible or clickable yet.
Expand Down Expand Up @@ -364,7 +364,7 @@ def test_force_delete_multiple_devices_with_deactivating_config(self):
By.CSS_SELECTOR, "#deactivating-warning .messagelist .warning p"
)
self.find_element(by=By.CSS_SELECTOR, value="#warning-ack").click()
# After accepting the warning, wee need to wait for the animation
# After accepting the warning, we need to wait for the animation
# to complete before trying to interact with the button,
# otherwise the test may fail due to the button not being fully
# visible or clickable yet.
Expand Down Expand Up @@ -630,7 +630,15 @@ def get_chrome_webdriver(cls):
return webdriver.Chrome(options=options)

def _is_unsaved_changes_alert_present(self):
for entry in self.get_browser_logs():
alert = self.web_driver.execute_script(
"var alert = sessionStorage.getItem('unsaved_changes_alert');"
"sessionStorage.removeItem('unsaved_changes_alert');"
"return alert;"
)
if alert and "You haven't saved your changes yet!" in alert:
return True

for entry in self.get_browser_logs() or []:
if (
entry["level"] == "WARNING"
and "You haven't saved your changes yet!" in entry["message"]
Expand All @@ -641,6 +649,7 @@ def _is_unsaved_changes_alert_present(self):
def _override_unsaved_changes_alert(self):
self.web_driver.execute_script(
'django.jQuery(window).on("beforeunload", function(e) {'
" sessionStorage.setItem('unsaved_changes_alert', e.returnValue);"
" console.warn(e.returnValue); });"
)

Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/config/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ def test_get_default_values_authorization(self):

def test_get_default_values_same_keys(self):
self._login()
# Atleast 4 templates are required to create enough entropy in database
# At least 4 templates are required to create enough entropy in database
# to make the test fail consistently without patch
template1 = self._create_template(name="VNI 1", default_values={"vn1": "1"})
template2 = self._create_template(
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/config/whois/tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ class TestWHOISSelenium(CreateWHOISMixin, SeleniumTestMixin, StaticLiveServerTes
def test_whois_device_admin(self):
def _assert_no_js_errors():
browser_logs = []
for log in self.get_browser_logs():
for log in self.get_browser_logs() or []:
if self.browser == "chrome" and log["source"] != "console-api":
continue
elif log["message"] in ["wrong event specified: touchleave"]:
Expand Down
4 changes: 2 additions & 2 deletions openwisp_controller/connection/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class AbstractCredentials(ConnectorMixin, ShareableOrgMixinUniqueName, BaseModel
"""

# Controls the number of objects which can be stored in memory
# before commiting them to database during bulk auto add operation.
# before committing them to database during bulk auto add operation.
chunk_size = 1000

connector = models.CharField(
Expand Down Expand Up @@ -564,7 +564,7 @@ def _save_without_resurrecting(self):

def _schedule_command(self):
"""
executes ``launch_command`` celery taks in the background
executes ``launch_command`` celery tasks in the background
once changes are committed to the database
"""
transaction.on_commit(lambda: launch_command.delay(self.pk))
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/connection/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"title": _("Confirm Password"),
},
},
"message": _("Your password must be atleast 6 characters long"),
"message": _("Your password must be at least 6 characters long"),
"additionalProperties": False,
"definitions": {
"password_regex": {
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/connection/connectors/openwrt/ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class OpenWrt(Ssh):
def update_config(self):
try:
output, exit_code = self.exec_command(
# "openwisp_config" for backword compatibility
# "openwisp_config" for backward compatibility
"(openwisp-config --version || openwisp_config --version) 2>/dev/null"
)
except Exception as error:
Expand Down
6 changes: 3 additions & 3 deletions openwisp_controller/connection/connectors/ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def _connect(self, address):
"""
Tries to instantiate the SSH connection,
if the connection fails, it tries again
by disabling the new deafult HostKeyAlgorithms
by disabling the new default HostKeyAlgorithms
used by newer versions of Paramiko
"""
params = self.params
Expand Down Expand Up @@ -187,11 +187,11 @@ def exec_command(
# paramiko expects timeout as a float
timeout = float(timeout)
logger.info("Executing command: {0}".format(command))
# execute commmand
# execute command
try:
start_cmd = time.perf_counter()
stdin, stdout, stderr = self.shell.exec_command(command, timeout=timeout)
# re-raise socket.timeout to avoid being catched
# re-raise socket.timeout to avoid being caught
# by the subsequent `except Exception as e` block
except socket.timeout:
raise socket.timeout()
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/connection/tests/test_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def test_default_notification_type_already_unregistered(self):
# by some other module

# Unregister "config_error" and "device_registered" notification
# types to avoid getting rasing ImproperlyConfigured exceptions
# types to avoid getting raising ImproperlyConfigured exceptions
unregister_notification_type("connection_is_not_working")
unregister_notification_type("connection_is_working")

Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/connection/tests/test_selenium.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def test_command_widget_on_device(self):
self.find_element(by=By.CSS_SELECTOR, value="#ow-command-confirm-yes").click()

self.assertEqual(Command.objects.count(), 1)
# TODO: Selenium tests does not support websocket connections.
# TODO: Selenium tests do not support websocket connections.
# Thus, we need to refresh the page. Remove this when support for
# websockets is added.
self.open(path)
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/connection/tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class SshServer(Server):
def _run(self, *args, **kwargs):
"""
Hides 'Bad file descriptor' system issue which
does not affect the effectivness of the tests
does not affect the effectiveness of the tests
"""
try:
return super()._run(*args, **kwargs)
Expand Down
4 changes: 2 additions & 2 deletions openwisp_controller/geo/tests/pytest.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ async def test_common_location_org_isolation(self):
connected, _ = await admin_communicator.connect()
assert connected

# Updating co-ordinates for org1_location should notify org1_user and admin,
# Updating coordinates for org1_location should notify org1_user and admin,
await self._save_location(str(org1_location.pk))
org1_response = await org1_communicator.receive_json_from(timeout=1)
assert org1_response["id"] == str(org1_location.pk)
Expand All @@ -181,7 +181,7 @@ async def test_common_location_org_isolation(self):
connected, _ = await org2_communicator.connect()
assert connected

# Updating co-ordinates for org2_location should notify org2_user and admin,
# Updating coordinates for org2_location should notify org2_user and admin,
await self._save_location(str(org2_location.pk))
org2_response = await org2_communicator.receive_json_from(timeout=1)
assert org2_response["id"] == str(org2_location.pk)
Expand Down
4 changes: 2 additions & 2 deletions openwisp_controller/geo/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,14 @@ def test_deactivated_device(self):
url = "{0}?key={1}".format(reverse(self.url_name, args=[device.pk]), device.key)
device.deactivate()

with self.subTest("Test retrieving device co-ordinates"):
with self.subTest("Test retrieving device coordinates"):
response = self.client.get(
url,
content_type="application/json",
)
self.assertEqual(response.status_code, 200)

with self.subTest("Test updating device co-ordinates"):
with self.subTest("Test updating device coordinates"):
response = self.client.put(
url,
content_type="application/json",
Expand Down
59 changes: 58 additions & 1 deletion openwisp_controller/geo/tests/test_selenium.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,22 @@
from django.test import tag
from django.urls.base import reverse
from django_loci.tests import TestAdminMixin
from unittest.mock import MagicMock
from django_loci.base import geocoding_views
from django_loci.tests.base.test_selenium import BaseTestDeviceAdminSelenium
from time import sleep
from selenium.webdriver import ActionChains
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait

class MockLocation:
def __init__(self, address="Lazio 00185, ITA", latitude=41.898903, longitude=12.512124):
self.address = address
self.latitude = latitude
self.longitude = longitude

geocoding_views.geocode = MagicMock(return_value=MockLocation())
geocoding_views.reverse_geocode = MagicMock(return_value=MockLocation())
Comment on lines +21 to +22

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid module-level global monkeypatching of geocoding_views in tests.

At Line 21 and Line 22, patching geocoding_views.geocode / reverse_geocode at import time can leak mocked behavior into other tests in the same process and create order-dependent failures. Scope these mocks to the test/class lifecycle and restore originals.

Proposed scoped patching approach
+from unittest.mock import patch
+
 class TestDeviceAdminGeoSelenium(
@@
     inline_field_prefix = "devicelocation"
+
+    `@classmethod`
+    def setUpClass(cls):
+        super().setUpClass()
+        cls._geocode_patcher = patch.object(
+            geocoding_views, "geocode", return_value=MockLocation()
+        )
+        cls._reverse_geocode_patcher = patch.object(
+            geocoding_views, "reverse_geocode", return_value=MockLocation()
+        )
+        cls._geocode_patcher.start()
+        cls._reverse_geocode_patcher.start()
+
+    `@classmethod`
+    def tearDownClass(cls):
+        cls._reverse_geocode_patcher.stop()
+        cls._geocode_patcher.stop()
+        super().tearDownClass()
@@
-geocoding_views.geocode = MagicMock(return_value=MockLocation())
-geocoding_views.reverse_geocode = MagicMock(return_value=MockLocation())
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/geo/tests/test_selenium.py` around lines 21 - 22, The
patches to geocoding_views.geocode and geocoding_views.reverse_geocode are
applied at module-level (import time), causing mocked behavior to leak into
other tests and create order-dependent failures. Move these MagicMock
assignments from the module-level scope into the appropriate test setup method
(such as setUp in a test class) or use a context manager like
unittest.mock.patch to scope the mocks to individual test lifecycles, ensuring
the originals are properly restored after each test completes.

from selenium.webdriver.common.by import By
from swapper import load_model

Expand Down Expand Up @@ -63,6 +78,39 @@ def _fill_device_form(self):
self.find_element(by=By.CLASS_NAME, value="select2-results__option").click()
super()._fill_device_form()

def test_real_time_update_address_field(self):
# Changing the address in tab 1 should update it in tab 0 in real time without a page reload.
# We increase the alert check timeout to 10s to prevent TimeoutException on slow CI runs.
location = self._create_location()
self.login()
url = reverse(f"admin:{self.app_label}_location_change", args=[location.id])
self.open(url)
self.web_driver.switch_to.new_window("tab")
tabs = self.web_driver.window_handles
self.web_driver.switch_to.window(tabs[-1])
self.open(url)
address_input = self.find_element(by=By.ID, value="id_address")
self.assertEqual(address_input.get_attribute("value"), location.address)
self.find_element(
by=By.XPATH, value='//a[@class="leaflet-draw-draw-marker"]'
).click()
elem = self.find_element(by=By.ID, value="id_geometry-map")
ActionChains(self.web_driver).move_to_element(elem).move_by_offset(
30, 15
).click().perform()
Comment on lines +98 to +100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Document the map-click offset rationale or replace magic numbers with named constants.

At Line 98-Line 100, move_by_offset(30, 15) is non-obvious and fragile without context. Add a brief “why” comment and/or constants so future maintainers know why this offset is acceptable.

As per coding guidelines, cryptic code must include a concise explanation of why the complexity is needed and acceptable.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/geo/tests/test_selenium.py` around lines 98 - 100, The
move_by_offset(30, 15) call in the ActionChains method chain uses magic numbers
without explanation, making the code unclear and fragile. Replace the numeric
literals 30 and 15 with named constants (for example, OFFSET_X and OFFSET_Y)
defined at the top of the test class or test method, and add a concise comment
explaining why this specific offset is necessary for the map interaction (such
as ensuring the click lands on a specific map element or avoiding unintended
interactions). This makes the test more maintainable and helps future developers
understand the intent.

Source: Coding guidelines

alert = WebDriverWait(self.web_driver, 10).until(EC.alert_is_present())
alert.accept()
sleep(0.05)
new_address = "Lazio 00185, ITA"
address_input = self.find_element(by=By.ID, value="id_address")
self.assertIn(new_address, address_input.get_attribute("value"))
Comment on lines +103 to +106

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace fixed sleep with an explicit wait on address-field update.

At Line 103, sleep(0.05) is timing-sensitive and can still fail under CI jitter. Wait directly for the address value condition before asserting at Line 106.

Proposed deterministic wait
-        sleep(0.05)
         new_address = "Lazio 00185, ITA"
-        address_input = self.find_element(by=By.ID, value="id_address")
-        self.assertIn(new_address, address_input.get_attribute("value"))
+        WebDriverWait(self.web_driver, 10).until(
+            lambda d: new_address in d.find_element(By.ID, "id_address").get_attribute("value")
+        )
+        address_input = self.find_element(by=By.ID, value="id_address")
+        self.assertIn(new_address, address_input.get_attribute("value"))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/geo/tests/test_selenium.py` around lines 103 - 106,
Replace the fixed sleep(0.05) call with an explicit wait condition that checks
for the address field update before proceeding. Use WebDriverWait to wait until
the address_input element's value attribute contains "Lazio 00185, ITA" (the
new_address value), then remove the sleep statement entirely. This will make the
test deterministic and eliminate the timing-sensitive failure mode in CI
environments. The wait should target the element retrieved by By.ID with value
"id_address" and poll until the condition is satisfied before the assertion at
Line 106.

self.wait_for("element_to_be_clickable", by=By.NAME, value="_continue").click()
self.web_driver.close()
initial_tab = tabs.index(tabs[-1]) - 1
self.web_driver.switch_to.window(tabs[initial_tab])
address_input = self.find_element(by=By.ID, value="id_address")
self.assertIn(new_address, address_input.get_attribute("value"))


@tag("selenium_tests")
class TestDeviceAdminReadonly(
Expand Down Expand Up @@ -108,10 +156,19 @@ def test_unsaved_changes_readonly(self):
# so we just need to ensure it's set as expected
self.web_driver.execute_script(
'django.jQuery(window).on("beforeunload", function(e) {'
" sessionStorage.setItem('unsaved_changes_alert', e.returnValue);"
" console.warn(e.returnValue); });"
)
self.web_driver.refresh()
for entry in self.get_browser_logs():
alert = self.web_driver.execute_script(
"var alert = sessionStorage.getItem('unsaved_changes_alert');"
"sessionStorage.removeItem('unsaved_changes_alert');"
"return alert;"
)
if alert and "You haven't saved your changes yet!" in alert:
self.fail("Unsaved changes alert displayed without any change")

for entry in self.get_browser_logs() or []:
if (
entry["level"] == "WARNING"
and "You haven't saved your changes yet!" in entry["message"]
Expand Down
20 changes: 20 additions & 0 deletions openwisp_controller/pki/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,24 @@
from ..utils import UnqiueCommonNameMixin


# Avoids "DateTimeField received a naive datetime while time zone support is active"
# warning by returning an aware datetime when USE_TZ is True.
def default_validity_start():
from datetime import datetime, timedelta
from django.conf import settings
from django.utils import timezone
start = datetime.now() - timedelta(days=1)
start = start.replace(hour=0, minute=0, second=0, microsecond=0)
if settings.USE_TZ:
return timezone.make_aware(start)
return start
Comment on lines +18 to +22

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Build validity_start from Django timezone, not host-local naive time.

Current logic can shift the date when server timezone and Django configured timezone differ, because datetime.now() and timezone.make_aware() may use different timezone bases. Use timezone.now() as the source and normalize from there.

Proposed fix
 def default_validity_start():
-    from datetime import datetime, timedelta
+    from datetime import timedelta
     from django.conf import settings
     from django.utils import timezone
-    start = datetime.now() - timedelta(days=1)
-    start = start.replace(hour=0, minute=0, second=0, microsecond=0)
+    if settings.USE_TZ:
+        start = timezone.localtime(timezone.now()) - timedelta(days=1)
+    else:
+        start = timezone.now() - timedelta(days=1)
+    start = start.replace(hour=0, minute=0, second=0, microsecond=0)
     if settings.USE_TZ:
-        return timezone.make_aware(start)
+        return timezone.make_aware(start) if timezone.is_naive(start) else start
     return start
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openwisp_controller/pki/base/models.py` around lines 18 - 22, Replace
`datetime.now()` with `timezone.now()` when calculating the `start` variable to
ensure Django's configured timezone is consistently used throughout the
calculation. The `timezone.now()` function automatically respects Django's
`USE_TZ` setting and will return either a timezone-aware or naive datetime as
appropriate, eliminating the mismatch between how the initial datetime is
obtained and how it is later made aware, which prevents date shifting when the
server timezone differs from the Django configured timezone.



class AbstractCa(ShareableOrgMixin, UnqiueCommonNameMixin, BaseCa):
validity_start = models.DateTimeField(
blank=True, null=True, default=default_validity_start
)

class Meta(BaseCa.Meta):
abstract = True
constraints = [
Expand All @@ -21,6 +38,9 @@ class Meta(BaseCa.Meta):


class AbstractCert(ShareableOrgMixin, UnqiueCommonNameMixin, BaseCert):
validity_start = models.DateTimeField(
blank=True, null=True, default=default_validity_start
)
ca = models.ForeignKey(
get_model_name("django_x509", "Ca"),
verbose_name=_("CA"),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import openwisp_controller.pki.base.models
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("pki", "0012_alter_ca_extensions_alter_ca_key_length_and_more"),
]

operations = [
migrations.AlterField(
model_name="ca",
name="validity_start",
field=models.DateTimeField(
blank=True,
default=openwisp_controller.pki.base.models.default_validity_start,
null=True,
),
),
migrations.AlterField(
model_name="cert",
name="validity_start",
field=models.DateTimeField(
blank=True,
default=openwisp_controller.pki.base.models.default_validity_start,
null=True,
),
),
]
2 changes: 1 addition & 1 deletion openwisp_controller/subnet_division/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def _validate_master_subnet_consistency(self):
raise ValidationError(
{
"number_of_subnets": _(
"The master subnet is too small to acommodate the "
"The master subnet is too small to accommodate the "
'requested "number of subnets" plus the reserved '
"subnet, please increase the size of the master "
'subnet or decrease the "size of subnets" field.'
Expand Down
2 changes: 1 addition & 1 deletion openwisp_controller/subnet_division/rule_types/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ def get_config(cls, instance):
# check for real existence in DB to workaround
# this django-import-export bug:
# https://github.com/django-import-export/django-import-export/issues/1078
# TODO: if that issue is ever solved, we can remove the this block below
# TODO: if that issue is ever solved, we can remove this block below
Config = config._meta.model
if not Config.objects.filter(pk=config.pk).exists():
raise ObjectDoesNotExist()
Expand Down
Loading
Loading