[feature] Persistent Mass Upgrades - schema, retry backoff, batch state and Beat scanner#436
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:
📝 WalkthroughWalkthroughThis PR implements persistent offline device retries for firmware upgrades. When a batch upgrade targets an offline device, instead of failing after ~10 minutes, operations marked as persistent transition to a Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
ace6c8c to
5119b30
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Code Review SummaryStatus: 2 Issues Found | Recommendation: Merge with follow-up The persistent mass-upgrade pipeline is solid: atomic status claims ( The only outstanding items are two deploy-time migration index locks, both already noted inline. Consider a maintenance window or a Overview
Issue Details (click to expand)WARNING
Files Reviewed
Fix these issues in Kilo Cloud Previous Review Summaries (5 snapshots, latest commit b6f5543)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b6f5543)Status: 2 Issues Found | Recommendation: Merge with follow-up The persistent mass-upgrade pipeline is solid: atomic status claims, conditional re-checks before dispatch, an immutable Overview
Issue Details (click to expand)WARNING
Files Reviewed
Fix these issues in Kilo Cloud Previous review (commit a6f27f0)Status: 3 Issues Found | Recommendation: Merge with follow-up The persistent mass-upgrade pipeline is solid: atomic status claims, conditional Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)The The Beat scanner ( Files Reviewed (18 files)
Fix these issues in Kilo Cloud Previous review (commit fa67c1f)Status: 2 Issues Found | Recommendation: Merge with follow-up The persistence pipeline is well structured: atomic status claims, conditional Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Note: the earlier reminder-task concern is no longer valid — only persistent operations can reach Files Reviewed (18 files)
Fix these issues in Kilo Cloud Previous review (commit 1b86bff)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (21 files)
Fix these issues in Kilo Cloud Previous review (commit 7f53d39)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (7 files)
Reviewed by glm-5.2-20260616 · Input: 275.9K · Output: 6K · Cached: 296.6K |
There was a problem hiding this comment.
Pull request overview
This PR lays the model-layer groundwork for “persistent” mass upgrades by extending the upgrade/batch operation schema, adding a new pending operation status, propagating batch persistence to child operations, and enforcing immutability of the persistence flag after launch.
Changes:
- Added
persistentflags to batch and per-device upgrade operations, plusretry_countandnext_retry_atfields for retry scheduling. - Introduced
pendingtoUpgradeOperation.STATUS_CHOICESand propagatedbatch.persistentinto child operations during creation. - Added unit tests for defaults, propagation, DB index presence, and immutability guards; added corresponding migrations (including sample app migration).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
openwisp_firmware_upgrader/base/models.py |
Adds persistence fields, pending status, batch→child propagation, and immutability validation in clean(). |
openwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_persistent_and_more.py |
Introduces DB fields and extends status choices at the schema level. |
openwisp_firmware_upgrader/tests/test_models.py |
Adds tests for new field defaults, propagation behavior, DB index flag, and immutability validation. |
tests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_persistent_and_more.py |
Mirrors the migration in the sample app used by the test project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ("failed", _("failed")), # failed at late stage or can't reconnect | ||
| ("cancelled", _("cancelled")), # cancelled by the user | ||
| ("aborted", _("aborted")), # aborted due to prerequisites not met | ||
| ("pending", _("pending")), # offline device; waiting for periodic retry | ||
| ) |
| ("failed", _("failed")), # failed at late stage or can't reconnect | ||
| ("cancelled", _("cancelled")), # cancelled by the user | ||
| ("aborted", _("aborted")), # aborted due to prerequisites not met | ||
| ("pending", _("pending")), # offline device; waiting for periodic retry |
| persistent = models.BooleanField( | ||
| default=False, | ||
| verbose_name=_("persistent"), | ||
| help_text=_( | ||
| "if enabled, the operation stays pending and retries " |
| persistent = models.BooleanField( | ||
| default=True, | ||
| verbose_name=_("persistent"), | ||
| help_text=_( | ||
| "if enabled, the mass upgrade keeps retrying " |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@openwisp_firmware_upgrader/base/models.py`:
- Around line 617-631: When checking immutability for the persistent field in
the model (inside the BatchUpgradeOperation instance logic), use the persisted
DB status instead of the in-memory self.status so callers can't flip status to
"idle" during the same edit; fetch the stored status via
load_model("BatchUpgradeOperation").objects.values_list("status",
flat=True).get(pk=self.pk) (similar to the existing persistent fetch) and change
the gate from self.status != "idle" to stored_status != "idle" before comparing
self.persistent to the persisted persistent and raising the ValidationError.
In `@openwisp_firmware_upgrader/tests/test_models.py`:
- Around line 606-608: Replace hard-coded app-label string patching with
model-bound patching: import or reference the UpgradeOperation model class and
use mock.patch.object(UpgradeOperation, "upgrade", return_value=None) instead of
mock.patch(f"{self.app_label}.models.UpgradeOperation.upgrade", ...); do the
same for the other occurrence that patches UpgradeOperation.upgrade (the second
instance mentioned) so tests remain resilient to model swapping.
🪄 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: 4eaa450b-cfb3-458c-8956-a3dc2e9dbe2a
📒 Files selected for processing (4)
openwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_models.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_persistent_and_more.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: copilot-pull-request-reviewer
🧰 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/migrations/0018_batchupgradeoperation_persistent_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (3)
📚 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_batchupgradeoperation_persistent_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_persistent_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_batchupgradeoperation_persistent_and_more.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_models.py
🔇 Additional comments (4)
openwisp_firmware_upgrader/tests/test_models.py (1)
586-602: LGTM!Also applies to: 609-630, 634-673
openwisp_firmware_upgrader/base/models.py (1)
461-461: LGTM!Also applies to: 571-579, 846-846, 870-894, 896-913
openwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_persistent_and_more.py (1)
13-67: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_persistent_and_more.py (1)
13-67: LGTM!
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
6595d4f to
4dba3f9
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
It's looking good, please batch in more work or it will be hard for me to get an idea of whether we're really going in the right direction.
| blank=True, | ||
| null=True, | ||
| ) | ||
| persistent = models.BooleanField( |
There was a problem hiding this comment.
This is more consistent with other openwisp modules:
| persistent = models.BooleanField( | |
| is_persistent = models.BooleanField( |
| ("failed", _("failed")), # failed at late stage or can't reconnect | ||
| ("cancelled", _("cancelled")), # cancelled by the user | ||
| ("aborted", _("aborted")), # aborted due to prerequisites not met | ||
| ("pending", _("pending")), # offline device; waiting for periodic retry |
There was a problem hiding this comment.
make sure this is cancellable and add a test for this: pending upgrades can be cancled.
| status = models.CharField( | ||
| max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0] | ||
| ) | ||
| persistent = models.BooleanField( |
There was a problem hiding this comment.
This is more consistent with other openwisp modules:
| persistent = models.BooleanField( | |
| is_persistent = models.BooleanField( |
There was a problem hiding this comment.
I would include the minimum behavior that makes the new state coherent:
BatchUpgradeOperation.progress_report:pendingmust not be counted as completed.BatchUpgradeOperation.calculate_and_update_status(): a batch with pending child operations should remain active, not becomesuccess.UpgradeOperation.cancel(): pending operations should be cancellable.UpgradeOperation.upgrade(): an existingpendingoperation should block a second upgrade for the same device, the same wayin-progressdoes.
I would also update the BatchUpgradeOperation.clean() immutability check to use the stored database status, not self.status, otherwise a caller can set status="idle" and change is_persistent in the same edit.
Please rename persistent to is_persistent consistently on both BatchUpgradeOperation and UpgradeOperation.
For tests, I would add a batch with a mix of success and pending child operations. The expected behavior should be: progress does not count pending as completed, batch status remains active, pending operation can be cancelled, and another upgrade for the same device is blocked while the pending operation exists.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
2 times, most recently
from
45fad3d to
6d84175
Compare
Multiple Test Failures DetectedHello @Eeshu-Yadav, There are multiple test failures in your commit:
Fix: Please review the failing tests and their corresponding code to identify the root cause of the failures. Common issues include incorrect test setup, assertion errors, or unexpected behavior in the code being tested.
Fix: Escape backslashes in string literals and regular expressions by using double backslashes ( |
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
b32e764 to
f910f6a
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
f910f6a to
741148f
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| name="next_retry_at", | ||
| field=models.DateTimeField( | ||
| blank=True, | ||
| db_index=True, |
There was a problem hiding this comment.
WARNING: This migration adds a blocking index on upgradeoperation.next_retry_at
AddField(... db_index=True) will create the index with regular DDL during the deploy. On populated installations that can block writes to upgradeoperation, so this index should be moved into a separate non-atomic concurrent migration, for example with AddIndexConcurrently.
| if (completedInfo.length > 0) { | ||
| completedInfo.text(`${data.completed} out of ${data.total}`); | ||
| let pending = data.pending !== undefined ? data.pending : 0; | ||
| completedInfo.text(`${data.completed} complete, ${pending} pending`); |
There was a problem hiding this comment.
WARNING: This UI change is not covered by the existing browser test suite
The batch page now renders a new "X complete, Y pending" status string, but the Selenium coverage in openwisp_firmware_upgrader/tests/test_selenium.py still only exercises the older batch-status flow. Because this PR changes user-visible batch progress behavior and the project already has Selenium tests, a browser regression test should be added before merge.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
741148f to
509a56a
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
509a56a to
e25d8d8
Compare
Eeshu-Yadav
moved this to In progress
in [GSoC26] Persistent & Scheduled Firmware Upgrades
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
It's going in the right direction, next steps:
- admin
- docs: start adding a mention of this feature in the intro page, let's add a dedicated page for persistent upgrades
Keep pushing to this branch, I need to see the whole thing before I am confident in merging, if I merge now, reviewing the diff for other fragments will hide details that make my life as a reviewer harder. Go ahead, keep iterating, pushing, advancing.
Update the PR title and description when due, add screenshots when there's UI changes.
| batch_status, | ||
| stats["completed"], | ||
| stats["total_operations"], | ||
| stats["pending"], |
There was a problem hiding this comment.
check if the docs need to be updated due to this
There was a problem hiding this comment.
yes docs updated related to this
|
|
||
| def clean(self): | ||
| super().clean() | ||
| if not self._state.adding: |
There was a problem hiding this comment.
move this to a private method which should have a docstring summarizing what validation it enforces, we have this kind of pattern pretty much everywhere in openwisp
There was a problem hiding this comment.
moved to _validate_is_persistent_immutable in both classes
| completedInfo.text(`${data.completed} complete, ${pending} pending`); | ||
| } else { | ||
| completedInfo.text(`${data.completed} out of ${data.total}`); | ||
| } |
There was a problem hiding this comment.
add assertions in the existing selenium tests for this logic please
| ) | ||
| PERSISTENT_RETRY_DISPATCH_JITTER = getattr( | ||
| settings, "OPENWISP_FIRMWARE_UPGRADER_PERSISTENT_RETRY_DISPATCH_JITTER", 300 | ||
| ) |
There was a problem hiding this comment.
These need to be documented. Are you sure we can't add one setting with a dictionary for all of them to avoid bloating the docs?
| CELERY_BROKER_URL = "memory://" | ||
| CELERY_RESULT_BACKEND = "cache+memory://" | ||
|
|
||
| CELERY_BEAT_SCHEDULE = { |
There was a problem hiding this comment.
do we have issues in ansible-openwisp2 and docker-openwisp to add these?
There was a problem hiding this comment.
i have opened the issues in the ansible and docker repos :
openwisp/docker-openwisp#623
openwisp/ansible-openwisp2#621
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (1)
95-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPending should not be rendered as fully complete.
Line 100 makes pending operations display as
100%, which conflicts with pending being a non-terminal state.Suggested fix
const STATUSES_WITH_FULL_PROGRESS = new Set([ FW_UPGRADE_STATUS.SUCCESS, FW_UPGRADE_STATUS.FAILED, FW_UPGRADE_STATUS.ABORTED, FW_UPGRADE_STATUS.CANCELLED, - FW_UPGRADE_STATUS.PENDING, ]);🤖 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/static/firmware-upgrader/js/upgrade-utils.js` around lines 95 - 101, STATUSES_WITH_FULL_PROGRESS currently includes FW_UPGRADE_STATUS.PENDING which causes pending upgrades to render as 100%; remove FW_UPGRADE_STATUS.PENDING from the STATUSES_WITH_FULL_PROGRESS set in upgrade-utils.js (keep SUCCESS, FAILED, ABORTED, CANCELLED) and ensure any UI logic that checks STATUSES_WITH_FULL_PROGRESS continues to treat PENDING as a non-terminal state so it renders incremental progress rather than full progress.
🤖 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/settings.rst`:
- Around line 114-115: Update the docs entry that currently states reminders are
sent as ``generic_message`` to instead reference the actual notification type
``pending_upgrade_reminder`` so docs match the code; specifically, change the
wording in settings.rst to mention ``pending_upgrade_reminder`` (the type used
in notify.send(type="pending_upgrade_reminder", ...)) and ensure the description
clarifies that this notification is sent to the batch's organization admins and
all superusers.
In `@docs/user/upgrade-status.rst`:
- Around line 55-58: Update docs/user/upgrade-status.rst to clarify that the
"pending" upgrade state is considered an active/non-terminal state for deletion
guards: add a concise note in the User Actions paragraph (and mirror it in the
"Deleting Upgrade Operations" section) stating that pending operations cannot be
deleted immediately and are guarded the same as in-progress operations
(cancellation is allowed but direct deletion is blocked until they leave
pending). Use the term "pending" exactly and reference the deletion guard
behavior so readers don’t assume immediate deletion is permitted.
In `@openwisp_firmware_upgrader/api/serializers.py`:
- Around line 170-180: The current update method raises a ValidationError
whenever "is_persistent" is in validated_data regardless of batch state; change
the condition to only reject changes when the batch is not in the "idle" state
(e.g., if "is_persistent" in validated_data and instance.status != "idle": raise
the same ValidationError). Update the check inside the update(self, instance,
validated_data) method to gate the immutability on instance.status instead of
unconditionally blocking, keeping the existing error payload and returning
super().update(...) otherwise.
In `@openwisp_firmware_upgrader/apps.py`:
- Around line 69-96: The notification type display strings passed to
register_notification_type are not marked for translation; update the
verbose_name, verb, and email_subject values in the two
register_notification_type calls so they use the gettext_lazy alias _
(preserving placeholders like {site.name} and {notification.target})—i.e., wrap
the current literal strings for the "pending_upgrade_reminder" and
"persistent_upgrade_failed" registrations (which reference models
BatchUpgradeOperation, UpgradeOperation, Device) with _(...) to make them
translatable.
- Around line 165-168: The current try/except in connect_monitoring_signals()
swallows any ImportError when importing
openwisp_monitoring.device.signals.health_status_changed and can mask unrelated
errors; change the handler to only ignore a missing openwisp_monitoring
package/module (use ModuleNotFoundError or inspect the exception.name/message)
and re-raise other ImportError cases so real issues inside openwisp_monitoring
surface. Specifically, update the import block that references
health_status_changed to catch ModuleNotFoundError (or check e.name contains
"openwisp_monitoring" / "openwisp_monitoring.device.signals") and return only in
that case, otherwise re-raise the exception.
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 746-747: The progress_report function currently wraps f-strings in
_() so gettext cannot extract static msgids; change the two translatable strings
to use placeholder-based messages passed to _() (e.g. _("%(completed)s complete,
%(pending)s pending") and _("%(completed)s out of %(total)s")) and then format
them with .format() or % using the stats dict/ self.total_operations (refer to
progress_report, stats and self.total_operations) so translations can be
extracted.
- Around line 1117-1120: Wrap the user-facing log string in Django's translation
function so it is marked translatable (e.g. use gettext/gettext_lazy as _).
Locate the log call in the persistent-retry code path (the multi-line f-string
that includes {cause}, {self.retry_count}, and {self.next_retry_at}) and change
the string to _(f"...") (ensure you import gettext/gettext_lazy from
django.utils.translation at the top). Keep the f-string interpolation for cause,
self.retry_count and self.next_retry_at, but wrap the entire message with the
translation helper so Django i18n picks it up.
In `@openwisp_firmware_upgrader/settings.py`:
- Around line 27-38: The current assignment to PERSISTENT_RETRY_OPTIONS replaces
the entire options dict when
settings.OPENWISP_FIRMWARE_UPGRADER_PERSISTENT_RETRY_OPTIONS is provided, which
can drop required keys; change it to start from the default dict (base_delay,
multiplier, jitter, max_delay, dispatch_jitter, signal_jitter) and update/merge
with any keys present in
settings.OPENWISP_FIRMWARE_UPGRADER_PERSISTENT_RETRY_OPTIONS so partial
overrides only replace specific keys; locate the PERSISTENT_RETRY_OPTIONS
variable in settings.py and implement merging (e.g., copy default then
.update(override)) using those exact symbol names.
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.css`:
- Around line 1-3: Remove the global override of the CSS variable
--ow-color-danger in the :root block; instead either delete the :root {
--ow-color-danger: var(--ow-color-primary); } rule entirely or scope a
component-specific variable (e.g. .firmware-upgrader { --upgrader-color-danger:
var(--ow-color-primary); }) and update any uses within the firmware upgrader
styles to reference the new local variable; ensure no other files rely on a
global remap of --ow-color-danger.
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 203-208: The new user-facing strings in the completedInfo.text
calls are hard-coded in English; update the two branches that set
completedInfo.text (the branch using `${data.completed} complete, ${pending}
pending` and the else branch `${data.completed} out of ${data.total}`) to use
the project's JS translation helper (e.g., gettext/_.gettext/gettextCatalog
depending on your app) and format/interpolate data.completed, pending and
data.total via that function so the messages are localizable while preserving
the same content and variables.
In `@openwisp_firmware_upgrader/tasks.py`:
- Around line 170-179: In send_pending_upgrade_reminders the
BatchUpgradeOperation update sets last_reminder_at during the optimistic "claim"
step before recomputing pending_count, which means if pending_count becomes zero
due to concurrent changes you skip notify.send(...) but have already stamped
last_reminder_at and suppressed future reminders; change the flow so you only
update last_reminder_at after confirming pending_count > 0 and just before
calling notify.send(...). Concretely: use
BatchUpgradeOperation.objects.filter(pk=batch.pk).filter(due_condition).select_for_update()/claim
logic to test claim, compute pending_count from batch.upgradeoperation_set, and
only call
BatchUpgradeOperation.objects.filter(pk=batch.pk).update(last_reminder_at=timezone.now())
immediately before notify.send(...) (and only if pending_count > 0) to avoid
false suppression by due_condition.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js`:
- Around line 95-101: STATUSES_WITH_FULL_PROGRESS currently includes
FW_UPGRADE_STATUS.PENDING which causes pending upgrades to render as 100%;
remove FW_UPGRADE_STATUS.PENDING from the STATUSES_WITH_FULL_PROGRESS set in
upgrade-utils.js (keep SUCCESS, FAILED, ABORTED, CANCELLED) and ensure any UI
logic that checks STATUSES_WITH_FULL_PROGRESS continues to treat PENDING as a
non-terminal state so it renders incremental progress rather than full progress.
🪄 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: 1ffd1009-a023-4cb5-a1cb-755c988695dc
📒 Files selected for processing (30)
docs/user/intro.rstdocs/user/settings.rstdocs/user/upgrade-status.rstdocs/user/websocket-api.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.jsopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.rst
📄 CodeRabbit inference engine (AGENTS.md)
Update docs when behavior, settings, public APIs, setup steps, or supported versions change
Files:
docs/user/intro.rstdocs/user/websocket-api.rstdocs/user/upgrade-status.rstdocs/user/settings.rst
**/*.{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:
tests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/settings.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Mark user-facing strings for translation with Django i18n helpers in Django code
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/migrations/0006_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/settings.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.py
🧠 Learnings (6)
📚 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/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.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_batchupgradeoperation_last_reminder_at.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.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:
tests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/settings.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/tests/test_selenium.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.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/upgrade-progress.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.css
📚 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_selenium.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-03-07T01:16:15.471Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:15.471Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. For the UpgradeOperationAdmin.change_view, model-level has_view_permission(request) suffices to show breadcrumbs for batch, and object-level permission checks are not required.
Applied to files:
openwisp_firmware_upgrader/admin.py
🔇 Additional comments (18)
openwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.py (1)
1-67: LGTM!openwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.py (1)
1-24: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.py (1)
1-67: LGTM!tests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.py (1)
1-27: LGTM!openwisp_firmware_upgrader/tests/test_models.py (1)
10-22: LGTM!Also applies to: 593-988, 1067-1077
openwisp_firmware_upgrader/admin.py (1)
128-135: LGTM!Also applies to: 139-139, 227-227, 238-238, 253-253, 397-398, 404-404, 413-432, 439-441, 517-524, 528-528, 542-542, 555-555, 809-810
openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
82-85: LGTM!openwisp_firmware_upgrader/api/serializers.py (1)
88-127: LGTM!Also applies to: 133-145
openwisp_firmware_upgrader/api/views.py (1)
98-98: LGTM!Also applies to: 103-107
openwisp_firmware_upgrader/tests/test_admin.py (1)
203-283: LGTM!Also applies to: 2131-2176
openwisp_firmware_upgrader/websockets.py (1)
254-260: LGTM!Also applies to: 268-268, 336-336, 506-513, 525-525
openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.css (1)
22-42: LGTM!openwisp_firmware_upgrader/tests/test_websockets.py (1)
403-409: LGTM!Also applies to: 705-787
openwisp_firmware_upgrader/tests/test_selenium.py (1)
994-1045: LGTM!tests/openwisp2/settings.py (1)
3-3: LGTM!Also applies to: 191-200
openwisp_firmware_upgrader/tests/test_tasks.py (1)
1-164: LGTM!openwisp_firmware_upgrader/tests/test_handlers.py (1)
1-163: LGTM!openwisp_firmware_upgrader/tests/test_notifications.py (1)
1-209: LGTM!
| f"All immediate retries exhausted: {cause}. " | ||
| f"Scheduled persistent retry #{self.retry_count} " | ||
| f"at {self.next_retry_at}.", | ||
| save=False, |
There was a problem hiding this comment.
Mark persistent-retry log text for translation.
This new log line is user-facing in operation logs but is not wrapped with Django i18n helpers.
Suggested fix
- self.log_line(
- f"All immediate retries exhausted: {cause}. "
- f"Scheduled persistent retry #{self.retry_count} "
- f"at {self.next_retry_at}.",
- save=False,
- )
+ self.log_line(
+ _(
+ "All immediate retries exhausted: %(cause)s. "
+ "Scheduled persistent retry #%(retry_count)s at %(next_retry_at)s."
+ )
+ % {
+ "cause": cause,
+ "retry_count": self.retry_count,
+ "next_retry_at": self.next_retry_at,
+ },
+ save=False,
+ )As per coding guidelines, for Django pull requests all user-facing strings must be marked as translatable using Django i18n.
🤖 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 1117 - 1120, Wrap the
user-facing log string in Django's translation function so it is marked
translatable (e.g. use gettext/gettext_lazy as _). Locate the log call in the
persistent-retry code path (the multi-line f-string that includes {cause},
{self.retry_count}, and {self.next_retry_at}) and change the string to _(f"...")
(ensure you import gettext/gettext_lazy from django.utils.translation at the
top). Keep the f-string interpolation for cause, self.retry_count and
self.next_retry_at, but wrap the entire message with the translation helper so
Django i18n picks it up.
Resolve issues raised in review of the persistent mass upgrades work: - Only stamp last_reminder_at after confirming a batch still has pending children, so a concurrent drain no longer suppresses future reminders. - Gate the batch serializer's is_persistent immutability on the stored status so idle batches can still toggle the flag, matching the model's clean() and the error message. - Drop the global :root override of --ow-color-danger (a real, defined openwisp-utils token) that repainted unrelated danger UI; failed bars resolve to the danger colour directly. - Mark notification type strings, the progress_report text and the batch completion JS as translatable. - Narrow the monitoring import guard to ModuleNotFoundError so real import errors inside openwisp-monitoring surface instead of being swallowed. - Merge PERSISTENT_RETRY_OPTIONS overrides onto the defaults to avoid missing-key crashes on partial overrides. - Correct the reminder notification type name and document that pending operations are guarded against deletion. Related to #379
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
a92a594 to
baeef7c
Compare
|
@coderabbitai any outstanding issue? Please one last full review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== admin.py is_persistent fields ==="
grep -n "is_persistent\|retry_count\|next_retry_at\|IN_PROGRESS_STATUS\|pending" openwisp_firmware_upgrader/admin.py | head -60
echo ""
echo "=== serializers.py ==="
cat openwisp_firmware_upgrader/api/serializers.py
echo ""
echo "=== settings.py ==="
cat openwisp_firmware_upgrader/settings.py
echo ""
echo "=== tasks.py beat tasks ==="
grep -n "check_pending\|send_pending\|retry_pending\|CELERY_BEAT\|beat" openwisp_firmware_upgrader/tasks.py | head -30
echo ""
echo "=== test settings beat schedule ==="
grep -n "CELERY_BEAT" tests/openwisp2/settings.py
echo ""
echo "=== docs settings.rst ==="
grep -n "PERSISTENT\|pending" docs/user/settings.rst | head -40
echo ""
echo "=== docs upgrade-status.rst ==="
grep -n "pending\|Pending" docs/user/upgrade-status.rst | head -20
echo ""
echo "=== models.py - _recoverable_failure_handler, cancel, pending guard ==="
grep -n "pending\|_recoverable_failure\|CANCELLABLE\|concurrent\|handle_health\|notify_on_failed\|from_db\|_calculate_next" openwisp_firmware_upgrader/base/models.py | head -60
echo ""
echo "=== Check for max retry count cap ==="
grep -n "max_retry\|MAX_RETRY\|retry_count.*>\|> retry_count\|retry_count.*max\|max.*retry_count" openwisp_firmware_upgrader/base/models.py openwisp_firmware_upgrader/tasks.py openwisp_firmware_upgrader/settings.py 2>/dev/null
echo ""
echo "=== Check apps.py for signal/notification wiring ==="
grep -n "connect\|register\|signal\|notify\|health" openwisp_firmware_upgrader/apps.py | head -40
echo ""
echo "=== Check websockets.py ==="
grep -n "pending\|completed\|publish_batch" openwisp_firmware_upgrader/websockets.py | head -30
echo ""
echo "=== Check views.py for is_persistent ==="
grep -n "is_persistent" openwisp_firmware_upgrader/api/views.py
echo ""
echo "=== Check for any missing migrations dependency chain ==="
grep -n "dependencies" openwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.py
grep -n "dependencies" openwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.py
echo ""
echo "=== Check if BatchUpgradeConfirmationForm includes is_persistent ==="
grep -n "is_persistent\|BatchUpgradeConfirmationForm" openwisp_firmware_upgrader/admin.py | head -20
echo ""
echo "=== Check for clean() extra query issue ==="
grep -n "def clean\|_state.adding\|from_db\|is_persistent" openwisp_firmware_upgrader/base/models.py | head -30Length of output: 22346 🏁 Script executed: #!/bin/bash
echo "=== Full admin.py ==="
cat openwisp_firmware_upgrader/admin.py
echo ""
echo "=== Full models.py excerpt - clean and _recoverable_failure_handler ==="
grep -n "" openwisp_firmware_upgrader/base/models.py | grep -A30 "def clean\|_recoverable_failure_handler\|def cancel\|concurrent\|handle_health"Length of output: 51584
Final Review Report — PR
|
| # | Issue | Status |
|---|---|---|
| 1 | is_persistent not in Django admin |
✅ Fixed — added to UpgradeOperationAdmin (list/detail/filter) and BatchUpgradeOperationAdmin (list/detail/filter); BatchUpgradeConfirmationForm exposes it at launch time |
| 3 | New settings undocumented | ✅ Fixed — docs/user/settings.rst documents OPENWISP_FIRMWARE_UPGRADER_PERSISTENT_RETRY_OPTIONS and OPENWISP_FIRMWARE_UPGRADER_PERSISTENT_REMINDER_PERIOD |
| 4 | is_persistent absent from REST API serializers |
✅ Fixed — BatchUpgradeSerializer, UpgradeOperationSerializer, and DeviceUpgradeOperationSerializer all expose is_persistent; retry_count/next_retry_at are read-only; both serializers block is_persistent changes via update() |
| 7 | Beat schedule wiring | ✅ Fixed — tests/openwisp2/settings.py wires check_pending_upgrades (600 s) and send_pending_upgrade_reminders (604800 s) |
🟡 Remaining Low-Priority Items
1. signal_jitter undocumented in settings.rst
PERSISTENT_RETRY_OPTIONS in settings.py ships with a signal_jitter: 120 default key (used by handle_health_status_changed to add randomness when the monitoring signal fires). The settings.rst documentation mentions the other five keys but does not mention signal_jitter. Worth adding a line.
2. BatchUpgradeOperationAdmin: is_persistent always read-only
readonly_fields on BatchUpgradeOperationAdmin includes is_persistent unconditionally. The model's clean() permits changing it while status == "idle", but the admin never exposes an editable widget for it — meaning administrators cannot toggle it before launching. This is a very minor UX gap; the confirmation form covers the common case. If editable-before-launch is desired in the change view, is_persistent would need to be removed from readonly_fields and a conditional get_readonly_fields used instead.
3. Extra DB query in UpgradeOperation.clean() (pre-existing low-priority)
_validate_is_persistent_immutable issues one SELECT per call on existing instances. No change since last review; still low priority at scale.
4. retry_count unbounded (acknowledged by design)
The delay is capped at max_delay (12 h default), so the effective retry window doesn't grow, but the counter itself has no ceiling. No action required unless a future PR wants to add an optional max-retries-to-failed path.
Summary
The implementation is complete and well-tested. The core data model, retry pipeline, admin surfaces, REST API serializers, WebSocket integration, documentation, and Celery Beat schedule are all correctly in place. The only remaining actionable item worth tracking is the missing signal_jitter entry in settings.rst. Everything else is either by design or negligible.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
fa33375 to
19847b7
Compare
…ests #429 - time_travel context manager in tests/base.py freezes timezone.now() so the backoff, Beat-scan and reminder-cadence tests are deterministic. - test_persistence_loop_offline_then_back_online drives the loop: an offline op pends, the Beat scan retries it once due, it pends again with a later next_retry_at, and succeeds once the device is reachable again. - Selenium: the mass-upgrade confirmation page renders the is_persistent checkbox pre-checked and submitting propagates it to the batch and its children; a pending child renders the orange progress indicator. Related to #429
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
19847b7 to
b404531
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great to see so much progress here! See my comments below to continue improving the solution.
| ) | ||
| operation.save() | ||
| return | ||
| operation.save() |
There was a problem hiding this comment.
I think retry_pending_upgrade() still has a race after the atomic claim. The task flips pending to in-progress, loads the operation, then later calls operation.save() on that stale instance before dispatching upgrade_firmware. If an admin cancels the operation between the claim and this save, the save can write status="in-progress" back over cancelled and the task will still enqueue the upgrade. The deactivated-device branch can similarly overwrite a concurrent cancellation with failed. I would avoid saving the stale instance with all fields, make the failed transition conditional on status="in-progress", and re-check that the row is still in-progress immediately before dispatching.
There was a problem hiding this comment.
fixed — the deactivated branch flips to failed only via a conditional filter(status="in-progress").update(...), the dispatch save uses update_fields=["log"] so it never rewrites status, and we re-check the row is still in-progress before dispatch and again at the top of upgrade_firmware.
| ABORTED: gettext("aborted"), | ||
| CANCELLED: gettext("cancelled"), | ||
| IN_PROGRESS: gettext("in progress"), | ||
| PENDING: gettext("pending"), |
There was a problem hiding this comment.
Pending operations are cancellable in the model and REST API, and the new docs say they can be cancelled from the admin too, but the live operation/device page only renders the cancel button for FW_STATUS_GROUPS.IN_PROGRESS. A pending retry shown on the admin page has no cancel action, so the UI does not expose the backend behavior. I would treat pending as an active cancellable status here, render it with the pending progress class, and add a Selenium regression for the pending case.
There was a problem hiding this comment.
done , added a CANCELLABLE group (in-progress + pending), the cancel button renders for pending with the pending class, and added a selenium regression.
| model_name="batchupgradeoperation", | ||
| name="is_persistent", | ||
| field=models.BooleanField( | ||
| default=True, |
There was a problem hiding this comment.
Existing BatchUpgradeOperation rows are backfilled as is_persistent=True, even though every historical mass upgrade was non-persistent before this migration. This is visible in the admin/API for old rows and can change behavior for any queued idle batch that is launched after deploy. Let's backfill existing batches as False, then alter the model default to True for new batches. The sample app migration at tests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.py should mirror the same pattern .
There was a problem hiding this comment.
done,backfilled existing batches as False then altered the default to True; the sample migration mirrors it.
| ``next_retry_at`` has elapsed and re-dispatches them | ||
| - A device deactivated while pending is set to ``failed`` and not retried | ||
|
|
||
| **User Actions**: Pending operations can be cancelled the same way as |
There was a problem hiding this comment.
This new section says pending operations can be cancelled, but the cancellation section later still says cancellation is possible only while the status is in-progress. I would update that bullet to mention both in-progress and pending.
There was a problem hiding this comment.
updated the cancellation bullet and the cancel-button line to mention both in-progress and pending.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
4 times, most recently
from
5cc6274 to
1b86bff
Compare
| ) | ||
| qs = ( | ||
| BatchUpgradeOperation.objects.filter( | ||
| status="in-progress", upgradeoperation__status="pending" |
There was a problem hiding this comment.
WARNING: Reminder task can notify non-persistent batches
This queryset only filters on status="in-progress" plus pending children, so any batch that happens to have a pending child will emit pending_upgrade_reminder even when batch.is_persistent is False. The new reminder flow is documented and tested as persistence-specific behavior, so this should also constrain the batch queryset with is_persistent=True.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
1b86bff to
fa67c1f
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
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/migrations/0019_batchupgradeoperation_last_reminder_at.py (1)
16-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIndex
last_reminder_atto avoid periodic reminder scans degrading at scale.Line 19 defines
last_reminder_atwithout an index, but the reminder task filters on this field repeatedly (send_pending_upgrade_remindersdue-condition query). This can become a hot table scan path as batch rows increase.💡 Suggested fix
migrations.AddField( model_name="batchupgradeoperation", name="last_reminder_at", field=models.DateTimeField( blank=True, + db_index=True, help_text="timestamp of the last pending-upgrade reminder fired for this batch, null if no reminder has been sent yet", null=True, verbose_name="last reminder at", ), ),Also mirror this in:
openwisp_firmware_upgrader/base/models.py(last_reminder_at = models.DateTimeField(..., db_index=True, ...))tests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.py🤖 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/migrations/0019_batchupgradeoperation_last_reminder_at.py` around lines 16 - 24, The `last_reminder_at` DateTimeField in the migration lacks a database index, which causes performance degradation when the reminder task repeatedly filters on this field during `send_pending_upgrade_reminders` operations at scale. Add `db_index=True` parameter to the DateTimeField definition in the migration file. Additionally, apply the same change by adding `db_index=True` to the `last_reminder_at` field definition in the base model class in the models file, and mirror the change in the test sample migration file to keep them synchronized.
🤖 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/api/serializers.py`:
- Around line 115-145: Add an update() method to
DeviceUpgradeOperationSerializer that prevents is_persistent from being modified
after creation, mirroring the existing protection in
UpgradeOperationSerializer's update() method. Additionally, add status, log, and
progress to the read_only_fields tuple in the Meta class of both
UpgradeOperationSerializer and DeviceUpgradeOperationSerializer to explicitly
mark these worker-controlled fields as immutable through the API.
In `@openwisp_firmware_upgrader/base/models.py`:
- Around line 1115-1116: The user-facing strings in the two self.log_line calls
are not wrapped with Django i18n translation helpers, preventing them from being
extracted for localization. Wrap both string arguments passed to self.log_line
calls with Django's gettext translation function (typically imported as
underscore or gettext_lazy). For the f-string containing the cause variable on
line 1115, ensure the translation function wraps the entire f-string expression,
and do the same for the plain string literal on line 1116 to mark both messages
as translatable.
---
Outside diff comments:
In
`@openwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.py`:
- Around line 16-24: The `last_reminder_at` DateTimeField in the migration lacks
a database index, which causes performance degradation when the reminder task
repeatedly filters on this field during `send_pending_upgrade_reminders`
operations at scale. Add `db_index=True` parameter to the DateTimeField
definition in the migration file. Additionally, apply the same change by adding
`db_index=True` to the `last_reminder_at` field definition in the base model
class in the models file, and mirror the change in the test sample migration
file to keep them synchronized.
🪄 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: 6bcaa621-fa97-41a4-a9e6-aed72ed17265
⛔ Files ignored due to path filters (5)
docs/images/1.3/persistent-mass-upgrade-batch.pngis excluded by!**/*.pngdocs/images/1.3/persistent-mass-upgrade-confirm.pngis excluded by!**/*.pngdocs/images/1.3/persistent-upgrade-notifications.pngis excluded by!**/*.pngdocs/images/1.3/persistent-upgrade-operation-pending.pngis excluded by!**/*.pngdocs/images/1.3/persistent-upgrade-pending-changelist.pngis excluded by!**/*.png
📒 Files selected for processing (34)
docs/index.rstdocs/user/intro.rstdocs/user/persistent-mass-upgrades.rstdocs/user/settings.rstdocs/user/upgrade-status.rstdocs/user/websocket-api.rstopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/api/views.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/base/models.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.jsopenwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.jsopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.rst
📄 CodeRabbit inference engine (AGENTS.md)
Update docs when behavior, settings, public APIs, setup steps, or supported versions change
Files:
docs/index.rstdocs/user/intro.rstdocs/user/persistent-mass-upgrades.rstdocs/user/websocket-api.rstdocs/user/upgrade-status.rstdocs/user/settings.rst
**/*.{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/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/api/views.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.htmlopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_notifications.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file. Only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Mark user-facing strings for translation with Django i18n helpers in Django code
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:
openwisp_firmware_upgrader/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/api/views.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_notifications.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.py
🧠 Learnings (7)
📚 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/0019_batchupgradeoperation_last_reminder_at.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.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/migrations/0019_batchupgradeoperation_last_reminder_at.pyopenwisp_firmware_upgrader/api/views.pytests/openwisp2/sample_firmware_upgrader/migrations/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/settings.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_notifications.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tasks.pyopenwisp_firmware_upgrader/websockets.pyopenwisp_firmware_upgrader/apps.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/admin.pyopenwisp_firmware_upgrader/api/serializers.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.pyopenwisp_firmware_upgrader/base/models.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_batchupgradeoperation_last_reminder_at.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.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/migrations/0006_batchupgradeoperation_last_reminder_at.pytests/openwisp2/settings.pyopenwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_notifications.pytests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/base.pyopenwisp_firmware_upgrader/tests/test_api.pyopenwisp_firmware_upgrader/tests/test_notifications.pyopenwisp_firmware_upgrader/tests/test_selenium.pyopenwisp_firmware_upgrader/tests/test_admin.pyopenwisp_firmware_upgrader/tests/test_handlers.pyopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/tests/test_tasks.pyopenwisp_firmware_upgrader/tests/test_models.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/upgrade-selected-confirmation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.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.43.0)
openwisp_firmware_upgrader/migrations/0018_batchupgradeoperation_is_persistent_and_more.py
[info] 62-73: use help_text to document model columns
Context: models.CharField(
choices=[
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
("cancelled", "cancelled"),
("aborted", "aborted"),
("pending", "pending"),
],
default="in-progress",
max_length=12,
)
Note: Security best practice.
(model-help-text)
tests/openwisp2/sample_firmware_upgrader/migrations/0005_batchupgradeoperation_is_persistent_and_more.py
[info] 62-73: use help_text to document model columns
Context: models.CharField(
choices=[
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
("cancelled", "cancelled"),
("aborted", "aborted"),
("pending", "pending"),
],
default="in-progress",
max_length=12,
)
Note: Security best practice.
(model-help-text)
openwisp_firmware_upgrader/tasks.py
[info] 164-164: use secrets package over random package
Context: random.uniform(0, jitter)
Note: [CWE-330].
(avoid-random-python)
openwisp_firmware_upgrader/base/models.py
[info] 1061-1061: use secrets package over random package
Context: random.uniform(1 - jitter, 1 + jitter)
Note: [CWE-330].
(avoid-random-python)
[info] 1081-1081: use secrets package over random package
Context: random.uniform(0, jitter)
Note: [CWE-330].
(avoid-random-python)
🔇 Additional comments (32)
openwisp_firmware_upgrader/tests/test_models.py (3)
10-22: LGTM!
593-988: LGTM!
1067-1076: LGTM!openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-utils.js (1)
10-10: LGTM!Also applies to: 20-20, 35-35, 63-69, 86-86, 99-103
openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js (1)
252-255: LGTM!openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
203-216: LGTM!openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-progress.css (1)
52-55: LGTM!openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-selected-confirmation.css (1)
22-42: LGTM!openwisp_firmware_upgrader/websockets.py (1)
254-260: LGTM!Also applies to: 268-269, 336-337, 506-513, 525-525
docs/index.rst (1)
42-42: LGTM!docs/user/intro.rst (1)
14-16: LGTM!openwisp_firmware_upgrader/admin.py (6)
128-139: LGTM!
227-239: LGTM!Also applies to: 248-254
397-407: LGTM!
413-444: LGTM!
517-528: LGTM!Also applies to: 538-555
807-812: LGTM!openwisp_firmware_upgrader/templates/admin/upgrade_selected_confirmation.html (1)
82-85: LGTM!openwisp_firmware_upgrader/api/serializers.py (1)
86-96: LGTM!Also applies to: 170-180
openwisp_firmware_upgrader/api/views.py (1)
98-107: LGTM!openwisp_firmware_upgrader/tests/test_admin.py (1)
203-283: LGTM!Also applies to: 2131-2177
openwisp_firmware_upgrader/tests/test_api.py (1)
10-10: LGTM!Also applies to: 672-695, 1045-1065, 1949-1973, 2167-2229
openwisp_firmware_upgrader/tests/test_websockets.py (1)
403-409: LGTM!Also applies to: 705-787
openwisp_firmware_upgrader/tests/test_selenium.py (1)
278-311: LGTM!Also applies to: 378-411, 1062-1135
openwisp_firmware_upgrader/settings.py (1)
27-40: LGTM!openwisp_firmware_upgrader/apps.py (1)
3-3: LGTM!Also applies to: 30-35, 65-97, 128-132, 160-180
openwisp_firmware_upgrader/tasks.py (1)
2-13: LGTM!Also applies to: 35-46, 115-205
openwisp_firmware_upgrader/tests/base.py (1)
2-2: LGTM!Also applies to: 11-11, 27-33
openwisp_firmware_upgrader/tests/test_tasks.py (1)
1-14: LGTM!Also applies to: 64-74, 85-271
openwisp_firmware_upgrader/tests/test_handlers.py (1)
1-163: LGTM!openwisp_firmware_upgrader/tests/test_notifications.py (1)
1-209: LGTM!tests/openwisp2/settings.py (1)
3-3: LGTM!Also applies to: 191-201
| read_only_fields = ("retry_count", "next_retry_at") | ||
|
|
||
| def update(self, instance, validated_data): | ||
| if "is_persistent" in validated_data: | ||
| raise serializers.ValidationError( | ||
| { | ||
| "is_persistent": _( | ||
| "is_persistent cannot be changed after the upgrade " | ||
| "operation has been saved." | ||
| ) | ||
| } | ||
| ) | ||
| return super().update(instance, validated_data) | ||
|
|
||
|
|
||
| class DeviceUpgradeOperationSerializer(serializers.ModelSerializer): | ||
| class Meta: | ||
| model = UpgradeOperation | ||
| fields = ("id", "device", "image", "status", "log", "progress", "modified") | ||
| fields = ( | ||
| "id", | ||
| "device", | ||
| "image", | ||
| "is_persistent", | ||
| "retry_count", | ||
| "next_retry_at", | ||
| "status", | ||
| "log", | ||
| "progress", | ||
| "modified", | ||
| ) | ||
| read_only_fields = ("retry_count", "next_retry_at") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect operation serializers and their API view usage without executing repository code.
# Expected: if update-capable views use these serializers, task-owned fields are read-only
# and both serializers reject `is_persistent` updates.
rg -n -C4 --type=py \
'class .*UpgradeOperation.*(View|ViewSet)|serializer_class = (UpgradeOperationSerializer|DeviceUpgradeOperationSerializer)' \
openwisp_firmware_upgrader/api/views.py
rg -n -C8 --type=py \
'class (UpgradeOperationSerializer|DeviceUpgradeOperationSerializer)|read_only_fields|def update' \
openwisp_firmware_upgrader/api/serializers.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 6955
🏁 Script executed:
rg -n --type=py 'UpgradeOperationSerializer|DeviceUpgradeOperationSerializer' openwisp_firmware_upgrader/Repository: openwisp/openwisp-firmware-upgrader
Length of output: 3086
🏁 Script executed:
rg -n --type=py 'class (UpgradeOperationListView|UpgradeOperationDetailView|DeviceUpgradeOperationListView|DeviceUpgradeOperationDetailView)' -A 10 openwisp_firmware_upgrader/api/views.pyRepository: openwisp/openwisp-firmware-upgrader
Length of output: 1423
Add is_persistent update guard to DeviceUpgradeOperationSerializer for consistency.
UpgradeOperationSerializer guards against is_persistent changes via its update() method, but DeviceUpgradeOperationSerializer lacks this protection despite exposing the same field. While current API views are read-only (ListAPIView, RetrieveAPIView), add the guard for consistency and to prevent unintended mutation if update-capable endpoints are added in the future. Also consider explicitly marking status, log, and progress as read-only across both serializers since these are worker-controlled fields.
🤖 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/api/serializers.py` around lines 115 - 145, Add an
update() method to DeviceUpgradeOperationSerializer that prevents is_persistent
from being modified after creation, mirroring the existing protection in
UpgradeOperationSerializer's update() method. Additionally, add status, log, and
progress to the read_only_fields tuple in the Meta class of both
UpgradeOperationSerializer and DeviceUpgradeOperationSerializer to explicitly
mark these worker-controlled fields as immutable through the API.
| self.log_line(f"Detected a recoverable failure: {cause}.\n", save=False) | ||
| self.log_line("The upgrade operation will be retried soon.") |
There was a problem hiding this comment.
Mark recoverable-retry log lines for translation.
Line 1115 and Line 1116 add user-facing log messages without Django i18n wrapping, so they won’t be extracted for localization.
💡 Suggested fix
- self.log_line(f"Detected a recoverable failure: {cause}.\n", save=False)
- self.log_line("The upgrade operation will be retried soon.")
+ self.log_line(
+ _("Detected a recoverable failure: %(cause)s.") % {"cause": cause},
+ save=False,
+ )
+ self.log_line(_("The upgrade operation will be retried soon."))As per coding guidelines, for Django pull requests all user-facing strings must be marked as translatable using Django i18n helpers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.log_line(f"Detected a recoverable failure: {cause}.\n", save=False) | |
| self.log_line("The upgrade operation will be retried soon.") | |
| self.log_line( | |
| _("Detected a recoverable failure: %(cause)s.") % {"cause": cause}, | |
| save=False, | |
| ) | |
| self.log_line(_("The upgrade operation will be retried soon.")) |
🤖 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 1115 - 1116, The
user-facing strings in the two self.log_line calls are not wrapped with Django
i18n translation helpers, preventing them from being extracted for localization.
Wrap both string arguments passed to self.log_line calls with Django's gettext
translation function (typically imported as underscore or gettext_lazy). For the
f-string containing the cause variable on line 1115, ensure the translation
function wraps the entire f-string expression, and do the same for the plain
string literal on line 1116 to mark both messages as translatable.
Source: Coding guidelines
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The images have to be moved to the docs branch:
https://github.com/openwisp/openwisp-firmware-upgrader/tree/docs/docs/images
Also, 1.3 is openwisp 26, your work will end up in 1.4 (openwisp 27)
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I found a few outstanding issues that need to be addressed before merging.
| "or the operation is cancelled" | ||
| ), | ||
| ) | ||
| last_reminder_at = models.DateTimeField( |
There was a problem hiding this comment.
This field is used by send_pending_upgrade_reminders() to find due batches on every Beat run, but it is not indexed. On installations with many historical mass upgrades this can become a recurring scan before the pending-child join. I agree with CodeRabbit here: please add db_index=True on the model field and mirror it in 0019_batchupgradeoperation_last_reminder_at.py and the sample migration.
There was a problem hiding this comment.
added db_index=True on last_reminder_at and mirrored it in 0019 and the sample 0006 migration.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
2 times, most recently
from
ff4d389 to
a6f27f0
Compare
| name="last_reminder_at", | ||
| field=models.DateTimeField( | ||
| blank=True, | ||
| db_index=True, |
There was a problem hiding this comment.
WARNING: last_reminder_at is added with db_index=True, so Django emits a blocking CREATE INDEX (no CONCURRENTLY) on the populated batchupgradeoperation table during deploy.
This is the same class of deploy-time lock already flagged on next_retry_at (migration 0018, line 45). The AddField itself is safe (null=True, no backfill), but on a large table the index build takes a brief ACCESS EXCLUSIVE lock. Consider running this migration during a maintenance window, or building the index concurrently via a separate RunSQL operation (Django cannot emit CONCURRENTLY from db_index).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
a6f27f0 to
b6f5543
Compare
Test Failure in Firmware UpgraderHello @Eeshu-Yadav, There is one test failure:
|
Related to #379
Eeshu-Yadav
force-pushed
the
issues/417-persistence-schema-fields
branch
from
b6f5543 to
8c60dc3
Compare
Checklist
Reference to Existing Issue
Closes #417, closes #422, closes #423, closes #424.
Related to #379 (parent — Persistent Mass Upgrades, Phase 1).
Description of Changes
Today, when a mass upgrade hits an offline device the operation goes from
in-progresstofailedonce Celery's auto-retries run out (about 10 minutes). The admin then has to chase down each failed device by hand. Fine for one device, painful at thirty, unmanageable at three hundred.This PR makes the upgrader stop giving up. Offline devices transition to a new
pendingstatus with a backoff-scheduled retry time, a periodic Beat task picks them up later and re-dispatches the upgrade, and the batch stays active until every device is either upgraded or cancelled. Four sub-issues land together because the new state only makes sense when its schema (#417), its producer (#422), the code that has to recognise it (#423), and the Beat scanner that actually retries it (#424) all ship in one piece — splitting them either leaves the enum dead or quietly breaks the existing batch failure path.What this PR makes possible:
pendingwith a scheduled next-retry time, and a Beat task runs every ten minutes (configurable) picking up due retries and re-dispatching the upgrade. Each retry follows an exponential backoff (10m → 20m → 40m → ... capped at 12h, ±25% jitter) until the device comes back or the admin cancels.pendingas a blocking state.failedrather than attempting to flash a device that's been decommissioned."X complete, Y pending"on first paint and on every live WebSocket update, with both renders driven by the same count so the page does not flicker between formats when the socket connects.is_persistentflag is locked in for the lifetime of the run — an admin can no longer flip the policy halfway through a batch.is_persistentflag, so single-device upgrades can opt into persistence too (off by default for standalone upgrades).openwisp-monitoringinstalled, a device coming back online retries its pending operations immediately, without waiting for the next Beat scan.Screenshots :
