[feature] GSoC25: Improve UX and Flexibility of the Firmware Upgrader Module#377
Conversation
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds real-time firmware upgrade progress and cancellation features, batch filtering by DeviceGroup and Location, model fields for progress and cancellation, migrations, WebSocket consumers/publishers and routing, a new signal, API cancel endpoint and serializers, admin forms/widgets/templates and Select2 integration, frontend JS/CSS for live updates, upgrade cancellation logic in openwrt upgrader, test runner/script changes, and extensive unit/async/Selenium tests covering websockets and cancellation flows. Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant WS as WebSocket Server
participant Signal as Signal/Publisher
participant Model as UpgradeOperation Model
participant Device as Device Upgrader
UI->>WS: Connect to ws/firmware-upgrader/upgrade-operation/{id}/
WS->>WS: Authenticate & authorize user
UI->>WS: {"type":"request_current_state","operation_id":id}
WS->>Model: query current state
Model->>WS: return state
WS->>UI: operation_progress / operation_update messages
Device->>Model: update_progress / log_line / status change
Model->>Signal: emit firmware_upgrader_log_updated / post_save
Signal->>WS: publish_progress / publish_operation_update
WS->>UI: push websocket messages
UI->>UI: render progress/log/status updates
sequenceDiagram
participant Admin as Admin UI
participant API as BuildBatchUpgradeView (REST)
participant Model as Build / BatchUpgradeOperation
participant DB as Devices DB
Admin->>API: POST /build/{id}/batch-upgrade/ (group=g, location=loc)
API->>API: validate via BatchUpgradeSerializer
API->>Model: batch_upgrade(build, group=g, location=loc)
Model->>DB: dry_run -> query devices filtered by group+location
DB-->>Model: matching devices
alt devices found
Model->>Model: create BatchUpgradeOperation + UpgradeOperation per device
API->>Admin: 201 Created (batch_id)
else no devices
Model->>API: raise ValidationError
API->>Admin: 400 Bad Request
end
sequenceDiagram
participant UI as Browser/Admin UI
participant API as UpgradeOperationCancelView
participant Model as UpgradeOperation
participant Upgrader as OpenWrt Upgrader
UI->>API: POST /upgrade-operation/{id}/cancel/
API->>Model: get operation & check permissions
alt cancellable (in-progress & below threshold)
API->>Model: call cancel()
Model->>Upgrader: _check_cancellation -> request stop
Upgrader->>Model: raise UpgradeCancelled / set status cancelled
Model->>API: return 200 OK
API->>UI: show cancelled status
else not cancellable
API->>UI: 409 Conflict (cannot cancel)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/ci.yml (1)
100-100: Invalid matrix reference inflag-name.The
flag-namereferencesmatrix.env.envbut the matrix definition only containspython-versionanddjango-version. This will result in an empty or incorrect flag name for coverage uploads.Proposed fix
- flag-name: python-${{ matrix.env.env }} + flag-name: python-${{ matrix.python-version }}-${{ matrix.django-version }}openwisp_firmware_upgrader/migrations/0004_batch_upgrade_operation_idle_status.py (1)
5-21: Avoid importing app constants in migrations.Using
AbstractBatchUpgradeOperation.STATUS_CHOICESties this migration to runtime code and can break old migrations if choices change later. Freeze the choices in the migration.🔧 Proposed fix
-from ..base.models import AbstractBatchUpgradeOperation +STATUS_CHOICES = ( + ("idle", "idle"), + ("in-progress", "in progress"), + ("cancelled", "completed with some cancellations"), + ("success", "completed successfully"), + ("failed", "completed with some failures"), +) ... field=models.CharField( - choices=AbstractBatchUpgradeOperation.STATUS_CHOICES, + choices=STATUS_CHOICES, default="idle", max_length=12, ),openwisp_firmware_upgrader/base/models.py (1)
590-599: Wire the new status calculator into batch updates.
update()/save()still use the legacy success/failed logic, so cancelled/aborted operations may not propagate into batch status. Usecalculate_and_update_status()to keep batch state consistent with new statuses.🔧 Suggested integration
def update(self): - operations = self.upgradeoperation_set - if operations.filter(status="in-progress").exists(): - return - # if there's any failed operation, mark as failure - if operations.filter(status="failed").exists(): - self.status = "failed" - else: - self.status = "success" - self.save() + self.calculate_and_update_status()def save(self, *args, **kwargs): super().save(*args, **kwargs) # when an operation is completed # trigger an update on the batch operation if self.batch and self.status != "in-progress": - self.batch.update() + self.batch.calculate_and_update_status() return selfAlso applies to: 730-802, 988-994
openwisp_firmware_upgrader/api/views.py (1)
94-155: Return 400 for invalid group/location IDs instead of silently ignoring them.Invalid filter IDs currently fall back to
None, which triggers an unfiltered mass upgrade. This behavior is intentionally designed (validated by tests), but it's a security risk—returning 400 when users supply bad filters prevents accidental scope widening and clearly signals filter errors.Note: Tests currently expect the silent fallback behavior (returning 201 with
batch.group=Noneandbatch.location=None), so updating them will be necessary alongside this change.
🤖 Fix all issues with AI agents
In `@docs/developer/installation.rst`:
- Around line 123-130: The docs still show the old sample-app command using
"SAMPLE_APP=1 ./runtests.py --keepdb --failfast"; update that example to use the
new entrypoint by replacing "./runtests.py" with "./runtests" (i.e.,
"SAMPLE_APP=1 ./runtests --keepdb --failfast") so it matches the rest of the
document and the new runtests entrypoint.
In `@docs/user/upgrade-status.rst`:
- Around line 121-150: Update the "**What happens when the upgrade operation
cancels:**" header to use correct passive grammar (e.g., "What happens when the
upgrade operation is canceled:"), and reformat the "**Typical problematic
upgrade:**" bullet list so the steps are numbered sequentially (1 → 4) and
clearly ordered: 1. in-progress, 2. aborted (pre-condition failure), 3. failed
(unexpected error), 4. cancelled (user stopped); ensure the explanatory phrases
currently attached to out-of-order numbers are moved to the matching items so
the list reads logically under the "Typical problematic upgrade" heading.
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 161-182: The code in batch_upgrade is computing total_devices by
calling len() on QuerySets returned in dry_run_result, which forces full
evaluation; change the logic to use QuerySet.exists() checks instead: call
dry_run_result = load_model("BatchUpgradeOperation").dry_run(...), then check if
not dry_run_result["device_firmwares"].exists() and not
dry_run_result["devices"].exists() and raise the ValidationError in that case.
Update references in batch_upgrade to avoid len() on those QuerySets and use
.exists() to determine emptiness efficiently.
In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js`:
- Around line 380-388: Remove the dead function calculateProgressFromLogLength
from upgrade-progress.js: delete its entire definition (the function name
calculateProgressFromLogLength and its body) and ensure there are no remaining
references or exports to it elsewhere in the file; if any tests or other modules
import or reference calculateProgressFromLogLength, remove those references as
well to avoid unused symbol warnings.
- Around line 24-26: The code assumes the global owFirmwareUpgraderApiHost
exists and directly reads owFirmwareUpgraderApiHost.host to build wsUrl; add a
defensive existence check before using it (e.g., confirm typeof
owFirmwareUpgraderApiHost !== "undefined" and that
owFirmwareUpgraderApiHost.host is defined), and if missing handle gracefully by
logging an error or using a sensible fallback (e.g., compute wsHost from
location or abort building wsUrl), updating the section that defines
wsHost/wsUrl (references: owFirmwareUpgraderApiHost, wsHost, wsUrl,
getWebSocketUrl, pageType, pageId) so the script won’t throw when the global is
absent.
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 100-106: Fix the typo in the submit input's title attribute for
the button with name="upgrade_related": change "firwmare" to "firmware" so the
title reads "Devices which have no related firmware image information will be
skipped."; update only the title string inside the <input ...
name="upgrade_related" ...> element in the template.
- Around line 109-111: The anchor element with class "button cancel-link" in the
template upgrade_selected_confirmation.html contains an incomplete inline style
attribute style="margin-left"; remove the attribute or replace it with a valid
CSS declaration (e.g., style="margin-left: 1rem;" or an appropriate value) so
the HTML is valid and the element styling is explicit; update the <a
class="button cancel-link"> element accordingly and prefer moving styling to a
stylesheet if possible.
In `@openwisp_firmware_upgrader/websockets.py`:
- Around line 82-86: When _is_user_authenticated() returns False you must close
the websocket before returning; update the connect methods to call and await
self.close() instead of returning immediately. Specifically, in the connect
method where auth_result = self._is_user_authenticated() and the early return
happens, replace the bare return with await self.close() then return; apply the
same change to BatchUpgradeProgressConsumer.connect and
DeviceUpgradeProgressConsumer.connect so all three connect implementations
properly close the connection when authentication fails.
- Around line 411-416: The created background task from
asyncio.create_task(_send_messages()) is not stored and may be garbage
collected; create a module-level set (e.g., _background_tasks) to hold task
references, change the asyncio.create_task(...) call to assign the Task to a
variable, add that Task to _background_tasks, and attach a done callback that
removes the Task from _background_tasks when finished; keep the
async_to_sync(_send_messages)() branch unchanged but ensure the create_task path
references the Task so it isn't collected.
In `@runtests`:
- Around line 4-11: The coverage runs currently overwrite .coverage so coverage
combine fails; update each `coverage run` invocation in the runtests script (the
lines invoking `coverage run runtests.py`, `coverage run runtests.py
--parallel`, `SAMPLE_APP=1 coverage run ./runtests.py`) to pass parallel-mode
(e.g., add `--parallel-mode` or the short `-p`) so each run writes a unique
.coverage.* file and `coverage combine` will work correctly.
In `@tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py`:
- Around line 20-21: The migration imports the BatchUpgradeOperation model and
pulls STATUS_CHOICES from the model, which breaks reproducibility; update the
migration to stop importing models and instead embed the STATUS_CHOICES as a
hardcoded tuple/list literal used in the CreateModel/field definition inside the
migration (replace references to BatchUpgradeOperation.STATUS_CHOICES with the
literal choices), and remove the "from ..models import BatchUpgradeOperation"
import; also apply the same change for the other import occurrence that
references STATUS_CHOICES so the migration is self-contained.
🧹 Nitpick comments (19)
openwisp_firmware_upgrader/static/firmware-upgrader/js/mass-upgrade-select2.js (1)
29-32: Minor: Redundant width styling.Setting both
widthandmin-widthto the same value is redundant. Also, applying styles via JavaScript after Select2 initialization may cause a brief layout shift.Suggested simplification
$(".select2-input").next(".select2").find(".select2-selection").css({ - width: "222px", - "min-width": "222px", + width: "222px" });Consider moving this fixed width to CSS instead of applying it via JavaScript to avoid potential layout flash.
openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.css (1)
24-118: Considerprefers-reduced-motionfor animations.Nice UI, but adding a reduced-motion override improves accessibility for motion-sensitive users.
🧩 Suggested addition
+@media (prefers-reduced-motion: reduce) { + .upgrade-progress-fill { + transition: none; + } + .ow-cancel-confirmation-dialog { + animation: none; + } +}openwisp_firmware_upgrader/tests/test_selenium.py (4)
61-70: Consider extracting shared helper to avoid code duplication.The
_assert_no_js_errorsmethod is duplicated fromtest_websocket_selenium.py(lines 114-123). Consider extracting this to a shared base class or mixin to maintain DRY principles.
347-347: Prefix unused unpacked variables with underscore.Multiple unpacked variables are never used. Prefix them with
_to indicate they are intentionally unused.♻️ Suggested fix
- org, category, build1, build2, image1, image2, device = self._set_up_env() + _, _, _, _, _, image2, device = self._set_up_env()
393-396: Remove redundant modal variable re-assignment.The
modalvariable is assigned on line 393, then reassigned on line 396 without the first assignment being used.♻️ Suggested fix
# Verify modal is visible and not hidden modal = self.find_element(By.ID, "ow-cancel-confirmation-modal") self.assertTrue(modal.is_displayed()) - modal = self.find_element(By.ID, "ow-cancel-confirmation-modal") - title_element = WebDriverWait(self.web_driver, 2).until(
344-446: Consider verifying the cancellation result.The test validates the modal UI workflow thoroughly but doesn't verify that clicking "Yes" actually cancels the upgrade operation. Consider adding an assertion to confirm the operation status changed to "cancelled" after the modal confirmation.
♻️ Suggested enhancement
# Modal should close after confirming WebDriverWait(self.web_driver, 10).until( EC.invisibility_of_element_located((By.ID, "ow-cancel-confirmation-modal")) ) + + # Verify the upgrade operation was actually cancelled + upgrade_op = UpgradeOperation.objects.first() + upgrade_op.refresh_from_db() + self.assertEqual(upgrade_op.status, "cancelled")openwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.html (1)
12-17: Consider usingjson_scriptfilter for safer JavaScript variable injection.While
request.get_hosttypically returns a safe hostname, directly embedding it in a JavaScript context without proper escaping could be a security concern if the host contains characters that have special meaning in JavaScript strings (e.g., quotes, backslashes).♻️ Suggested improvement using Django's json_script filter
-<script> - // Configuration for WebSocket connection - var owFirmwareUpgraderApiHost = { - host: '{{ request.get_host }}' - }; -</script> +{{ request.get_host|json_script:"ow-firmware-upgrader-host" }} +<script> + // Configuration for WebSocket connection + var owFirmwareUpgraderApiHost = { + host: JSON.parse(document.getElementById('ow-firmware-upgrader-host').textContent) + }; +</script>Alternatively, if you prefer to keep the current approach, using the
escapejsfilter would also work:host: '{{ request.get_host|escapejs }}'openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (1)
66-83: Consider extracting service list to avoid duplication.The service list here duplicates
_non_critical_servicesfromopenwisp_firmware_upgrader/upgraders/openwrt.py(lines 117-124). While test isolation is valuable, this creates a maintenance burden if the service list changes.You could import and reuse the list from the module under test:
from ..upgraders.openwrt import OpenWrt # Then use OpenWrt._non_critical_servicesHowever, if you prefer test isolation to avoid coupling, this is acceptable as-is.
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css (1)
213-215: Inconsistent color for partial-success status.The
partial-successclass uses different colors in different contexts:
- Line 214:
#fc9240- Line 348:
#ff9800Consider using a consistent color for the partial-success state.
♻️ Suggested fix
.batch-main-progress .upgrade-progress-fill.partial-success { - background-color: `#ff9800`; + background-color: `#fc9240`; }Or use CSS custom properties for consistent color management across the file.
Also applies to: 347-349
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.html (2)
150-162: Pagination links may lose some filter parameters.The pagination links only preserve the
q(search) parameter. Other filter parameters fromrequest.GET(like status filters from the filter specs) might be lost when navigating pages.Consider including all relevant filter parameters in the pagination links, similar to how the search form preserves them (lines 90-94).
♻️ Suggested fix to preserve all filter params in pagination
{% if page_obj.has_previous %} - <a href="?{% if request.GET.q %}q={{ request.GET.q }}&{% endif %}page={{ page_obj.previous_page_number }}"> + <a href="?{% for param, value in request.GET.items %}{% if param != 'page' %}{{ param }}={{ value }}&{% endif %}{% endfor %}page={{ page_obj.previous_page_number }}"> {% trans 'Previous' %} </a> {% endif %} <span class="current-page"> {% trans 'Page' %} {{ page_obj.number }} {% trans 'of' %} {{ page_obj.paginator.num_pages }} </span> {% if page_obj.has_next %} - <a href="?{% if request.GET.q %}q={{ request.GET.q }}&{% endif %}page={{ page_obj.next_page_number }}"> + <a href="?{% for param, value in request.GET.items %}{% if param != 'page' %}{{ param }}={{ value }}&{% endif %}{% endfor %}page={{ page_obj.next_page_number }}"> {% trans 'Next' %} </a> {% endif %}
173-178: WebSocket host configuration may need adjustment for reverse proxy setups.Using
{{ request.get_host }}works for direct connections but may return incorrect values behind load balancers or reverse proxies that don't forward theHostheader correctly. Consider making this configurable or documenting the expected proxy configuration.openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (1)
98-116: EnsurestatusClassis sanitized to prevent XSS.The
renderProgressBarHtmlfunction concatenatesstatusClassdirectly into the HTML string. While in this codebase the status classes come from the defined constants, if this function were ever called with user-controlled input, it could lead to XSS.Consider using DOM methods or ensuring callers only pass predefined CSS classes.
openwisp_firmware_upgrader/api/views.py (1)
438-463: Preferlogger.exceptionfor unexpected failures.This preserves stack traces and avoids silent loss of diagnostic details.
🔧 Suggested logging tweak
- except Exception as e: - logger.error(f"Failed to cancel upgrade operation {pk}: {str(e)}") + except Exception: + logger.exception("Failed to cancel upgrade operation %s", pk) return self._error_response( "Failed to cancel upgrade operation", status.HTTP_500_INTERNAL_SERVER_ERROR, )openwisp_firmware_upgrader/websockets.py (2)
489-501: Inconsistent async context handling between publishers.
BatchUpgradeProgressPublisher.publish_progressalways usesasync_to_sync, whileUpgradeProgressPublisher.publish_progresschecks for a running event loop first. This inconsistency could cause issues ifBatchUpgradeProgressPublisheris called from an async context.♻️ Proposed fix to align behavior
def publish_progress(self, data): data = _convert_lazy_translations(data) async def _send_message(): await self.channel_layer.group_send( self.group_name, { "type": "batch_upgrade_progress", "data": {**data, "timestamp": timezone.now().isoformat()}, }, ) - async_to_sync(_send_message)() + try: + asyncio.get_running_loop() + task = asyncio.create_task(_send_message()) + _background_tasks.add(task) + task.add_done_callback(_background_tasks.discard) + except RuntimeError: + async_to_sync(_send_message)()
100-114: Consider usinglogging.exceptionin exception handlers for automatic traceback.Within exception handlers,
logging.exceptionautomatically includes the traceback, providing better debugging context thanlogging.errorwith formatted exception messages.♻️ Example fix for one location
except (AssertionError, KeyError) as e: - logger.error(f"Error in operation websocket connect: {e}") + logger.exception("Error in operation websocket connect") await self.close()Apply similar changes to other
logger.errorcalls inside exception handlers (lines 107, 111-113, 122-124, 161-163, 190, 196, 200-202, 262-264, 266, 293, 299, 303-305, 365-367, 369).openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (2)
320-327: Extract magic number 60 to a named constant.The cancel threshold of 60% is used in two places. Extracting it to a constant improves maintainability and makes the business rule explicit.
♻️ Proposed fix
Add near the top of the file (after line 45):
// Progress threshold beyond which cancellation is not allowed (firmware flashing starts) const CANCEL_PROGRESS_THRESHOLD = 60;Then update both usages:
- const canCancel = progressPercentage < 60; + const canCancel = progressPercentage < CANCEL_PROGRESS_THRESHOLD;Also applies to: 520-527
749-750: Hardcoded API URL path.The API endpoint
/api/v1/firmware-upgrader/upgrade-operation/${operationId}/cancel/is hardcoded. If the API prefix changes, this will break. Consider using a URL from the server or a configurable constant.This could be addressed by having the server provide the API base URL alongside
owFirmwareUpgraderApiHost, or by using Django's URL reversing mechanism in the template.openwisp_firmware_upgrader/admin.py (2)
450-555: Consider extracting filter classes outside ofchange_view.The
StatusFilterandOrganizationFilterclasses are defined inside the method on each request. While functional, this is unusual and could be refactored for clarity.These filter classes could be defined at module level or as nested classes within
BatchUpgradeOperationAdminfor better organization and potential reuse.
510-527: Organization filter should display only relevant organizations from the batch operations.While
Organization.objects.all()displays all organizations in the system, consider restricting the filter to organizations that actually have upgrade operations in this batch. This improves UX and avoids confusion. Extract organization IDs from the operations queryset instead:🔧 Suggested improvement
# Organization filter org_choices = [ {"display": _("All"), "selected": not current_org, "query_string": "?"} ] - for org in Organization.objects.all().order_by("name"): + org_ids = upgrades_qs.values_list('device__organization_id', flat=True).distinct() + for org in Organization.objects.filter(id__in=org_ids).order_by("name"): org_choices.append(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
.github/workflows/ci.ymldocker-compose.ymldocs/developer/index.rstdocs/developer/installation.rstdocs/developer/utils.rstdocs/index.rstdocs/user/intro.rstdocs/user/quickstart.rstdocs/user/rest-api.rstdocs/user/upgrade-status.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/api/urls.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/exceptions.pyopenwisp_firmware_upgrader/filters.pyopenwisp_firmware_upgrader/migrations/0004_batch_upgrade_operation_idle_status.pyopenwisp_firmware_upgrader/migrations/0012_upgradeoperation_progress.pyopenwisp_firmware_upgrader/migrations/0013_alter_upgradeoperation_status.pyopenwisp_firmware_upgrader/migrations/0014_add_group_to_batchupgradeoperation.pyopenwisp_firmware_upgrader/migrations/0015_batchupgradeoperation_location.pyopenwisp_firmware_upgrader/routing.pyopenwisp_firmware_upgrader/signals.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/mass-upgrade-select2.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.jsopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.htmlopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.htmlopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_openwrt_upgrader.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_websocket_selenium.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/upgraders/openwrt.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/widgets.pyrequirements-test.txtrunteststests/openwisp2/postgresql_settings.pytests/openwisp2/routing.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_upgradeoperation_progress.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_upgradeoperation_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.pytests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (1)
- docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (29)
openwisp_firmware_upgrader/migrations/0012_upgradeoperation_progress.py (1)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_upgradeoperation_progress.py (1)
Migration(7-25)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (2)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (10)
status(309-309)progressPercentage(146-147)progressPercentage(310-310)progressPercentage(362-362)statusClass(149-149)statusClass(314-314)statusClass(363-363)showPercentageText(148-148)showPercentageText(315-315)showPercentageText(364-364)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (5)
status(295-295)status(500-500)progressPercentage(297-297)progressPercentage(501-501)protocol(645-645)
openwisp_firmware_upgrader/routing.py (1)
openwisp_firmware_upgrader/websockets.py (3)
BatchUpgradeProgressConsumer(169-269)DeviceUpgradeProgressConsumer(272-375)UpgradeProgressConsumer(77-166)
openwisp_firmware_upgrader/tests/base.py (1)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)
tests/openwisp2/routing.py (1)
openwisp_firmware_upgrader/routing.py (1)
get_routes(25-26)
openwisp_firmware_upgrader/apps.py (3)
openwisp_firmware_upgrader/websockets.py (7)
BatchUpgradeProgressPublisher(479-564)UpgradeProgressPublisher(378-476)connect(82-116)connect(174-206)connect(277-308)handle_upgrade_operation_post_save(432-476)handle_batch_upgrade_operation_saved(545-564)openwisp_firmware_upgrader/models.py (2)
UpgradeOperation(43-46)BatchUpgradeOperation(37-40)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (5)
ALL_VALID_FW_STATUSES(35-38)FW_UPGRADE_CSS_CLASSES(22-30)FW_UPGRADE_STATUS(4-11)FW_STATUS_GROUPS(41-68)FW_UPGRADE_DISPLAY_STATUS(14-19)
openwisp_firmware_upgrader/upgraders/openwrt.py (3)
openwisp_firmware_upgrader/exceptions.py (2)
UpgradeCancelled(21-24)RecoverableFailure(33-36)openwisp_firmware_upgrader/base/models.py (6)
log_line(843-853)save(421-430)save(988-994)update_progress(855-858)upgrade(601-606)upgrade(885-986)openwisp_firmware_upgrader/admin.py (1)
save(623-631)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (2)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/upgraders/openwrt.py (4)
connect(134-138)_check_cancellation(211-221)upgrade(197-209)disconnect(140-144)
openwisp_firmware_upgrader/api/serializers.py (1)
openwisp_firmware_upgrader/base/models.py (1)
cancelled_rate(691-695)
openwisp_firmware_upgrader/tests/test_api.py (4)
openwisp_firmware_upgrader/models.py (3)
UpgradeOperation(43-46)BatchUpgradeOperation(37-40)DeviceFirmware(31-34)tests/openwisp2/sample_firmware_upgrader/models.py (3)
UpgradeOperation(45-47)BatchUpgradeOperation(40-42)DeviceFirmware(35-37)openwisp_firmware_upgrader/base/models.py (4)
save(421-430)save(988-994)upgrade(601-606)upgrade(885-986)openwisp_firmware_upgrader/api/views.py (3)
post(94-127)post(438-463)get(129-159)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_upgradeoperation_progress.py (1)
openwisp_firmware_upgrader/migrations/0012_upgradeoperation_progress.py (1)
Migration(7-25)
openwisp_firmware_upgrader/migrations/0004_batch_upgrade_operation_idle_status.py (1)
openwisp_firmware_upgrader/base/models.py (1)
AbstractBatchUpgradeOperation(526-802)
tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py (2)
openwisp_firmware_upgrader/models.py (1)
BatchUpgradeOperation(37-40)tests/openwisp2/sample_firmware_upgrader/models.py (1)
BatchUpgradeOperation(40-42)
tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py (3)
openwisp_firmware_upgrader/migrations/0014_add_group_to_batchupgradeoperation.py (1)
Migration(8-27)openwisp_firmware_upgrader/migrations/0015_batchupgradeoperation_location.py (1)
Migration(8-27)tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py (1)
Migration(8-27)
openwisp_firmware_upgrader/migrations/0014_add_group_to_batchupgradeoperation.py (3)
openwisp_firmware_upgrader/migrations/0015_batchupgradeoperation_location.py (1)
Migration(8-27)tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py (1)
Migration(8-27)tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py (1)
Migration(8-27)
openwisp_firmware_upgrader/tests/test_models.py (3)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/base/models.py (1)
batch_upgrade(161-191)openwisp_firmware_upgrader/tests/base.py (4)
_create_category(54-62)_create_build(64-75)_create_firmware_image(77-92)_create_device_group(206-215)
openwisp_firmware_upgrader/migrations/0013_alter_upgradeoperation_status.py (1)
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_upgradeoperation_status.py (1)
Migration(6-28)
tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py (3)
openwisp_firmware_upgrader/migrations/0014_add_group_to_batchupgradeoperation.py (1)
Migration(8-27)openwisp_firmware_upgrader/migrations/0015_batchupgradeoperation_location.py (1)
Migration(8-27)tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py (1)
Migration(8-27)
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_upgradeoperation_status.py (1)
openwisp_firmware_upgrader/migrations/0013_alter_upgradeoperation_status.py (1)
Migration(6-28)
openwisp_firmware_upgrader/migrations/0015_batchupgradeoperation_location.py (3)
openwisp_firmware_upgrader/migrations/0014_add_group_to_batchupgradeoperation.py (1)
Migration(8-27)tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py (1)
Migration(8-27)tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py (1)
Migration(8-27)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (2)
openwisp_firmware_upgrader/websockets.py (6)
BatchUpgradeProgressPublisher(479-564)UpgradeProgressPublisher(378-476)publish_operation_update(418-420)publish_log(422-423)publish_batch_status(524-532)publish_operation_progress(503-522)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)
openwisp_firmware_upgrader/tests/test_selenium.py (3)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (1)
_assert_no_js_errors(115-124)openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
status(309-309)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (2)
status(295-295)status(500-500)
openwisp_firmware_upgrader/tests/test_admin.py (2)
openwisp_firmware_upgrader/admin.py (4)
BatchUpgradeConfirmationForm(103-161)save(623-631)organization(179-180)organization(561-562)openwisp_firmware_upgrader/base/models.py (3)
batch_upgrade(161-191)save(421-430)save(988-994)
openwisp_firmware_upgrader/tests/test_websockets.py (2)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/websockets.py (16)
BatchUpgradeProgressConsumer(169-269)BatchUpgradeProgressPublisher(479-564)DeviceUpgradeProgressConsumer(272-375)UpgradeProgressConsumer(77-166)UpgradeProgressPublisher(378-476)connect(82-116)connect(174-206)connect(277-308)disconnect(118-125)disconnect(208-212)disconnect(310-314)publish_progress(392-416)publish_progress(489-501)publish_operation_update(418-420)publish_operation_progress(503-522)publish_batch_status(524-532)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (2)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (5)
FW_STATUS_HELPERS(70-77)ALL_VALID_FW_STATUSES(35-38)FW_STATUS_GROUPS(41-68)FW_UPGRADE_STATUS(4-11)protocol(119-119)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.js (5)
FW_STATUS_HELPERS(70-77)ALL_VALID_FW_STATUSES(35-38)FW_STATUS_GROUPS(41-68)FW_UPGRADE_STATUS(4-11)protocol(119-119)
openwisp_firmware_upgrader/websockets.py (5)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/upgraders/openwrt.py (2)
connect(134-138)disconnect(140-144)openwisp_firmware_upgrader/api/serializers.py (2)
UpgradeOperationSerializer(84-96)DeviceUpgradeOperationSerializer(99-102)openwisp_firmware_upgrader/base/models.py (4)
upgrade_operations(657-658)total_operations(661-662)update(590-599)calculate_and_update_status(730-802)openwisp_firmware_upgrader/admin.py (1)
completed(566-567)
openwisp_firmware_upgrader/base/models.py (2)
openwisp_firmware_upgrader/exceptions.py (3)
UpgradeCancelled(21-24)UpgradeNotNeeded(9-12)UpgradeAborted(15-18)openwisp_firmware_upgrader/swapper.py (2)
load_model(7-8)get_model_name(11-12)
openwisp_firmware_upgrader/admin.py (4)
openwisp_firmware_upgrader/filters.py (3)
BuildFilter(32-35)GroupFilter(44-47)LocationFilter(38-41)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/widgets.py (3)
MassUpgradeSelect2Widget(31-61)media(25-28)media(52-61)openwisp_firmware_upgrader/models.py (2)
BatchUpgradeOperation(37-40)UpgradeOperation(43-46)
🪛 GitHub Check: CodeQL
openwisp_firmware_upgrader/api/views.py
[warning] 467-467: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.14)
openwisp_firmware_upgrader/migrations/0012_upgradeoperation_progress.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/upgraders/openwrt.py
221-221: Avoid specifying long messages outside the exception class
(TRY003)
226-226: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py
47-47: Unused function argument: exit_codes
(ARG001)
47-47: Unused function argument: timeout
(ARG001)
47-47: Unused function argument: raise_unexpected_exit
(ARG001)
875-875: Unused method argument: mock_putfo
(ARG002)
877-877: Unpacked variable device_fw is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
877-877: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
909-909: Unpacked variable device_fw is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
909-909: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
917-917: Unused function argument: kwargs
(ARG001)
943-943: Unpacked variable device_fw is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
943-943: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_upgradeoperation_progress.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/migrations/0014_add_group_to_batchupgradeoperation.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/tests/test_models.py
708-708: Unused method argument: args
(ARG002)
790-790: Unused method argument: args
(ARG002)
882-882: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/migrations/0013_alter_upgradeoperation_status.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_upgradeoperation_status.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/migrations/0015_batchupgradeoperation_location.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
511-511: Do not catch blind exception: Exception
(BLE001)
639-639: Unused lambda argument: driver
(ARG005)
649-649: Unused lambda argument: driver
(ARG005)
774-774: Unused lambda argument: driver
(ARG005)
openwisp_firmware_upgrader/tests/test_selenium.py
347-347: Unpacked variable org is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable category is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable build1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable build2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable image1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
478-478: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/tests/test_admin.py
905-905: Unused method argument: args
(ARG002)
951-951: Unused method argument: args
(ARG002)
1007-1007: Unused method argument: args
(ARG002)
1081-1081: Unused method argument: args
(ARG002)
1119-1119: Unused method argument: args
(ARG002)
1145-1145: Unused method argument: args
(ARG002)
1162-1162: Unused method argument: args
(ARG002)
1179-1179: Unused method argument: args
(ARG002)
1200-1200: Unused method argument: args
(ARG002)
1265-1265: Unused method argument: args
(ARG002)
1282-1282: Unused method argument: args
(ARG002)
1310-1310: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/api/views.py
185-185: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
450-450: Do not catch blind exception: Exception
(BLE001)
451-451: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
451-451: Use explicit conversion flag
Replace with conversion flag
(RUF010)
openwisp_firmware_upgrader/tests/test_websockets.py
123-123: Unused method argument: args
(ARG002)
217-217: Unused method argument: args
(ARG002)
250-250: Unused method argument: args
(ARG002)
264-266: try-except-pass detected, consider logging the exception
(S110)
264-264: Do not catch blind exception: Exception
(BLE001)
271-271: Unused method argument: args
(ARG002)
284-285: try-except-pass detected, consider logging the exception
(S110)
284-284: Do not catch blind exception: Exception
(BLE001)
289-289: Unused method argument: args
(ARG002)
328-328: Unused method argument: args
(ARG002)
433-434: try-except-pass detected, consider logging the exception
(S110)
433-433: Do not catch blind exception: Exception
(BLE001)
441-441: Unused method argument: args
(ARG002)
469-469: Unused method argument: args
(ARG002)
480-480: Unused method argument: args
(ARG002)
497-498: try-except-pass detected, consider logging the exception
(S110)
497-497: Do not catch blind exception: Exception
(BLE001)
511-512: try-except-pass detected, consider logging the exception
(S110)
511-511: Do not catch blind exception: Exception
(BLE001)
517-517: Unused method argument: args
(ARG002)
534-534: Unused method argument: args
(ARG002)
563-563: Unused method argument: args
(ARG002)
598-598: Unused method argument: args
(ARG002)
610-611: try-except-pass detected, consider logging the exception
(S110)
610-610: Do not catch blind exception: Exception
(BLE001)
621-622: try-except-pass detected, consider logging the exception
(S110)
621-621: Do not catch blind exception: Exception
(BLE001)
626-626: Unused method argument: args
(ARG002)
646-646: Unused method argument: args
(ARG002)
688-688: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/websockets.py
101-101: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
111-113: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
118-118: Unused method argument: close_code
(ARG002)
122-124: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
140-140: Unused method argument: content
(ARG002)
161-163: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
190-190: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
196-196: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
200-202: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
208-208: Unused method argument: close_code
(ARG002)
227-227: Unused method argument: content
(ARG002)
262-264: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
266-266: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
293-293: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
299-299: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
303-305: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
310-310: Unused method argument: close_code
(ARG002)
324-324: Unused method argument: content
(ARG002)
365-367: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
369-369: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
414-414: Store a reference to the return value of asyncio.create_task
(RUF006)
432-432: Unused class method argument: sender
(ARG003)
432-432: Unused class method argument: kwargs
(ARG003)
545-545: Unused class method argument: sender
(ARG003)
545-545: Unused class method argument: kwargs
(ARG003)
openwisp_firmware_upgrader/base/models.py
864-864: Avoid specifying long messages outside the exception class
(TRY003)
867-869: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/admin.py
240-240: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
301-301: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
324-324: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
347-347: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
348-348: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
349-349: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
350-350: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
351-351: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
352-359: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
386-386: Unused method argument: request
(ARG002)
389-389: Unused method argument: request
(ARG002)
389-389: Unused method argument: obj
(ARG002)
395-395: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
396-404: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
405-405: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
406-406: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
408-421: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
422-422: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
423-430: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
450-450: Unused method argument: form_url
(ARG002)
661-665: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
666-670: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
717-720: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
721-726: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def batch_upgrade( | ||
| self, firmwareless, upgrade_options=None, group=None, location=None | ||
| ): | ||
| upgrade_options = upgrade_options or {} | ||
|
|
||
| # Check if there are any devices to upgrade with the given filters | ||
| dry_run_result = load_model("BatchUpgradeOperation").dry_run( | ||
| build=self, group=group, location=location | ||
| ) | ||
|
|
||
| # If no devices match the filters, don't start the upgrade | ||
| total_devices = len(dry_run_result["device_firmwares"]) + len( | ||
| dry_run_result["devices"] | ||
| ) | ||
| if total_devices == 0: | ||
| raise ValidationError( | ||
| _( | ||
| "No devices found matching the specified filters. " | ||
| "Please adjust your group and/or location filters." | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root/project && find . -type f -name "*.py" -path "*/openwisp_firmware_upgrader/*" | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 142
🏁 Script executed:
find /root/project -type f -name "models.py" -path "*/openwisp_firmware_upgrader/*" | grep -E "base/models.py|models.py"Repository: openwisp/openwisp-firmware-upgrader
Length of output: 127
🏁 Script executed:
cat -n /root/project/openwisp_firmware_upgrader/base/models.py | sed -n '161,182p'Repository: openwisp/openwisp-firmware-upgrader
Length of output: 166
🏁 Script executed:
# Get more context around the batch_upgrade method and the dry_run implementation
cat -n /root/project/openwisp_firmware_upgrader/base/models.py | sed -n '140,200p'Repository: openwisp/openwisp-firmware-upgrader
Length of output: 166
🏁 Script executed:
# Find the BatchUpgradeOperation class and its dry_run method
rg -A 30 "class BatchUpgradeOperation" /root/project --type pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 132
🏁 Script executed:
# Find the dry_run method definition
rg -B 2 -A 20 "def dry_run" /root/project --type py | head -100Repository: openwisp/openwisp-firmware-upgrader
Length of output: 132
🏁 Script executed:
git ls-files | grep -E "base/models\.py|models\.py" | head -10Repository: openwisp/openwisp-firmware-upgrader
Length of output: 256
🏁 Script executed:
# List the structure to understand layout
git ls-files | grep "openwisp_firmware_upgrader" | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 1078
🏁 Script executed:
# Find the models.py file in the base directory
find . -type f -name "models.py" 2>/dev/null | grep -v __pycache__Repository: openwisp/openwisp-firmware-upgrader
Length of output: 214
🏁 Script executed:
cat -n ./openwisp_firmware_upgrader/base/models.py | sed -n '161,182p'Repository: openwisp/openwisp-firmware-upgrader
Length of output: 1046
🏁 Script executed:
# Get more context around the batch_upgrade method
cat -n ./openwisp_firmware_upgrader/base/models.py | sed -n '140,210p'Repository: openwisp/openwisp-firmware-upgrader
Length of output: 3013
🏁 Script executed:
# Find BatchUpgradeOperation class and dry_run method
rg -B 5 -A 30 "def dry_run" ./openwisp_firmware_upgrader/base/models.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 1318
🏁 Script executed:
# Search for BatchUpgradeOperation class definition
rg -B 2 -A 50 "class BatchUpgradeOperation" ./openwisp_firmware_upgrader/base/models.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 61
🏁 Script executed:
# Find the _find_firmwareless_devices method to see what it returns
rg -B 2 -A 20 "_find_firmwareless_devices" ./openwisp_firmware_upgrader/base/models.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 3666
🏁 Script executed:
# Get the complete _find_related_device_firmwares method to confirm return type
rg -B 2 -A 30 "_find_related_device_firmwares" ./openwisp_firmware_upgrader/base/models.py | head -60Repository: openwisp/openwisp-firmware-upgrader
Length of output: 2124
🏁 Script executed:
# Check if there are any tests that might clarify the structure
rg -B 3 -A 10 "batch_upgrade" ./openwisp_firmware_upgrader/tests/test_models.py | head -80Repository: openwisp/openwisp-firmware-upgrader
Length of output: 3854
🏁 Script executed:
# Double-check: verify that len() on these querysets would actually be inefficient
# by confirming they're not pre-evaluated lists
rg -B 10 "def batch_upgrade" ./openwisp_firmware_upgrader/base/models.py | grep -A 15 "def batch_upgrade"Repository: openwisp/openwisp-firmware-upgrader
Length of output: 101
Avoid evaluating full querysets just to check emptiness.
The dry_run() method returns Django QuerySet objects. Using len(queryset) forces full database evaluation and loads all matching objects into memory; for large device batches, this scales poorly. Use .exists() instead to efficiently check for matching records.
🛠️ Suggested fix
- total_devices = len(dry_run_result["device_firmwares"]) + len(
- dry_run_result["devices"]
- )
- if total_devices == 0:
+ has_related = dry_run_result["device_firmwares"].exists()
+ has_firmwareless = dry_run_result["devices"].exists()
+ if not (has_related or has_firmwareless):
raise ValidationError(
_(
"No devices found matching the specified filters. "
"Please adjust your group and/or location filters."
)
)🤖 Prompt for AI Agents
In `@openwisp_firmware_upgrader/base/models.py` around lines 161 - 182, The code
in batch_upgrade is computing total_devices by calling len() on QuerySets
returned in dry_run_result, which forces full evaluation; change the logic to
use QuerySet.exists() checks instead: call dry_run_result =
load_model("BatchUpgradeOperation").dry_run(...), then check if not
dry_run_result["device_firmwares"].exists() and not
dry_run_result["devices"].exists() and raise the ValidationError in that case.
Update references in batch_upgrade to avoid len() on those QuerySets and use
.exists() to determine emptiness efficiently.
There was a problem hiding this comment.
@nemesifier, no, this has not been addressed yet. The code still uses len() on the querysets returned by dry_run(), which forces full database evaluation:
total_devices = len(dry_run_result["device_firmwares"]) + len(
dry_run_result["devices"]
)
if total_devices == 0:For better performance, especially with large device counts, you should use .exists() instead:
has_related = dry_run_result["device_firmwares"].exists()
has_firmwareless = dry_run_result["devices"].exists()
if not (has_related or has_firmwareless):This avoids loading all matching records into memory just to check if any exist. Would you like me to help plan this change?
🧠 Learnings used
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:34.134Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_firmware_upgrader/migrations/0004_batch_upgrade_operation_idle_status.py (1)
5-21: Avoid importing model classes in migrations.Importing
AbstractBatchUpgradeOperationfrom the models module creates a dependency on the current state of Python code. Django migrations should be self-contained for reproducibility. IfSTATUS_CHOICESchanges in the future, running this migration from scratch would use the new values, causing inconsistencies.🛠️ Suggested fix - inline the choices
-from ..base.models import AbstractBatchUpgradeOperation - class Migration(migrations.Migration): dependencies = [ ("firmware_upgrader", "0003_private_media"), ] operations = [ migrations.AlterField( model_name="batchupgradeoperation", name="status", field=models.CharField( - choices=AbstractBatchUpgradeOperation.STATUS_CHOICES, + choices=[ + ("idle", "idle"), + ("in-progress", "in progress"), + ("cancelled", "completed with some cancellations"), + ("success", "completed successfully"), + ("failed", "completed with some failures"), + ], default="idle", max_length=12, ), ), ]openwisp_firmware_upgrader/upgraders/openwrt.py (1)
388-416: Progress doesn’t advance when checksum file is missing.For the “no checksum file” path, progress remains at 15% until the later jump to 65%. Consider updating progress to 20% in both branches for consistent UX.
🔧 Suggested fix
else: self.log( _( "Image checksum file not found, proceeding " "with the upload of the new image..." ) ) + self.upgrade_operation.update_progress(20)
🤖 Fix all issues with AI agents
In `@docs/developer/utils.rst`:
- Around line 27-33: Update the completion status sentence in the docs to
include the cancellation state: locate the paragraph that reads "when upgrade
operations complete (success, failed, or aborted)" and change it to include
"cancelled" (e.g., "success, failed, aborted, or cancelled") so the
documentation accurately lists all terminal states for the upgrade log signal.
In `@openwisp_firmware_upgrader/api/views.py`:
- Around line 99-117: The current parsing of group_id and location_id in
openwisp_firmware_upgrader/api/views.py silently ignores invalid or non‑existent
IDs (group_id, location_id,
swapper.load_model("config","DeviceGroup").objects.get, Location.objects.get),
which can cause an unscoped mass upgrade; change this to validate IDs and fail
fast by returning an HTTP 400 with a clear error message when ValueError or
DoesNotExist is raised for either group or location. Update both places where
this pattern appears (the current block handling group_id/location_id and the
similar block later) by replacing the try/except that sets group/location to
None with a try/except that returns a 400 Bad Request (include which ID was
invalid), or extract the logic into a small helper (e.g., validate_group_or_400
and validate_location_or_400) and call it from both locations to avoid
duplication.
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 988-994: In AbstractUpgradeOperation.save(), remove the
unnecessary "return self" so the method matches Django's Model.save() behavior;
keep the call to super().save(*args, **kwargs) and the batch update logic (if
self.batch and self.status != "in-progress": self.batch.update()), and ensure
there are no callers expecting a return value from save() elsewhere in the
codebase before committing the change.
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 274-287: The aria-label for the anchor created in the $link
element is inconsistent with its href (deviceUrl pointing to the upgrade
operation change page); update the aria-label to reflect the upgrade operation
resource (for example "View upgrade operation for {data.device_name}" or include
data.operation_id) so it accurately describes the link target—modify the
.attr("aria-label", ...) on $link to reference data.operation_id and/or
data.device_name accordingly.
In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js`:
- Around line 307-335: The status string is injected into statusHtml without
escaping, creating an XSS risk; create a small DOM-escaping helper (e.g.,
escapeHtml) and use it when interpolating user-facing values like status (and
any other strings such as cancelButtonTitle or progressPercentage if they can be
influenced) into the template used by upgrade-progress.js; update the
construction of statusHtml (and related fragments that use progressClass,
cancelButtonClass, and data-operation-id="${operation.id}") to pass values
through escapeHtml so the rendered HTML is safe while keeping the existing
FW_STATUS_GROUPS logic intact.
In `@openwisp_firmware_upgrader/tests/test_models.py`:
- Around line 706-945: The test functions
test_batch_upgrade_with_group_filtering,
test_batch_upgrade_with_location_filtering, and
test_batch_upgrade_with_group_and_location_filtering currently accept unused
patched arguments as *args which triggers Ruff; change each test signature to
accept the patched args as *_ (e.g., def
test_batch_upgrade_with_group_filtering(self, *_):) so the mocks remain applied
but the linter recognizes the arguments as intentionally unused.
In `@openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py`:
- Around line 46-48: The helper function mocked_exec_upgrade_success has unused
parameters causing Ruff warnings; update its signature
(mocked_exec_upgrade_success) to prefix unused args with an underscore (e.g.,
_exit_codes, _timeout, _raise_unexpected_exit) so the parameters remain for
compatibility but are marked as intentionally unused and silence the linter.
- Around line 874-969: Rename unused test args and locals to be prefixed with an
underscore to satisfy linting: change the patched argument mock_putfo in
test_upgrade_cancellation_early_stage to _mock_putfo; when unpacking the return
of _trigger_upgrade in all three tests, rename device_fw and output (and the
trailing unused value) to _device_fw, _output, _unused (or similar underscored
names) instead of device_fw/output/_; and update the mock_exec_command signature
in test_upgrade_cancellation_services_restart to accept **_kwargs (or *_args,
**_kwargs) instead of **kwargs. Ensure references to these variables in the
tests are updated consistently (e.g., keep using device_conn, upgrade_op
unchanged).
In `@openwisp_firmware_upgrader/tests/test_websocket_selenium.py`:
- Around line 213-231: After each WebSocket publish, replace the immediate
assertions with explicit waits using WebDriverWait/expected_conditions: wait for
the element located by By.CSS_SELECTOR ".upgrade-progress-text" to have text
"75%", wait for ".upgrade-progress-fill" to have a style attribute containing
"width: 75%", and wait for ".field-log .readonly" to have innerHTML containing
the expected log lines ("Device identity verified successfully", "Uploading
firmware image", "Upload progress: 75%"); apply the same pattern for the abort
status checks later in the file. Locate the existing checks that call
find_element(By.CSS_SELECTOR, ".upgrade-progress-text"),
find_element(By.CSS_SELECTOR, ".upgrade-progress-fill"), and
find_element(By.CSS_SELECTOR, ".field-log .readonly") and replace the immediate
self.assert* calls with WebDriverWait-based waits that poll until the element
text/attribute/innerHTML contains the expected value before asserting. Ensure
imports/use of WebDriverWait and appropriate expected_conditions are added where
missing.
In `@openwisp_firmware_upgrader/tests/test_websockets.py`:
- Around line 43-44: The test class defines an async def tearDown which Django's
TransactionTestCase won't call; change it to a synchronous method by replacing
async def tearDown(self): return await sync_to_async(super().tearDown()) with
def tearDown(self): super().tearDown() so the TransactionTestCase cleanup runs
correctly (locate the tearDown method in
openwisp_firmware_upgrader/tests/test_websockets.py and update the method
signature and body accordingly).
In `@tests/openwisp2/settings.py`:
- Around line 178-186: The test settings set CELERY_BROKER_URL to "memory://"
when TESTING is true but do not also override CELERY_RESULT_BACKEND, leaving it
pointed at Redis; update the TESTING branch to set CELERY_RESULT_BACKEND to the
in-memory backend (matching CELERY_BROKER_URL) and keep CELERY_TASK_ALWAYS_EAGER
and CELERY_TASK_EAGER_PROPAGATES as-is so tests run without requiring Redis;
modify the block that checks TESTING and assigns
CELERY_BROKER_URL/CELERY_TASK_ALWAYS_EAGER/CELERY_TASK_EAGER_PROPAGATES to also
assign CELERY_RESULT_BACKEND, referencing the existing symbols TESTING,
CELERY_BROKER_URL, and CELERY_RESULT_BACKEND.
🧹 Nitpick comments (20)
tests/openwisp2/settings.py (1)
18-24: Test database conditional logic relies on exact tag string matching.The condition checks for specific
--exclude-tagarguments by exact string match. This works but is somewhat fragile—if the test runner is invoked with a slightly different format (e.g.,--exclude-tag no_parallelwithout=), the condition may not behave as expected.Consider whether this complexity is necessary, or document the expected invocation format.
requirements-test.txt (1)
1-1: Use the latest stable release (1.2.2) instead of the unreleased development branch.The dependency currently points to the
1.3branch (refs/heads/1.3.tar.gz), which is a development branch rather than a tagged release. The latest stable version1.2.2(released Jan 28, 2026) includes thechannels-testextra and is available on PyPI.Recommend changing to
openwisp-utils[qa,selenium,channels-test]==1.2.2for reproducible builds, unless a specific feature from the unreleased1.3branch is required.openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.css (1)
60-75: Style the disabled attribute too.If the button is disabled via HTML (
disabledattribute), it won’t pick up the.disabledclass styles.🎨 Optional CSS tweak
-.upgrade-cancel-btn.disabled { +.upgrade-cancel-btn.disabled, +.upgrade-cancel-btn:disabled { background-color: `#6c757d`; cursor: not-allowed; opacity: 0.6; }openwisp_firmware_upgrader/base/models.py (1)
860-874: Consider defining exception classes for cancellation errors.The
cancel()method raisesValueErrorwith inline messages. While acceptable, defining specific exception classes (e.g.,UpgradeNotCancellable) would improve testability and allow callers to catch specific error conditions. Also, thereturnat line 874 is unnecessary.♻️ Suggested improvement
def cancel(self): """Cancels the upgrade operation if conditions are met.""" # Validate cancellation conditions if self.status != self._CANCELLABLE_STATUS: - raise ValueError(f"Cannot cancel operation with status: {self.status}") + raise ValueError(f"Cannot cancel: status is '{self.status}'") if self.progress >= self._MAX_CANCELLABLE_PROGRESS: - raise ValueError( - "Cannot cancel upgrade: firmware reflashing has already started" - ) + raise ValueError("Cannot cancel: firmware reflashing has started") # Update status and save self.log_line("Upgrade operation has been cancelled by user", save=False) self.status = "cancelled" self.save() - returndocs/user/rest-api.rst (1)
213-259: Clarify group/location semantics and org constraints in docs.The API behavior is stricter than the text implies (group/location must belong to the same organization as the build, and specifying both should intersect the device set). Consider documenting this explicitly for both POST and GET.
✍️ Suggested doc tweak
The batch upgrade operation accepts the following optional parameters in the request body: - ``group`` (Device group ID): limit the upgrade to devices belonging to a specific group - ``location`` (Location ID): limit the upgrade to devices at a specific geographic location + (If both are provided, only devices matching both filters are selected. + Group and location must belong to the same organization as the build.) ... The dry-run batch upgrade operation accepts the following optional query parameters: - ``group`` (Device group ID): limit the preview to devices belonging to a specific group - ``location`` (Location ID): limit the preview to devices at a specific geographic location + (If both are provided, only devices matching both filters are selected. + Group and location must belong to the same organization as the build.)openwisp_firmware_upgrader/upgraders/openwrt.py (1)
197-229: Consider checking cancellation before connecting.If an operation is already cancelled, the upgrader still attempts a connection before the first cancellation check. An early check avoids needless work and side effects.
🔧 Suggested tweak
def upgrade(self, image): + self._check_cancellation() self._test_connection() self._check_cancellation()openwisp_firmware_upgrader/tests/test_selenium.py (2)
344-355: Drop unused tuple elements for clarity. Line 347 unpacks several unused values.🛠️ Proposed fix
- org, category, build1, build2, image1, image2, device = self._set_up_env() + _, _, _, _, _, image2, device = self._set_up_env()
478-489: Use an unused-arg placeholder for patched params. Line 478 can suppress lint without changing behavior.🛠️ Proposed fix
- def test_upgrade_operation_admin_no_submit_row(self, *args): + def test_upgrade_operation_admin_no_submit_row(self, *_):openwisp_firmware_upgrader/tests/test_admin.py (1)
905-1310: Remove unused*argsfrom new tests without decorators. Lines 905, 951, 1007, 1081, 1119, 1145, 1162, 1179, 1200, 1265, 1282, and 1310 don’t need varargs.🛠️ Example fix (apply similarly to the other methods)
- def test_batch_upgrade_operation_status_filter(self, *args): + def test_batch_upgrade_operation_status_filter(self):openwisp_firmware_upgrader/tests/test_websocket_selenium.py (1)
10-13: Catch specific Selenium exceptions instead of a broadException. This avoids masking unexpected failures while still handling transient DOM issues.🛠️ Proposed fix
-from selenium.webdriver.common.by import By +from selenium.common.exceptions import NoSuchElementException, StaleElementReferenceException +from selenium.webdriver.common.by import By @@ - except Exception: + except (StaleElementReferenceException, NoSuchElementException): return False @@ - except Exception: + except (StaleElementReferenceException, NoSuchElementException): return FalseAlso applies to: 488-513
openwisp_firmware_upgrader/tests/test_websockets.py (1)
261-266: Broad exception handling withpassin test assertions.While the static analysis flags these as issues, in test code where we're verifying that connections fail, catching generic
Exceptionand passing is acceptable. However, adding a brief comment explaining the intent would improve clarity.💡 Consider adding comments for clarity
try: connected, _ = await communicator.connect() self.assertEqual(connected, False) except Exception: - # If connection is forcibly closed then treat it as failed connection + # Connection forcibly closed is expected - treat as failed connection passAlso applies to: 281-285, 430-434, 495-498, 510-512, 607-611, 618-622
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.js (2)
88-96: Redundant existence check forFW_STATUS_HELPERS.
FW_STATUS_HELPERSis defined in the same file scope (line 70), so checking for its existence on lines 88-91 is unnecessary and adds noise.♻️ Simplified version
- if ( - FW_STATUS_HELPERS && - FW_STATUS_HELPERS.isCompleted && - FW_STATUS_HELPERS.isCompleted(status) - ) { + if (FW_STATUS_HELPERS.isCompleted(status)) { return 100; }
131-142: DOM helper functions assume jQuery-wrapped elements.
isScrolledToBottomandscrollToBottomexpect elements with a.lengthproperty and array-like access (element[0]), indicating jQuery objects. This implicit dependency should be documented or the functions should handle raw DOM elements as well for flexibility.📝 Consider adding JSDoc or supporting raw elements
+/** + * Check if a jQuery-wrapped element is scrolled to bottom. + * `@param` {jQuery} element - jQuery-wrapped element + * `@returns` {boolean} + */ function isScrolledToBottom(element) { if (!element || !element.length) return false; let el = element[0]; return el.scrollHeight - el.clientHeight <= el.scrollTop + 1; } +/** + * Scroll a jQuery-wrapped element to bottom. + * `@param` {jQuery} element - jQuery-wrapped element + */ function scrollToBottom(element) {openwisp_firmware_upgrader/websockets.py (3)
65-74:_convert_lazy_translationsrelies on internal Django attribute.The check
hasattr(obj, "_proxy____cast")relies on an internal Django implementation detail that could change between versions. Consider usingdjango.utils.functional.Promisefor a more robust check.♻️ More robust lazy string detection
+from django.utils.functional import Promise + def _convert_lazy_translations(obj): """Recursively convert Django lazy translation objects to strings for JSON serialization.""" if isinstance(obj, dict): return {key: _convert_lazy_translations(value) for key, value in obj.items()} elif isinstance(obj, (list, tuple)): return type(obj)(_convert_lazy_translations(item) for item in obj) - elif hasattr(obj, "__str__") and hasattr(obj, "_proxy____cast"): + elif isinstance(obj, Promise): return str(obj) else: return obj
431-476: Signal handlerhandle_upgrade_operation_post_savelogic has unclear early return.The condition on line 437
if created and not (hasattr(instance, "batch") and instance.batch)is confusing. It returns early when the operation is newly created AND doesn't belong to a batch, but the comment says "Only publish updates for existing operations." The double negative makes this hard to follow.💡 Clearer condition
`@classmethod` def handle_upgrade_operation_post_save(cls, sender, instance, created, **kwargs): """ Handle UpgradeOperation post_save events by publishing status updates to WebSocket channels. """ - # Only publish updates for existing operations - if created and not (hasattr(instance, "batch") and instance.batch): + # Publish updates for: + # 1. Existing operations (updates, not creates) + # 2. Newly created operations that belong to a batch + is_batch_operation = hasattr(instance, "batch") and instance.batch + if created and not is_batch_operation: return
329-343: Hardcoded status values should reference model constants.The status values in the filter (
"in-progress","in progress","success", etc.) are hardcoded strings. Consider referencing status constants from the model or a shared location to maintain consistency.openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (2)
292-368: Significant code duplication betweenupdateStatusWithProgressBarandupdateSingleOperationStatusWithProgressBar.These two functions share nearly identical logic for rendering progress bars and cancel buttons. Consider extracting the shared HTML generation into a helper function to reduce duplication and maintenance burden.
♻️ Extract shared logic
function buildProgressBarHtml(status, progressPercentage, progressClass, operationId) { let statusHtml = `<span class="upgrade-status-${progressClass}">${status}</span>`; if (FW_STATUS_GROUPS.IN_PROGRESS.has(status)) { const canCancel = progressPercentage < 60; const cancelButtonClass = canCancel ? "upgrade-cancel-btn" : "upgrade-cancel-btn disabled"; const cancelButtonTitle = canCancel ? gettext("Cancel upgrade") : gettext("Cannot cancel - firmware flashing in progress"); statusHtml += ` <div class="upgrade-progress-bar"> <div class="upgrade-progress-fill in-progress" style="width: ${progressPercentage}%"></div> </div> <span class="upgrade-progress-text">${progressPercentage}%</span> <button class="${cancelButtonClass}" data-operation-id="${operationId}" title="${cancelButtonTitle}" ${!canCancel ? "disabled" : ""}> ${gettext("Cancel")} </button> `; } else if (status === FW_UPGRADE_STATUS.SUCCESS) { // ... success case } // ... other cases return statusHtml; }Also applies to: 498-577
749-758: Hardcoded API endpoint path.The cancel API endpoint
/api/v1/firmware-upgrader/upgrade-operation/${operationId}/cancel/is hardcoded. Consider using a configuration variable or Django's URL reverse mechanism passed from the template to improve maintainability.openwisp_firmware_upgrader/admin.py (2)
125-149: Repetitive queryset assignment in form__init__.The logic for setting group and location querysets is duplicated across multiple branches. Consider consolidating this.
♻️ Simplified queryset assignment
def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - # Set the appropriate queryset for group field based on the build's organization - if self.initial.get("build"): - build = self.initial["build"] - if hasattr(build, "category") and build.category.organization: - self.fields["group"].queryset = swapper.load_model( - "config", "DeviceGroup" - ).objects.filter(organization=build.category.organization) - self.fields["location"].queryset = Location.objects.filter( - organization=build.category.organization - ) - else: - # For shared builds, allow all groups and locations - self.fields["group"].queryset = swapper.load_model( - "config", "DeviceGroup" - ).objects.all() - self.fields["location"].queryset = Location.objects.all() - else: - # If no build in initial, show all groups and locations - self.fields["group"].queryset = swapper.load_model( - "config", "DeviceGroup" - ).objects.all() - self.fields["location"].queryset = Location.objects.all() + DeviceGroupModel = swapper.load_model("config", "DeviceGroup") + build = self.initial.get("build") + org = getattr(getattr(build, "category", None), "organization", None) if build else None + + if org: + self.fields["group"].queryset = DeviceGroupModel.objects.filter(organization=org) + self.fields["location"].queryset = Location.objects.filter(organization=org) + else: + self.fields["group"].queryset = DeviceGroupModel.objects.all() + self.fields["location"].queryset = Location.objects.all()
510-527: Querying all organizations on everychange_viewrequest could be expensive.
Organization.objects.all().order_by("name")is executed on every change view request. For installations with many organizations, this could impact performance. Consider caching or limiting the queryset.💡 Consider filtering organizations to relevant ones
- for org in Organization.objects.all().order_by("name"): + # Only show organizations that have devices with upgrade operations + relevant_org_ids = upgrades_qs.values_list( + 'device__organization_id', flat=True + ).distinct() + for org in Organization.objects.filter(id__in=relevant_org_ids).order_by("name"):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
.github/workflows/ci.ymldocker-compose.ymldocs/developer/index.rstdocs/developer/installation.rstdocs/developer/utils.rstdocs/index.rstdocs/user/intro.rstdocs/user/quickstart.rstdocs/user/rest-api.rstdocs/user/upgrade-status.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/api/urls.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/exceptions.pyopenwisp_firmware_upgrader/filters.pyopenwisp_firmware_upgrader/migrations/0004_batch_upgrade_operation_idle_status.pyopenwisp_firmware_upgrader/migrations/0013_upgradeoperation_progress.pyopenwisp_firmware_upgrader/migrations/0014_alter_upgradeoperation_status.pyopenwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.pyopenwisp_firmware_upgrader/migrations/0016_batchupgradeoperation_location.pyopenwisp_firmware_upgrader/routing.pyopenwisp_firmware_upgrader/signals.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/mass-upgrade-select2.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.jsopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.htmlopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.htmlopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_openwrt_upgrader.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_websocket_selenium.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/upgraders/openwrt.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/widgets.pyrequirements-test.txtrunteststests/openwisp2/postgresql_settings.pytests/openwisp2/routing.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_upgradeoperation_progress.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_upgradeoperation_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.pytests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (1)
- docker-compose.yml
✅ Files skipped from review due to trivial changes (2)
- docs/index.rst
- docs/user/intro.rst
🚧 Files skipped from review as they are similar to previous changes (14)
- openwisp_firmware_upgrader/routing.py
- openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css
- docs/developer/installation.rst
- .github/workflows/ci.yml
- docs/user/upgrade-status.rst
- openwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.html
- openwisp_firmware_upgrader/static/firmware-upgrader/js/mass-upgrade-select2.js
- openwisp_firmware_upgrader/tests/base.py
- openwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.html
- docs/developer/index.rst
- openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.css
- openwisp_firmware_upgrader/api/serializers.py
- tests/openwisp2/routing.py
- tests/openwisp2/postgresql_settings.py
🧰 Additional context used
🧬 Code graph analysis (19)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (2)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (4)
status(309-309)progressPercentage(146-147)progressPercentage(310-310)progressPercentage(362-362)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (5)
status(295-295)status(500-500)progressPercentage(297-297)progressPercentage(501-501)protocol(645-645)
openwisp_firmware_upgrader/migrations/0016_batchupgradeoperation_location.py (3)
openwisp_firmware_upgrader/migrations/0013_upgradeoperation_progress.py (1)
Migration(7-25)openwisp_firmware_upgrader/migrations/0014_alter_upgradeoperation_status.py (1)
Migration(6-28)openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py (1)
Migration(8-27)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (2)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/upgraders/openwrt.py (4)
connect(134-138)_check_cancellation(211-221)upgrade(197-209)disconnect(140-144)
openwisp_firmware_upgrader/apps.py (2)
openwisp_firmware_upgrader/websockets.py (7)
BatchUpgradeProgressPublisher(479-564)UpgradeProgressPublisher(378-476)connect(82-116)connect(174-206)connect(277-308)handle_upgrade_operation_post_save(432-476)handle_batch_upgrade_operation_saved(545-564)openwisp_firmware_upgrader/models.py (2)
UpgradeOperation(43-46)BatchUpgradeOperation(37-40)
tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py (2)
openwisp_firmware_upgrader/models.py (1)
BatchUpgradeOperation(37-40)tests/openwisp2/sample_firmware_upgrader/models.py (1)
BatchUpgradeOperation(40-42)
openwisp_firmware_upgrader/tests/test_api.py (2)
openwisp_firmware_upgrader/tests/base.py (1)
_create_upgrade_env(121-186)openwisp_firmware_upgrader/api/views.py (3)
post(94-127)post(438-463)get(129-159)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (3)
openwisp_firmware_upgrader/tests/base.py (3)
_get_category(46-52)_create_build(64-75)_create_firmware_image(77-92)openwisp_firmware_upgrader/websockets.py (5)
BatchUpgradeProgressPublisher(479-564)UpgradeProgressPublisher(378-476)publish_operation_update(418-420)publish_log(422-423)publish_batch_status(524-532)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.js (2)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (4)
status(309-309)progressPercentage(146-147)progressPercentage(310-310)progressPercentage(362-362)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (5)
status(295-295)status(500-500)progressPercentage(297-297)progressPercentage(501-501)protocol(645-645)
openwisp_firmware_upgrader/tests/test_websockets.py (2)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/websockets.py (16)
BatchUpgradeProgressConsumer(169-269)BatchUpgradeProgressPublisher(479-564)DeviceUpgradeProgressConsumer(272-375)UpgradeProgressConsumer(77-166)UpgradeProgressPublisher(378-476)connect(82-116)connect(174-206)connect(277-308)disconnect(118-125)disconnect(208-212)disconnect(310-314)publish_progress(392-416)publish_progress(489-501)publish_operation_update(418-420)publish_operation_progress(503-522)publish_batch_status(524-532)
openwisp_firmware_upgrader/migrations/0004_batch_upgrade_operation_idle_status.py (1)
openwisp_firmware_upgrader/base/models.py (1)
AbstractBatchUpgradeOperation(526-802)
openwisp_firmware_upgrader/tests/test_selenium.py (2)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (1)
_assert_no_js_errors(115-124)openwisp_firmware_upgrader/tests/base.py (1)
_create_device_firmware(105-119)
openwisp_firmware_upgrader/websockets.py (5)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/upgraders/openwrt.py (2)
connect(134-138)disconnect(140-144)openwisp_firmware_upgrader/api/serializers.py (2)
UpgradeOperationSerializer(84-96)DeviceUpgradeOperationSerializer(99-102)openwisp_firmware_upgrader/base/models.py (4)
upgrade_operations(657-658)total_operations(661-662)update(590-599)calculate_and_update_status(730-802)openwisp_firmware_upgrader/admin.py (1)
completed(566-567)
openwisp_firmware_upgrader/widgets.py (1)
openwisp_firmware_upgrader/admin.py (7)
media(152-161)Media(78-95)Media(317-318)Media(323-324)Media(600-602)Media(660-670)Media(716-726)
openwisp_firmware_upgrader/tests/test_admin.py (3)
openwisp_firmware_upgrader/admin.py (4)
BatchUpgradeConfirmationForm(103-161)save(623-631)organization(179-180)organization(561-562)openwisp_firmware_upgrader/base/models.py (3)
batch_upgrade(161-191)save(421-430)save(988-994)openwisp_firmware_upgrader/models.py (1)
BatchUpgradeOperation(37-40)
openwisp_firmware_upgrader/migrations/0014_alter_upgradeoperation_status.py (3)
openwisp_firmware_upgrader/migrations/0013_upgradeoperation_progress.py (1)
Migration(7-25)openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py (1)
Migration(8-27)openwisp_firmware_upgrader/migrations/0016_batchupgradeoperation_location.py (1)
Migration(8-27)
openwisp_firmware_upgrader/upgraders/openwrt.py (2)
openwisp_firmware_upgrader/exceptions.py (2)
UpgradeCancelled(21-24)RecoverableFailure(33-36)openwisp_firmware_upgrader/base/models.py (6)
log_line(843-853)save(421-430)save(988-994)update_progress(855-858)upgrade(601-606)upgrade(885-986)
openwisp_firmware_upgrader/admin.py (5)
openwisp_firmware_upgrader/filters.py (3)
BuildFilter(32-35)GroupFilter(44-47)LocationFilter(38-41)openwisp_firmware_upgrader/widgets.py (4)
FirmwareSchemaWidget(12-28)MassUpgradeSelect2Widget(31-61)media(25-28)media(52-61)openwisp_firmware_upgrader/models.py (2)
BatchUpgradeOperation(37-40)UpgradeOperation(43-46)openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (2)
data(110-110)status(309-309)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (3)
data(220-220)status(295-295)status(500-500)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (3)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (7)
$(143-143)$(237-237)$(264-264)$(308-308)statusField(207-207)statusField(350-350)status(309-309)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (5)
FW_STATUS_HELPERS(70-77)ALL_VALID_FW_STATUSES(35-38)FW_STATUS_GROUPS(41-68)FW_UPGRADE_STATUS(4-11)protocol(119-119)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.js (5)
FW_STATUS_HELPERS(70-77)ALL_VALID_FW_STATUSES(35-38)FW_STATUS_GROUPS(41-68)FW_UPGRADE_STATUS(4-11)protocol(119-119)
tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py (1)
tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py (1)
Migration(8-27)
🪛 GitHub Check: CodeQL
openwisp_firmware_upgrader/api/views.py
[warning] 467-467: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.14)
openwisp_firmware_upgrader/migrations/0013_upgradeoperation_progress.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/migrations/0016_batchupgradeoperation_location.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/tests/test_models.py
708-708: Unused method argument: args
(ARG002)
790-790: Unused method argument: args
(ARG002)
882-882: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py
47-47: Unused function argument: exit_codes
(ARG001)
47-47: Unused function argument: timeout
(ARG001)
47-47: Unused function argument: raise_unexpected_exit
(ARG001)
875-875: Unused method argument: mock_putfo
(ARG002)
877-877: Unpacked variable device_fw is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
877-877: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
909-909: Unpacked variable device_fw is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
909-909: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
917-917: Unused function argument: kwargs
(ARG001)
943-943: Unpacked variable device_fw is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
943-943: Unpacked variable output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
openwisp_firmware_upgrader/api/views.py
185-185: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
450-450: Do not catch blind exception: Exception
(BLE001)
451-451: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
451-451: Use explicit conversion flag
Replace with conversion flag
(RUF010)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
511-511: Do not catch blind exception: Exception
(BLE001)
639-639: Unused lambda argument: driver
(ARG005)
649-649: Unused lambda argument: driver
(ARG005)
774-774: Unused lambda argument: driver
(ARG005)
openwisp_firmware_upgrader/tests/test_websockets.py
123-123: Unused method argument: args
(ARG002)
217-217: Unused method argument: args
(ARG002)
250-250: Unused method argument: args
(ARG002)
264-266: try-except-pass detected, consider logging the exception
(S110)
264-264: Do not catch blind exception: Exception
(BLE001)
271-271: Unused method argument: args
(ARG002)
284-285: try-except-pass detected, consider logging the exception
(S110)
284-284: Do not catch blind exception: Exception
(BLE001)
289-289: Unused method argument: args
(ARG002)
328-328: Unused method argument: args
(ARG002)
433-434: try-except-pass detected, consider logging the exception
(S110)
433-433: Do not catch blind exception: Exception
(BLE001)
441-441: Unused method argument: args
(ARG002)
469-469: Unused method argument: args
(ARG002)
480-480: Unused method argument: args
(ARG002)
497-498: try-except-pass detected, consider logging the exception
(S110)
497-497: Do not catch blind exception: Exception
(BLE001)
511-512: try-except-pass detected, consider logging the exception
(S110)
511-511: Do not catch blind exception: Exception
(BLE001)
517-517: Unused method argument: args
(ARG002)
534-534: Unused method argument: args
(ARG002)
563-563: Unused method argument: args
(ARG002)
598-598: Unused method argument: args
(ARG002)
610-611: try-except-pass detected, consider logging the exception
(S110)
610-610: Do not catch blind exception: Exception
(BLE001)
621-622: try-except-pass detected, consider logging the exception
(S110)
621-621: Do not catch blind exception: Exception
(BLE001)
626-626: Unused method argument: args
(ARG002)
646-646: Unused method argument: args
(ARG002)
688-688: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/tests/test_selenium.py
347-347: Unpacked variable org is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable category is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable build1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable build2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable image1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
478-478: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/websockets.py
101-101: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
111-113: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
118-118: Unused method argument: close_code
(ARG002)
122-124: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
140-140: Unused method argument: content
(ARG002)
161-163: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
190-190: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
196-196: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
200-202: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
208-208: Unused method argument: close_code
(ARG002)
227-227: Unused method argument: content
(ARG002)
262-264: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
266-266: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
293-293: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
299-299: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
303-305: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
310-310: Unused method argument: close_code
(ARG002)
324-324: Unused method argument: content
(ARG002)
365-367: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
369-369: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
414-414: Store a reference to the return value of asyncio.create_task
(RUF006)
432-432: Unused class method argument: sender
(ARG003)
432-432: Unused class method argument: kwargs
(ARG003)
545-545: Unused class method argument: sender
(ARG003)
545-545: Unused class method argument: kwargs
(ARG003)
openwisp_firmware_upgrader/tests/test_admin.py
905-905: Unused method argument: args
(ARG002)
951-951: Unused method argument: args
(ARG002)
1007-1007: Unused method argument: args
(ARG002)
1081-1081: Unused method argument: args
(ARG002)
1119-1119: Unused method argument: args
(ARG002)
1145-1145: Unused method argument: args
(ARG002)
1162-1162: Unused method argument: args
(ARG002)
1179-1179: Unused method argument: args
(ARG002)
1200-1200: Unused method argument: args
(ARG002)
1265-1265: Unused method argument: args
(ARG002)
1282-1282: Unused method argument: args
(ARG002)
1310-1310: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/migrations/0014_alter_upgradeoperation_status.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/upgraders/openwrt.py
221-221: Avoid specifying long messages outside the exception class
(TRY003)
226-226: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/admin.py
240-240: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
301-301: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
324-324: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
347-347: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
348-348: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
349-349: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
350-350: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
351-351: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
352-359: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
386-386: Unused method argument: request
(ARG002)
389-389: Unused method argument: request
(ARG002)
389-389: Unused method argument: obj
(ARG002)
395-395: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
396-404: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
405-405: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
406-406: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
408-421: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
422-422: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
423-430: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
450-450: Unused method argument: form_url
(ARG002)
661-665: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
666-670: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
717-720: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
721-726: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/base/models.py
864-864: Avoid specifying long messages outside the exception class
(TRY003)
867-869: Avoid specifying long messages outside the exception class
(TRY003)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_upgradeoperation_progress.py
9-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
13-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_upgradeoperation_status.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_firmware_upgrader/migrations/0007_batchupgradeoperation_group.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/openwisp2/sample_firmware_upgrader/migrations/0008_batchupgradeoperation_location.py
10-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
15-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (12)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | 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.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Force Redis for development to ensure async task execution | ||
| CELERY_BROKER_URL = "redis://localhost/2" | ||
| CELERY_RESULT_BACKEND = "redis://localhost/2" | ||
|
|
||
| # Only use eager mode for actual tests | ||
| if TESTING: | ||
| CELERY_TASK_ALWAYS_EAGER = True | ||
| CELERY_TASK_EAGER_PROPAGATES = True | ||
| CELERY_BROKER_URL = "memory://" |
There was a problem hiding this comment.
CELERY_RESULT_BACKEND is not overridden during testing.
The Celery broker URL is correctly switched to memory:// for tests, but CELERY_RESULT_BACKEND remains set to redis://localhost/2. This could cause test failures in CI environments where Redis may not be available, or lead to inconsistent test behavior.
Proposed fix
# Only use eager mode for actual tests
if TESTING:
CELERY_TASK_ALWAYS_EAGER = True
CELERY_TASK_EAGER_PROPAGATES = True
CELERY_BROKER_URL = "memory://"
+ CELERY_RESULT_BACKEND = "cache+memory://"📝 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.
| # Force Redis for development to ensure async task execution | |
| CELERY_BROKER_URL = "redis://localhost/2" | |
| CELERY_RESULT_BACKEND = "redis://localhost/2" | |
| # Only use eager mode for actual tests | |
| if TESTING: | |
| CELERY_TASK_ALWAYS_EAGER = True | |
| CELERY_TASK_EAGER_PROPAGATES = True | |
| CELERY_BROKER_URL = "memory://" | |
| # Force Redis for development to ensure async task execution | |
| CELERY_BROKER_URL = "redis://localhost/2" | |
| CELERY_RESULT_BACKEND = "redis://localhost/2" | |
| # Only use eager mode for actual tests | |
| if TESTING: | |
| CELERY_TASK_ALWAYS_EAGER = True | |
| CELERY_TASK_EAGER_PROPAGATES = True | |
| CELERY_BROKER_URL = "memory://" | |
| CELERY_RESULT_BACKEND = "cache+memory://" |
🤖 Prompt for AI Agents
In `@tests/openwisp2/settings.py` around lines 178 - 186, The test settings set
CELERY_BROKER_URL to "memory://" when TESTING is true but do not also override
CELERY_RESULT_BACKEND, leaving it pointed at Redis; update the TESTING branch to
set CELERY_RESULT_BACKEND to the in-memory backend (matching CELERY_BROKER_URL)
and keep CELERY_TASK_ALWAYS_EAGER and CELERY_TASK_EAGER_PROPAGATES as-is so
tests run without requiring Redis; modify the block that checks TESTING and
assigns CELERY_BROKER_URL/CELERY_TASK_ALWAYS_EAGER/CELERY_TASK_EAGER_PROPAGATES
to also assign CELERY_RESULT_BACKEND, referencing the existing symbols TESTING,
CELERY_BROKER_URL, and CELERY_RESULT_BACKEND.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/developer/utils.rst`:
- Around line 21-25: The docs for the firmware_upgrader_log_updated signal are
missing the emitted "line" keyword; update the Arguments list in
docs/developer/utils.rst to include a bullet for ``line`` describing that it
contains the log line (string or appropriate type) provided when calling
firmware_upgrader_log_updated.send(...), alongside the existing ``sender``,
``instance``, and ``**kwargs`` entries so the documentation accurately reflects
the signal payload.
In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js`:
- Around line 339-345: The UI uses a 60% threshold for canceling upgrades which
conflicts with the backend's 70% rule; update the check that computes canCancel
(and the related cancelButtonClass and cancelButtonTitle logic) to use 70
instead of 60 wherever it appears (including the duplicate block around the
other instance at lines 529-535) so the frontend allows cancellation up to the
backend limit; keep progressPercentage as the input and only change the numeric
threshold to 70.
- Around line 54-56: The formatLogForDisplay function currently returns raw log
text with newlines converted to <br>, which risks XSS when inserted via
.html(...); update formatLogForDisplay to first call the existing escapeHtml
function on logContent, then replace newline characters with "<br>" (i.e., use
escapeHtml(logContent).replace(/\n/g, "<br>")) and keep the empty-string
fallback so all uses of formatLogForDisplay (and any .html(...) injections)
receive escaped, safe HTML.
In `@tests/openwisp2/settings.py`:
- Around line 106-112: CHANNEL_LAYERS currently always configures a Redis
backend which forces tests to require Redis; make CHANNEL_LAYERS conditional on
the TESTING flag (same pattern used for CACHES and CELERY) and switch to the
in-memory backend when TESTING is true by using
channels.layers.InMemoryChannelLayer for the "default" BACKEND entry so tests
run without Redis.
🧹 Nitpick comments (2)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (1)
66-83: Consider importing_non_critical_servicesto avoid duplication.This services list duplicates
OpenWrt._non_critical_servicesdefined inopenwisp_firmware_upgrader/upgraders/openwrt.py. If the canonical list changes, this mock could become inconsistent.You could reference the list directly:
for service in OpenWrt._non_critical_services:However, keeping test data separate is also a valid approach for isolation. This is a minor suggestion.
openwisp_firmware_upgrader/tests/test_websockets.py (1)
247-286: Consider being more specific with exception handling in rejection tests.The
try-except-passblocks catch blindExceptionto handle forcible connection closure. While acceptable for these rejection scenarios, catching more specific exceptions would improve clarity.Optional improvement
try: connected, _ = await communicator.connect() self.assertEqual(connected, False) - except Exception: - # If connection is forcibly closed then treat it as failed connection + except (AssertionError, RuntimeError): + # Connection was forcibly closed - expected for rejection pass
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/developer/utils.rstdocs/user/upgrade-status.rstopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.jsopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_openwrt_upgrader.pyopenwisp_firmware_upgrader/tests/test_websocket_selenium.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/settings.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/user/upgrade-status.rst
- openwisp_firmware_upgrader/tests/test_models.py
- tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T18:09:17.215Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:17.215Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
Applied to files:
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py
🧬 Code graph analysis (6)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (2)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/upgraders/openwrt.py (6)
connect(134-138)exec_command(146-150)_check_cancellation(211-221)upgrade(197-209)disconnect(140-144)_stop_non_critical_services(329-339)
openwisp_firmware_upgrader/websockets.py (4)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/api/serializers.py (2)
UpgradeOperationSerializer(84-96)DeviceUpgradeOperationSerializer(99-102)openwisp_firmware_upgrader/base/models.py (4)
upgrade_operations(657-658)total_operations(661-662)update(590-599)calculate_and_update_status(730-802)openwisp_firmware_upgrader/admin.py (1)
completed(566-567)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (2)
openwisp_firmware_upgrader/websockets.py (6)
BatchUpgradeProgressPublisher(487-572)UpgradeProgressPublisher(384-484)publish_operation_update(426-428)publish_log(430-431)publish_batch_status(532-540)publish_operation_progress(511-530)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)
openwisp_firmware_upgrader/tests/test_websockets.py (2)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/websockets.py (15)
BatchUpgradeProgressConsumer(173-274)BatchUpgradeProgressPublisher(487-572)DeviceUpgradeProgressConsumer(277-381)UpgradeProgressConsumer(80-170)UpgradeProgressPublisher(384-484)connect(85-120)connect(178-211)connect(282-314)disconnect(122-129)disconnect(213-217)disconnect(316-320)publish_progress(398-424)publish_progress(497-509)publish_operation_progress(511-530)publish_batch_status(532-540)
openwisp_firmware_upgrader/base/models.py (3)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/swapper.py (2)
load_model(7-8)get_model_name(11-12)openwisp_firmware_upgrader/admin.py (3)
cancelled_rate(578-579)completed(566-567)save(623-631)
openwisp_firmware_upgrader/api/views.py (2)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/base/models.py (3)
batch_upgrade(161-191)dry_run(609-619)cancel(860-874)
🪛 GitHub Check: CodeQL
openwisp_firmware_upgrader/api/views.py
[warning] 473-473: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.14)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py
47-47: Unused function argument: exit_codes
(ARG001)
47-47: Unused function argument: timeout
(ARG001)
47-47: Unused function argument: raise_unexpected_exit
(ARG001)
openwisp_firmware_upgrader/websockets.py
105-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
111-111: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
115-117: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
122-122: Unused method argument: close_code
(ARG002)
126-128: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
144-144: Unused method argument: content
(ARG002)
165-167: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
195-195: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
201-201: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
205-207: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
213-213: Unused method argument: close_code
(ARG002)
232-232: Unused method argument: content
(ARG002)
267-269: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
271-271: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
299-299: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
305-305: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
309-311: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
316-316: Unused method argument: close_code
(ARG002)
330-330: Unused method argument: content
(ARG002)
371-373: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
375-375: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
440-440: Unused class method argument: sender
(ARG003)
440-440: Unused class method argument: kwargs
(ARG003)
553-553: Unused class method argument: sender
(ARG003)
553-553: Unused class method argument: kwargs
(ARG003)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py
509-509: Consider moving this statement to an else block
(TRY300)
510-510: Do not catch blind exception: Exception
(BLE001)
518-518: Do not catch blind exception: Exception
(BLE001)
646-646: Unused lambda argument: driver
(ARG005)
656-656: Unused lambda argument: driver
(ARG005)
781-781: Unused lambda argument: driver
(ARG005)
openwisp_firmware_upgrader/tests/test_websockets.py
123-123: Unused method argument: args
(ARG002)
217-217: Unused method argument: args
(ARG002)
250-250: Unused method argument: args
(ARG002)
264-266: try-except-pass detected, consider logging the exception
(S110)
264-264: Do not catch blind exception: Exception
(BLE001)
271-271: Unused method argument: args
(ARG002)
284-285: try-except-pass detected, consider logging the exception
(S110)
284-284: Do not catch blind exception: Exception
(BLE001)
289-289: Unused method argument: args
(ARG002)
328-328: Unused method argument: args
(ARG002)
433-434: try-except-pass detected, consider logging the exception
(S110)
433-433: Do not catch blind exception: Exception
(BLE001)
441-441: Unused method argument: args
(ARG002)
469-469: Unused method argument: args
(ARG002)
480-480: Unused method argument: args
(ARG002)
497-498: try-except-pass detected, consider logging the exception
(S110)
497-497: Do not catch blind exception: Exception
(BLE001)
511-512: try-except-pass detected, consider logging the exception
(S110)
511-511: Do not catch blind exception: Exception
(BLE001)
517-517: Unused method argument: args
(ARG002)
534-534: Unused method argument: args
(ARG002)
563-563: Unused method argument: args
(ARG002)
598-598: Unused method argument: args
(ARG002)
610-611: try-except-pass detected, consider logging the exception
(S110)
610-610: Do not catch blind exception: Exception
(BLE001)
621-622: try-except-pass detected, consider logging the exception
(S110)
621-621: Do not catch blind exception: Exception
(BLE001)
626-626: Unused method argument: args
(ARG002)
646-646: Unused method argument: args
(ARG002)
688-688: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/base/models.py
864-864: Avoid specifying long messages outside the exception class
(TRY003)
867-869: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/api/views.py
191-191: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
456-456: Do not catch blind exception: Exception
(BLE001)
457-457: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
457-457: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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). (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (37)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (4)
19-19: LGTM!The import of
UpgradeCancelledis necessary for the new cancellation tests and correctly references the exception class defined inopenwisp_firmware_upgrader/exceptions.py.
874-905: LGTM!The test correctly verifies that
UpgradeCancelledis raised during early upgrade cancellation. The unused variables are now properly prefixed with_, addressing the previous review feedback.
907-939: LGTM!The test correctly verifies that non-critical services are restarted when an upgrade is cancelled. The unused
**_kwargsproperly addresses the previous lint feedback.Minor note: Line 926 manually sets
_non_critical_services_stopped = True, but_stop_non_critical_services()on line 925 already sets this flag internally. The redundant assignment is harmless but unnecessary.
941-969: LGTM!The test provides good coverage of the
_check_cancellationmethod behavior across different operation statuses:
in-progress: No exception (normal operation)cancelled: RaisesUpgradeCancelled(user-initiated cancellation)aborted: No exception (handled elsewhere in the flow)This aligns with the implementation in
openwisp_firmware_upgrader/upgraders/openwrt.pywhere only the"cancelled"status triggers the exception.tests/openwisp2/settings.py (3)
18-24: LGTM for conditional test DB override.This looks fine.
201-205: LGTM: targeted debug logging for websockets/channels.Looks good.
5-5: The project uses Django's built-in test runner exclusively (viaruntests.pywhich callsexecute_from_command_line). The"test" in sys.argvdetection is the correct and appropriate approach for this infrastructure. No pytest, tox, or alternative test runners are configured or used in this project, making the suggested enhancement unnecessary.Likely an incorrect or invalid review comment.
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (4)
4-11: Good asset wiring for the new form widgets.The added CSS/JS dependencies align with the new Select2 and schema UI usage.
17-24: Breadcrumbs update looks clean and consistent.
44-91: Upgrade Filters fieldset is well-structured.Clear labels/errors/help text make the new group/location inputs easy to scan.
118-126: Footer scripts and schema bootstrap look good.openwisp_firmware_upgrader/api/views.py (2)
184-192: Filterset expansion is a helpful API enhancement.
407-473: Cancel endpoint design looks solid.Clear 404/409/500 handling + consistent error response shape is a nice API UX.
openwisp_firmware_upgrader/websockets.py (6)
20-77: Base consumer utilities look good.Auth checks + lazy translation conversion are clean and reusable.
80-171: UpgradeProgressConsumer implementation is solid.
173-275: BatchUpgradeProgressConsumer flow is clear and consistent.
277-382: DeviceUpgradeProgressConsumer looks correct.
384-485: Publisher task retention is a good reliability improvement.
487-571: Batch publisher/status update path looks good.openwisp_firmware_upgrader/base/models.py (3)
526-589: Group/location fields + org validation look good.
730-802: Batch status aggregation is well-structured.
805-874: Progress tracking and cancellation flow look solid.openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (1)
24-31: Nice defensive guard for missingowFirmwareUpgraderApiHost.openwisp_firmware_upgrader/tests/test_websocket_selenium.py (7)
31-46: LGTM!The test class is well-structured with appropriate markers for async tests (
@pytest.mark.asyncio), database transactions (@pytest.mark.django_db(transaction=True)), and proper inheritance hierarchy for Selenium + Channels testing.
48-109: LGTM!Good use of unique suffixes to prevent test conflicts in parallel execution, and comprehensive fixture setup covering both single device and batch upgrade scenarios.
111-143: LGTM!The
_preparemethod correctly waits for the WebSocket connection to be established (readyState === 1) before proceeding, which is essential for test reliability.
234-314: LGTM!Good use of
WebDriverWaitto wait for the success status element (Lines 290-292) before making subsequent assertions. The test properly verifies both UI state and log content after status transition.
354-483: LGTM!Both tests properly use
WebDriverWaitto wait for DOM updates after WebSocket publishes. The aborted test (Lines 471-476) correctly implements an explicit wait for the log content before making assertions.
495-519: Catching broad exceptions is acceptable for these WebDriverWait helpers.The
_check_operation_progressand_check_row_countmethods are designed as predicates forWebDriverWait.until(), where returningFalseon any exception allows the wait to retry. This handlesStaleElementReferenceExceptionand other transient Selenium errors gracefully.
537-790: LGTM!The batch upgrade tests comprehensively cover various scenarios with proper explicit waits. The unused
driverparameter in the lambda functions (Lines 646, 656, 781) is a common pattern withWebDriverWaitand doesn't affect functionality.openwisp_firmware_upgrader/tests/test_websockets.py (7)
1-44: LGTM!The
tearDownmethod is now correctly defined as synchronous (Lines 43-44), which is compatible with Django'sTransactionTestCase. The imports and class structure are well-organized for async WebSocket testing.
46-119: LGTM!The helper methods are well-structured and provide clean abstractions for creating test fixtures and WebSocket communicators. Proper use of
sync_to_asyncfor database operations.
121-212: LGTM!Both tests provide good coverage of WebSocket message handling, verifying different message types (status, log, error, batch_status, operation_progress) flow correctly through the consumers.
287-338: LGTM!Good test coverage for current state request functionality and unknown message handling. The mock setup properly isolates the async operations being tested.
340-420: LGTM!Comprehensive testing of both publishers with proper verification of:
- Dual-channel publishing for device and operation channels
- Message structure including type, data, and timestamps
- Different publish methods (progress, operation_progress, batch_status)
422-639: LGTM!Excellent coverage of error handling scenarios including:
- Channel layer errors (ConnectionError, RuntimeError, AttributeError)
- Authentication edge cases (missing user, unauthenticated user)
- Multiple concurrent connections
- Message serialization with complex data structures
The
try-except-passpatterns are acceptable here since they handle expected connection rejection scenarios.
683-725: LGTM!The
test_no_duplicate_messages_on_log_line_with_batchtest is particularly valuable - it ensures that logging a line on an operation associated with a batch doesn't result in duplicate WebSocket messages, which is important for efficiency and correctness.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const canCancel = progressPercentage < 60; | ||
| const cancelButtonClass = canCancel | ||
| ? "upgrade-cancel-btn" | ||
| : "upgrade-cancel-btn disabled"; | ||
| const cancelButtonTitle = canCancel | ||
| ? gettext("Cancel upgrade") | ||
| : gettext("Cannot cancel - firmware flashing in progress"); |
There was a problem hiding this comment.
Align cancel threshold with backend rule.
UI disables cancel at 60%, while backend allows cancellation until 70%. This creates a mismatch and blocks valid cancels.
🔧 Suggested fix
- const canCancel = progressPercentage < 60;
+ const canCancel = progressPercentage < 70;- const canCancel = progressPercentage < 60;
+ const canCancel = progressPercentage < 70;Also applies to: 529-535
🤖 Prompt for AI Agents
In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js`
around lines 339 - 345, The UI uses a 60% threshold for canceling upgrades which
conflicts with the backend's 70% rule; update the check that computes canCancel
(and the related cancelButtonClass and cancelButtonTitle logic) to use 70
instead of 60 wherever it appears (including the duplicate block around the
other instance at lines 529-535) so the frontend allows cancellation up to the
backend limit; keep progressPercentage as the input and only change the numeric
threshold to 70.
There was a problem hiding this comment.
@nemesifier, no, this has not been addressed yet. The code still uses the 60% threshold instead of the recommended 70%:
Line 339:
const canCancel = progressPercentage < 60;This mismatch means users won't be able to cancel upgrades between 60-70% progress through the UI, even though the backend would allow it. The threshold should be updated to < 70 to align with the backend's cancellation rule (also needs to be applied at the second occurrence around line 529-535).
🧠 Learnings used
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.css:11-13
Timestamp: 2026-02-24T00:04:04.187Z
Learning: In the openwisp-firmware-upgrader project, hiding empty submit rows with CSS (e.g., `#upgradeoperation_form .submit-row { display: none; }`) is preferred over removing them in templates for simplicity and speed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_firmware_upgrader/api/views.py (1)
376-459: Map cancel endpoint POST to "change" permission (not "add").Cancelling an operation is semantically a state change, but DRF's default
DjangoModelPermissionsmaps POST requests to theaddpermission. SinceUpgradeOperationCancelViewdoesn't override permission mapping, users would needadd_upgradeoperationpermission instead of the semantically correctchange_upgradeoperation. Add a custom permission class with the corrected perms_map:Fix
+class UpgradeOperationCancelPermission(DjangoModelPermissions): + perms_map = { + **DjangoModelPermissions.perms_map, + "POST": ["%(app_label)s.change_%(model_name)s"], + } + class UpgradeOperationCancelView(ProtectedAPIMixin, generics.GenericAPIView): + permission_classes = [UpgradeOperationCancelPermission] queryset = UpgradeOperation.objects.all() serializer_class = serializers.Serializer
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/tests/test_api.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T18:09:17.215Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:17.215Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
Applied to files:
openwisp_firmware_upgrader/tests/test_api.py
🧬 Code graph analysis (2)
openwisp_firmware_upgrader/api/serializers.py (3)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)tests/openwisp2/sample_firmware_upgrader/models.py (5)
Meta(16-17)Meta(21-22)Meta(26-27)BatchUpgradeOperation(40-42)UpgradeOperation(45-47)openwisp_firmware_upgrader/models.py (2)
BatchUpgradeOperation(37-40)UpgradeOperation(43-46)
openwisp_firmware_upgrader/api/views.py (3)
openwisp_firmware_upgrader/api/serializers.py (1)
BatchUpgradeSerializer(86-95)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/base/models.py (1)
cancel(860-874)
🪛 GitHub Check: CodeQL
openwisp_firmware_upgrader/api/views.py
[warning] 442-442: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.14)
openwisp_firmware_upgrader/api/serializers.py
92-95: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/api/views.py
92-92: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
95-95: Unused method argument: pk
(ARG002)
160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
425-425: Do not catch blind exception: Exception
(BLE001)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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). (12)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | 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
🔇 Additional comments (10)
openwisp_firmware_upgrader/api/serializers.py (4)
18-20: LGTM: swapped DeviceGroup/Location bindings are in place.
86-95: LGTM: batch upgrade input fields are cleanly modeled.
98-116: LGTM: progress is now exposed for upgrade operations.
127-133: LGTM: cancelled_rate is surfaced for batch reporting.openwisp_firmware_upgrader/api/views.py (3)
1-45: LGTM: module wiring (logger + swapper bindings) looks good.
153-162: LGTM: created filter added for batch list view.
95-129: Org-scoping validation is already enforced.The
clean()method inBatchUpgradeOperation(lines 561–590) explicitly validates that bothgroupandlocationorganizations match the build category's organization. Thebatch_upgrade()method callsfull_clean()before saving, triggering this validation. Additionally,FilterSerializerByOrgManagedin the serializer filters related field querysets by organization context, and_find_firmwareless_devices()filters by organization when the build category has an organization. Cross-org IDs cannot slip through.Likely an incorrect or invalid review comment.
openwisp_firmware_upgrader/tests/test_api.py (3)
2-41: LGTM: new test dependencies wired for group/location/cancel coverage.
323-696: LGTM: solid coverage for group/location filtering and edge cases.
2114-2165: LGTM: cancel endpoint tests cover success, conflict, and not‑found paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
This is a good catch:
openwisp_firmware_upgrader/api/views.py (1)
376-459: Map cancel endpoint POST to "change" permission (not "add").
Cancelling an operation is semantically a state change, but DRF's default DjangoModelPermissions maps POST requests to the add permission. Since UpgradeOperationCancelView doesn't override permission mapping, users would need add_upgradeoperation permission instead of the semantically correct change_upgradeoperation. Add a custom permission class with the corrected perms_map:
Fix:
+class UpgradeOperationCancelPermission(DjangoModelPermissions):
+ perms_map = {
+ **DjangoModelPermissions.perms_map,
+ "POST": ["%(app_label)s.change_%(model_name)s"],
+ }
+
class UpgradeOperationCancelView(ProtectedAPIMixin, generics.GenericAPIView):
+ permission_classes = [UpgradeOperationCancelPermission]
queryset = UpgradeOperation.objects.all()
serializer_class = serializers.Serializer
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_firmware_upgrader/base/models.py (1)
590-619:⚠️ Potential issue | 🟠 MajorThe
update()method doesn't account for the "cancelled" status, creating inconsistent behavior withcalculate_and_update_status().When an operation status changes to "cancelled", calling
update()will incorrectly set the batch status to "success" instead of "cancelled" (since it only checks for "failed" operations, then defaults to "success"). The newcalculate_and_update_status()method correctly handles this case. Both methods are called from different parts of the codebase (update()at line 993,calculate_and_update_status()in websockets), creating divergent status logic. Refactorupdate()to delegate tocalculate_and_update_status()or remove it if no longer needed.
🤖 Fix all issues with AI agents
In `@openwisp_firmware_upgrader/api/views.py`:
- Around line 436-441: The broad except block should log the traceback with lazy
formatting and avoid masking bugs; replace the logger.error f-string with
logger.exception("Failed to cancel upgrade operation %s", pk) so the exception
traceback is recorded, and consider narrowing the caught exception or re-raising
in non-production (keep the existing return self._error_response on handled
errors); update the except in the view code that calls self._error_response for
canceling upgrade operations (uses pk) accordingly.
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 784-797: The status precedence is wrong in
calculate_and_update_status: cancelled currently takes precedence over
failed/aborted; change the conditional order so that any failures or aborts set
new_status = "failed" before checking for cancellations (i.e., check
stats["failed"] > 0 or stats["aborted"] > 0 prior to stats["cancelled"] > 0),
leaving the rest of the logic (in-progress, success, else keep self.status)
unchanged.
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 307-337: The per-operation progress bar never shows percentage
because showPercentageText is initialized false and never changed in
updateBatchStatusWithProgressBar; update it the same way as updateBatchProgress:
set showPercentageText = true when status is in FW_STATUS_GROUPS.IN_PROGRESS and
when status is in FW_STATUS_GROUPS.SUCCESS (or when you set statusClass =
FW_UPGRADE_CSS_CLASSES.SUCCESS), and leave it false for failed/aborted/cancelled
— alternatively, if hiding percentages on per-operation bars was intentional,
add a short comment above showPercentageText explaining that decision; modify
inside updateBatchStatusWithProgressBar before calling renderProgressBarHtml.
- Line 274: The code hardcodes "/admin/" when building deviceUrl; instead expose
the admin base path from the Django template into JS (e.g., add a template
variable like owAdminBaseUrl populated from {% url "admin:index" %} and
truncated to the base path) and use that variable when constructing the URL used
by deviceUrl in batch-upgrade-progress.js; update the deviceUrl construction to
concatenate owAdminBaseUrl with
"firmware_upgrader/upgradeoperation/{data.operation_id}/change/" so the admin
mount point is respected (ensure owAdminBaseUrl ends with a slash or handle
joining accordingly).
In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js`:
- Around line 273-278: updateUpgradeOperationDisplay is vulnerable because
operation.id is interpolated into a jQuery attribute selector
(operationIdInputField) which can break or be exploited if the id contains
special chars; fix by escaping the selector with $.escapeSelector when building
the selector or replace the attribute selector lookup with a safe filter-based
lookup that compares input.value to operation.id (e.g., query all relevant
inputs and .filter to match value), ensuring you update the use of
operationIdInputField accordingly.
- Around line 507-586: The function updateSingleOperationStatusWithProgressBar
injects unescaped values (progressClass, status, progressPercentage,
cancelButtonClass, cancelButtonTitle, operation.id and the status used in the
failure branch) into template literals — replicate the fix used in
updateStatusWithProgressBar by wrapping all interpolated user-controllable
values with escapeHtml(...) before building statusHtml; update every occurrence
where those symbols are interpolated (including the cancel button attributes and
the failure branch fill class) so statusContainer.html(...) receives only
escaped content.
- Around line 399-451: The frontend is appending incoming logData.content to
every visible operation because updateUpgradeOperationLog iterates all
operations and doesn't check which operation the message belongs to; modify the
backend to include an operation identifier (e.g., operation_id) in the published
payload ({"type":"log","operation_id":id,"content":..., "status":...}) and then
change updateUpgradeOperationLog to read logData.operation_id and only update
the operation whose operationId (derived from
operationFieldset.find("input[name*='id'][value]") / variable operationId)
matches that operation_id; keep existing accumulatedLogContent and
updateStatusWithProgressBar logic for the matched operation only, or
alternatively publish logs to per-operation channels and filter by channel
instead.
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 118-127: Add a missing semicolon to the inline script that assigns
the global variable firmwareUpgraderSchema: update the assignment statement for
firmwareUpgraderSchema in the template (the inline <script> that sets var
firmwareUpgraderSchema = {{ firmware_upgrader_schema | safe }}) to end with a
semicolon so the line becomes a proper JS statement and avoids issues during
concatenation/minification.
In `@openwisp_firmware_upgrader/tests/test_websocket_selenium.py`:
- Around line 420-431: The current assertions read the log immediately after
detecting the failed progress fill and can race DOM updates; change the direct
checks after progress_fill = self.find_element(By.CSS_SELECTOR,
".upgrade-progress-fill.failed") to use a WebDriverWait that polls the log
element (e.g. locate the element via self.find_element(By.CSS_SELECTOR,
".field-log .readonly") or a locator variable like log_element) and waits until
its get_attribute("innerHTML") contains the expected substrings ("Image check
failed", "aborting upgrade", "operation failed", "Starting non critical
services"), mirroring the wait pattern used in
test_real_time_status_change_to_aborted so the assertions only run once the log
DOM has been updated.
- Around line 305-323: The log assertions after finding
".upgrade-status-success" race because they access the log immediately; update
the test to wait explicitly for the log content before asserting: use
WebDriverWait (the same pattern used earlier in the test) to wait for the
".field-log .readonly" element to be visible and/or for its innerHTML/text to
contain the expected substrings ("Connection successful", "Device identity
verified", "Sysupgrade test passed", "completed successfully") before calling
find_element, get_attribute("innerHTML") and the assertIn checks; adjust the
test method where those assertions are made to replace the immediate accesses
with WebDriverWait-based waits targeting the ".field-log .readonly" element and
the expected text.
🧹 Nitpick comments (15)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (2)
111-113:_snoozeis defined but never called.This helper is unused anywhere in the file. Consider removing it to avoid dead code.
508-532: Consider narrowing the broadexcept Exceptionclauses.These helpers are used as WebDriverWait predicates, so catching exceptions to return
Falseis a valid pattern. However, catching bareExceptioncan mask unexpected errors. Narrowing to Selenium-specific exceptions would improve debuggability.Suggested narrowing
+from selenium.common.exceptions import ( + NoSuchElementException, + StaleElementReferenceException, +) + def _check_operation_progress(self, status_class, progress_width): ... - except Exception: + except (NoSuchElementException, StaleElementReferenceException): return False def _check_row_count(self, expected_count): ... - except Exception: + except (NoSuchElementException, StaleElementReferenceException): return Falseopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (3)
252-255: Use.text()instead of.html()for the modified timestamp to stay consistent with the XSS-safe approach.Line 254 uses
.html(getFormattedDateTimeString(data.modified)). While the date formatter likely returns safe content, using.text()would be consistent with the safe-DOM-construction approach used elsewhere in this file (e.g., lines 294–295).If the formatted string contains HTML elements (e.g.,
<time>tags), then.html()is intentional — but please confirm.
253-253: Fragile column selectortd:nth-child(4)— will break if columns are reordered or added.Consider using a class or
data-*attribute on the modified-time cell, similar to how.status-cellis used for the status column.
188-198: Inconsistent HTML construction: template literals here vs. safe DOM methods inaddNewOperationRow.
updateBatchProgressandinitializeMainProgressBarbuild progress bar HTML via template literals (lines 190–196, 388–394), whileaddNewOperationRowuses jQuery DOM construction to prevent XSS. Although the interpolated values here (statusClass,progressPercentage) are derived from safe constants/computed numbers, the inconsistency could lead to vulnerabilities if future changes introduce user-controlled values.Consider extracting a shared DOM-based builder or consistently using
renderProgressBarHtml(which you already call inupdateBatchStatusWithProgressBar).openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
44-92: Mismatched<div>/{% endif %}nesting could confuse future maintainers.The
<div>on line 46 is closed on line 92, but the{% if %}block on line 49 doesn't close until line 107. This means the submit buttons (lines 94–106) are inside the{% if %}but outside the<div>, which produces valid HTML but makes the template structure hard to follow. Consider aligning the</div>with the{% endif %}or restructuring so the conditional and the element boundaries match.openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (4)
311-387: Significant code duplication betweenupdateStatusWithProgressBarandupdateSingleOperationStatusWithProgressBar.These two functions share ~90% of their logic (progress bar rendering, cancel button, status branching). The only difference is the failure-status branching in the single-operation version (which splits
cancelledout separately). This duplication already led to the escapeHtml inconsistency above and will make future changes error-prone.Consider extracting the shared rendering into a single helper, parameterized by any differing behavior.
Also applies to: 507-586
752-791: Hardcoded API URL path.The cancel endpoint URL on line 759 is hardcoded. If the API URL prefix is configured differently (e.g., via
OPENWISP_API_PREFIXor reverse URL resolution), this will break.Consider deriving the URL from a server-rendered variable or a shared config, similar to how
owFirmwareUpgraderApiHostis used for the WebSocket host.
58-68:escapeHtmlreturns the original value unchanged for non-string inputs.If
unsafeis a number or object, it passes through unescaped. SinceescapeHtmlis used as a security boundary (e.g., forprogressPercentagewhich is a number), consider coercing to string first so callers don't need to worry about type.📝 Proposed fix
function escapeHtml(unsafe) { - if (typeof unsafe !== "string") { - return unsafe; - } + if (unsafe === null || unsafe === undefined) { + return ""; + } + unsafe = String(unsafe); return unsafe .replace(/&/g, "&")
673-685:getObjectIdFromUrlregex is fragile.The UUID regex
/((\w{4,12}-?)){5}/is a loose approximation that could match non-UUID strings. Additionally, the integer fallback regex/\/(\d+)\//will match the first numeric path segment, which might not be the object ID. Both patterns silently return incorrect IDs if the URL structure changes.This is low-risk since the URL format is controlled by Django admin, but worth noting for robustness.
openwisp_firmware_upgrader/api/views.py (1)
424-431: CatchingHttp404fromget_object()is redundant with DRF's default exception handling.DRF's exception handler already converts
Http404into a404JSON response. The manual catch here only customizes the response body shape ({"error": "..."}instead of{"detail": "Not found."}). If consistent error format is intentional, this is fine — just worth noting the trade-off of duplicating DRF's built-in behavior.openwisp_firmware_upgrader/tests/test_api.py (3)
489-526: Assertions are too weak — they won't catch filtering regressions.The
assertTrue(len(...) > 0 or len(...) > 0)check (lines 496, 515) and conditional assertions (if device_fw_ids:,if device_ids:) mean the test passes even if the endpoint returns zero results for one category or returns devices from the wrong group. Use strict assertions on exact counts and expected device identities.For example, the GET-with-group subtest should assert that only
d1andDevice3appear (via device_firmwares and devices respectively), and thatd2does not appear:Sketch of tighter assertions
with self.subTest("Test GET with group filter"): r = self.client.get(url, {"group": group1.pk}) self.assertEqual(r.status_code, 200) - device_ids = r.data["devices"] - device_fw_ids = r.data["device_firmwares"] - - self.assertTrue(len(device_ids) > 0 or len(device_fw_ids) > 0) - - if device_fw_ids: - device_fws = DeviceFirmware.objects.filter(pk__in=device_fw_ids) - device_fw_names = [df.device.name for df in device_fws] - self.assertIn("device1", device_fw_names) - - if device_ids: - devices = Device.objects.filter(pk__in=device_ids) - device_names = [d.name for d in devices] - self.assertIn("Device3", device_names) + self.assertEqual(len(r.data["device_firmwares"]), 1) + self.assertEqual(len(r.data["devices"]), 1) + self.assertIn(str(env["device_fw1"].pk), r.data["device_firmwares"]) + self.assertIn(str(device3.pk), r.data["devices"])
634-637: GET with combined filters only asserts status 200 — verify response content too.This subtest doesn't check whether the returned
device_firmwaresanddeviceslists actually contain only the expecteddevice_both. Without content assertions, a bug that returns all devices (or none) would go undetected.
2114-2164: Cancel endpoint tests lack authorization and additional edge-case coverage.The current tests cover the happy path, progress-too-far, and not-found cases. Consider adding tests for:
- Non-cancellable status — e.g., cancelling an already "success" or "failed" operation (should return 409).
- Authorization — a user from a different organization or a non-manager attempting to cancel (should return 403).
- Already cancelled — cancelling the same operation twice.
These gaps could let permission bypasses or incorrect 500s go undetected.
openwisp_firmware_upgrader/base/models.py (1)
860-874: Barereturnat end ofcancel()is unnecessary.Line 874 has an explicit
returnwith no value at the end of the method. Python methods implicitly returnNonewhen execution reaches the end.Cleanup
self.log_line("Upgrade operation has been cancelled by user", save=False) self.status = "cancelled" self.save() - return
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/developer/utils.rstopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.jsopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_websocket_selenium.py
✅ Files skipped from review due to trivial changes (1)
- docs/developer/utils.rst
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T18:09:27.769Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
Applied to files:
openwisp_firmware_upgrader/tests/test_api.py
🧬 Code graph analysis (4)
openwisp_firmware_upgrader/tests/test_api.py (6)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/models.py (3)
UpgradeOperation(43-46)BatchUpgradeOperation(37-40)DeviceFirmware(31-34)openwisp_firmware_upgrader/tests/base.py (2)
_create_upgrade_env(121-186)_create_device_group(206-215)openwisp_firmware_upgrader/base/models.py (5)
save(421-430)save(988-993)boards(277-278)upgrade(601-606)upgrade(885-986)openwisp_firmware_upgrader/api/views.py (3)
post(95-115)post(424-449)get(117-134)openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (5)
data(110-110)operation(63-67)operation(246-250)operation(299-303)status(309-309)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (2)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (5)
ALL_VALID_FW_STATUSES(35-38)FW_UPGRADE_CSS_CLASSES(22-30)FW_UPGRADE_STATUS(4-11)FW_STATUS_GROUPS(41-68)FW_UPGRADE_DISPLAY_STATUS(14-19)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrader-utils.js (5)
ALL_VALID_FW_STATUSES(35-38)FW_UPGRADE_CSS_CLASSES(22-30)FW_UPGRADE_STATUS(4-11)FW_STATUS_GROUPS(41-68)FW_UPGRADE_DISPLAY_STATUS(14-19)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (2)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
status(309-309)openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (5)
FW_STATUS_HELPERS(70-77)ALL_VALID_FW_STATUSES(35-38)FW_STATUS_GROUPS(41-68)FW_UPGRADE_STATUS(4-11)protocol(119-119)
openwisp_firmware_upgrader/api/views.py (4)
openwisp_firmware_upgrader/api/serializers.py (1)
BatchUpgradeSerializer(86-95)openwisp_firmware_upgrader/models.py (2)
BatchUpgradeOperation(37-40)UpgradeOperation(43-46)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/base/models.py (3)
batch_upgrade(161-191)dry_run(609-619)cancel(860-874)
🪛 GitHub Check: CodeQL
openwisp_firmware_upgrader/api/views.py
[warning] 453-453: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.15.0)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py
[warning] 522-522: Consider moving this statement to an else block
(TRY300)
[warning] 523-523: Do not catch blind exception: Exception
(BLE001)
[warning] 531-531: Do not catch blind exception: Exception
(BLE001)
[warning] 659-659: Unused lambda argument: driver
(ARG005)
[warning] 669-669: Unused lambda argument: driver
(ARG005)
[warning] 794-794: Unused lambda argument: driver
(ARG005)
openwisp_firmware_upgrader/base/models.py
[warning] 864-864: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 867-869: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/api/views.py
[warning] 92-92: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 95-95: Unused method argument: pk
(ARG002)
[warning] 160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 377-380: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 436-436: Do not catch blind exception: Exception
(BLE001)
[warning] 437-437: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 437-437: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (16)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (2)
145-243: LGTM!Good use of
WebDriverWaitfor all assertions after the WebSocket publish. The explicit waits for progress text, progress fill style, and log content are properly structured.
550-803: Batch test methods look well-structured.Good use of helper methods (
_check_progress_text,_check_operation_progress,_check_row_count) as WebDriverWait predicates, and proper waits for WebSocket-driven DOM updates throughout.Note: The Ruff warnings about unused
driverlambda arguments (lines 659, 669, 794) are false positives —WebDriverWait.until()requires a callable that accepts the driver instance.openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (3)
1-23: Initialization and WebSocket setup look solid.Clean extraction of the batch ID with early return, proper WebSocket configuration with reconnection parameters, and appropriate exposure on
windowfor cross-script coordination.
82-140: WebSocket lifecycle handling is well-structured.Good separation of message types, proper try/catch around JSON parsing, and the re-initialization logic on reconnect is sensible.
263-305: XSS-safe DOM construction inaddNewOperationRow— nice work.Using jQuery's
.text()and.attr()for user-supplied data (device_name,operation_id, etc.) is the right approach.openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
4-11: LGTM on the extrahead block.CSS and JS assets are properly loaded with appropriate static paths, and the form media is included.
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (3)
25-31: LGTM on defensive check forowFirmwareUpgraderApiHost.The guard clause properly prevents runtime errors when the global is undefined, addressing a previously flagged concern.
Also applies to: 35-44
54-56: LGTM —formatLogForDisplaynow escapes before injecting HTML.Properly uses
escapeHtmlbefore replacing newlines with<br>, addressing the previously flagged XSS concern.
339-339: Backend and UI cancel thresholds are correctly aligned at 65%.The backend constant
_MAX_CANCELLABLE_PROGRESS = 65(base/models.py:808) and the cancel checkif self.progress >= self._MAX_CANCELLABLE_PROGRESSmatch the UI logicprogressPercentage < 65. Both systems disallow cancellation once progress reaches 65%, which marks the start of firmware reflashing (openwrt.py:447).Likely an incorrect or invalid review comment.
openwisp_firmware_upgrader/api/views.py (1)
100-115: Good use of serializer-based validation for group/location inputs.Using
BatchUpgradeSerializerwithis_valid(raise_exception=True)properly rejects invalid/non-existent group and location IDs with a 400 before proceeding, which addresses the previous concern about silently ignoring invalid IDs.openwisp_firmware_upgrader/tests/test_api.py (1)
323-334: LGTM on the updated batch upgrade test.The test now uses the env dict pattern consistently and updated query count reflects the new serializer validation path.
openwisp_firmware_upgrader/base/models.py (5)
528-541: Group and location fields look well-structured.
SET_NULLwithblank=True, null=Trueis the correct choice for optional FK filters — the batch operation remains valid even if the group/location is later deleted.
561-588: Organization validation inclean()is correctly guarded for shared builds.The conditions properly skip validation when
build.category.organizationisNone(shared builds), which avoids false negatives for cross-org comparisons.
730-802:calculate_and_update_statusis well-structured — single-query aggregate is efficient.The use of conditional
Countaggregations avoids N+1 queries and correctly handles all status transitions in one pass.
952-954: No changes needed. TheUpgradeCancelledexception handler is implemented correctly.The upgrader's
_check_cancellation()method callsrefresh_from_db()on the operation instance before raisingUpgradeCancelled, ensuring that the in-memory operation is synced with the database status. This means when the exception is caught andself.save()is called, the status is already "cancelled" and will be saved correctly.Likely an incorrect or invalid review comment.
843-853: This signal is a public API for optional external consumers, not an internal mechanism.
firmware_upgrader_log_updatedis documented as a signal for external subscribers (e.g., third-party extensions) to perform custom actions on log updates. There are no internal consumers of this signal in the codebase—progress updates are published viapost_savesignal handlers inUpgradeProgressPublisher, which read from the instance directly, not the database. The signal design is correct: consumers receive both theinstanceand thelineargument, so they can safely use the passedlinetext without re-querying the database.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| let rowClass = existingRows % 2 === 0 ? "row1" : "row2"; | ||
| tbody.find("tr td[colspan]").parent().remove(); | ||
|
|
||
| let deviceUrl = `/admin/firmware_upgrader/upgradeoperation/${data.operation_id}/change/`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the exact code at the specified location
cd openwisp_firmware_upgrader/static/firmware-upgrader/js && head -n 280 batch-upgrade-progress.js | tail -n 20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 735
🏁 Script executed:
# Check if the file exists and get context around line 274
wc -l openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsRepository: openwisp/openwisp-firmware-upgrader
Length of output: 163
🏁 Script executed:
# Search for hardcoded /admin/ paths in JS files
rg -n '/admin/' --type=js -g '!node_modules' -g '!*.min.js' | head -50Repository: openwisp/openwisp-firmware-upgrader
Length of output: 255
🏁 Script executed:
# Search for owFirmwareUpgraderApiHost pattern to understand how JS variables are passed from Django
rg -n 'owFirmwareUpgraderApiHost\|window\.' --type=js -g '!node_modules' -g '!*.min.js' | head -30Repository: openwisp/openwisp-firmware-upgrader
Length of output: 61
🏁 Script executed:
# Look for Django template files that might set JS variables
fd -e html -e js | xargs grep -l 'owFirmwareUpgraderApiHost\|script' | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 599
🏁 Script executed:
# Check if there are any Django settings or configuration files related to admin URL
find . -name '*.py' -type f | xargs grep -l 'ADMIN_SITE_URL\|admin.*url\|AdminSite' | head -20Repository: openwisp/openwisp-firmware-upgrader
Length of output: 104
🏁 Script executed:
# Check the related template file that likely includes this JS file
cat openwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.htmlRepository: openwisp/openwisp-firmware-upgrader
Length of output: 7087
🏁 Script executed:
# Look at the upgrade_operation_change_form.html to see how variables are passed
cat openwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.htmlRepository: openwisp/openwisp-firmware-upgrader
Length of output: 834
🏁 Script executed:
# Check if there are any existing patterns for passing admin URL or base paths to JS
rg -n 'window\.|script.*var\|djangoAdmin\|adminUrl' --type=html --type=js | head -30Repository: openwisp/openwisp-firmware-upgrader
Length of output: 3963
🏁 Script executed:
# Check the test URL configuration to understand if admin URL is customizable
cat tests/openwisp2/urls.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 1146
🏁 Script executed:
# Search for Django AdminSite configuration in the project
find . -name '*.py' -type f | xargs grep -l 'AdminSite\|admin.site' | head -10Repository: openwisp/openwisp-firmware-upgrader
Length of output: 191
Pass the admin URL base path from Django template to JavaScript.
The hardcoded /admin/ URL at line 274 breaks if Django's admin site is mounted elsewhere (e.g., via custom AdminSite configuration). The project already establishes a pattern for passing template variables to JS (see owFirmwareUpgraderApiHost in batch_upgrade_operation_change_form.html). Add a similar variable to pass the admin base path:
var owAdminBaseUrl = '{% url "admin:index" %}' | truncate path to base;Then use it in the code:
let deviceUrl = `${owAdminBaseUrl}firmware_upgrader/upgradeoperation/${data.operation_id}/change/`;🤖 Prompt for AI Agents
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`
at line 274, The code hardcodes "/admin/" when building deviceUrl; instead
expose the admin base path from the Django template into JS (e.g., add a
template variable like owAdminBaseUrl populated from {% url "admin:index" %} and
truncated to the base path) and use that variable when constructing the URL used
by deviceUrl in batch-upgrade-progress.js; update the deviceUrl construction to
concatenate owAdminBaseUrl with
"firmware_upgrader/upgradeoperation/{data.operation_id}/change/" so the admin
mount point is respected (ensure owAdminBaseUrl ends with a slash or handle
joining accordingly).
There was a problem hiding this comment.
See openwisp/openwisp-controller#1234 (review), seems this is important for some users.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the referenced PR review comment to understand the context
gh api repos/openwisp/openwisp-controller/pulls/1234/reviews | jq -r '.[] | select(.id == 3843500683) | .body'
Length of output: 462
🏁 Script executed:
#!/bin/bash
# Also check if there are other hardcoded /admin/ paths in the JS files that need fixing
rg -n "'/admin/" openwisp_firmware_upgrader/static/firmware-upgrader/js/ --type js
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how the template currently passes variables to JS
cat openwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.html | grep -A 5 -B 5 "owFirmwareUpgraderApiHost"
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Check the exact content around line 274 in batch-upgrade-progress.js
sed -n '270,280p' openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Look for all occurrences of /admin/ in the JavaScript file (might be escaped or in strings)
grep -n "admin" openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Fetch the openwisp-controller examples to see how they handle admin URLs
curl -s "https://raw.githubusercontent.com/openwisp/openwisp-controller/master/openwisp_controller/config/templates/admin/config/change_form.html" | grep -A 10 -B 10 "admin"
Length of output: 274
🏁 Script executed:
#!/bin/bash
# Check what upgrade_operation_path is set to in the admin view
rg -n "upgrade_operation_path" openwisp_firmware_upgrader/admin.py -A 3 -B 3
Length of output: 364
🏁 Script executed:
#!/bin/bash
# Search for any hardcoded /admin/ in the JavaScript files
rg -n '"/admin/' openwisp_firmware_upgrader/static/firmware-upgrader/js/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the full file to see if there's a hardcoded path elsewhere
cat openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js | grep -n "/admin/"
Length of output: 0
@nemesifier Good news! The code in this PR already handles custom admin URLs correctly, following the same pattern you referenced from openwisp-controller.
Here's how it works:
-
In the Django template (
batch_upgrade_operation_change_form.html):var owDeviceUpgradeOperationUrl = "{{ upgrade_operation_path }}";
-
In the admin view (
admin.pylines 280-282):"upgrade_operation_path": reverse( f"admin:{app_label}_upgradeoperation_change", args=["00000000-0000-0000-0000-000000000000"], ),
-
In the JavaScript (line 271-274):
let deviceUrl = owDeviceUpgradeOperationUrl.replace( "00000000-0000-0000-0000-000000000000", data.operation_id, );
This approach uses Django's reverse() function to generate the URL, which respects custom admin mount points (just like the openwisp-controller examples you linked). The placeholder UUID is then replaced with the actual operation ID in JavaScript.
There is no hardcoded /admin/ path in the current code. CodeRabbit's original flag appears to be incorrect. The implementation is already correct and will work with custom AdminSite configurations! ✅
🧠 Learnings used
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.css:11-13
Timestamp: 2026-02-24T00:04:04.187Z
Learning: In the openwisp-firmware-upgrader project, hiding empty submit rows with CSS (e.g., `#upgradeoperation_form .submit-row { display: none; }`) is preferred over removing them in templates for simplicity and speed.
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:34.134Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
| # Validate cancellation conditions | ||
| if self.status != self._CANCELLABLE_STATUS: | ||
| raise ValueError(f"Cannot cancel operation with status: {self.status}") | ||
|
|
| @@ -0,0 +1,159 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
This file looks like a duplicate of upgrade-utils.js.
| <script> | ||
| var firmwareUpgraderSchema = {{ inline_admin_formset.formset.extra_context | default:"null" | safe }} | ||
| var owFirmwareUpgraderApiHost = { | ||
| host: '{{ request.get_host }}' |
There was a problem hiding this comment.
| host: '{{ request.get_host }}' | |
| host: "{{ request.get_host }}" |
| <script> | ||
| // Configuration for WebSocket connection | ||
| var owFirmwareUpgraderApiHost = { | ||
| host: '{{ request.get_host }}' |
There was a problem hiding this comment.
| host: '{{ request.get_host }}' | |
| host: "{{ request.get_host }}" |
| "upgrader:api_upgradeoperation_cancel", | ||
| kwargs={"pk": operation.pk}, | ||
| ) | ||
|
|
There was a problem hiding this comment.
@pandafy let's get rid of the extra blank lines please
| ssh.exec_command = mocked_exec_upgrade_success | ||
|
|
||
| upgrader = OpenWrt(upgrade_op, device_conn) | ||
|
|
There was a problem hiding this comment.
same here and in other test files, too many needless blank lines contributing to the hugeness
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
|
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_firmware_upgrader/base/models.py (1)
860-992:⚠️ Potential issue | 🟠 MajorBatch status update bypasses new cancellation logic.
save()still callsbatch.update(), which doesn’t account for cancelled operations, so batch status can end up “success” even after cancellations. Prefer the newcalculate_and_update_status()here.✅ Suggested fix
- if self.batch and self.status != "in-progress": - self.batch.update() + if self.batch and self.status != "in-progress": + self.batch.calculate_and_update_status()openwisp_firmware_upgrader/admin.py (1)
209-281:⚠️ Potential issue | 🟡 MinorDry-run in confirmation view ignores selected filters after validation errors.
If the user submits group/location and hits an error (e.g., no devices), the confirmation page is rebuilt using an unfiltered dry-run, which can be confusing. Consider using the form’s cleaned values (when available) for the dry-run context.🧩 Suggested tweak
- result = BatchUpgradeOperation.dry_run(build=build) + group = form.cleaned_data.get("group") if not form.errors else None + location = form.cleaned_data.get("location") if not form.errors else None + result = BatchUpgradeOperation.dry_run( + build=build, group=group, location=location + )
🧹 Nitpick comments (8)
openwisp_firmware_upgrader/tests/test_selenium.py (3)
61-70: Consider extracting_assert_no_js_errorsto a shared test mixin.This helper method is duplicated in
test_websocket_selenium.py(lines 109-118). Extract it to a shared base class or mixin to maintain DRY principles.
347-354: Prefix unused unpacked variables with underscores.Static analysis flags unused variables from
_set_up_env(). Prefix them with_to indicate intentional non-use.🔧 Suggested fix
- org, category, build1, build2, image1, image2, device = self._set_up_env() + _org, _category, _build1, _build2, _image1, image2, device = self._set_up_env()
455-474: Prefix unused method argument with underscore.Static analysis flags
argsas unused. Prefix with_to indicate intentional non-use for the mock decorators.🔧 Suggested fix
- def test_upgrade_operation_admin_no_submit_row(self, *args): + def test_upgrade_operation_admin_no_submit_row(self, *_args):openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
302-317: Clarify design intent for per-operation progress bars not showing percentage text.
showPercentageTextis alwaysfalsefor individual operation progress bars (line 315), unlike the main batch progress bar which shows percentage text for in-progress and success states. If this is intentional (to keep per-operation rows compact), a brief comment would help future maintainers understand this design decision.📝 Suggested comment
function renderOperationProgressBarInCell(statusCell, operation) { // Renders a visual progress bar in the given status cell based on the // operation's status and progress. let $ = django.jQuery; let status = operation.status; let progressPercentage = normalizeProgress(operation.progress, status); statusCell.empty(); statusCell.append('<div class="upgrade-status-container"></div>'); let statusContainer = statusCell.find(".upgrade-status-container"); let statusClass = STATUS_TO_CSS_CLASS[status] || ""; if (STATUSES_WITH_FULL_PROGRESS.has(status)) { progressPercentage = 100; } + // Per-operation bars don't show percentage text to keep table rows compact let progressHtml = renderProgressBarHtml(progressPercentage, statusClass, false); statusContainer.html(progressHtml); }openwisp_firmware_upgrader/websockets.py (1)
104-118: Consider usinglogger.exceptionin exception handlers for automatic stack traces.Within
exceptblocks,logger.exception()automatically includes the stack trace, which aids debugging. Currently usinglogger.error()with manual exception interpolation loses this benefit.🔧 Example fix (apply similar pattern to other except blocks)
except (AssertionError, KeyError) as e: - logger.error(f"Error in operation websocket connect: {e}") + logger.exception("Error in operation websocket connect") await self.close()This pattern applies to multiple locations flagged by static analysis (lines 105, 111, 115-117, 126-128, 165-167, 195, 201, 205-207, 267-269, 271, 299, 305, 309-311, 371-373, 375).
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (1)
453-477: Consider catching specific Selenium exceptions instead of blindException.The helper methods
_check_operation_progressand_check_row_countcatch all exceptions. While this is pragmatic for test helpers, catching specific Selenium exceptions would be more precise.🔧 Suggested fix
+from selenium.common.exceptions import StaleElementReferenceException, NoSuchElementException + def _check_operation_progress(self, status_class, progress_width): """Helper method to safely check individual operation progress without stale element issues""" try: containers = self.find_elements( By.CSS_SELECTOR, "#result_list .upgrade-status-container" ) for container in containers: progress_fill = container.find_element( By.CSS_SELECTOR, ".upgrade-progress-fill" ) if status_class in progress_fill.get_attribute( "class" ) and progress_width in progress_fill.get_attribute("style"): return True return False - except Exception: + except (StaleElementReferenceException, NoSuchElementException): return False def _check_row_count(self, expected_count): """Helper method to safely check row count without stale element issues""" try: rows = self.find_elements(By.CSS_SELECTOR, "#result_list tbody tr") return len(rows) == expected_count - except Exception: + except (StaleElementReferenceException, NoSuchElementException): return Falseopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (2)
353-358: Escapestatusin failure branch progress bar class.In the failure branch,
statusis interpolated directly into the CSS class without escaping. While status values come from the backend and are likely controlled, for consistency with other branches, apply escaping.🔧 Suggested fix
} else if (FW_STATUS_GROUPS.FAILURE.has(status)) { statusHtml += ` <div class="upgrade-progress-bar"> - <div class="upgrade-progress-fill ${status}" style="width: 100%"></div> + <div class="upgrade-progress-fill ${escapeHtml(status)}" style="width: 100%"></div> </div> `;
743-783: Consider making the cancel API URL configurable.The cancel endpoint URL is hardcoded as
/api/v1/firmware-upgrader/upgrade-operation/${operationId}/cancel/. If the API prefix is customizable or the admin is mounted at a non-standard path, this could break. Consider passing the API base URL from Django template similar toowFirmwareUpgraderApiHost.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.jsopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.htmlopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.htmlopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.htmlopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_openwrt_upgrader.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_websocket_selenium.pyopenwisp_firmware_upgrader/upgraders/openwrt.pyopenwisp_firmware_upgrader/websockets.py
🚧 Files skipped from review as they are similar to previous changes (5)
- openwisp_firmware_upgrader/templates/admin/firmware_upgrader/device_firmware_inline.html
- openwisp_firmware_upgrader/templates/admin/firmware_upgrader/upgrade_operation_change_form.html
- openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html
- openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js
- openwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-29T18:09:27.769Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: docs/developer/installation.rst:123-130
Timestamp: 2026-01-29T18:09:27.769Z
Learning: In openwisp-firmware-upgrader, the `./runtests` script is a wrapper that runs tests for both openwisp_firmware_upgrader and SAMPLE_APP with coverage. To run only the sample app tests, developers should use `SAMPLE_APP=1 ./runtests.py --keepdb --failfast` directly, not the `./runtests` wrapper.
Applied to files:
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.pyopenwisp_firmware_upgrader/tests/test_api.py
🧬 Code graph analysis (7)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (2)
openwisp_firmware_upgrader/websockets.py (5)
BatchUpgradeProgressPublisher(481-566)UpgradeProgressPublisher(384-478)publish_operation_update(426-428)publish_batch_status(526-534)publish_operation_progress(505-524)openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (2)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/upgraders/openwrt.py (6)
connect(134-138)exec_command(146-150)_check_cancellation(211-221)upgrade(197-209)disconnect(140-144)_stop_non_critical_services(329-339)
openwisp_firmware_upgrader/upgraders/openwrt.py (2)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/base/models.py (6)
log_line(843-853)save(421-430)save(987-992)update_progress(855-858)upgrade(601-606)upgrade(884-985)
openwisp_firmware_upgrader/tests/test_selenium.py (2)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (1)
_assert_no_js_errors(110-119)openwisp_firmware_upgrader/tests/base.py (1)
_create_device_firmware(105-119)
openwisp_firmware_upgrader/websockets.py (3)
openwisp_firmware_upgrader/swapper.py (1)
load_model(7-8)openwisp_firmware_upgrader/api/serializers.py (2)
UpgradeOperationSerializer(98-110)DeviceUpgradeOperationSerializer(113-116)openwisp_firmware_upgrader/base/models.py (4)
upgrade_operations(657-658)total_operations(661-662)update(590-599)calculate_and_update_status(730-802)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (7)
FW_STATUS_HELPERS(70-77)FW_UPGRADE_CSS_CLASSES(22-30)FW_UPGRADE_STATUS(4-11)FW_STATUS_GROUPS(41-68)FW_UPGRADE_DISPLAY_STATUS(14-19)STATUS_TO_CSS_CLASS(80-86)STATUSES_WITH_FULL_PROGRESS(89-94)
openwisp_firmware_upgrader/base/models.py (2)
openwisp_firmware_upgrader/exceptions.py (1)
UpgradeCancelled(21-24)openwisp_firmware_upgrader/swapper.py (2)
load_model(7-8)get_model_name(11-12)
🪛 GitHub Check: CodeQL
openwisp_firmware_upgrader/api/views.py
[warning] 453-453: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.15.0)
openwisp_firmware_upgrader/tests/test_admin.py
[warning] 925-925: Unused method argument: args
(ARG002)
[warning] 969-969: Unused method argument: args
(ARG002)
[warning] 1018-1018: Unused method argument: args
(ARG002)
[warning] 1084-1084: Unused method argument: args
(ARG002)
[warning] 1119-1119: Unused method argument: args
(ARG002)
[warning] 1144-1144: Unused method argument: args
(ARG002)
[warning] 1161-1161: Unused method argument: args
(ARG002)
[warning] 1178-1178: Unused method argument: args
(ARG002)
[warning] 1198-1198: Unused method argument: args
(ARG002)
[warning] 1263-1263: Unused method argument: args
(ARG002)
[warning] 1280-1280: Unused method argument: args
(ARG002)
[warning] 1308-1308: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/tests/test_websocket_selenium.py
[warning] 467-467: Consider moving this statement to an else block
(TRY300)
[warning] 468-468: Do not catch blind exception: Exception
(BLE001)
[warning] 476-476: Do not catch blind exception: Exception
(BLE001)
[warning] 604-604: Unused lambda argument: driver
(ARG005)
[warning] 614-614: Unused lambda argument: driver
(ARG005)
[warning] 739-739: Unused lambda argument: driver
(ARG005)
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py
[warning] 47-47: Unused function argument: exit_codes
(ARG001)
[warning] 47-47: Unused function argument: timeout
(ARG001)
[warning] 47-47: Unused function argument: raise_unexpected_exit
(ARG001)
openwisp_firmware_upgrader/upgraders/openwrt.py
[warning] 221-221: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 226-226: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_firmware_upgrader/api/views.py
[warning] 92-92: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 95-95: Unused method argument: pk
(ARG002)
[warning] 160-160: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 377-380: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/tests/test_selenium.py
[warning] 347-347: Unpacked variable org is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 347-347: Unpacked variable category is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 347-347: Unpacked variable build1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 347-347: Unpacked variable build2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 347-347: Unpacked variable image1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
[warning] 463-463: Unused method argument: args
(ARG002)
openwisp_firmware_upgrader/admin.py
[warning] 153-157: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 158-165: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[error] 244-244: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
[warning] 308-308: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 331-331: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 354-354: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 355-355: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 356-356: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 357-357: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 358-358: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 359-366: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 393-393: Unused method argument: request
(ARG002)
[warning] 396-396: Unused method argument: request
(ARG002)
[warning] 396-396: Unused method argument: obj
(ARG002)
[warning] 402-402: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 403-411: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 412-412: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 413-413: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 415-428: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 429-429: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 430-437: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 457-457: Unused method argument: form_url
(ARG002)
[warning] 680-684: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 685-689: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 736-739: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 740-745: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_firmware_upgrader/websockets.py
[warning] 105-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 111-111: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 115-117: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 122-122: Unused method argument: close_code
(ARG002)
[warning] 126-128: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 144-144: Unused method argument: content
(ARG002)
[warning] 165-167: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 195-195: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 201-201: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 205-207: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 213-213: Unused method argument: close_code
(ARG002)
[warning] 232-232: Unused method argument: content
(ARG002)
[warning] 267-269: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 271-271: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 299-299: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 305-305: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 309-311: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 316-316: Unused method argument: close_code
(ARG002)
[warning] 330-330: Unused method argument: content
(ARG002)
[warning] 371-373: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 375-375: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 434-434: Unused class method argument: sender
(ARG003)
[warning] 434-434: Unused class method argument: kwargs
(ARG003)
[warning] 547-547: Unused class method argument: sender
(ARG003)
[warning] 547-547: Unused class method argument: kwargs
(ARG003)
openwisp_firmware_upgrader/base/models.py
[warning] 864-864: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 866-868: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (41)
openwisp_firmware_upgrader/tests/test_models.py (1)
706-942: LGTM!The three new test methods comprehensively cover batch upgrade workflows with group filtering, location filtering, and combined group+location filtering. The test setup properly creates all necessary fixtures (organizations, categories, builds, images, devices, credentials, connections, firmware associations) and validates the expected behavior including:
- Correct devices being upgraded based on filter criteria
- Firmwareless device handling
- Batch status transitions to "success"
The
*_argspattern correctly indicates unused mock arguments.openwisp_firmware_upgrader/tests/test_selenium.py (1)
344-431: LGTM on new Selenium tests!The new tests (
test_upgrade_cancel_modal,test_mass_upgrade_confirmation_page_widgets,test_upgrade_operation_admin_no_submit_row) properly use WebDriverWait for element visibility and clickability, avoiding flaky race conditions. The cancel modal workflow test is thorough in verifying both the close and confirm paths.openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (2)
255-300: LGTM on safe DOM construction inaddNewOperationRow.The function properly uses jQuery's
.text()method for user-controllable data (data.device_name,data.status,imageDisplay,modifiedTime), which automatically escapes HTML and prevents XSS. The explicit comment about XSS prevention is helpful.
1-132: LGTM on WebSocket initialization and reconnection handling.The implementation correctly:
- Extracts batch ID from URL with error handling
- Uses ReconnectingWebSocket with sensible timeout/retry configuration
- Handles WebSocket lifecycle events (open, close, error, message)
- Implements retry logic for initialization with
isRetryparameter- Requests current state on reconnection to sync UI
openwisp_firmware_upgrader/websockets.py (2)
433-478: LGTM onhandle_upgrade_operation_post_saveimplementation.The handler correctly:
- Skips publishing for newly created operations without a batch (avoiding duplicate messages)
- Uses lazy serializer import to avoid circular imports and NotReady errors
- Converts ForeignKey fields to strings for JSON serialization
- Publishes to both device-specific and batch-specific channels when applicable
- Has proper error handling for connection issues
The unused
senderandkwargsarguments are standard for Django signal handlers.
16-17: LGTM on background task reference management.The module-level
_background_tasksset properly holds references to asyncio tasks, preventing garbage collection before completion. Theadd_done_callbackpattern correctly cleans up completed tasks.Also applies to: 417-424
openwisp_firmware_upgrader/tests/test_websocket_selenium.py (2)
135-219: LGTM on real-time progress update test.The test properly:
- Creates an in-progress operation
- Waits for WebSocket connection establishment
- Publishes a progress update via
UpgradeProgressPublisher- Uses explicit
WebDriverWaitconditions to verify UI updates without race conditions- Validates progress bar, percentage text, and log content updates
495-561: LGTM on batch progress bar tests.The batch upgrade WebSocket tests comprehensively cover:
- Main progress bar initialization and updates
- Individual operation progress within batch
- Batch completion with mixed results (partial success)
- All-successful batch completion
- Dynamic operation addition to batch view
The tests properly use
BatchUpgradeProgressPublisherto simulate real-time updates.openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (3)
54-68: LGTM on HTML escaping utilities.The
escapeHtmlfunction properly handles all HTML special characters and theformatLogForDisplayfunction correctly escapes content before replacing newlines with<br>tags, preventing XSS when displaying log content.
264-300: LGTM on safe jQuery selector construction.The use of
$.escapeSelector(operation.id)on line 266 properly sanitizes the operation ID before using it in a jQuery attribute selector, preventing selector injection issues.
330-336: No action needed. The backend cancellation threshold (_MAX_CANCELLABLE_PROGRESS = 65in models.py) aligns correctly with the frontend logic (progressPercentage < 65). Both prevent cancellation when progress reaches or exceeds 65%, ensuring consistent behavior.openwisp_firmware_upgrader/tests/test_api.py (5)
2-2: Imports and model bindings look good.
New test dependencies and swapper model bindings are appropriate for the added scenarios.Also applies to: 34-40
323-330: Query-count update matches the new setup.
The revised expectations align with the expanded setup path.
409-411: Query-count update is consistent with shared-build flow.
Looks aligned with the current data path.
423-661: Solid coverage for group/location filtering paths.
The tests cover valid, invalid, combined, and no-match scenarios comprehensively.
2042-2085: Cancellation API tests are thorough.
Success, conflict, and not-found cases are all validated.openwisp_firmware_upgrader/tests/test_admin.py (4)
16-18: New imports are appropriate for the added admin tests.
No issues with the additional test dependencies.Also applies to: 43-45
158-185: Intermediate-page assertions look correct after query-count adjustments.
The changes align with the updated flow.
893-894: Schema-null assertion aligns with unsupported upgrader case.
Looks good.
925-1342: Great coverage for admin filters, organization scoping, and location/group behavior.
The scenarios are well-exercised and reflect the new UX paths.openwisp_firmware_upgrader/api/views.py (3)
1-46: Import and logger additions are clean.
Necessary dependencies are now in place.
88-161: Serializer-driven batch upgrade parsing is a good improvement.
Validation and filter additions read cleanly.
376-453: Cancel endpoint is well-structured and consistent.
Permissions and error handling are clear.Also applies to: 470-470
openwisp_firmware_upgrader/base/models.py (5)
9-35: Imports for validators, cancellation, and signals are appropriate.
No concerns.
193-237: Group/location filtering propagation looks correct.
The updated filters are applied consistently to device firmware and firmwareless queries.
526-589: Group/location fields and org validation are solid.
The clean() checks protect against cross-org mismatches.
608-655: Dry-run and upgrade paths correctly thread through group/location.
Looks consistent across related and firmwareless flows.
690-695: Cancelled-rate metric integrates cleanly.
No issues spotted.openwisp_firmware_upgrader/admin.py (5)
12-53: New imports and model bindings are fine.
No issues.
103-165: Group/location fields are wired cleanly into the confirmation form.
Queryset scoping by organization is a good touch.
351-399: UpgradeOperationAdmin wiring looks solid.
Readonly field handling and batch visibility logic read cleanly.
400-597: BatchUpgradeOperationAdmin enhancements are well integrated.
Filters, pagination, and new fields align with the UX goals.
679-745: Progress UI media additions are appropriate.
Assets are consistently included across inlines.openwisp_firmware_upgrader/upgraders/openwrt.py (4)
13-20: Cancellation handling is consistently wired through the upgrade flow.Checks in Line 199–206 and the Line 211–222 helper make cancellation responsive and keep cleanup centralized.
Also applies to: 199-206, 211-222
129-132: Good fix for lazy-translation logging.Casting to
strbefore persisting prevents serialization issues while keeping logging behavior unchanged.
194-195: Progress milestones are clear and non-blocking.The additional updates around connection, UUID verification, checksum check, and reflashing provide useful UX granularity.
Also applies to: 227-228, 389-390, 446-447
543-553: The review comment is incorrect. Progress does reach 100% on successful upgrade completion.When
_write_checksum()completes normally (returning without exception), control returns to thetry-except-elseblock inbase/models.py(line 939). Since no exception is raised byupgrader.upgrade(), theelseclause executes (line 977), which explicitly callsself.update_progress(100, save=False)at line 978. This is the correct place for this call—the orchestrating model class finalizes progress after the upgrader completes successfully, not within the upgrader itself.Likely an incorrect or invalid review comment.
openwisp_firmware_upgrader/tests/test_openwrt_upgrader.py (4)
67-84: Mock coverage for service stop/start looks solid.This makes cancellation tests more realistic and reduces brittle command mismatches.
19-19: Early-stage cancellation test is well targeted.The import and test setup cover cancellation before heavy operations kick off.
Also applies to: 874-899
901-929: Service restart on cancellation is correctly exercised.The test verifies non-critical services are restarted and cancellation propagates as expected.
931-954: Direct_check_cancellationcoverage is helpful.The three-state verification (in-progress, cancelled, aborted) is clear and focused.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good, this is my final review.
Add Explicit Escaping in Templates
Location: openwisp_firmware_upgrader/templates/admin/firmware_upgrader/batch_upgrade_operation_change_form.html:176
<!-- Current -->
host: "{{ request.get_host }}"
<!-- Suggested -->
host: "{{ request.get_host|escapejs }}"Reason: While Django auto-escapes, explicit |escapejs filter provides clearer security intent for JavaScript context.
Centralize Magic Numbers
Location: openwisp_firmware_upgrader/base/models.py and openwisp_firmware_upgrader/upgraders/openwrt.py
# Current: Progress values scattered throughout code
_MAX_CANCELLABLE_PROGRESS = 65
# Progress updates: 10, 15, 20, 65, 90 in openwrt.py
# Suggested: Create constants module or settings
class UpgradeProgress:
CONNECTION_SUCCESS = 10
DEVICE_VERIFIED = 15
CHECKSUM_VERIFIED = 20
REFLASHING = 65
RECONNECTED = 90
COMPLETE = 100
CANCELLATION_THRESHOLD = 65Reason: Makes progress milestones easier to modify, test, and understand.
Implement Exponential Backoff for WebSocket Reconnections
Location: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js
// Current: Fixed 3-second retry interval
const upgradeProgressWebSocket = new ReconnectingWebSocket(wsUrl, null, {
automaticOpen: false,
timeoutInterval: 7000,
maxRetries: 5,
retryInterval: 3000,
});
// Suggested: Exponential backoff
const upgradeProgressWebSocket = new ReconnectingWebSocket(wsUrl, null, {
automaticOpen: false,
timeoutInterval: 7000,
maxRetries: 10,
retryInterval: function(retryAttempt) {
return Math.min(1000 * Math.pow(2, retryAttempt), 30000);
},
});Reason: Reduces server load during network outages by backing off retry attempts.
Add Validation for Progress Field
Location: openwisp_firmware_upgrader/base/models.py
def update_progress(self, progress, save=True):
"""Update progress with validation."""
if not isinstance(progress, (int, float)):
raise ValueError(f"Progress must be numeric, got {type(progress)}")
if not 0 <= progress <= 100:
raise ValueError(f"Progress must be between 0-100, got {progress}")
self.progress = int(progress)
if save:
self.save()Reason: Catches development errors early with explicit validation.
Document WebSocket API
Location: Create docs/user/websocket-api.rst
WebSocket API
=============
Connection URLs
- Single operation: ``ws://<host>/ws/firmware-upgrader/upgrade-operation/<operation_id>/``
- Batch operation: ``ws://<host>/ws/firmware-upgrader/batch-upgrade-operation/<batch_id>/``
- Device updates: ``ws://<host>/ws/firmware-upgrader/device/<device_id>/``
Message Types
-
operation_progress
~~~~~~~~~~~~~~~~~~
Sent when an individual operation's progress updates.
.. code-block:: json
{
"type": "operation_progress",
"operation_id": "uuid",
"status": "in-progress",
"progress": 45
}Reason: Helps developers building custom integrations with the firmware upgrader.
Add Integration Test for Concurrent Cancellations
Location: openwisp_firmware_upgrader/tests/test_models.py
def test_concurrent_cancellation_race_condition(self):
"""Test that concurrent cancellation attempts don't cause errors."""
operation = self._create_upgrade_operation()
# Simulate two users trying to cancel simultaneously
with mock.patch.object(operation, 'save') as mock_save:
# First call succeeds
operation.cancel()
# Second call should raise ValueError (already cancelled)
with self.assertRaises(ValueError):
operation.cancel()Reason: Ensures race conditions during cancellation are handled gracefully.
Add Database Index on Progress Field (Optional)
Location: Migration 0013_upgradeoperation_progress.py (if you need progress range queries)
class Migration(migrations.Migration):
operations = [
migrations.AddField(
model_name="upgradeoperation",
name="progress",
field=models.IntegerField(
default=0,
db_index=True, # Add if filtering by progress ranges
validators=[...]
),
),
]Reason: Only needed if implementing features like "show all operations > 90% complete". Skip if not needed.
Use calculate_and_update_status() for Batch Status Updates
Location: openwisp_firmware_upgrader/base/models.py:860-992
- if self.batch and self.status != "in-progress":
- self.batch.update()
+ if self.batch and self.status != "in-progress":
+ self.batch.calculate_and_update_status()Reason: save() currently calls batch.update(), which does not account for cancelled operations. This can result in a batch being marked as success even after cancellations. Using calculate_and_update_status() ensures consistency with the new cancellation logic.
Preserve Selected Filters in Dry Run After Validation Errors
Location: openwisp_firmware_upgrader/admin.py:209-281
- result = BatchUpgradeOperation.dry_run(build=build)
+ group = form.cleaned_data.get("group") if not form.errors else None
+ location = form.cleaned_data.get("location") if not form.errors else None
+ result = BatchUpgradeOperation.dry_run(
+ build=build, group=group, location=location
+ )Reason: When validation fails, the confirmation view rebuilds the page using an unfiltered dry run, which may confuse users. Using cleaned_data when available preserves the originally selected filters.
Extract _assert_no_js_errors to Shared Test Mixin
Location: openwisp_firmware_upgrader/tests/test_selenium.py:61-70
Suggestion: The _assert_no_js_errors helper is duplicated in test_websocket_selenium.py. Extract it into a shared base class or mixin to avoid duplication and maintain DRY principles.
Reason: Centralizing shared test utilities improves maintainability and reduces code drift.
Prefix Unused Unpacked Variables with Underscores
Location: openwisp_firmware_upgrader/tests/test_selenium.py:347-354
- org, category, build1, build2, image1, image2, device = self._set_up_env()
+ _org, _category, _build1, _build2, _image1, image2, device = self._set_up_env()Reason: Static analysis flags unused variables. Prefixing with _ clearly signals intentional non use.
Document Design Intent for Per Operation Progress Bars
Location: openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js:302-317
function renderOperationProgressBarInCell(statusCell, operation) {
// Renders a visual progress bar in the given status cell based on the
// operation's status and progress.
let $ = django.jQuery;
let status = operation.status;
let progressPercentage = normalizeProgress(operation.progress, status);
statusCell.empty();
statusCell.append('<div class="upgrade-status-container"></div>');
let statusContainer = statusCell.find(".upgrade-status-container");
let statusClass = STATUS_TO_CSS_CLASS[status] || "";
if (STATUSES_WITH_FULL_PROGRESS.has(status)) {
progressPercentage = 100;
}
+ // Per operation bars do not show percentage text to keep table rows compact
let progressHtml = renderProgressBarHtml(progressPercentage, statusClass, false);
statusContainer.html(progressHtml);
}Reason: showPercentageText is always false for per operation rows. A short comment clarifies that this is a deliberate UI choice.
Use logger.exception in Exception Handlers
Location: openwisp_firmware_upgrader/websockets.py:104-118
(Also applies to other similar except blocks throughout the file.)
except (AssertionError, KeyError) as e:
- logger.error(f"Error in operation websocket connect: {e}")
+ logger.exception("Error in operation websocket connect")
await self.close()Reason: logger.exception() automatically includes the stack trace, improving observability and debugging without manual interpolation.
Catch Specific Selenium Exceptions in Test Helpers
Location: openwisp_firmware_upgrader/tests/test_websocket_selenium.py:453-477
+from selenium.common.exceptions import StaleElementReferenceException, NoSuchElementException
+
def _check_operation_progress(self, status_class, progress_width):
"""Helper method to safely check individual operation progress without stale element issues"""
try:
containers = self.find_elements(
By.CSS_SELECTOR, "#result_list .upgrade-status-container"
)
for container in containers:
progress_fill = container.find_element(
By.CSS_SELECTOR, ".upgrade-progress-fill"
)
if status_class in progress_fill.get_attribute(
"class"
) and progress_width in progress_fill.get_attribute("style"):
return True
return False
- except Exception:
+ except (StaleElementReferenceException, NoSuchElementException):
return False
def _check_row_count(self, expected_count):
"""Helper method to safely check row count without stale element issues"""
try:
rows = self.find_elements(By.CSS_SELECTOR, "#result_list tbody tr")
return len(rows) == expected_count
- except Exception:
+ except (StaleElementReferenceException, NoSuchElementException):
return FalseReason: Catching specific Selenium exceptions is more precise and avoids masking unrelated errors.
Escape status in Failure Branch Progress Bar Class
Location: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:353-358
} else if (FW_STATUS_GROUPS.FAILURE.has(status)) {
statusHtml += `
<div class="upgrade-progress-bar">
- <div class="upgrade-progress-fill ${status}" style="width: 100%"></div>
+ <div class="upgrade-progress-fill ${escapeHtml(status)}" style="width: 100%"></div>
</div>
`;Reason: Although status originates from the backend, escaping ensures consistency and prevents unintended CSS class injection.
I don't think this is required because we never filter upgrade operations by progress.
Implemented this using the |
pandafy
force-pushed
the
gsoc25
branch
2 times, most recently
from
7835850 to
ec8967c
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Working to improve the docs before merging.
Final line-by-line review pending, will do asap.
|
✅ Secure - No security vulnerabilities This is an excellent implementation of real-time WebSocket progress updates, cancellation support, and group/location filtering for firmware upgrades. Great work on this substantial feature addition! 🎉 🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
Show real time progress in the UI for individual upgrade operations with live feedback and status updates. In the Mass Upgrade details, the upgrade operation django admin inlines were replaced with a paginated table view which allows filtering by status and organization. Closes #224
Related to #346 --------- Co-authored-by: Roshan Kumar <roshan.kumar@cred.club>
Fixed several bugs and inefficiencies found during testing of the real-time progress updates. Co-authored-by: Gagan Deep <pandafy.dev@gmail.com>
|
Merging! 🎉 🎉 🎉 Thanks a lot to all those who contributed to this! @youhaveme9 @pandafy @Aryamanz29 @okraits 👏 🙏 |
github-project-automation
Bot
moved this from In Progress
to Done
in [GSoC25] Firmware Upgrader UX improvements
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
4 participants
Checklist
Reference to Existing Issue
Closes #224
Closes #208
Closes #213
Closes #225
Completion of GSoC Project: Improve UX and Flexibility of the Firmware Upgrader Module.
Github Board: [GSoC25] Firmware Upgrader UX improvements.