[fix] Prevent 500 and warn about credentialless devices on mass upgrade #302#401
Open
Adityashandilya555 wants to merge 2 commits intoopenwisp:masterfrom
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThe mass-upgrade confirmation flow now detects devices without SSH credentials and reports them to the confirmation template. The admin action collects valid device IDs from enabled DeviceConnection rows whose Sequence DiagramsequenceDiagram
participant User as User/Admin
participant AdminView as AdminView (upgrade_selected)
participant Model as BatchUpgradeOperation
participant DB as Database
participant Template as Confirmation Template
User->>AdminView: POST mass-upgrade request
AdminView->>DB: Query DeviceConnection(enabled=True, update_strategy__icontains="ssh")
DB-->>AdminView: valid_device_ids
AdminView->>DB: Query related DeviceFirmware & firmwareless Devices
DB-->>AdminView: device_firmwares, devices
AdminView->>AdminView: Exclude valid_device_ids -> credentialless lists
AdminView->>AdminView: Build credentialless_devices (limit MAX_CREDENTIALLESS_DISPLAY) and credentialless_more
AdminView->>Model: Request upgrader schema/class for candidates
Model->>DB: Fetch DeviceConnection for device (via get_upgrader_class_for_device)
alt Device has SSH credentials
DB-->>Model: Connection found
Model-->>AdminView: Upgrader class
else Device lacks SSH credentials
DB-->>Model: DoesNotExist
Model->>Model: Catch exception and skip/select next candidate
Model-->>AdminView: Upgrader class or None
end
AdminView->>Template: Render confirmation with credentialless_* context
Template->>User: Show credentialless message + device links
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Adityashandilya555
force-pushed
the
fix/302-mass-upgrade-credentialless-devices
branch
from
ac8e7d7 to
f6cee89
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/admin.py (1)
265-317:⚠️ Potential issue | 🟠 MajorThis warning is never enforced on the successful confirm POST.
These
credentialless_*querysets are only used to decorate the intermediate page. Theupgrade_all/upgrade_relatedbranch above still returns frombuild.batch_upgrade(...)before this logic affects batch creation, so admins can confirm a mass upgrade that still contains credentialless devices. SinceAbstractBatchUpgradeOperation.upgrade_related_devices()/upgrade_firmwareless_devices()still callfull_clean()on every selected device, that batch can still fail as soon as it reaches one of them. Either block submission when credentialless devices remain, or exclude them consistently from the actual upgrade path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/admin.py` around lines 265 - 317, The page only warns about credentialless devices but does not prevent POST submission, so ensure the same credentialless filtering is applied before creating the batch: in the POST handling path that calls build.batch_upgrade (the upgrade_all / upgrade_related branch), detect credentialless devices using the same logic (DeviceConnection filter used to compute credentialless_fw and credentialless_firmwareless) and either (A) abort the POST with an error/HttpResponseBadRequest informing the admin when total_credentialless > 0, or (B) remove those credentialless entries from the related_device_fw and firmwareless_devices passed into build.batch_upgrade so AbstractBatchUpgradeOperation.upgrade_related_devices() / upgrade_firmwareless_devices() never reaches full_clean() for credentialless devices; reference credentialless_fw, credentialless_firmwareless, total_credentialless, build.batch_upgrade and the AbstractBatchUpgradeOperation.upgrade_related_devices()/upgrade_firmwareless_devices() methods when implementing the check/filter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 705-723: The code uses "related_device_fw = related_device_fw or
self.build._find_related_device_firmwares(...)" and similarly for
"firmwareless_devices", which treats empty QuerySets as falsey and loses the
caller's scoped filter; change these to only call the build-scoped finder when
the incoming variable is None (e.g., if related_device_fw is None:
related_device_fw =
self.build._find_related_device_firmwares(select_devices=True)), and do the same
for firmwareless_devices (use is None checks), keeping the subsequent filtering
and calls to get_upgrader_class_for_device(df.device) and
get_upgrader_class_for_device(device) unchanged.
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 38-42: The template uses "{% blocktrans with
count=credentialless_more %}" which prevents plural-aware translations; change
it to use a count-aware blocktrans by using "{% blocktrans count
count=credentialless_more %}" (keeping the variable name credentialless_more)
and add a "{% plural %}" section so you provide both singular and plural strings
referencing the count variable; update the text inside the singular and plural
branches to mention the count variable (e.g., "... and {{ count }} more device
without credentials (not shown)." and the plural "... and {{ count }} more
devices without credentials (not shown).") and close with "{% endblocktrans %}"
so translators receive proper plural forms.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 249-289: The test
test_upgrade_intermediate_page_credentialless_more_than_max only asserts banner
text but not the actual truncation logic; after posting to build_list_url check
the response.context keys used by the view/template (e.g.,
'credentialless_devices' and 'credentialless_more') so the overflow logic is
validated: assert len(response.context['credentialless_devices']) ==
MAX_CREDENTIALLESS_DISPLAY and assert response.context['credentialless_more'] ==
(total_credentialless_count - MAX_CREDENTIALLESS_DISPLAY) (where
total_credentialless_count is the number of devices created), and optionally
assert that a device past the display limit (e.g.,
devices[MAX_CREDENTIALLESS_DISPLAY]) is not included in
response.context['credentialless_devices'] while an earlier device is included;
reference the test name
test_upgrade_intermediate_page_credentialless_more_than_max, the constant
MAX_CREDENTIALLESS_DISPLAY, ACTION_CHECKBOX_NAME and build_list_url to find
where to add these assertions.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 265-317: The page only warns about credentialless devices but does
not prevent POST submission, so ensure the same credentialless filtering is
applied before creating the batch: in the POST handling path that calls
build.batch_upgrade (the upgrade_all / upgrade_related branch), detect
credentialless devices using the same logic (DeviceConnection filter used to
compute credentialless_fw and credentialless_firmwareless) and either (A) abort
the POST with an error/HttpResponseBadRequest informing the admin when
total_credentialless > 0, or (B) remove those credentialless entries from the
related_device_fw and firmwareless_devices passed into build.batch_upgrade so
AbstractBatchUpgradeOperation.upgrade_related_devices() /
upgrade_firmwareless_devices() never reaches full_clean() for credentialless
devices; reference credentialless_fw, credentialless_firmwareless,
total_credentialless, build.batch_upgrade and the
AbstractBatchUpgradeOperation.upgrade_related_devices()/upgrade_firmwareless_devices()
methods when implementing the check/filter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d18be7ef-1db5-49f9-9b1e-127a6cc6beae
📒 Files selected for processing (4)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-02-24T00:04:16.310Z
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:16.310Z
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.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
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.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
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 (1)
openwisp_firmware_upgrader/admin.py (1)
265-319:⚠️ Potential issue | 🟠 MajorThe credential filter never reaches the confirmed upgrade path.
This block only builds template metadata. The follow-up POST still calls
build.batch_upgrade()with justgroup/location, so the devices listed here remain part of the batch if the admin clicks through. Onceupgrade_related_devices()orupgrade_firmwareless_devices()reaches one of them,DeviceFirmware.full_clean()raises becausedeviceconnection_setis empty. Exclude credentialless devices in the batch path as well, or block submission when no upgradeable devices remain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/admin.py` around lines 265 - 319, The UI-only credentialless filter currently only builds template metadata (credentialless_devices) but does not remove those devices from the actual batch, so when the POST path still calls BatchUpgradeOperation(build=build).batch_upgrade(...) with the original related_device_fw and firmwareless_devices the upgrade functions (upgrade_related_devices / upgrade_firmwareless_devices) will hit DeviceFirmware.full_clean() because DeviceConnection objects are missing; fix by filtering out credentialless devices from the batch before calling batch_upgrade (or abort the POST if no upgradeable devices remain): identify credentialless IDs using DeviceConnection (same logic as used to compute credentialless_fw and credentialless_firmwareless), remove those device IDs from related_device_fw and firmwareless_devices passed into BatchUpgradeOperation.build.batch_upgrade (or return an error/redirect when resulting queryset/count is zero) so only devices with valid SSH credentials are processed.
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
38-42:⚠️ Potential issue | 🟡 MinorMake the overflow message plural-aware.
{% blocktrans with %}gives translators only one message id here, socredentialless_morecannot be localized with proper singular/plural forms. Use{% blocktrans count ... %}with a{% plural %}branch instead.Suggested fix
- {% blocktrans with count=credentialless_more %} - ... and {{ count }} more device(s) without credentials (not shown). + {% blocktrans count count=credentialless_more %} + ... and {{ count }} more device without credentials (not shown). + {% plural %} + ... and {{ count }} more devices without credentials (not shown). {% endblocktrans %}In Django templates, should count-dependent translated strings use `{% blocktrans count ... %}` with a `{% plural %}` branch instead of `{% blocktrans with ... %}`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html` around lines 38 - 42, Replace the current `{% blocktrans with count=credentialless_more %}` usage so the translation can handle singular/plural forms: use `{% blocktrans count credentialless_more as count %}` and add a `{% plural %}` branch that renders the plural message; ensure you reference the same `credentialless_more` variable and keep existing surrounding markup and the `if credentialless_more` check.openwisp_firmware_upgrader/base/models.py (1)
705-719:⚠️ Potential issue | 🟠 MajorKeep the caller’s scoped QuerySets when they’re empty.
dry_run()passes filtered QuerySets into_get_upgrader_class(). Usingqs or ...treats an intentionally empty scope as falsey and falls back to build-wide devices, so the confirmation page can derive its upgrader/schema from outside the selected group or location.Suggested fix
- related_device_fw = ( - related_device_fw - or self.build._find_related_device_firmwares(select_devices=True) - ) + if related_device_fw is None: + related_device_fw = self.build._find_related_device_firmwares( + select_devices=True + ) @@ - firmwareless_devices = ( - firmwareless_devices or self.build._find_firmwareless_devices() - ) + if firmwareless_devices is None: + firmwareless_devices = self.build._find_firmwareless_devices()In Django, are empty QuerySet objects falsy in boolean contexts, and does `qs or fallback_qs` replace an intentionally empty filtered QuerySet with the fallback?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/base/models.py` around lines 705 - 719, The code uses "related_device_fw = related_device_fw or self.build._find_related_device_firmwares(...)" and similarly for "firmwareless_devices", which treats an intentionally empty QuerySet as falsey and replaces it with the build-wide QuerySet; instead, preserve caller-scoped empty QuerySets by only supplying the fallback when the parameter is None (not falsy). Update _get_upgrader_class (and calls from dry_run) to set related_device_fw and firmwareless_devices using an "is None" check (e.g., if related_device_fw is None: related_device_fw = self.build._find_related_device_firmwares(...)) so empty filtered QuerySets remain unchanged, ensuring get_upgrader_class_for_device(df.device) and downstream logic use the caller's scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 265-319: The UI-only credentialless filter currently only builds
template metadata (credentialless_devices) but does not remove those devices
from the actual batch, so when the POST path still calls
BatchUpgradeOperation(build=build).batch_upgrade(...) with the original
related_device_fw and firmwareless_devices the upgrade functions
(upgrade_related_devices / upgrade_firmwareless_devices) will hit
DeviceFirmware.full_clean() because DeviceConnection objects are missing; fix by
filtering out credentialless devices from the batch before calling batch_upgrade
(or abort the POST if no upgradeable devices remain): identify credentialless
IDs using DeviceConnection (same logic as used to compute credentialless_fw and
credentialless_firmwareless), remove those device IDs from related_device_fw and
firmwareless_devices passed into BatchUpgradeOperation.build.batch_upgrade (or
return an error/redirect when resulting queryset/count is zero) so only devices
with valid SSH credentials are processed.
---
Duplicate comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 705-719: The code uses "related_device_fw = related_device_fw or
self.build._find_related_device_firmwares(...)" and similarly for
"firmwareless_devices", which treats an intentionally empty QuerySet as falsey
and replaces it with the build-wide QuerySet; instead, preserve caller-scoped
empty QuerySets by only supplying the fallback when the parameter is None (not
falsy). Update _get_upgrader_class (and calls from dry_run) to set
related_device_fw and firmwareless_devices using an "is None" check (e.g., if
related_device_fw is None: related_device_fw =
self.build._find_related_device_firmwares(...)) so empty filtered QuerySets
remain unchanged, ensuring get_upgrader_class_for_device(df.device) and
downstream logic use the caller's scope.
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 38-42: Replace the current `{% blocktrans with
count=credentialless_more %}` usage so the translation can handle
singular/plural forms: use `{% blocktrans count credentialless_more as count %}`
and add a `{% plural %}` branch that renders the plural message; ensure you
reference the same `credentialless_more` variable and keep existing surrounding
markup and the `if credentialless_more` check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0343a51b-3143-4017-9123-379c4c4a9351
📒 Files selected for processing (4)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2026-02-24T00:04:16.310Z
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:16.310Z
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.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
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.
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
Adityashandilya555
force-pushed
the
fix/302-mass-upgrade-credentialless-devices
branch
from
f6cee89 to
47797ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/tests/test_admin.py (1)
279-289:⚠️ Potential issue | 🟡 MinorAssert the truncation state, not just the overflow sentence.
This still passes if
credentialless_moreis off by one or if the template renders more thanMAX_CREDENTIALLESS_DISPLAYdevices. Assert the context values used by the view instead of only checking the rawdevice(s)text.Tighten the test
self.assertEqual(r.status_code, 200) self.assertContains(r, "no SSH credentials assigned") - self.assertContains(r, "more device(s) without credentials") + self.assertEqual(r.context["credentialless_more"], 2) + self.assertEqual( + len(r.context["credentialless_devices"]), MAX_CREDENTIALLESS_DISPLAY + ) + self.assertContains(r, "... and 2 more")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_admin.py` around lines 279 - 289, The test currently only checks rendered text; instead assert the view context to ensure truncation is correct: after posting to self.build_list_url with ACTION_CHECKBOX_NAME, assert r.context['credentialless_more'] is True/False as appropriate and assert len(r.context['credentialless']) == MAX_CREDENTIALLESS_DISPLAY (or equals the expected number) so the template truncation is validated; reference the test method that posts to self.build_list_url and the context keys 'credentialless' and 'credentialless_more' to locate where to add these assertions.openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
38-42:⚠️ Potential issue | 🟡 MinorUse a count-aware
blocktransfor the overflow notice.
blocktrans withgives translators only one source string, so singular/plural forms ofcredentialless_morecannot be localized correctly here.Suggested fix
- {% blocktrans with count=credentialless_more %} - ... and {{ count }} more device(s) without credentials (not shown). + {% blocktrans count count=credentialless_more %} + ... and {{ count }} more device without credentials (not shown). + {% plural %} + ... and {{ count }} more devices without credentials (not shown). {% endblocktrans %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html` around lines 38 - 42, Replace the current non-count-aware blocktrans using "with count=credentialless_more" with a count-aware pluralization blocktrans by using "blocktrans count credentialless_more as count" and adding a {% plural %} section; reference the credentialless_more variable and the existing blocktrans block so translators can provide singular and plural forms and render {{ count }} correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 700-722: The code currently filters devices with credentials only
for schema selection (using valid_device_ids from DeviceConnection) but later
execution in upgrade_related_devices() and upgrade_firmwareless_devices() still
iterates over unfiltered device querysets causing AbstractDeviceFirmware.clean()
to raise for credentialless devices; update those execution querysets to reuse
the same valid_device_ids filter (or add a guard right before calling
full_clean()/save to skip devices where device.deviceconnection_set.count() < 1)
so only devices with DeviceConnection entries are processed—look for references
to upgrade_related_devices, upgrade_firmwareless_devices,
AbstractDeviceFirmware.clean, and valid_device_ids to apply the fix.
---
Duplicate comments:
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 38-42: Replace the current non-count-aware blocktrans using "with
count=credentialless_more" with a count-aware pluralization blocktrans by using
"blocktrans count credentialless_more as count" and adding a {% plural %}
section; reference the credentialless_more variable and the existing blocktrans
block so translators can provide singular and plural forms and render {{ count
}} correctly.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 279-289: The test currently only checks rendered text; instead
assert the view context to ensure truncation is correct: after posting to
self.build_list_url with ACTION_CHECKBOX_NAME, assert
r.context['credentialless_more'] is True/False as appropriate and assert
len(r.context['credentialless']) == MAX_CREDENTIALLESS_DISPLAY (or equals the
expected number) so the template truncation is validated; reference the test
method that posts to self.build_list_url and the context keys 'credentialless'
and 'credentialless_more' to locate where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a783cee-8f4c-488f-81c9-a4d373e10674
📒 Files selected for processing (4)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2026-02-24T00:04:16.310Z
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:16.310Z
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.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
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.
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T18:15:31.491Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-25T18:15:31.491Z
Learning: For large PRs (5000+ LOC) in the openwisp/openwisp-firmware-upgrader repository, be ruthlessly practical: only flag bugs, security issues, data loss risks, or broken functionality. Never flag theoretical concerns, style preferences, hypothetical refactorings, or issues that are functionally correct. The team needs to ship, not debate imaginary problems.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
Adityashandilya555
force-pushed
the
fix/302-mass-upgrade-credentialless-devices
branch
from
47797ec to
0d96cce
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/tests/test_admin.py (1)
249-289: 🧹 Nitpick | 🔵 TrivialConsider adding context assertions for more robust overflow testing.
The test verifies the UI text is present, but asserting the actual context values would catch edge cases like off-by-one errors in the overflow calculation.
Suggested improvement
self.assertEqual(r.status_code, 200) self.assertContains(r, "no SSH credentials assigned") self.assertContains(r, "more device(s) without credentials") + # Verify the overflow logic is correct + self.assertEqual(r.context["credentialless_more"], 2) + self.assertEqual( + len(r.context["credentialless_devices"]), MAX_CREDENTIALLESS_DISPLAY + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_admin.py` around lines 249 - 289, After the POST (r = self.client.post(...)) add assertions that validate the view context payload for the credentialless overflow: check that r.context contains the expected keys (e.g. 'credentialless_devices' and a total/overflow count key such as 'credentialless_total' or 'credentialless_overflow_count'), assert len(r.context['credentialless_devices']) == MAX_CREDENTIALLESS_DISPLAY and assert the total/overflow count equals MAX_CREDENTIALLESS_DISPLAY + 2 (or that overflow flag is True) so the test fails on off-by-one or incorrect overflow calculation; reference MAX_CREDENTIALLESS_DISPLAY, build_list_url and ACTION_CHECKBOX_NAME to locate the POST/test block.openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
38-43:⚠️ Potential issue | 🟡 MinorOverflow message still uses
blocktrans withinstead ofblocktrans count.The main credentialless message at lines 27-31 correctly uses
blocktrans countwith{% plural %}, but the overflow message at lines 40-42 still usesblocktrans with, which doesn't allow proper pluralization for translators.Suggested fix for proper pluralization
{% if credentialless_more %} <p> - {% blocktrans with count=credentialless_more %} - ... and {{ count }} more device(s) without credentials (not shown). + {% blocktrans count count=credentialless_more %} + ... and {{ count }} more device without credentials (not shown). + {% plural %} + ... and {{ count }} more devices without credentials (not shown). {% endblocktrans %} </p> {% endif %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html` around lines 38 - 43, The overflow message uses "{% blocktrans with count=credentialless_more %}" which prevents proper pluralization; replace it with a plural-aware blocktrans (e.g. "{% blocktrans count count=credentialless_more %}" and include a "{% plural %}" section) and reference the variable {{ count }} inside both singular and plural forms so translators can provide correct plural translations for credentialless_more.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 38-43: The overflow message uses "{% blocktrans with
count=credentialless_more %}" which prevents proper pluralization; replace it
with a plural-aware blocktrans (e.g. "{% blocktrans count
count=credentialless_more %}" and include a "{% plural %}" section) and
reference the variable {{ count }} inside both singular and plural forms so
translators can provide correct plural translations for credentialless_more.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 249-289: After the POST (r = self.client.post(...)) add assertions
that validate the view context payload for the credentialless overflow: check
that r.context contains the expected keys (e.g. 'credentialless_devices' and a
total/overflow count key such as 'credentialless_total' or
'credentialless_overflow_count'), assert
len(r.context['credentialless_devices']) == MAX_CREDENTIALLESS_DISPLAY and
assert the total/overflow count equals MAX_CREDENTIALLESS_DISPLAY + 2 (or that
overflow flag is True) so the test fails on off-by-one or incorrect overflow
calculation; reference MAX_CREDENTIALLESS_DISPLAY, build_list_url and
ACTION_CHECKBOX_NAME to locate the POST/test block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7dc028b3-6b5a-4632-a7d0-ebf1f0de65e9
📒 Files selected for processing (4)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- 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.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
📚 Learning: 2026-02-24T00:04:16.310Z
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:16.310Z
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.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
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.
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T18:15:31.491Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-25T18:15:31.491Z
Learning: For large PRs (5000+ LOC) in the openwisp/openwisp-firmware-upgrader repository, be ruthlessly practical: only flag bugs, security issues, data loss risks, or broken functionality. Never flag theoretical concerns, style preferences, hypothetical refactorings, or issues that are functionally correct. The team needs to ship, not debate imaginary problems.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
🔇 Additional comments (7)
openwisp_firmware_upgrader/base/models.py (2)
615-621: LGTM! Credential filtering now applied in execution paths.The
upgrade_related_devices()andupgrade_firmwareless_devices()methods now correctly filter devices to only those with enabled SSH credentials before callingfull_clean(), preventing theValidationErrorthat would occur when devices lack aDeviceConnection.Also applies to: 637-646
702-730: LGTM! Robust fallback logic with proper exception handling and credential filtering.The method now:
- Catches
ObjectDoesNotExistfor the initialupgrade_operationslookup- Uses
is Nonechecks instead oforto preserve scoped querysets (addresses past review)- Filters fallback candidates by
valid_device_idsto ensure only devices with enabled SSH credentials are consideredThis prevents the 500 error when devices lack credentials.
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
24-36: LGTM! Proper error notification for credentialless devices.The template correctly:
- Uses the
errornoteclass for consistent Django admin styling- Provides proper plural-aware messaging via
blocktrans count- Lists affected devices with admin change links for easy navigation
openwisp_firmware_upgrader/admin.py (2)
265-303: LGTM! Well-structured credentialless device detection.The implementation:
- Uses the same credential filter criteria (
update_strategy__icontains="ssh",enabled=True) asbase/models.py, ensuring consistency- Correctly excludes valid devices from both
related_device_fwandfirmwareless_devices- Prioritizes firmware-related devices in the display list before firmwareless ones
- Caps the display list at
MAX_CREDENTIALLESS_DISPLAYwith proper overflow calculation
317-319: LGTM!Context variables correctly passed to the template for rendering the credentialless warning.
openwisp_firmware_upgrader/tests/test_admin.py (2)
190-247: LGTM! Comprehensive test coverage for credentialless scenarios.The tests properly cover:
- Related devices without credentials
- Firmwareless devices without credentials
- All devices credentialless (verifies no 500 error)
- Proper assertions for device change URLs and names
161-161: Query count increases are expected.The additional queries (15→21 and 14→18) are reasonable given the new credentialless detection logic that requires:
- Filtering
DeviceConnectionfor valid SSH credentials- Computing credentialless subsets via exclusion
- Fetching device details for the warning display
Also applies to: 175-175
Adityashandilya555
force-pushed
the
fix/302-mass-upgrade-credentialless-devices
branch
from
0d96cce to
6ca4d31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Line 30: In the plural branch of the template
admin/upgrade_selected_confirmation.html (the line containing "The following {{
count }} device(s) cannot be upgraded because they have no SSH credentials
assigned. Please assign credentials to proceed with the upgrade."), remove the
redundant "(s)" and change "device(s)" to the plural form "devices" so the
message reads "The following {{ count }} devices cannot be upgraded..."; update
only that plural branch string in the template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 378e7c76-da8d-4124-9718-7827c45f5ca3
📒 Files selected for processing (4)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.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.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
📚 Learning: 2026-02-24T00:04:16.310Z
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:16.310Z
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.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
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.
Applied to files:
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-25T18:15:31.491Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-25T18:15:31.491Z
Learning: For large PRs (5000+ LOC) in the openwisp/openwisp-firmware-upgrader repository, be ruthlessly practical: only flag bugs, security issues, data loss risks, or broken functionality. Never flag theoretical concerns, style preferences, hypothetical refactorings, or issues that are functionally correct. The team needs to ship, not debate imaginary problems.
Applied to files:
openwisp_firmware_upgrader/base/models.py
🔇 Additional comments (9)
openwisp_firmware_upgrader/base/models.py (2)
615-621: LGTM on credential filtering in upgrade methods.The filtering logic correctly restricts upgrades to devices with valid SSH-enabled
DeviceConnectionrecords. Usingswapper.load_modelis appropriate for the swappable model, and the filtering is consistently applied in bothupgrade_related_devicesandupgrade_firmwareless_devices.Also applies to: 637-646
704-730: LGTM on the_get_upgrader_classimprovements.The changes correctly:
- Catch
ObjectDoesNotExistwhen accessingself.upgrade_operations[0].device(addressing the 500 error)- Use
is Nonechecks instead oforoperator to preserve scoped querysets from the admin dry-run- Apply consistent SSH credential filtering via
valid_device_idsopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
24-48: LGTM on the credentialless warning block.The template correctly implements the warning UI with:
- Proper
blocktrans countfor i18n pluralization (addressing past feedback)- Clickable device links for easy navigation
- Overflow message with singular/plural forms
openwisp_firmware_upgrader/admin.py (3)
43-43: LGTM on the display limit constant.Having this as a named constant makes it easy to adjust and test against.
265-303: LGTM on credentialless device identification logic.The implementation correctly:
- Uses consistent filtering criteria (
update_strategy__icontains="ssh"andenabled=True)- Handles both related device firmwares and firmwareless devices
- Respects the display limit with proper overflow calculation
- Builds admin URLs for easy device access
317-319: LGTM on context variables.The context correctly passes
credentialless_devices,credentialless_count, andcredentialless_moreto the template.openwisp_firmware_upgrader/tests/test_admin.py (3)
161-161: LGTM on query count adjustments.The increased query counts (15→21 and 14→18) are expected due to the additional
DeviceConnectionqueries for credential filtering.Also applies to: 175-175
190-247: LGTM on the credentialless test cases.Good coverage of:
- Related devices without credentials
- Firmwareless devices without credentials
- All devices without credentials (critical for preventing the 500 error from
#302)
249-298: LGTM on the overflow test case.The test correctly validates the truncation behavior by:
- Creating
MAX_CREDENTIALLESS_DISPLAY + 2devices without credentials- Asserting
credentialless_deviceslist length equalsMAX_CREDENTIALLESS_DISPLAY- Asserting
credentialless_moreequals the overflow count (2)This addresses the past review feedback about asserting actual truncation values, not just banner text.
openwisp#302 - _get_upgrader_class() now uses an ORM subquery to find the first device with a valid SSH credential instead of blindly taking the first device, preventing DoesNotExist crashes when all selected devices lack credentials - upgrade_selected() collects devices without SSH credentials after dry_run() and passes them to the confirmation template with admin URLs - Confirmation page shows credentialless devices as clickable links inside an errornote warning block; limits display to MAX_CREDENTIALLESS_DISPLAY (5) and appends an "and N more" message when the list is truncated - Added 4 tests covering: credentialless related device, credentialless firmwareless device, all-credentialless scenario (no 500), and overflow beyond the display limit Fixes openwisp#302
Adityashandilya555
force-pushed
the
fix/302-mass-upgrade-credentialless-devices
branch
from
6ca4d31 to
fbb21af
Compare
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 615-621: The pre-check path currently uses the unfiltered
dry_run() results which can include credentialless devices; update the
validation in AbstractBuild.batch_upgrade() to apply the same credential filter
using DeviceConnection and valid_device_ids so fully credentialless submissions
are rejected before creating a batch. Specifically, compute valid_device_ids via
swapper.load_model("connection", "DeviceConnection")
filter(update_strategy__icontains="ssh", enabled=True).values("device_id") and
then use
dry_run_result["device_firmwares"].filter(device_id__in=valid_device_ids).exists()
and (if firmwareless)
dry_run_result["devices"].filter(pk__in=valid_device_ids).exists() to decide
whether to raise the existing ValidationError instead of proceeding to create
the batch.
- Around line 713-726: The fallback upgrader lookup ignores the batch's scope so
validation can pick a device outside the batch's group/location; update the
logic in the infer-upgrader section (around related_device_fw,
self.build._find_related_device_firmwares, firmwareless_devices and
self.build._find_firmwareless_devices) to pass the batch's group and location
when resolving devices (e.g., include self.group and self.location as
filters/arguments to _find_related_device_firmwares and
_find_firmwareless_devices) and then use the resolved df.device with
get_upgrader_class_for_device as before so the inferred upgrader respects the
batch scope.
In
`@openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html`:
- Around line 27-30: The warning message in the upgrade confirmation (the block
using blocktrans with count=credentialless_count) incorrectly implies the entire
batch is blocked; update the copy in that block (and any related text
referencing credentialless_count or credentialless devices) to clearly state
that devices without SSH credentials will be skipped while the rest of the batch
will proceed (e.g., "The following {{ count }} device(s) have no SSH credentials
and will be skipped; devices with credentials will continue to be upgraded."),
and ensure the message references credentialless_count and/or the list of
skipped devices so admins understand only those devices are excluded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d89797f-08e3-4dbd-87d4-20c3f61386af
📒 Files selected for processing (4)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.py
📜 Review details
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
📚 Learning: 2026-02-24T00:04:16.310Z
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:16.310Z
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.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
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.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-21T20:21:04.228Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/migrations/0015_add_group_to_batchupgradeoperation.py:16-26
Timestamp: 2026-02-21T20:21:04.228Z
Learning: In openwisp-firmware-upgrader project, the team prefers not to add explicit `related_name` to ForeignKey fields in migrations when the default reverse accessor (`<model>_set`) is acceptable. They are comfortable with Django's default naming convention.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-27T19:09:01.705Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:09:01.705Z
Learning: In openwisp-firmware-upgrader tests, the BaseTestAdmin class should derive app_label from Build._meta.app_label and config_app_label from Device._meta.app_label instead of hard-coding them. This ensures tests work correctly when models are swapped.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-02-23T21:36:30.581Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py:61-67
Timestamp: 2026-02-23T21:36:30.581Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, the sample app (tests/openwisp2/sample_firmware_upgrader) can have a different migration path from the main app. New migration files are generally not created for small changes in the sample app since it's only used for demonstration and testing purposes. The sample app's initial migration may include fields or choices that are added incrementally in the main app's migration history.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_admin.py
📚 Learning: 2026-02-25T18:15:31.491Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-25T18:15:31.491Z
Learning: For large PRs (5000+ LOC) in the openwisp/openwisp-firmware-upgrader repository, be ruthlessly practical: only flag bugs, security issues, data loss risks, or broken functionality. Never flag theoretical concerns, style preferences, hypothetical refactorings, or issues that are functionally correct. The team needs to ship, not debate imaginary problems.
Applied to files:
openwisp_firmware_upgrader/base/models.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_admin.py
- batch_upgrade: filter credentialless devices before 'no devices' check so all-credentialless submissions raise ValidationError instead of creating a no-op batch - _get_upgrader_class: pass group/location when building fallback related_device_fw/firmwareless_devices queries so scoped batches validate against devices within the selected scope - Template: clarify that credentialless devices are skipped while the upgrade proceeds for the remaining devices Related to openwisp#302
Adityashandilya555
force-pushed
the
fix/302-mass-upgrade-credentialless-devices
branch
from
941ec72 to
7212812
Compare
|
Hi @Adityashandilya555 i was going through your approach and i had a question |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When triggering a mass firmware upgrade, if any device (or all devices) lacked a
DeviceConnectionwith SSH credentials,_get_upgrader_class()would callget_upgrader_class_for_device()on an uncredentialed device, raising an unhandledDoesNotExistand returning a 500 error.Additionally, there was no UI feedback to inform administrators which devices would be skipped due to missing credentials.
Changes
base/models.py—_get_upgrader_class()upgrade_operationspath intry/except ObjectDoesNotExistrelated_device_fwandfirmwareless_devices, instead of blindly using the first device regardless of credentialsadmin.py—upgrade_selected()dry_run(), identifies devices without valid SSH credentials from both querysets using ORM exclusionMAX_CREDENTIALLESS_DISPLAY(5) devices with their admin change URLscredentialless_more) for the "and N more" messagecredentialless_devices,credentialless_count, andcredentialless_moreto the template contexttemplates/admin/upgrade_selected_confirmation.htmlerrornotewarning block shown whencredentialless_count > 0tests/test_admin.pytest_upgrade_intermediate_page_credentialless_related— credentialless device in related DeviceFirmwaretest_upgrade_intermediate_page_credentialless_firmwareless— credentialless firmwareless devicetest_upgrade_intermediate_page_all_credentialless— all devices lack credentials, no 500test_upgrade_intermediate_page_credentialless_more_than_max— overflow beyond display limit triggers "and N more"Fixes
Fixes #302