[feature] Automatic metadata extraction: model layer, async task, notifications, admin UI#437
[feature] Automatic metadata extraction: model layer, async task, notifications, admin UI#437atif09 wants to merge 11 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThis PR adds asynchronous firmware metadata extraction, persisted extraction and build statuses, metadata-based device pairing, validation for confirmed images, and migration support. The admin UI displays extraction states, supports re-extraction and manual confirmation, and schedules extraction after file changes. Tasks map extractor outcomes to statuses and notifications. Static hardware mappings are removed, API ordering/filtering is updated, and tests and documentation are revised. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental re-review of the changes since The changes cleanly address the outstanding maintainer review findings and are backed by regression tests:
Existing reviewer comments on the changed lines are left intact and were not duplicated. Files Reviewed (incremental, 6 files)
Previous Review Summaries (7 snapshots, latest commit f9e1cc1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f9e1cc1)Status: No Issues Found | Recommendation: Merge Incremental re-review of the changes since Highlights verified as sound:
Existing reviewer comments on the changed lines are left intact and were not duplicated. Files Reviewed (incremental, 4 files)
Previous review (commit bc3b118)Status: No Issues Found | Recommendation: Merge Incremental re-review of the changes since Lenient review mode: only critical bugs and security vulnerabilities are flagged. None were found in the changed code. Highlights verified as sound:
Existing reviewer comments on the changed lines are left intact and were not duplicated. Files Reviewed (incremental, 25 files)
Previous review (commit 57d4cfe)Status: No Issues Found | Recommendation: Merge Incremental re-review of the changes since The new action is sound:
Previously reviewed findings on unchanged files are carried forward unchanged. Files Reviewed (2 files, incremental)
Previous review (commit ef60188)Status: No Issues Found | Recommendation: Merge Full re-review performed after the branch history was force-pushed (the previous review commit The previously flagged critical issues remain resolved in the current code:
Additional checks performed on the new/changed production code:
Regression/behavior tests are present and aligned with the changes ( Files Reviewed (15 files)
Previous review (commit d9174a7)Status: No Issues Found | Recommendation: Merge The previously flagged CRITICAL issues have been resolved in the latest commits:
The new Files Reviewed (incremental: 6 files)
Previous review (commit 7e44f65)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (15 files)
Fix these issues in Kilo Cloud Previous review (commit 048b616)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
OverviewThis PR implements a comprehensive metadata extraction system for firmware images:
Code Quality Observations
The implementation follows Django and OpenWISP patterns and appears ready for merge. Reviewed by hy3:free · Input: 50.2K · Output: 5.1K · Cached: 299.4K |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 273-277: The code currently flips obj.extraction_status from
FirmwareImage.STATUS_SUCCESS to FirmwareImage.STATUS_MANUALLY_CONFIRMED whenever
obj.source == "dtb" regardless of what was edited; change this to only flip when
one of the DTB-editable metadata fields was actually changed by checking
form.changed_data. In the admin save/update path where obj and form are
available (the branch that checks obj.extraction_status ==
FirmwareImage.STATUS_SUCCESS and obj.source == "dtb"), compute the intersection
between form.changed_data and the set of DTB-editable field names (e.g., the
metadata fields you consider editable for DTB) and only set
obj.extraction_status = FirmwareImage.STATUS_MANUALLY_CONFIRMED if that
intersection is non-empty.
- Around line 255-263: The branch that handles file replacement (the block
starting with if change and "file" in form.changed_data) resets extracted
metadata but omits compat_version; update that block to also clear
obj.compat_version (set to the empty string or None consistent with the model)
so stale compatibility data isn't retained when a new file is uploaded; locate
the block that sets obj.extraction_status, obj.extraction_log,
obj.failure_reason, obj.board, obj.compatible, obj.target, obj.fw_version and
add clearing of obj.compat_version there.
- Around line 264-265: The task is enqueued immediately after super().save_model
which can run before the DB transaction commits; wrap the Celery enqueue in
django.db.transaction.on_commit so the task is scheduled only after commit. In
the save_model override (the method containing super().save_model(request, obj,
form, change)), replace the direct extract_firmware_metadata.delay(obj.pk) call
with transaction.on_commit(lambda: extract_firmware_metadata.delay(obj.pk)) and
ensure django.db.transaction is imported at the top of the file.
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 755-765: auto_create_device_firmwares currently enqueues
create_all_device_firmwares on every save when instance.extraction_status is
success/manually_confirmed; change it to only enqueue when the status
transitions into those states by reading the previous persisted status and
comparing it to the current one. In auto_create_device_firmwares, when created
is False, fetch the previous extraction_status (e.g. prev =
cls.objects.filter(pk=instance.pk).values_list("extraction_status",
flat=True).first()), then only call
transaction.on_commit(partial(create_all_device_firmwares.delay,
str(instance.pk))) if prev is not in (STATUS_SUCCESS, STATUS_MANUALLY_CONFIRMED)
and instance.extraction_status is in (STATUS_SUCCESS,
STATUS_MANUALLY_CONFIRMED); keep the early returns for created and for
non-confirmed current status.
- Around line 504-533: The current _validate_locked only checks the incoming
self.extraction_status which allows changing metadata when extraction_status is
moved into a locked state during the same save; modify _validate_locked to
always fetch the original row (keep the existing early return only for missing
pk), include "extraction_status" in the .values(...) call, and then proceed only
if either the original['extraction_status'] or self.extraction_status is in
self.LOCKED_STATUSES; if locked, compare the original metadata fields
("board","compatible","target","fw_version","compat_version","source") against
self and raise ValidationError on changes. Ensure you reference
_validate_locked, LOCKED_STATUSES and the original variable when implementing
this.
- Around line 418-430: The model fields extraction_log, failure_reason, board,
compatible, target, fw_version, compat_version, and source need translatable
verbose_name wrappers; update each field declaration (e.g., extraction_log =
models.TextField(...), failure_reason = models.CharField(...), board =
models.CharField(...), compatible = models.JSONField(...), target =
models.CharField(...), fw_version = models.CharField(...), compat_version =
models.CharField(...), source = models.CharField(...)) to pass a human-readable
first argument wrapped with gettext_lazy ( _("...") ) and ensure gettext_lazy is
imported as _ at the top of the file so Django i18n picks up labels in
admin/forms.
- Around line 337-350: The notification code hard-codes the admin reverse and
builds untranslated/raw-HTML messages; update _notify_extraction_complete in
AbstractFirmwareImage (and the similar notification in tasks.py) to reverse the
correct admin change URL using self._meta.app_label and self._meta.model_name
(build admin name dynamically), wrap HTML with format_html and
gettext/translation (e.g., use _()) to produce a translatable, escaped message,
and ensure any exception during reverse is logged/propagated instead of being
silently swallowed; modify AbstractDeviceFirmware.auto_create_device_firmwares
to only call create_all_device_firmwares on a transition into the confirmed
extraction_status (i.e., detect the status change on save to avoid recreating
OneToOne DeviceFirmware duplicates) and short-circuit if DeviceFirmware already
exists for the device; finally add verbose_name translations for metadata fields
on AbstractFirmwareImage (board, compatible, target, compat_version, source)
using gettext_lazy for user-facing labels.
In
`@openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py`:
- Around line 13-99: The migration adds defaults that leave existing
FirmwareImage rows with extraction_status="unconfirmed" but
trigger_metadata_extraction() is only invoked on post_save when created, so
existing rows will never be queued and will block Build.batch_upgrade() and
AbstractDeviceFirmware.clean(); fix by adding a RunPython data migration in
0018_build_status_firmwareimage_board_and_more.py that finds FirmwareImage
objects with extraction_status "unconfirmed" and either (a) calls
FirmwareImage.trigger_metadata_extraction() (or enqueues the
extract_firmware_metadata Celery task with the image PK) for each row, or (b) if
you can safely infer confirmation, set extraction_status to "success" or
"manually_confirmed" accordingly; ensure the migration imports FirmwareImage and
the task name extract_firmware_metadata and performs the enqueueing/updates
atomically and idempotently.
In `@openwisp_firmware_upgrader/tasks.py`:
- Around line 225-229: Mark the user-facing notification assigned to the message
variable as translatable and ensure HTML is safely rendered: import Django i18n
and HTML helpers (e.g. gettext/gettext_lazy as _ and format_html), replace the
plain f-string with a translatable string that uses named placeholders (e.g.
_('Metadata extraction failed for <a href="{admin_url}">{image}</a>: {reason}.
You can manually enter metadata or re-upload the image.')) and then call
format_html(...) or format_html_lazy(...) with admin_url=image and
reason=update.get("failure_reason", "unknown error") to safely interpolate
values; update the message assignment accordingly in
openwisp_firmware_upgrader/tasks.py.
- Around line 106-111: The _compat_blocks_pairing function incorrectly rejects
semantic versions with more than two components (e.g., "1.0.1"); update its
parsing so it only considers the major and minor components: split
compat_version on ".", take the first two pieces (treat missing minor as 0),
convert them to ints, then compare (major, minor) > (1, 0); keep the existing
exception handling for ValueError/AttributeError/TypeError to return False.
Ensure you update the logic inside _compat_blocks_pairing to handle strings,
ints, and malformed inputs robustly while preserving the intended comparison.
- Around line 216-218: Don't hard-code the admin route name; instead construct
the URL name from the actual Build model metadata so swapped/custom models work.
Retrieve the Build model (e.g. via image._meta.get_field('build').related_model
or apps.get_model if a string was used), then build the reverse name with
"%s_%s_change" % (build_model._meta.app_label, build_model._meta.model_name) and
call reverse("admin:%s" % that_name, args=[str(image.build_id)]). Replace the
literal "admin:firmware_upgrader_build_change" assignment to admin_url in
tasks.py (the line that sets admin_url using image.build_id) with this dynamic
construction. Ensure any exception handling still surfaces errors instead of
silently swallowing them.
In `@openwisp_firmware_upgrader/tests/test_models.py`:
- Around line 35-38: The class TestModels currently hardcodes app_label and
image_type which makes tests brittle; update TestModels to compute these from
the actual model metadata/constants instead of literals: remove the class
attributes app_label and image_type and set them by reading the target model's
meta and constants (e.g. use the model class referenced by TestUpgraderMixin or
FirmwareImage._meta.app_label and a canonical image type constant on the model
such as FirmwareImage.DEFAULT_IMAGE_TYPE or similar) so tests use
model._meta.app_label and the model's image-type constant at runtime.
🪄 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: 9d26c19e-bee9-472e-87ce-8ce246abb75c
📒 Files selected for processing (12)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/apps.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.py
🧠 Learnings (4)
📚 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/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-21T20:21:02.014Z
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:02.014Z
Learning: In Django migrations within this repo, avoid adding explicit related_name to ForeignKey fields when the default reverse accessor (<model>_set) is acceptable. Rely on Django’s default naming convention to keep migrations concise and maintainable. Only specify related_name if you have a concrete, needed override for reverse relations.
Applied to files:
openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
📚 Learning: 2026-02-23T21:36:22.028Z
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:22.028Z
Learning: In the sample_firmware_upgrader app within tests, avoid creating new migration files for small changes since the sample is for demonstration and testing. Review migrations with this context: the sample's initial migration may differ from the main app and need not be strictly aligned. If you change models for the demo, keep migrations minimal and document why divergence exists; don’t require parity with the main app migrations in PRs focused on the sample.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.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
🔇 Additional comments (6)
openwisp_firmware_upgrader/apps.py (1)
32-33: LGTM!Also applies to: 58-63, 126-133
openwisp_firmware_upgrader/tests/test_models.py (1)
614-730: LGTM!openwisp_firmware_upgrader/tests/base.py (1)
80-81: LGTM!openwisp_firmware_upgrader/tests/test_api.py (1)
329-329: LGTM!Also applies to: 419-419, 1162-1162
openwisp_firmware_upgrader/tests/test_extractors.py (1)
7-8: LGTM!Also applies to: 77-78
openwisp_firmware_upgrader/tests/test_tasks.py (1)
1-260: LGTM!
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_firmware_upgrader/tasks.py`:
- Line 142: Replace direct use of image.file.path (which fails for
non-filesystem storages) by streaming the file to a local temporary file and
passing that temp path to the extractor: in the extract_firmware_metadata flow
replace the call site where meta = extractor_class(image.file.path).extract()
with logic that opens image.file (or reads it) and writes its contents to a
tempfile.NamedTemporaryFile (or TemporaryDirectory) then call
extractor_class(temp_path).extract(); ensure the temp file is cleaned up after
extraction and preserve existing exception handling and status updates (use the
same extractor_class symbol and the same extraction logic, just feed it a local
filesystem path instead of image.file.path).
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 785-860: Extract a helper on the test class (e.g.,
_assert_readonly_fields_for_status) that uses self._create_firmware_image(),
sets fw.extraction_status and optionally fw.source, creates MockRequest with
User.objects.first(), instantiates FirmwareImageAdmin and calls
get_readonly_fields(request, obj=fw), then performs the membership assertions
for lists of fields (expected_in and expected_not_in) instead of repeating the
same setup in each test; replace bodies of test_firmware_image_readonly_fields_*
to call this helper with the appropriate status, source, and expected field
lists so all duplicated setup and loops are centralized.
In `@openwisp_firmware_upgrader/tests/test_tasks.py`:
- Around line 188-194: The test test_extract_firmware_metadata_image_not_found
currently checks mock_warning.call_args.args for the UUID which is brittle;
update the assertion to inspect the actual logged message(s) robustly by
converting the call arguments to strings and asserting that at least one
argument contains fake_pk (e.g., after calling
tasks.extract_firmware_metadata.run(fake_pk) use
mock_warning.assert_called_once() and then assert any(fake_pk in str(arg) for
arg in mock_warning.call_args_list[0].args) so the check works regardless of
logger formatting).
🪄 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: 4ec40440-09d1-4d51-9ed7-2e5f1889abd1
📒 Files selected for processing (12)
openwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_extractors.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (4)
📚 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_api.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-21T20:21:02.014Z
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:02.014Z
Learning: In Django migrations within this repo, avoid adding explicit related_name to ForeignKey fields when the default reverse accessor (<model>_set) is acceptable. Rely on Django’s default naming convention to keep migrations concise and maintainable. Only specify related_name if you have a concrete, needed override for reverse relations.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py
📚 Learning: 2026-02-23T21:36:22.028Z
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:22.028Z
Learning: In the sample_firmware_upgrader app within tests, avoid creating new migration files for small changes since the sample is for demonstration and testing. Review migrations with this context: the sample's initial migration may differ from the main app and need not be strictly aligned. If you change models for the demo, keep migrations minimal and document why divergence exists; don’t require parity with the main app migrations in PRs focused on the sample.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.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
🔇 Additional comments (33)
openwisp_firmware_upgrader/apps.py (3)
32-32: LGTM!
58-63: LGTM!
126-133: LGTM!openwisp_firmware_upgrader/tests/test_models.py (2)
38-38: Avoid hard-codedimage_typetest fixture values.Please derive this from shared model/test constants to keep tests stable if model/image mappings are swapped.
Based on learnings: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding, and apply this pattern across
openwisp_firmware_upgrader/tests.
614-729: LGTM!openwisp_firmware_upgrader/tests/test_tasks.py (1)
1-1: LGTM!Also applies to: 10-10, 15-15, 18-19, 75-187, 195-260
openwisp_firmware_upgrader/tests/base.py (1)
80-81: LGTM!openwisp_firmware_upgrader/tests/test_api.py (1)
329-329: LGTM!Also applies to: 419-419, 1162-1162
openwisp_firmware_upgrader/tests/test_extractors.py (1)
7-8: LGTM!Also applies to: 77-78
openwisp_firmware_upgrader/base/models.py (5)
337-354: Use dynamic admin URL + translated safe notification message.This is still unresolved: Line 338 hard-codes the admin URL name, and Line 347-Line 350 builds a non-translated HTML message string.
418-430: Wrap new user-facing metadata labels in Django i18n.These new field labels remain non-translatable (for example, Line 418-Line 430), so admin/form labels won’t be localized.
As per coding guidelines: “For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework”.
504-533: Lock validation can be bypassed when status changes in the same save.This remains unresolved:
_validate_lockedgates only on currentself.extraction_status(Line 505), so metadata edits can slip through when transitioning out of a locked persisted status.
755-765: Only enqueue pairing on transition into confirmed states.Still unresolved: this hook schedules on every save while confirmed, instead of only when status transitions into success/manually_confirmed.
150-323: LGTM!Also applies to: 495-503, 629-638
openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py (1)
13-100: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py (1)
13-100: LGTM!openwisp_firmware_upgrader/tests/test_admin.py (2)
23-23: LGTM!Also applies to: 551-552
862-921: LGTM!openwisp_firmware_upgrader/tasks.py (7)
106-111: Semantic version parsing still fails for versions with more than two components.The existing concern from previous review is still present: versions like
"1.0.1"will throwValueErrorduring tuple unpacking, causing the function to returnFalseand incorrectly allowing auto-pairing for versions that should be blocked.
216-229: Previously raised issues remain: hard-coded admin route and non-translatable message.Two concerns from prior review still apply:
Line 216-218: The admin URL uses a hard-coded route name
"admin:firmware_upgrader_build_change"which breaks for swapped/customized models.Line 225-229: The notification message is user-facing but not marked as translatable using Django i18n. As per coding guidelines for
**/*.{py,html}: user-facing strings must be marked translatable.
7-16: LGTM!
114-132: LGTM!
155-204: LGTM!
234-240: LGTM!
242-251: LGTM!openwisp_firmware_upgrader/admin.py (8)
255-265: Previous review issues remain unaddressed.Two issues from prior review are still present:
Missing
compat_versionclear (lines 259-263): When the file is replaced,compat_versionis not reset, leaving stale compatibility data that affects the pairing gate.Missing
transaction.on_commit(line 265): Callingextract_firmware_metadata.delay(obj.pk)immediately after save risks the Celery task running before the transaction commits, causing a race condition where the worker can't find the row.Suggested fix
+from django.db import transaction ... if change and "file" in form.changed_data: obj.extraction_status = FirmwareImage.STATUS_UNCONFIRMED obj.extraction_log = "" obj.failure_reason = "" obj.board = "" obj.compatible = [] obj.target = "" obj.fw_version = "" + obj.compat_version = "" obj.source = "" super().save_model(request, obj, form, change) - extract_firmware_metadata.delay(obj.pk) + transaction.on_commit( + lambda pk=obj.pk: extract_firmware_metadata.delay(pk) + ) return
273-277: DTB branch setsmanually_confirmedwithout checking which fields changed.This block runs for any edit on a successful DTB image, including changes to
buildortype, which shouldn't trigger the status flip. Gate on the editable metadata fields.Suggested fix
elif ( obj.extraction_status == FirmwareImage.STATUS_SUCCESS and obj.source == "dtb" + and any( + field in form.changed_data for field in ["target", "fw_version"] + ) ): obj.extraction_status = FirmwareImage.STATUS_MANUALLY_CONFIRMED
56-93: LGTM!
117-121: LGTM!
148-204: LGTM!
206-233: LGTM!
235-252: LGTM!
345-377: LGTM!
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @atif09, this is a significant piece of work and the foundation looks solid. I see it is still marked as draft and depends on #421, so I will hold off on a detailed line-by-line review until it is ready.
A few high-level points to keep in mind while you finish it up:
- Sequencing with #421. This depends on the extractor foundation in #421. Let's agree on the overall scope first (ideally on #418), then decide whether to land the two together or behind a feature flag. I would rather review them as a coherent pair than piecemeal.
- Migrations. New model fields plus a state machine means migrations (and sample-app migrations) need to be correct and reversible. Make sure they are included and that the state-machine field has sensible defaults for existing rows.
- Async task safety. The async extraction task should be idempotent, bounded in resource use (it parses firmware binaries), and must fail into a clear error state rather than leaving objects stuck mid-state. Tie the states explicitly to the exception hierarchy from #421.
Ping me when it is out of draft and #421's scope is settled, and I will go through it properly.
There was a problem hiding this comment.
Pull request overview
Adds an asynchronous firmware-metadata extraction pipeline to OpenWISP Firmware Upgrader by introducing extraction metadata fields + state tracking on FirmwareImage, build-level aggregate extraction status on Build, and wiring Celery tasks + admin behaviors to automate extraction, notifications, and device pairing safeguards.
Changes:
- Added extraction-related model fields/statuses for
FirmwareImageand aggregateBuild.status, plus validation to block mass upgrades until metadata is confirmed. - Introduced
extract_firmware_metadataCelery task, post-save scheduling, build status recomputation, and notifications on completion/failure. - Updated admin to display extraction statuses/logs and enforce read-only metadata editing rules; expanded/adjusted test suite accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py | Test app migration adding build/image extraction fields for sample project. |
| openwisp_firmware_upgrader/tests/test_tasks.py | New tests for extraction task behavior, failure modes, and pairing gating. |
| openwisp_firmware_upgrader/tests/test_models.py | New tests for build status aggregation, locking rules, and upgrade blocking. |
| openwisp_firmware_upgrader/tests/test_extractors.py | Asserts OpenWrt upgrader is wired to OpenWrt metadata extractor. |
| openwisp_firmware_upgrader/tests/test_api.py | Updates query-count expectations impacted by new validation/status logic. |
| openwisp_firmware_upgrader/tests/test_admin.py | Adds admin tests for read-only behavior and save_model extraction/manual-confirm flows. |
| openwisp_firmware_upgrader/tests/base.py | Changes test helper default extraction status to simplify setup. |
| openwisp_firmware_upgrader/tasks.py | Adds _compat_blocks_pairing and extract_firmware_metadata task + failure notification logic. |
| openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py | Main app migration adding build/image extraction fields. |
| openwisp_firmware_upgrader/base/models.py | Adds build status tracking + notifications, image extraction fields + lock validation, and blocks upgrades for unconfirmed metadata. |
| openwisp_firmware_upgrader/apps.py | Connects metadata-extraction post_save signal and adds Firmware Images to the admin menu group. |
| openwisp_firmware_upgrader/admin.py | Introduces FirmwareImageAdmin, status badges/log display, and save_model behaviors for re-extraction/manual confirmation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7743c6b to
048b616
Compare
1ea0b6d to
a0ed12b
Compare
CI Failures: Code Style and Test FailuresHello @atif09,
|
codesankalp
left a comment
codesankalp
left a comment
There was a problem hiding this comment.
I added some comments below:
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…rdware.py #412 - Conditionally show failure_reason (only show when the extraction status is Failed) - Add color badges for different extraction statuses - Ordered the metadata fields in a way that 'Source' appears over 'Target' and 'Firmware version' - Removed the dependency on the hardcoded firmware image map in hardware.py and updated tests accordingly - Converted the 'Compatible' field info into plain text instead of a python list - The user is linked directly to the manual input page and gets auto-scrolled to the 'Device Metadata' input section - Updated docs according to the current system, removed references of hardware.py from the docs Related to #412
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/automatic-device-firmware-detection.rst`:
- Around line 11-17: Correct the typo in the device model path referenced by the
automatic firmware detection docs: in the prose describing how openwisp-config
reads the board name, replace the invalid /temp/sysinfo/model path with
/tmp/sysinfo/model. Keep the rest of the explanation about Device.model and
OpenWISP Firmware Upgrader unchanged.
In
`@openwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.py`:
- Around line 6-28: The migration conversion is still writing through the
historical JSONField in convert_compatible_to_text, so the compatible values end
up quoted or left as arrays instead of plain text. Update the Migration so the
data rewrite happens on the text column side, either by moving the conversion
after AlterField or by replacing the RunPython logic with a RunSQL/USING-style
rewrite that converts list values and empty arrays directly into the new
TextField representation. Use the existing convert_compatible_to_text and
FirmwareImage migration operations as the places to adjust.
🪄 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: 6c720c6c-723d-49b9-8087-80b087dc0ae8
📒 Files selected for processing (32)
AGENTS.mddocs/user/automatic-device-firmware-detection.rstdocs/user/quickstart.rstdocs/user/settings.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/hardware.pyopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.cssopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pytests/openwisp2/sample_firmware_upgrader/admin.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (5)
- openwisp_firmware_upgrader/settings.py
- tests/openwisp2/settings.py
- openwisp_firmware_upgrader/hardware.py
- openwisp_firmware_upgrader/api/views.py
- docs/user/settings.rst
|
device pairing currently matches on image.board, eventually this will |
- Fixed incorrect path in the documentation - Used raw SQL in data migration to avoid JSONField double-serializing the compatible values Related to #412
|
@coderabbitai full review |
✅ Action performedFull review finished. |
- Fix extractor class always defaulting to OpenWrtMetadataExtractor regardless of category - Move compat check into create_all_device_firmwares so all callers are protected - Read image file through the storage backend instead of a direct filesystem path - Restore accidentally removed docstring and comment Related to #413
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 30 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 4
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/tests/test_tasks.py (1)
70-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove duplicated dead code block in
test_batch_upgrade_operation_resilience.Lines 77-81 duplicate lines 72-76 verbatim, but sit outside the
with mock.patch(self._mock_connect, ...)block (one indent level less). This looks like leftover copy-paste from a merge, re-running the same task call and assertion redundantly.🐛 Proposed fix
def test_batch_upgrade_operation_resilience(self, mocked_logger, *args): with mock.patch(self._mock_connect, return_value=True): batch_id = BatchUpgradeOperation().id tasks.batch_upgrade_operation.run(batch_id=batch_id, firmwareless=False) mocked_logger.assert_called_with( f"The BatchUpgradeOperation object with id {batch_id} has been deleted" ) - batch_id = BatchUpgradeOperation().id - tasks.batch_upgrade_operation.run(batch_id=batch_id, firmwareless=False) - mocked_logger.assert_called_with( - f"The BatchUpgradeOperation object with id {batch_id} has been deleted" - )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_firmware_upgrader/tests/test_tasks.py` around lines 70 - 82, The test_batch_upgrade_operation_resilience method contains a duplicated BatchUpgradeOperation task call and logger assertion outside the mock.patch(self._mock_connect, return_value=True) block; remove the extra repeated block so the test only exercises the intended path once, keeping the existing mocked_logger assertion and task invocation inside test_batch_upgrade_operation_resilience.
♻️ Duplicate comments (2)
openwisp_firmware_upgrader/base/models.py (1)
742-773: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
create_for_deviceauto-discovery bypasses thecompat_versionpairing gate.
create_all_device_firmwares(tasks.py) skips auto-pairing when_compat_blocks_pairing(fw_image.compat_version)is true, butcreate_for_device()'s auto-discovery branch (used whenfirmware_imageisn't explicitly passed, e.g. via thecreate_device_firmwaretask triggered on a newDeviceConnection) has no equivalent check. A device newly connected can therefore get auto-paired with a high-compat_versionimage that the image-triggered flow would have explicitly skipped, producing inconsistent pairing behavior between the two flows.This matches the earlier unresolved review discussion ("should we keep it in
create_all_device_firmwaresfor consistency? currently this check is only applicable forextract_firmware_metadata").Suggested fix
if not firmware_image: if not device.model: return firmware_image = FirmwareImage.objects.filter( build__category__organization_id=device.organization_id, build__os=device.os, board=device.model, extraction_status__in=FirmwareImage.LOCKED_STATUSES, - ).first() - if not firmware_image: + ).first() + if not firmware_image or _compat_blocks_pairing(firmware_image.compat_version): return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_firmware_upgrader/base/models.py` around lines 742 - 773, The auto-discovery path in create_for_device currently ignores the compat_version pairing restriction, causing behavior to differ from create_all_device_firmwares. Add the same _compat_blocks_pairing(firmware_image.compat_version) guard in the branch where FirmwareImage is selected automatically (before creating DeviceFirmware), and return early when the image should be excluded so DeviceFirmware creation stays consistent across both flows.tests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.py (1)
6-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSame PostgreSQL incompatibility and empty-list issue as migration 0022.
The
convert_compatible_to_textfunction here has the identical problem flagged in0022_alter_firmwareimage_compatible.py: raw SQL writes plain text to ajsonbcolumn (fails on PostgreSQL), and empty lists are skipped. The same fix applies — reorderAlterFieldbeforeRunPythonand rewrite the conversion to parse JSON strings through the historicalTextField.🔄 Proposed fix: reorder operations and rewrite conversion
# Generated by Django 5.2.15 on 2026-07-03 09:07 +import json + from django.db import migrations, models def convert_compatible_to_text(apps, schema_editor): FirmwareImage = apps.get_model("sample_firmware_upgrader", "FirmwareImage") - table = FirmwareImage._meta.db_table for image in FirmwareImage.objects.iterator(): - if isinstance(image.compatible, list) and image.compatible: - text_value = "\n".join(image.compatible) - schema_editor.execute( - f"UPDATE {table} SET compatible = %s WHERE id = %s", - [text_value, str(image.pk)], - ) + if not image.compatible: + continue + try: + values = json.loads(image.compatible) + except (json.JSONDecodeError, TypeError): + continue + if isinstance(values, list): + text_value = "\n".join(str(v) for v in values) if values else "" + FirmwareImage.objects.filter(pk=image.pk).update(compatible=text_value) class Migration(migrations.Migration): dependencies = [ ("sample_firmware_upgrader", "0006_alter_firmwareimage_board_and_more"), ] operations = [ + migrations.AlterField( + model_name="firmwareimage", + name="compatible", + field=models.TextField(blank=True, default="", verbose_name="compatible"), + ), migrations.RunPython(convert_compatible_to_text, migrations.RunPython.noop), migrations.AlterField( model_name="firmwareimage", - name="compatible", - field=models.TextField(blank=True, default="", verbose_name="compatible"), - ), - migrations.AlterField( - model_name="firmwareimage", name="type", field=models.CharField( blank=True, help_text="firmware image type: model or architecture. Leave blank to attempt determining automatically", max_length=128, ), ), ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.py` around lines 6 - 40, The convert_compatible_to_text migration has the same PostgreSQL jsonb conversion bug and empty-list omission as the earlier migration: it writes plain text into the current compatible column and skips empty arrays. Update the Migration operations around convert_compatible_to_text and the AlterField for FirmwareImage.compatible so the field is first altered to TextField, then RunPython performs the conversion, and finally the field is altered to the final TextField definition. In convert_compatible_to_text, use the historical FirmwareImage model from apps.get_model and convert the stored JSON/list values by reading the old text representation, handling empty lists explicitly, and then saving the normalized newline-joined text back for every row.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_firmware_upgrader/admin.py`:
- Around line 66-84: The status badge setup is duplicated with two different
config shapes, which can drift between `_STATUS_CONFIG` and
`_BUILD_STATUS_CONFIG`. Consolidate `build_status_display` to use the same
badge-rendering pattern as `_extraction_status_badge`, and make
`_BUILD_STATUS_CONFIG` match `_STATUS_CONFIG` so both rely on one consistent
status-to-badge mapping. Update the `build_status_display` logic and any helper
it uses to consume `_BUILD_STATUS_CONFIG` directly instead of concatenating
`"ow-status-{}"` separately.
- Around line 335-339: The FAILED-state manual confirmation logic in the admin
edit flow misses `compatible`, so changing only that field in
`FirmwareImageAdmin` does not transition the image out of `STATUS_FAILED`.
Update the `metadata_fields` check in the status-handling branch that inspects
`form.changed_data` so `compatible` is treated like the other manually editable
metadata fields, and add a regression test in `test_admin.py` that exercises a
`compatible`-only edit on a failed image and verifies it becomes manually
confirmed.
- Around line 106-111: The _compatible_display_html helper currently builds HTML
with manual mark_safe plus join, which should be refactored to the idiomatic
Django pattern. Update _compatible_display_html to use format_html_join for
rendering obj.compatible.splitlines() with the <br> separator, and remove the
explicit mark_safe wrapper while keeping the same display behavior and escaping
semantics.
In
`@openwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.py`:
- Around line 6-31: The backfill in convert_compatible_to_text is running before
FirmwareImage.compatible is altered to TextField, so the raw UPDATE still
targets the jsonb column and can fail on PostgreSQL; move the RunPython after
the AlterField in the Migration.operations order. Also update
convert_compatible_to_text to write the joined value through the ORM on
FirmwareImage instead of schema_editor.execute, and make sure empty lists are
handled so rows with [] are converted to an empty string rather than being left
with the old JSON value.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/tests/test_tasks.py`:
- Around line 70-82: The test_batch_upgrade_operation_resilience method contains
a duplicated BatchUpgradeOperation task call and logger assertion outside the
mock.patch(self._mock_connect, return_value=True) block; remove the extra
repeated block so the test only exercises the intended path once, keeping the
existing mocked_logger assertion and task invocation inside
test_batch_upgrade_operation_resilience.
---
Duplicate comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 742-773: The auto-discovery path in create_for_device currently
ignores the compat_version pairing restriction, causing behavior to differ from
create_all_device_firmwares. Add the same
_compat_blocks_pairing(firmware_image.compat_version) guard in the branch where
FirmwareImage is selected automatically (before creating DeviceFirmware), and
return early when the image should be excluded so DeviceFirmware creation stays
consistent across both flows.
In
`@tests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.py`:
- Around line 6-40: The convert_compatible_to_text migration has the same
PostgreSQL jsonb conversion bug and empty-list omission as the earlier
migration: it writes plain text into the current compatible column and skips
empty arrays. Update the Migration operations around convert_compatible_to_text
and the AlterField for FirmwareImage.compatible so the field is first altered to
TextField, then RunPython performs the conversion, and finally the field is
altered to the final TextField definition. In convert_compatible_to_text, use
the historical FirmwareImage model from apps.get_model and convert the stored
JSON/list values by reading the old text representation, handling empty lists
explicitly, and then saving the normalized newline-joined text back for every
row.
🪄 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: b3f7b914-c6f9-47d3-8401-eaa4f093c2eb
📒 Files selected for processing (32)
AGENTS.mddocs/user/automatic-device-firmware-detection.rstdocs/user/quickstart.rstdocs/user/settings.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/hardware.pyopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.cssopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pytests/openwisp2/sample_firmware_upgrader/admin.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (5)
- docs/user/settings.rst
- openwisp_firmware_upgrader/settings.py
- tests/openwisp2/settings.py
- openwisp_firmware_upgrader/hardware.py
- openwisp_firmware_upgrader/api/views.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Python==3.11 | django~=5.1.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
🧰 Additional context used
📓 Path-based instructions (6)
AGENTS.md
📄 CodeRabbit inference engine (Custom checks)
If AGENTS.md is present, verify that code follows the rules set in it.
Files:
AGENTS.md
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_firmware_upgrader/tests/test_extractors.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/tests/test_api.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/admin.py
openwisp_firmware_upgrader/**/*.{py,html,js,css}
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_firmware_upgrader/**/*.{py,html,js,css}: Keep changes focused and avoid unrelated refactors and formatting churn.
Preserve public APIs, migrations, swappable models, upgrade state transitions, private storage behavior, and integration points unless explicitly required.
Files:
openwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.cssopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/admin.py
openwisp_firmware_upgrader/**/*.{py,html}
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings for translation with Django i18n helpers in Django code.
Files:
openwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/admin.py
openwisp_firmware_upgrader/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_firmware_upgrader/**/*.py: Place imports at the top of the file; defer imports only when necessary, such as Django model imports inside functions or methods before the app registry is ready.
Avoid unnecessary blank lines inside function and method bodies.
Prefer in-process tests so coverage tools can measure changed code.
Preserve tenant isolation and object-level permissions for firmware images, builds, categories, devices, and upgrade operations.
Be careful with upload validation, firmware metadata, upgrade scheduling, retry behavior, Celery tasks, signals, websocket updates, serializers, and admin actions.
When changing APIs, include tests for permissions, validation, filtering, pagination, and tenant boundaries.
Watch for cross-tenant data leaks, permission bypasses, unsafe file paths, unsafe downloads, insecure firmware handling, and secrets.
Preserve validation around firmware images, checksums, metadata, private storage paths, upgrade commands, and URLs.
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside it.
Files:
openwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/admin.py
openwisp_firmware_upgrader/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_firmware_upgrader/tests/**/*.py: Add or update tests for every behavior change.
For bug fixes, write the regression test first, run it against the unfixed code to confirm it fails for the expected reason, then implement the fix.
Files:
openwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Use `docs/developer/installation.rst` and `docs/developer/index.rst` as the source of truth for local setup, services, and baseline test commands.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Use `.github/workflows/ci.yml` as the source of truth for CI-tested dependencies, QA/test commands, environment variables, and supported Python/Django versions.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Use GitHub issue and pull request templates when opening issues or PRs.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Follow the DRY principle: do not duplicate information or code across files.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Update documentation when behavior, settings, public APIs, setup steps, or supported versions change.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Use targeted tests while iterating, then run the documented full test command before considering the change complete.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Run `openwisp-qa-format` after editing when available.
Learnt from: CR
Repo: openwisp/openwisp-firmware-upgrader
Timestamp: 2026-07-08T14:45:33.829Z
Learning: Run `./run-qa-checks` when present and treat failures as blocking unless confirmed unrelated and reported.
📚 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_extractors.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-06-02T08:16:00.439Z
Learnt from: asmodehn
Repo: openwisp/openwisp-firmware-upgrader PR: 362
File: tests/openwisp2/urls.py:10-38
Timestamp: 2026-06-02T08:16:00.439Z
Learning: In this repository, treat `SAMPLE_APP` as a conventionally truthy environment variable: code should use the established pattern `os.environ.get("SAMPLE_APP", False)` and rely on the resulting value’s truthiness (truthy string → enabled). During code reviews, avoid recommending changes to explicit boolean parsing (e.g., checking for `
Applied to files:
openwisp_firmware_upgrader/tests/test_extractors.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_api.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/admin.py
📚 Learning: 2026-06-15T20:19:28.326Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 437
File: openwisp_firmware_upgrader/tests/test_admin.py:1072-1147
Timestamp: 2026-06-15T20:19:28.326Z
Learning: In this codebase (OpenWISP projects, including openwisp-firmware-upgrader), prefer test methods that are fully self-contained: each test should do its own setup and make its own explicit assertions within the test body. Avoid introducing shared assertion helpers or parameterized helper methods that abstract away assertions across tests, even if it leads to some duplication.
Applied to files:
openwisp_firmware_upgrader/tests/test_extractors.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tests/base.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_api.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-21T20:21:02.014Z
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:02.014Z
Learning: In Django migrations within this repo, avoid adding explicit related_name to ForeignKey fields when the default reverse accessor (<model>_set) is acceptable. Rely on Django’s default naming convention to keep migrations concise and maintainable. Only specify related_name if you have a concrete, needed override for reverse relations.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
📚 Learning: 2026-02-23T21:36:22.028Z
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:22.028Z
Learning: In the sample_firmware_upgrader app within tests, avoid creating new migration files for small changes since the sample is for demonstration and testing. Review migrations with this context: the sample's initial migration may differ from the main app and need not be strictly aligned. If you change models for the demo, keep migrations minimal and document why divergence exists; don’t require parity with the main app migrations in PRs focused on the sample.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
📚 Learning: 2026-02-24T00:04:04.187Z
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 openwisp-firmware-upgrader, prefer hiding empty submit rows via CSS (e.g., `#upgradeoperation_form` .submit-row { display: none; }) rather than removing elements in templates. This keeps template logic simple and can improve rendering performance. Apply this pattern to CSS files in openwisp_firmware_upgrader/static/firmware-upgrader/css/*.css for consistent behavior across upgrade-option related styles.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.css
📚 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
🪛 ast-grep (0.44.1)
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.py
[info] 15-15: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200, verbose_name="board")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 20-22: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=10, verbose_name="compat version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-48: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
verbose_name="failure reason",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 53-53: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20, verbose_name="source")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 58-58: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100, verbose_name="target")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py
[info] 15-27: use help_text to document model columns
Context: models.CharField(
choices=[
("analyzing", "Analyzing"),
("success", "Success"),
("failed", "Failed"),
("invalid", "Invalid"),
("manually_confirmed", "Manually Confirmed"),
],
db_index=True,
default="analyzing",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 32-32: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-37: use help_text to document model columns
Context: models.CharField(blank=True, max_length=10)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 52-65: use help_text to document model columns
Context: models.CharField(
choices=[
("unconfirmed", "Unconfirmed"),
("in_progress", "In Progress"),
("success", "Success"),
("failed", "Failed"),
("manually_confirmed", "Manually Confirmed"),
("invalid", "Invalid"),
],
db_index=True,
default="unconfirmed",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 70-80: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 85-87: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=50, verbose_name="firmware version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 92-92: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 97-97: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.py
[info] 15-15: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200, verbose_name="board")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 20-22: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=10, verbose_name="compat version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-48: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
verbose_name="failure reason",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 53-53: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20, verbose_name="source")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 58-58: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100, verbose_name="target")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/tasks.py
[warning] 137-137: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
[info] 15-27: use help_text to document model columns
Context: models.CharField(
choices=[
("analyzing", "Analyzing"),
("success", "Success"),
("failed", "Failed"),
("invalid", "Invalid"),
("manually_confirmed", "Manually Confirmed"),
],
db_index=True,
default="analyzing",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 32-32: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-37: use help_text to document model columns
Context: models.CharField(blank=True, max_length=10)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 52-65: use help_text to document model columns
Context: models.CharField(
choices=[
("unconfirmed", "Unconfirmed"),
("in_progress", "In Progress"),
("success", "Success"),
("failed", "Failed"),
("manually_confirmed", "Manually Confirmed"),
("invalid", "Invalid"),
],
db_index=True,
default="unconfirmed",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 70-80: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 85-87: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=50, verbose_name="firmware version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 92-92: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 97-97: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/apps.py
[warning] 126-126: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("firmware_upgrader", "FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_firmware_upgrader/base/models.py
[info] 101-101: use help_text to document model columns
Context: models.CharField(max_length=64, db_index=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 162-168: use help_text to document model columns
Context: models.CharField(
_("extraction status"),
max_length=20,
choices=BUILD_STATUS_CHOICES,
default=BUILD_STATUS_ANALYZING,
db_index=True,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 296-296: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("Build")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 297-297: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 422-428: use help_text to document model columns
Context: models.CharField(
_("extraction status"),
max_length=20,
choices=EXTRACTION_STATUS_CHOICES,
default=STATUS_UNCONFIRMED,
db_index=True,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 430-436: use help_text to document model columns
Context: models.CharField(
_("failure reason"),
max_length=20,
choices=FAILURE_REASON_CHOICES,
blank=True,
default="",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 437-437: use help_text to document model columns
Context: models.CharField(_("board"), max_length=200, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 439-439: use help_text to document model columns
Context: models.CharField(_("target"), max_length=100, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 440-440: use help_text to document model columns
Context: models.CharField(_("firmware version"), max_length=50, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 441-441: use help_text to document model columns
Context: models.CharField(_("compat version"), max_length=10, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 442-442: use help_text to document model columns
Context: models.CharField(_("source"), max_length=20, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 522-522: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("Build")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_firmware_upgrader/tests/test_tasks.py
[warning] 13-13: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("BatchUpgradeOperation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 14-14: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 15-15: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("UpgradeOperation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 319-319: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("Build")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 OpenGrep (1.23.0)
openwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.py
[ERROR] 12-15: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
tests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.py
[ERROR] 12-15: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
openwisp_firmware_upgrader/admin.py
[WARNING] 109-111: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (40)
openwisp_firmware_upgrader/tests/test_models.py (2)
34-37: Hardcodedapp_label/image_typestill present.This was already flagged in a prior review: derive
app_labelfrom model meta (e.g.Build._meta.app_label) andimage_typefrom an existing constant (e.g.TestUpgraderMixin.TPLINK_4300_IMAGE) rather than hardcoding literals, to keep tests stable if models/settings are swapped.
196-207: LGTM!Also applies to: 371-418, 612-656, 657-673, 674-711, 712-754, 755-797
AGENTS.md (1)
10-10: LGTM!openwisp_firmware_upgrader/tests/test_api.py (2)
329-329: LGTM!Also applies to: 419-419, 1162-1162
1310-1318: 🎯 Functional CorrectnessStale comment vs. new generic assertion; possible coverage regression.
The subtest comment still describes testing "device model does not match the image model," but the assertion now checks the generic
"Invalid pk"message — identical to the second subtest ("Test image pk validation") at Line 1327. This suggests the specific "Device model and image do not match" business-rule validation may no longer be exercised at the API layer (it's now presumably rejected earlier via queryset filtering), collapsing two previously distinct test cases into duplicates.Please confirm this reflects an intentional change in the underlying view/serializer validation flow (not shown in this file set) and update the comment/subtest name to match, or restore a distinct assertion if the specific mismatch message is still expected to surface via a different path.
openwisp_firmware_upgrader/tests/test_extractors.py (1)
11-12: LGTM!Also applies to: 90-92
tests/openwisp2/sample_firmware_upgrader/admin.py (1)
1-14: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py (1)
18-19: LGTM!Based on learnings: "the sample's initial migration may differ from the main app and need not be strictly aligned... keep migrations minimal" — editing this file directly instead of adding a new migration matches the established convention for this sample app.
Also applies to: 199-205
Source: Learnings
docs/user/automatic-device-firmware-detection.rst (1)
4-17: LGTM!docs/user/quickstart.rst (1)
92-96: LGTM!openwisp_firmware_upgrader/tests/base.py (1)
32-34: LGTM!Also applies to: 83-86
openwisp_firmware_upgrader/tests/test_admin.py (1)
24-30: LGTM!Also applies to: 216-329, 368-368, 665-665, 1185-1344
openwisp_firmware_upgrader/tests/test_selenium.py (1)
19-20: LGTM!Also applies to: 44-44, 482-482
openwisp_firmware_upgrader/tests/test_tasks.py (1)
1-22: LGTM!Also applies to: 83-350
openwisp_firmware_upgrader/admin.py (9)
4-4: LGTM!Also applies to: 16-16, 43-43
132-184: LGTM!
255-280: 🎯 Functional Correctness | 💤 Low valueMetadata fields remain editable during
STATUS_UNCONFIRMED.
get_readonly_fieldsdoesn't lockboard/compatible/target/fw_versionwhile status isunconfirmed(the brief window before the Celery task flips it toin_progress). Any edits made in that window will be silently overwritten once extraction completes. Low practical impact given the extraction task typically starts almost immediately viatransaction.on_commit, but worth confirming this is intentional.
282-295: LGTM!
297-319: LGTM!
350-378: LGTM!
444-477: LGTM!
66-95: 🎯 Functional CorrectnessKeep
_FAILURE_REASON_TEXTin sync withFirmwareImage.FAILURE_REASON_CHOICES.
If these keys drift,failure_reason_display()falls back to the stored code instead of the friendly admin message.
202-202: 🔒 Security & PrivacyRaw
build__categorymay expose cross-tenant category namesIf this changelist is visible to org-scoped staff, use the tenant-scoped category filter already used in
BuildAdmininstead of the default FK lookup here.openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.html (1)
1-11: LGTM!openwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.css (1)
1-26: 🚀 Performance & ScalabilityNo change needed Shared OpenWISP theme provides these
--ow-color-*variables, so these badge styles have the expected colors.> Likely an incorrect or invalid review comment.openwisp_firmware_upgrader/migrations/0001_initial.py (1)
15-309: LGTM!openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py (1)
13-100: Backfill concern from prior review now resolved.The previously flagged risk (legacy rows stuck at
extraction_status="unconfirmed", blockingbatch_upgrade()/DeviceFirmware.clean()) is addressed by the follow-up0019_backfill_extraction_status.pymigration, which backfills existing rows tomanually_confirmed. Field choices/defaults here are consistent with the model definitions.openwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.py (1)
1-30: LGTM!openwisp_firmware_upgrader/base/models.py (5)
296-368: LGTM!Atomic conditional-UPDATE pattern for
_update_extraction_status()and dynamic admin URL + i18n in_notify_extraction_complete()correctly address the previously-flagged TOCTOU race and hardcoded-URL/i18n issues.
423-452: LGTM!Verbose names are now translatable, and
_original_extraction_statusis correctly refreshed insave()after persisting (mirroring_update_old_image()), fixing the earlier staleness/typo concern.
529-565: LGTM!
_validate_locked()now checks both the original persisted status and the incoming status againstLOCKED_STATUSES, closing the earlier bypass where changingextraction_statusand metadata in the same save could evade the read-only guard.
664-673: LGTM!
AbstractDeviceFirmware.clean()'s lock-status/board-match checks andauto_create_device_firmwares()'s transition gating (via_original_extraction_status) look correct and match the covered test cases.Also applies to: 706-707, 785-798, 800-819
449-452: 🗄️ Data Integrity & IntegrationRefresh the parent build status on
extraction_statuschanges. Normalsave()paths should keepBuild.statusin sync with the image status.openwisp_firmware_upgrader/apps.py (1)
26-134: LGTM!openwisp_firmware_upgrader/tasks.py (2)
90-134: LGTM!
_compat_blocks_pairingand_get_image_admin_urlcorrectly implement the agreed dynamic-admin-URL pattern and the domain-confirmedmajor.minorparsing.
136-312: LGTM!
extract_firmware_metadatacorrectly uses an atomic conditional update to claim the extraction (UNCONFIRMED→IN_PROGRESS), maps each exception type to the appropriate status/failure_reason, and sends translated, safely-escaped notifications viaformat_html.openwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.py (1)
13-60: LGTM!openwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.py (1)
13-21: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py (1)
13-99: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.py (1)
13-60: LGTM!
atif09
changed the title
atif09
changed the title
- Remove duplicated dead code block in test_batch_upgrade_operation_resilience - Fix create_for_device auto-discovery bypassing compat_version pairing gate - Fix PostgreSQL jsonb incompatibility in sample_firmware_upgrader migration 0007 - Unify status badge configuration to avoid inconsistent rendering logic - Fix compatible field not clearing STATUS_FAILED when edited alone - Run the backfill after compatible becomes text in 0022 Related to #413
|
@coderabbitai full review, approve if there is nothing to flag |
|
Regarding conditional approval: I can't chain that automatically, since I won't know whether the review flagged anything until the review itself completes. Once the review finishes, if there's nothing significant to flag, feel free to follow up with The images you've shared (firmware image list with re-extract action, extraction detail pages for fwtool/DTB success and failure cases, the info/warning notifications, and the device metadata/pairing views) look consistent with the PR objectives described — I'll factor them in as context during the review. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
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/tasks.py (1)
83-100: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winFilter the device queryset by
boardbefore iterating, instead of relying on per-devicefull_clean()to discard mismatches.
queryset = Device.objects.filter(os=fw_image.build.os)iterates every device sharing the build's OS, instantiates aDeviceFirmwareand runsfull_clean()for each one, even though only devices whosemodel == fw_image.boardcan ever pass validation (seeAbstractDeviceFirmware.clean(), line 707-708). For fleets with many devices sharing an OS across different hardware models, this wastes DB queries and CPU on every successful extraction.⚡ Proposed fix
- queryset = Device.objects.filter(os=fw_image.build.os) + queryset = Device.objects.filter(os=fw_image.build.os, model=fw_image.board) for device in queryset.iterator(): DeviceFirmware.create_for_device(device, fw_image)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_firmware_upgrader/tasks.py` around lines 83 - 100, Update create_all_device_firmwares so the Device queryset filters by both os matching fw_image.build.os and model matching fw_image.board before iterator() is called. Keep the existing compatibility check and DeviceFirmware.create_for_device flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 212-225: Update batch_upgrade() and auto_create_device_firmwares()
to reuse FirmwareImage.LOCKED_STATUSES wherever the confirmed or locked firmware
statuses are checked. Remove the duplicated STATUS_SUCCESS and
STATUS_MANUALLY_CONFIRMED tuples while preserving the existing filtering and
validation behavior.
- Around line 754-767: Make the fallback FirmwareImage query in the device
pairing flow deterministic by adding an explicit ordering before .first(), using
the intended stable image-selection field(s) such as a build/image identifier.
Preserve the existing organization, OS, board, and LOCKED_STATUSES filters and
compatibility check.
- Around line 707-708: Update the validation in the image-locking flow around
the device-model check to reject locked images when image.board is empty, while
still requiring self.device.model to match a provided board. Remove the
truthiness guard or add an explicit board-required validation before allowing
the image to lock; preserve the existing mismatch ValidationError.
In `@openwisp_firmware_upgrader/tests/test_admin.py`:
- Around line 253-294: Update the FirmwareImage.objects.filter(...).update(...)
setup in test_re_extract_metadata_action so compatible uses the plain
newline-joined string representation expected by its TextField, rather than a
list; leave the re-extraction assertions and other metadata unchanged.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/tasks.py`:
- Around line 83-100: Update create_all_device_firmwares so the Device queryset
filters by both os matching fw_image.build.os and model matching fw_image.board
before iterator() is called. Keep the existing compatibility check and
DeviceFirmware.create_for_device flow unchanged.
🪄 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: 18413fc7-bf36-456a-b604-8d1ee7f13d11
📒 Files selected for processing (32)
AGENTS.mddocs/user/automatic-device-firmware-detection.rstdocs/user/quickstart.rstdocs/user/settings.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/hardware.pyopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.cssopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pytests/openwisp2/sample_firmware_upgrader/admin.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pytests/openwisp2/settings.py
💤 Files with no reviewable changes (5)
- openwisp_firmware_upgrader/settings.py
- docs/user/settings.rst
- openwisp_firmware_upgrader/hardware.py
- openwisp_firmware_upgrader/api/views.py
- tests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmltests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Follow the DRY principle; do not duplicate information or code across files.
Keep changes focused and avoid unrelated refactors and formatting churn.
Update documentation when behavior, settings, public APIs, setup steps, or supported versions change.
Use targeted tests while iterating, then run the documented full test command before considering the change complete.
Runopenwisp-qa-formatafter editing when available.
Run./run-qa-checkswhen present and treat failures as blocking unless confirmed unrelated and reported.
If setup, QA, or tests fail, check the documentation first, then compare with CI; when commands diverge, follow CI.
Files:
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.csstests/openwisp2/sample_firmware_upgrader/admin.pydocs/user/quickstart.rstopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyAGENTS.mdopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pydocs/user/automatic-device-firmware-detection.rstopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
**/*.{py,html,js,css}
📄 CodeRabbit inference engine (AGENTS.md)
Preserve public APIs, migrations, swappable models, upgrade state transitions, private storage behavior, and integration points unless explicitly required.
Files:
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.htmlopenwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.csstests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django internationalization helpers.
Place imports at the top of Python files; defer imports only when necessary, such as Django model imports inside functions or methods before app registry readiness.
Avoid unnecessary blank lines inside function and method bodies.
Prefer in-process tests so coverage tools can measure changed code.
Preserve tenant isolation and object-level permissions for firmware images, builds, categories, devices, and upgrade operations.
Write comments and docstrings only when they explain why code is shaped a certain way.
Put comments before the relevant code block instead of scattering them inside it.
Files:
tests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
**/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.py: Add or update tests for every behavior change.
For bug fixes, write the regression test first, run it against the unfixed code, confirm the expected failure, then implement the fix.
Files:
tests/openwisp2/sample_firmware_upgrader/admin.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
docs/**/*.rst
📄 CodeRabbit inference engine (AGENTS.md)
Use the developer installation and index documentation as the source of truth for local setup, services, and baseline test commands.
Files:
docs/user/quickstart.rstdocs/user/automatic-device-firmware-detection.rst
openwisp_firmware_upgrader/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
openwisp_firmware_upgrader/**/*.py: When changing APIs, include tests for permissions, validation, filtering, pagination, and tenant boundaries.
Preserve validation around firmware images, checksums, metadata, private storage paths, upgrade commands, and URLs.
Files:
openwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
AGENTS.md
📄 CodeRabbit inference engine (Custom checks)
If AGENTS.md is present, verify that code follows the rules set in it.
Files:
AGENTS.md
🧠 Learnings (7)
📚 Learning: 2026-02-24T00:04:04.187Z
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 openwisp-firmware-upgrader, prefer hiding empty submit rows via CSS (e.g., `#upgradeoperation_form` .submit-row { display: none; }) rather than removing elements in templates. This keeps template logic simple and can improve rendering performance. Apply this pattern to CSS files in openwisp_firmware_upgrader/static/firmware-upgrader/css/*.css for consistent behavior across upgrade-option related styles.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.css
📚 Learning: 2026-06-02T08:16:00.439Z
Learnt from: asmodehn
Repo: openwisp/openwisp-firmware-upgrader PR: 362
File: tests/openwisp2/urls.py:10-38
Timestamp: 2026-06-02T08:16:00.439Z
Learning: In this repository, treat `SAMPLE_APP` as a conventionally truthy environment variable: code should use the established pattern `os.environ.get("SAMPLE_APP", False)` and rely on the resulting value’s truthiness (truthy string → enabled). During code reviews, avoid recommending changes to explicit boolean parsing (e.g., checking for `
Applied to files:
tests/openwisp2/sample_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/tasks.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
📚 Learning: 2026-06-15T20:19:28.326Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 437
File: openwisp_firmware_upgrader/tests/test_admin.py:1072-1147
Timestamp: 2026-06-15T20:19:28.326Z
Learning: In this codebase (OpenWISP projects, including openwisp-firmware-upgrader), prefer test methods that are fully self-contained: each test should do its own setup and make its own explicit assertions within the test body. Avoid introducing shared assertion helpers or parameterized helper methods that abstract away assertions across tests, even if it leads to some duplication.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/admin.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_tasks.py
📚 Learning: 2026-02-21T20:21:02.014Z
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:02.014Z
Learning: In Django migrations within this repo, avoid adding explicit related_name to ForeignKey fields when the default reverse accessor (<model>_set) is acceptable. Rely on Django’s default naming convention to keep migrations concise and maintainable. Only specify related_name if you have a concrete, needed override for reverse relations.
Applied to files:
openwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.pyopenwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.pyopenwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.pyopenwisp_firmware_upgrader/migrations/0001_initial.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py
📚 Learning: 2026-02-23T21:36:22.028Z
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:22.028Z
Learning: In the sample_firmware_upgrader app within tests, avoid creating new migration files for small changes since the sample is for demonstration and testing. Review migrations with this context: the sample's initial migration may differ from the main app and need not be strictly aligned. If you change models for the demo, keep migrations minimal and document why divergence exists; don’t require parity with the main app migrations in PRs focused on the sample.
Applied to files:
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.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/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_extractors.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_tasks.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
🪛 ast-grep (0.44.1)
openwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.py
[info] 15-15: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200, verbose_name="board")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 20-22: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=10, verbose_name="compat version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-48: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
verbose_name="failure reason",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 53-53: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20, verbose_name="source")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 58-58: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100, verbose_name="target")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.py
[info] 15-15: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200, verbose_name="board")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 20-22: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=10, verbose_name="compat version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-48: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
verbose_name="failure reason",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 53-53: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20, verbose_name="source")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 58-58: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100, verbose_name="target")
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.py
[info] 23-23: use jsonify instead of json.dumps for JSON output
Context: json.dumps(lines)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py
[info] 15-27: use help_text to document model columns
Context: models.CharField(
choices=[
("analyzing", "Analyzing"),
("success", "Success"),
("failed", "Failed"),
("invalid", "Invalid"),
("manually_confirmed", "Manually Confirmed"),
],
db_index=True,
default="analyzing",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 32-32: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-37: use help_text to document model columns
Context: models.CharField(blank=True, max_length=10)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 52-65: use help_text to document model columns
Context: models.CharField(
choices=[
("unconfirmed", "Unconfirmed"),
("in_progress", "In Progress"),
("success", "Success"),
("failed", "Failed"),
("manually_confirmed", "Manually Confirmed"),
("invalid", "Invalid"),
],
db_index=True,
default="unconfirmed",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 70-80: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 85-87: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=50, verbose_name="firmware version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 92-92: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 97-97: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/apps.py
[warning] 126-126: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("firmware_upgrader", "FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.py
[info] 23-23: use jsonify instead of json.dumps for JSON output
Context: json.dumps(lines)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_firmware_upgrader/tasks.py
[warning] 137-137: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py
[info] 15-27: use help_text to document model columns
Context: models.CharField(
choices=[
("analyzing", "Analyzing"),
("success", "Success"),
("failed", "Failed"),
("invalid", "Invalid"),
("manually_confirmed", "Manually Confirmed"),
],
db_index=True,
default="analyzing",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 32-32: use help_text to document model columns
Context: models.CharField(blank=True, max_length=200)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 37-37: use help_text to document model columns
Context: models.CharField(blank=True, max_length=10)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 52-65: use help_text to document model columns
Context: models.CharField(
choices=[
("unconfirmed", "Unconfirmed"),
("in_progress", "In Progress"),
("success", "Success"),
("failed", "Failed"),
("manually_confirmed", "Manually Confirmed"),
("invalid", "Invalid"),
],
db_index=True,
default="unconfirmed",
max_length=20,
verbose_name="extraction status",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 70-80: use help_text to document model columns
Context: models.CharField(
blank=True,
choices=[
("unsupported_format", "Unsupported format"),
("out_of_memory", "Out of memory"),
("invalid_file", "Invalid file"),
("timeout", "Extraction timed out"),
],
default="",
max_length=20,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 85-87: use help_text to document model columns
Context: models.CharField(
blank=True, max_length=50, verbose_name="firmware version"
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 92-92: use help_text to document model columns
Context: models.CharField(blank=True, max_length=20)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 97-97: use help_text to document model columns
Context: models.CharField(blank=True, max_length=100)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_firmware_upgrader/base/models.py
[info] 102-102: use help_text to document model columns
Context: models.CharField(max_length=64, db_index=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 163-169: use help_text to document model columns
Context: models.CharField(
_("extraction status"),
max_length=20,
choices=BUILD_STATUS_CHOICES,
default=BUILD_STATUS_ANALYZING,
db_index=True,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 297-297: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("Build")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 298-298: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 423-429: use help_text to document model columns
Context: models.CharField(
_("extraction status"),
max_length=20,
choices=EXTRACTION_STATUS_CHOICES,
default=STATUS_UNCONFIRMED,
db_index=True,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 431-437: use help_text to document model columns
Context: models.CharField(
_("failure reason"),
max_length=20,
choices=FAILURE_REASON_CHOICES,
blank=True,
default="",
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 438-438: use help_text to document model columns
Context: models.CharField(_("board"), max_length=200, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 440-440: use help_text to document model columns
Context: models.CharField(_("target"), max_length=100, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 441-441: use help_text to document model columns
Context: models.CharField(_("firmware version"), max_length=50, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 442-442: use help_text to document model columns
Context: models.CharField(_("compat version"), max_length=10, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 443-443: use help_text to document model columns
Context: models.CharField(_("source"), max_length=20, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 523-523: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("Build")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_firmware_upgrader/tests/test_tasks.py
[warning] 13-13: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("BatchUpgradeOperation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 14-14: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("FirmwareImage")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 15-15: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("UpgradeOperation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 314-314: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("Build")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 HTMLHint (1.9.2)
openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🪛 OpenGrep (1.23.0)
openwisp_firmware_upgrader/admin.py
[WARNING] 112-114: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (41)
openwisp_firmware_upgrader/migrations/0001_initial.py (1)
15-309: LGTM!openwisp_firmware_upgrader/migrations/0018_build_status_firmwareimage_board_and_more.py (1)
13-99: LGTM! The previous review concern about legacy rows being left "unconfirmed" and blocking upgrades is now resolved by migration 0019, which backfills existing rows tomanually_confirmed.openwisp_firmware_upgrader/migrations/0020_alter_firmwareimage_board_and_more.py (1)
13-60: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0001_initial.py (1)
199-205: LGTM! Usingchoices=[]instead of importingFIRMWARE_IMAGE_TYPE_CHOICESis consistent with the PR's goal of removing static type choices. As per the learning that the sample app's initial migration may diverge from the main app, this change is appropriate. Based on learnings, the sample app's initial migration may differ from the main app and need not be strictly aligned.Source: Learnings
openwisp_firmware_upgrader/migrations/0019_backfill_extraction_status.py (1)
4-14: 🗄️ Data Integrity & IntegrationNo extra
compatiblebackfill is needed. Migration 0022 already includes aRunPythonstep that converts existing JSON values to text during the field change, so legacy[]rows won’t be left as"[]".> Likely an incorrect or invalid review comment.openwisp_firmware_upgrader/migrations/0021_firmwareimage_remove_type_choices.py (1)
1-22: LGTM!openwisp_firmware_upgrader/migrations/0022_alter_firmwareimage_compatible.py (1)
8-41: LGTM! The three concerns from the prior review are fully resolved:AlterFieldnow precedesRunPython(column isTextFieldwhen the conversion runs), data is written via ORMimage.save(update_fields=["compatible"])instead of raw SQL, andisinstance(value, list)correctly handles empty lists ("\n".join([])→"").tests/openwisp2/sample_firmware_upgrader/migrations/0005_build_status_firmwareimage_board_and_more.py (1)
1-100: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0006_alter_firmwareimage_board_and_more.py (1)
1-61: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0007_alter_firmwareimage_compatible_and_more.py (1)
1-50: LGTM! The test migration correctly mirrors production migrations 0021+0022, withAlterFieldprecedingRunPythonand the same ORM-based conversion logic.openwisp_firmware_upgrader/base/models.py (2)
12-17: LGTM!Also applies to: 33-41, 102-102, 150-171, 297-369, 388-453, 520-566, 665-674
462-473: LGTM!openwisp_firmware_upgrader/apps.py (1)
32-32: LGTM!Also applies to: 58-63, 126-134
openwisp_firmware_upgrader/tasks.py (1)
2-19: LGTM!Also applies to: 115-134, 136-241, 243-312
AGENTS.md (1)
10-10: LGTM!docs/user/automatic-device-firmware-detection.rst (1)
4-17: LGTM!openwisp_firmware_upgrader/tests/test_extractors.py (1)
11-12: LGTM!Also applies to: 90-92
openwisp_firmware_upgrader/admin.py (5)
112-114: Preferformat_html_joinover manualmark_safe+ join was previously discussed and withdrawn per@atif09(consistency with openwisp-monitoring). No new concern.
490-499:build_status_displaystill re-implements the same badge-rendering as_extraction_status_badgerather than reusing it; already raised in a prior review (also applies to 66-87).
323-352: LGTM!
357-382: LGTM!
89-97: 🎯 Functional CorrectnessNo change needed.
_FAILURE_REASON_TEXTmatchesFirmwareImage.FAILURE_*, and the admin copy is separate fromFirmwareImage.FAILURE_REASON_CHOICES.> Likely an incorrect or invalid review comment.openwisp_firmware_upgrader/tests/test_models.py (2)
35-37: Hard-codedapp_label/image_typeat the class level was already flagged in a prior review (derive from model meta per learnings).
371-435: LGTM!Also applies to: 629-814
openwisp_firmware_upgrader/static/firmware-upgrader/css/extraction-status.css (1)
1-25: LGTM!openwisp_firmware_upgrader/templates/admin/firmware_upgrader/firmwareimage_change_form.html (1)
1-10: LGTM!docs/user/quickstart.rst (1)
92-96: LGTM!tests/openwisp2/sample_firmware_upgrader/admin.py (1)
6-14: LGTM!openwisp_firmware_upgrader/tests/base.py (1)
32-34: LGTM!Also applies to: 83-86
openwisp_firmware_upgrader/tests/test_admin.py (8)
24-30: LGTM!
216-252: LGTM!
296-329: LGTM!
368-368: LGTM!
663-666: LGTM!
1185-1260: LGTM!
1262-1283: LGTM!
1284-1363: LGTM!openwisp_firmware_upgrader/tests/test_api.py (1)
329-329: LGTM!Also applies to: 419-419, 1162-1162, 1301-1327
openwisp_firmware_upgrader/tests/test_selenium.py (1)
19-20: LGTM!Also applies to: 44-44, 482-482
openwisp_firmware_upgrader/tests/test_tasks.py (2)
1-22: LGTM!Also applies to: 78-217, 295-345
218-294: LGTM!
- Filter the device queryset by board before iterating, instead of relying on per-device full_cean() to discard mismatches - Reuse LOCKED_STATUSES wherever applicable - Reject locked images with an empty board, add test for the same - Fix non-deterministic image selection in create_for_device auto-discovery - Fix compatible passed as list instead of string in test setup Related to #413
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
In regards to the pre-existing images that were already present in the DB, I am wondering if we can do the following, which should work:
- Keep hardware.py around (restore it, restore also the functionality for custom images defined with the old setting)
- Add a data migration, which reads pre-existing images, finds the corresponding entry in the data structure created by summing
hardware.pyand theCUSTOM_OPENWRT_IMAGESsetting, gets the value it needs and adds them to the new fields
This shouldn't be hard and won't require running the extraction on pre-exsting images, which would be a headache.
It also means that both hardware.py and the setting have to be deprecated and flagged as such in the docs (and code), we can remove those in version 1.5.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I left a few inline comments on issues that still need to be addressed.
- Mark extraction metadata and build status as read only in the API serializers, add regression test - Update build extraction status after manually confirming a failed image, add regression test - Filter devices by organization in create_all_device_firmwares for org specifc images Related to #413
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Checklist
Reference to Existing Issue
Closes #412
Closes #413
Closes #414
Closes #464
Related to #416
Description of Changes
Adds extraction metadata fields and a state machine to FirmwareImage to track async metadata extraction. A Celery task extracts firmware metadata (board, target, compatible, version, source) after upload, with a locked state preventing edits after confirmation. Prevents duplicate image uploads via a unique_together constraint and skips device firmware pairing when compat_version exceeds 1.0
On the admin side,
failure_reasonis hidden unless extraction failedcompat_versionis hidden as it is mostly an internal detailManual test plan
Setup
NOTE: For pre-existing images, run the bulk
Re-extract Metadataadmin action present in the drop down of the Firmware Image list page after selecting the images to run the extraction on them:Metadata Extraction
Unconfirmed, then transition toIn Progress, and finally resolve toSuccessorFailedonce the Celery task completes.Failure Reasonshould be visible on the image detail page.Manual Metadata Input Scenario
openwrt-23.05.5-sunxi-cortexa7-xunlog_orangepi-zero-ext4-sdcard.img.gzwhich doesn't have an fwtool trailerDevice Pairing via Board
Success) (or) an image which required manual input and has the statusManually Confirmed.(make sure the
Modelfield:matches the
Boardfield:of the firmware image).
Firmwaretab on the Device page and you will see the drop down populated with the firmware imagePending items
Conditionally show Failure reason: Hide the Failure reason in admin unless the image's extraction status is Failed (to prevent unnecessary noise)
Add color badges for different extraction status (Success, Failed, Manually confirmed, etc)
Order metadata fields logically + consistently: Apply logical field ordering in both the inline and the firmware image manual input page
compat version visibility to the user: Hide compat version field in the UI as it is not useful to the user and is an internal functionality
Remove dependency on the hardcoded image map: Migrate device pairing off the type field toward the new compatible/board metadata
Make 'Compatible' more human-readable
Improve failure notification: Link directly to the manual input page and auto-scroll to the 'Device Metadata' input section, drop the unnecessary "Click 'View'.." wording since the link lands there
(SEPARATE ISSUE OPENED) Auto-trigger metadata extraction for pre-existing images via data migration, avoiding any manual effort from users (similar to how openwisp-monitoring handles InfluxDB data migrations)
Documentation for the new extraction states, manual input workflow
Revisit extraction status semantics: Review each status from the user's perspective, clarify which statuses require user action (e.g. manual input), which are terminal failures, and whether current names/descriptions communicate this clearly enough
Move bulk re-extract action to Builds admin: The "Re-extract metadata" bulk action should be accessible from the Builds list view, not just FirmwareImage; also auto-detect which images need re-extraction based on their current status
Shift device pairing from image.board to image.compatible for more precise hardware matching