[fix] Corrected initial field value assignment in AbstractDevice class#1301
[fix] Corrected initial field value assignment in AbstractDevice class#1301nemesifier merged 2 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe patch modifies _get_initial_values_for_checked_fields() to restore only fields that were actually present after deferred-field detection by iterating saved present_values. For each (field, value) in present_values it sets initial{field} from the refreshed instance’s current attribute (getattr(self, field)) and then restores the instance field to the saved pre-refresh value. A test was added (test_deferred_fields_populated_correctly) that verifies _initial_last_ip contains the persisted value after saving a deferred field. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
moved this from To do (general)
to To do (Python & Django)
in OpenWISP Contributor's Board
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/base/device.py`:
- Around line 341-343: The loop currently iterates over
self._changed_checked_fields but then indexes present_values[field], which
raises KeyError because present_values only contains deferred fields that were
refreshed; change the loop to iterate over present_values.keys() (or
present_values) so only refreshed fields are processed, and keep the existing
setattr calls that set _initial_{field} from getattr(self, field) and then
assign setattr(self, field, present_values[field]).
🪄 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: 70fe1648-0ef1-4a3d-a2d6-bc0498e23d9a
📒 Files selected for processing (1)
openwisp_controller/config/base/device.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/base/device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/base/device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/base/device.py
**/*.{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_controller/config/base/device.py
🧠 Learnings (4)
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/base/device.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/base/device.py
github-project-automation
bot
moved this from To do (Python & Django)
to In progress
in OpenWISP Contributor's Board
pandafy
force-pushed
the
fix-device-get-deferred-values
branch
from
320571b to
4145eed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 544-557: The test test_deferred_fields_populated_correctly should
also assert that the instance retains the user-updated value after calling
Device._check_changed_fields(); after invoking device._check_changed_fields(),
add an assertion that device.last_ip == "172.16.1.1" (while still asserting
getattr(device, "_initial_last_ip") == "172.16.0.1"), referencing the test
function name and the Device._check_changed_fields and _initial_last_ip
attributes to locate where to add the extra assertion.
🪄 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: 9f9cfc99-0109-4550-94a2-1c527064a1bf
📒 Files selected for processing (2)
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
**/*.{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_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
🧠 Learnings (5)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/base/device.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_device.py (4)
openwisp_controller/config/tests/utils.py (1)
_create_device(31-44)openwisp_controller/config/models.py (1)
Device(16-23)tests/openwisp2/sample_config/models.py (1)
Device(26-32)openwisp_controller/config/base/device.py (1)
_check_changed_fields(309-315)
🔇 Additional comments (1)
openwisp_controller/config/base/device.py (1)
342-344: Guarded update correctly avoids deferred-field lookup errors.Line 342 now safely restricts updates to keys present in
present_values, and Line 343 correctly stores the refreshed DB value in_initial_<field>before restoring the in-memory value.
| def test_deferred_fields_populated_correctly(self): | ||
| device = self._create_device( | ||
| name="deferred-test", | ||
| last_ip="172.16.0.1", | ||
| management_ip="10.0.0.1", | ||
| ) | ||
| # Load the instance with deferred fields omitted | ||
| device = Device.objects.only("id").get(pk=device.pk) | ||
| device.last_ip = "172.16.1.1" | ||
| # Populate initial values for checked fields | ||
| device._check_changed_fields() | ||
| # Ensure `_initial_<field>` contains the actual value, not the field name | ||
| self.assertEqual(getattr(device, "_initial_last_ip"), "172.16.0.1") | ||
| self.assertNotEqual(getattr(device, "_initial_last_ip"), "last_ip") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the regression test with a post-check value assertion.
This test verifies _initial_last_ip, but it should also assert that last_ip still equals the user-updated value after Line 554.
✅ Suggested test addition
# Ensure `_initial_<field>` contains the actual value, not the field name
self.assertEqual(getattr(device, "_initial_last_ip"), "172.16.0.1")
self.assertNotEqual(getattr(device, "_initial_last_ip"), "last_ip")
+ self.assertEqual(device.last_ip, "172.16.1.1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/tests/test_device.py` around lines 544 - 557, The
test test_deferred_fields_populated_correctly should also assert that the
instance retains the user-updated value after calling
Device._check_changed_fields(); after invoking device._check_changed_fields(),
add an assertion that device.last_ip == "172.16.1.1" (while still asserting
getattr(device, "_initial_last_ip") == "172.16.0.1"), referencing the test
function name and the Device._check_changed_fields and _initial_last_ip
attributes to locate where to add the extra assertion.
4145eed to
4ea7ace
Compare
|
@nemesifier this will needed to be backported to 1.2 branch. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_device.py (1)
554-557:⚠️ Potential issue | 🟡 MinorAdd a post-check assertion for the current field value.
After Line 554, assert that
device.last_ipis still"172.16.1.1"so the test verifies both sides of the contract (initial DB snapshot + preserved in-memory update).Suggested test addition
device._check_changed_fields() # Ensure `_initial_<field>` contains the actual value, not the field name self.assertEqual(getattr(device, "_initial_last_ip"), "172.16.0.1") self.assertNotEqual(getattr(device, "_initial_last_ip"), "last_ip") + self.assertEqual(device.last_ip, "172.16.1.1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_device.py` around lines 554 - 557, Add a post-check assertion to verify the current in-memory field value is preserved: after calling device._check_changed_fields() and after asserting getattr(device, "_initial_last_ip") equals "172.16.0.1", add an assertion that device.last_ip == "172.16.1.1" so the test validates both the stored initial snapshot (_initial_last_ip) and the preserved in-memory update (last_ip).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 554-557: Add a post-check assertion to verify the current
in-memory field value is preserved: after calling device._check_changed_fields()
and after asserting getattr(device, "_initial_last_ip") equals "172.16.0.1", add
an assertion that device.last_ip == "172.16.1.1" so the test validates both the
stored initial snapshot (_initial_last_ip) and the preserved in-memory update
(last_ip).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cb91baf-6ac8-4750-b141-52407df11385
📒 Files selected for processing (2)
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.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). (9)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
**/*.{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_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
🧠 Learnings (5)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_device.pyopenwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/base/device.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_device.py (3)
openwisp_controller/config/tests/utils.py (1)
_create_device(31-44)openwisp_controller/connection/tests/utils.py (1)
_create_device(51-56)openwisp_controller/config/base/device.py (1)
_check_changed_fields(309-315)
🔇 Additional comments (1)
openwisp_controller/config/base/device.py (1)
341-343: Deferred-field restoration fix looks correct.This correctly scopes restoration to
present_values, captures refreshed DB values into_initial_<field>, and restores the in-memory value without the previous key-mismatch risk.
Test Failure in
|
Test Failure in Device FieldHello @pandafy, The CI failed due to an Fix: The test is likely attempting to verify the population of deferred fields. The attribute name |
| # Populate initial values for checked fields | ||
| device._check_changed_fields() | ||
| # Ensure `_initial_<field>` contains the actual value, not the field name | ||
| self.assertEqual(getattr(device, "_initial_last_ip"), "172.16.0.1") |
There was a problem hiding this comment.
The test fails here, let's make sure the test fails with a clear error message without the patch.
| device = Device.objects.only("id").get(pk=device.pk) | ||
| device.last_ip = "172.16.1.1" | ||
| # Populate initial values for checked fields | ||
| device._check_changed_fields() |
There was a problem hiding this comment.
Is there a way to accomplish the same here with a public method? This would make the test more readable.
When a Device instance was loaded with deferred fields (e.g. using QuerySet.only()), `_check_changed_fields()` incorrectly populated the `_initial_<field>` attributes. Instead of storing the original value retrieved from the database, it stored the field name itself. This happened because the code iterated over `_changed_checked_fields` and assigned the field name rather than the actual value coming from `present_values`. The logic has been updated to iterate over `present_values.items()`, store the current database value in `_initial_<field>`, and then restore the updated value on the instance. This ensures change detection works correctly even when fields were initially deferred. Changelog (Bugfix): Fixed incorrect initialization of `_initial_<field>` values when a Device instance is loaded with deferred fields, which could break change detection logic.
pandafy
force-pushed
the
fix-device-get-deferred-values
branch
from
4ea7ace to
fd4f6b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_device.py (1)
544-557:⚠️ Potential issue | 🟠 MajorExercise
save()here instead of the private helper.This only proves
_check_changed_fields()in isolation. The regression happens on the publicdevice.save()path, so this can stay green while deferred-field updates still lose the old DB value in production. Please drive the scenario throughsave()and assert an observable effect there, such asmanagement_ip_changedsignal args or WHOIS input.Based on learnings: "Regression test must fail if the patch is removed or commented out."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_device.py` around lines 544 - 557, Replace the direct call to the private helper `_check_changed_fields()` in the test `test_deferred_fields_populated_correctly` with exercising the public `Device.save()` path: load the instance via `Device.objects.only("id").get(...)`, modify `device.last_ip`, then call `device.save()` (or `device.save(update_fields=...)` as appropriate) and capture/assert the observable effect (e.g., subscribe to the `management_ip_changed` signal or inspect WHOIS input) to verify `_initial_last_ip` is populated with the DB value ("172.16.0.1") and not the field name; keep assertions on `getattr(device, "_initial_last_ip")` but drive the population through `Device.save()` so the regression would fail if the fix is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/base/device.py`:
- Around line 340-343: The helper currently sets _initial_<field> after the DB
write (via refresh_from_db()), causing deferred updates to appear as
already-applied; move the logic that populates _initial_<field> so it runs
before the write and before _check_last_ip()/_check_changed_fields(): call
self.refresh_from_db(fields=present_values.keys()) and then run the loop that
does setattr(self, f"_initial_{field}", getattr(self, field)) prior to invoking
super().save() (or before the call site that triggers _check_last_ip()/
_check_changed_fields()), ensuring _initial_* holds the pre-save values for
present_values.
---
Duplicate comments:
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 544-557: Replace the direct call to the private helper
`_check_changed_fields()` in the test `test_deferred_fields_populated_correctly`
with exercising the public `Device.save()` path: load the instance via
`Device.objects.only("id").get(...)`, modify `device.last_ip`, then call
`device.save()` (or `device.save(update_fields=...)` as appropriate) and
capture/assert the observable effect (e.g., subscribe to the
`management_ip_changed` signal or inspect WHOIS input) to verify
`_initial_last_ip` is populated with the DB value ("172.16.0.1") and not the
field name; keep assertions on `getattr(device, "_initial_last_ip")` but drive
the population through `Device.save()` so the regression would fail if the fix
is removed.
🪄 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: e3129ff1-9de5-43bf-81a9-ed0b8bb32aa3
📒 Files selected for processing (2)
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
**/*.{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_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
🧠 Learnings (6)
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/config/base/device.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/base/device.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Regression test must fail if the patch is removed or commented out
Applied to files:
openwisp_controller/config/tests/test_device.py
| self.refresh_from_db(fields=present_values.keys()) | ||
| for field in self._changed_checked_fields: | ||
| setattr(self, f"_initial_{field}", field) | ||
| setattr(self, field, present_values[field]) | ||
| for field, value in present_values.items(): | ||
| setattr(self, f"_initial_{field}", getattr(self, field)) | ||
| setattr(self, field, value) |
There was a problem hiding this comment.
This still captures the post-save value on the public update path.
save() does not reach this helper until after super().save() on Line 290 via _check_changed_fields() on Line 299. For a deferred field changed through device.save(), refresh_from_db() now reads the just-written value, so _initial_<field> becomes the new value and _check_*_changed() / _check_last_ip() no-op.
Please populate _initial_* before the write (and before _check_last_ip() on Line 292), otherwise deferred updates are still missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/base/device.py` around lines 340 - 343, The helper
currently sets _initial_<field> after the DB write (via refresh_from_db()),
causing deferred updates to appear as already-applied; move the logic that
populates _initial_<field> so it runs before the write and before
_check_last_ip()/_check_changed_fields(): call
self.refresh_from_db(fields=present_values.keys()) and then run the loop that
does setattr(self, f"_initial_{field}", getattr(self, field)) prior to invoking
super().save() (or before the call site that triggers _check_last_ip()/
_check_changed_fields()), ensuring _initial_* holds the pre-save values for
present_values.
There was a problem hiding this comment.
For a deferred field changed through device.save(), refresh_from_db() now reads the just-written value, so initial becomes the new value and check*_changed() / _check_last_ip() no-op.
If a deferred field is updated in the database, the the intial_<field> shall reflect the latest value from the database. We are not interested in the original value of the field.
@coderabbitai is my assumption correct?
There was a problem hiding this comment.
@pandafy, yes, your assumption is correct. I need to retract my earlier concern.
Tracing the execution order in save():
super().save()— writes the new value to the DB_check_last_ip()— for a deferred field,_initial_last_ip == models.DEFERRED, so it returns early (pre-existing guard)_check_changed_fields()→_get_initial_values_for_checked_fields()— this is where the fix applies
After step 3, _initial_<field> is set to the just-saved DB value. This is intentionally consistent with how the non-deferred path works: _check_last_ip() and _check_management_ip_changed() both end with self._initial_<field> = self.<field>, i.e., the post-save current value, which becomes the baseline for the next comparison.
The fix in this PR correctly addresses the bug (the field name string being stored instead of the actual value), and the behavior of _initial_<field> reflecting the latest DB value after a save is the right design.
My original comment was mistaken in treating the "missed signal for deferred field on first save" as a problem introduced here — that's governed by the pre-existing if self._initial_last_ip == models.DEFERRED: return guard and is separate from the bug this PR fixes.
🧠 Learnings used
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_device.py (1)
544-557:⚠️ Potential issue | 🟠 MajorAssert the original DB value in
_initial_last_ip, not the updated value.On Line 556, the expected value should be the pre-save persisted value (
"172.16.0.1"). Also assertdevice.last_ipremains"172.16.1.1"aftersave()to verify restoration behavior.Proposed fix
def test_deferred_fields_populated_correctly(self): device = self._create_device( name="deferred-test", last_ip="172.16.0.1", management_ip="10.0.0.1", ) # Load the instance with deferred fields omitted device = Device.objects.only("id").get(pk=device.pk) device.last_ip = "172.16.1.1" # Saving the device object will populate the deferred fields device.save() - # Ensure `_initial_<field>` contains the actual value, not the field name - self.assertEqual(getattr(device, "_initial_last_ip"), "172.16.1.1") + # Ensure `_initial_<field>` contains the original DB value, not the field name + self.assertEqual(getattr(device, "_initial_last_ip"), "172.16.0.1") self.assertNotEqual(getattr(device, "_initial_last_ip"), "last_ip") + self.assertEqual(device.last_ip, "172.16.1.1")Based on learnings: "Regression test must fail if the patch is removed or commented out".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_device.py` around lines 544 - 557, The test test_deferred_fields_populated_correctly currently asserts _initial_last_ip equals the updated value; change it to assert that getattr(device, "_initial_last_ip") == "172.16.0.1" (the pre-save DB value) and add an assertion that device.last_ip == "172.16.1.1" after device.save() to confirm the instance's last_ip was restored; update assertions around Device.objects.only(...), device.save(), _initial_last_ip and device.last_ip accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 544-557: The test test_deferred_fields_populated_correctly
currently asserts _initial_last_ip equals the updated value; change it to assert
that getattr(device, "_initial_last_ip") == "172.16.0.1" (the pre-save DB value)
and add an assertion that device.last_ip == "172.16.1.1" after device.save() to
confirm the instance's last_ip was restored; update assertions around
Device.objects.only(...), device.save(), _initial_last_ip and device.last_ip
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 776ca9af-7716-4dfe-95ef-49846651cea6
📒 Files selected for processing (1)
openwisp_controller/config/tests/test_device.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). (9)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/tests/test_device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/tests/test_device.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/tests/test_device.py
**/*.{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_controller/config/tests/test_device.py
🧠 Learnings (5)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-03-14T19:34:33.773Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.773Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Regression test must fail if the patch is removed or commented out
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/tests/test_device.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_device.py
Test Failures in
|
Test Failures in CI PipelineHello @pandafy, There are two test failures in the CI pipeline:
Please review the code related to |
pandafy
force-pushed
the
fix-device-get-deferred-values
branch
from
3e0db75 to
1a5fb89
Compare
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
|
❌ Cherry-pick to git fetch origin 1.2
git checkout -b backport/1301-to-1.2-1774630174 origin/1.2
git cherry-pick -x 0d17acdd75fa8d4bb49c47cf2e52516b72503568
# resolve conflicts
git cherry-pick --continue
git push origin backport/1301-to-1.2-1774630174 |
|
❌ Cherry-pick to git fetch origin 1.2
git checkout -b backport/1301-to-1.2-1774646484 origin/1.2
git cherry-pick -x 0d17acdd75fa8d4bb49c47cf2e52516b72503568
# resolve conflicts
git cherry-pick --continue
git push origin backport/1301-to-1.2-1774646484 |
|
❌ Cherry-pick to git fetch origin 1.2
git checkout -b backport/1301-to-1.2-1774655973 origin/1.2
git cherry-pick -x 0d17acdd75fa8d4bb49c47cf2e52516b72503568
# resolve conflicts
git cherry-pick --continue
git push origin backport/1301-to-1.2-1774655973 |
3 participants
Checklist
Description of Changes
When a Device instance was loaded with deferred fields (e.g. using
QuerySet.only()),
_check_changed_fields()incorrectly populated the_initial_<field>attributes. Instead of storing the original valueretrieved from the database, it stored the field name itself.
This happened because the code iterated over
_changed_checked_fieldsand assigned the field name rather than the actual value coming from
present_values.The logic has been updated to iterate over
present_values.items(),store the current database value in
_initial_<field>, and thenrestore the updated value on the instance.
This ensures change detection works correctly even when fields were
initially deferred.
Changelog (Bugfix):
Fixed incorrect initialization of
_initial_<field>values when aDevice instance is loaded with deferred fields, which could break
change detection logic.