[feature] Allow zero subnets in SubnetDivisionRuleFix/allow zero subnets#1287
[feature] Allow zero subnets in SubnetDivisionRuleFix/allow zero subnets#1287peterishakvr12 wants to merge 11 commits intoopenwisp:masterfrom
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 change allows AbstractSubnetDivisionRule.number_of_subnets to be 0 by updating its MinValueValidator from 1 to 0 and expanding the help_text to state that 0 means assigning IPs from the main subnet. The model's clean() now auto-fills organization_id from master_subnet.organization_id when master_subnet_id is set and organization_id is not provided. Tests were updated to accept number_of_subnets=0 (removed the expectation that zero raises a ValidationError). Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.Add a Ruff configuration file to your project to customize how CodeRabbit runs |
Black and Flake8 Failures in
|
![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 the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/subnet_division/base/models.py`:
- Around line 38-43: The help_text string literal passed to the model field (the
help_text argument just above validators=[MinValueValidator(0)]) is missing a
closing double-quote; fix by closing the string before the closing parenthesis
so the help_text becomes a valid quoted string literal (i.e., add the missing "
to terminate the string in that field definition).
- Around line 36-44: Test expects number_of_subnets=0 to raise a ValidationError
but the model field number_of_subnets (PositiveSmallIntegerField with
MinValueValidator(0)) deliberately allows 0; update the test in test_models.py
to treat 0 as valid instead of asserting a ValidationError — remove the
assertRaises/validation check for value 0 (or change it to assert that
instance.full_clean() passes or that the instance is valid) and adjust any
expected error message checks accordingly so the test accepts
number_of_subnets=0.
🪄 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: e031f080-985e-4ded-be4c-08d73598c9a8
📒 Files selected for processing (1)
openwisp_controller/subnet_division/base/models.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). (10)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.py
🧠 Learnings (3)
📚 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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.py
🔇 Additional comments (1)
openwisp_controller/subnet_division/base/models.py (1)
76-82: LGTM!The auto-fill logic correctly uses
master_subnet_idfor the initial check to avoid an unnecessary query, then appropriately copiesorganization_idwhen the master subnet has an organization set. This aligns with the validation at lines 154-160 that requires organization consistency.
|
Hi @nemesifier, I'm Peter, a "Python/Django/Flask" backend developer Please let me know if any changes are needed. I'm happy to |
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/subnet_division/base/models.py`:
- Line 76: Remove the redundant inline comment inside the clean() method (the
comment that states "Auto-fill organization from master subnet") since it simply
repeats what the code does; delete that comment line from the clean() function
in models.py and leave the implementation unchanged to improve readability.
🪄 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: f933a35c-4545-4a3c-8c1f-f875bac4b13e
📒 Files selected for processing (1)
openwisp_controller/subnet_division/base/models.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.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.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.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | 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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.py
🧠 Learnings (3)
📚 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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.py
🔇 Additional comments (1)
openwisp_controller/subnet_division/base/models.py (1)
38-43: Good update: validator and help text now align with zero-subnet behavior.This change is coherent and correctly marks the user-facing text as translatable.
| return import_string(self.type) | ||
|
|
||
| def clean(self): | ||
| # Auto-fill organization from master subnet |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant inline comment in clean().
The comment at Line [76] repeats what the code already makes clear and can be dropped for cleaner readability.
As per coding guidelines, "Avoid unnecessary comments or docstrings for code that is already clear."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/subnet_division/base/models.py` at line 76, Remove the
redundant inline comment inside the clean() method (the comment that states
"Auto-fill organization from master subnet") since it simply repeats what the
code does; delete that comment line from the clean() function in models.py and
leave the implementation unchanged to improve readability.
Missing Migrations and Test FailuresHello @peterishakvr12, There are two main issues identified in the CI failure logs:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/subnet_division/base/models.py (1)
76-76: 🧹 Nitpick | 🔵 TrivialRemove the redundant inline comment in
clean().Line [76] repeats what the following condition already states.
As per coding guidelines, "Avoid unnecessary comments or docstrings for code that is already clear."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/subnet_division/base/models.py` at line 76, Remove the redundant inline comment "Auto-fill organization from master subnet" inside the clean() method of the SubnetDivision model; the condition logic already makes this behavior explicit, so delete that single-line comment in the clean() function (search for clean() in the SubnetDivision class) and leave the code unchanged otherwise.
🤖 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/subnet_division/base/models.py`:
- Line 43: The model currently allows number_of_subnets=0 via
validators=[MinValueValidator(0)] but the validation methods
_validate_master_subnet_consistency and _validate_ip_address_consistency assume
at least one subnet and therefore reject or miscompute zero-subnet cases; fix by
either tightening the field validator to MinValueValidator(1) if zero should be
disallowed, or (preferable for zero-subnet support) keep MinValueValidator(0)
and update _validate_master_subnet_consistency and
_validate_ip_address_consistency to early-return (no-op) when
self.number_of_subnets == 0 so they skip child-subnet checks and avoid
referencing a non-existent target subnet size.
---
Duplicate comments:
In `@openwisp_controller/subnet_division/base/models.py`:
- Line 76: Remove the redundant inline comment "Auto-fill organization from
master subnet" inside the clean() method of the SubnetDivision model; the
condition logic already makes this behavior explicit, so delete that single-line
comment in the clean() function (search for clean() in the SubnetDivision class)
and leave the code unchanged otherwise.
🪄 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: 1f0248d7-5a69-4b97-9392-ffe23a0243f6
📒 Files selected for processing (1)
openwisp_controller/subnet_division/base/models.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.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.py
🧠 Learnings (4)
📚 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 **/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp} : Avoid unnecessary comments or docstrings for code that is already clear
Applied to files:
openwisp_controller/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.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/subnet_division/base/models.py
🔇 Additional comments (1)
openwisp_controller/subnet_division/base/models.py (1)
77-83: Good improvement: organization auto-fill is correctly placed before custom consistency checks.This helps prevent avoidable organization mismatch failures when
organization_idis omitted butmaster_subnetis set.
| "Set to 0 to assign IP addresses directly " | ||
| "from the main subnet." | ||
| ), | ||
| validators=[MinValueValidator(0)], |
There was a problem hiding this comment.
Allowing 0 here still conflicts with non-zero subnet assumptions in validation.
Line [43] enables number_of_subnets=0, but current validators still enforce child-subnet assumptions (_validate_master_subnet_consistency and _validate_ip_address_consistency). This can reject valid zero-subnet scenarios or validate against the wrong target subnet size.
💡 Suggested alignment for zero-subnet mode
diff --git a/openwisp_controller/subnet_division/base/models.py b/openwisp_controller/subnet_division/base/models.py
@@
- available = 2 ** (self.size - master_subnet.prefixlen)
- # Account for the reserved subnet
- available -= 1
- if self.number_of_subnets >= available:
+ available = 2 ** (self.size - master_subnet.prefixlen)
+ # Account for the reserved subnet only when subnets are requested
+ available -= 1
+ if self.number_of_subnets > 0 and self.number_of_subnets >= available:
raise ValidationError(
@@
def _validate_ip_address_consistency(self):
try:
- next(
- ip_network(str(self.master_subnet.subnet)).subnets(new_prefix=self.size)
- )[self.number_of_ips - 1]
+ target_subnet = ip_network(str(self.master_subnet.subnet))
+ if self.number_of_subnets > 0:
+ target_subnet = next(target_subnet.subnets(new_prefix=self.size))
+ target_subnet[self.number_of_ips - 1]
except IndexError:
raise ValidationError(
{
"number_of_ips": _(
f"Generated subnets of size /{self.size} cannot accommodate "
f"{self.number_of_ips} IP Addresses."
)
}
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/subnet_division/base/models.py` at line 43, The model
currently allows number_of_subnets=0 via validators=[MinValueValidator(0)] but
the validation methods _validate_master_subnet_consistency and
_validate_ip_address_consistency assume at least one subnet and therefore reject
or miscompute zero-subnet cases; fix by either tightening the field validator to
MinValueValidator(1) if zero should be disallowed, or (preferable for
zero-subnet support) keep MinValueValidator(0) and update
_validate_master_subnet_consistency and _validate_ip_address_consistency to
early-return (no-op) when self.number_of_subnets == 0 so they skip child-subnet
checks and avoid referencing a non-existent target subnet size.
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/subnet_division/tests/test_models.py`:
- Around line 136-140: The current test swallows any ValidationError by catching
it and only asserting "number_of_subnets" is not in e.message_dict; instead,
remove the try/except and call rule.full_clean() directly so the test fails if
any ValidationError is raised for number_of_subnets=0; alternatively, if you
must inspect the ValidationError, rework the block to let unexpected
ValidationError fail the test and only assert that e.message_dict does not
contain the "number_of_subnets" key when a ValidationError is intentionally
expected. Ensure changes reference the existing rule.full_clean() call and
ValidationError handling.
🪄 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: fe4c281f-3e82-459d-b9f5-311b84687b73
📒 Files selected for processing (1)
openwisp_controller/subnet_division/tests/test_models.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.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.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.2.0
- GitHub Check: Python==3.11 | 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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.py
🧠 Learnings (4)
📚 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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.py
Multiple CI Failures DetectedHello @peterishakvr12, Black Formatting Errors
Flake8 Linting Errors
Migrations Check Failed
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/subnet_division/tests/test_models.py`:
- Line 137: Fix the indentation for the statement calling rule.full_clean()
inside the test's with block so it matches the surrounding lines (remove one
extra leading space); locate the call to rule.full_clean() in the test_models.py
with-block and align it with the other statements in that block to avoid the
IndentationError.
- Around line 132-137: The integration test should ensure that when a
SubnetDivisionRule is created with number_of_subnets=0 the provisioning assigns
IPs from the master subnet; update behavior in SubnetDivisionRule by changing
create_subnets and/or create_ips so that if number_of_subnets == 0
create_subnets returns a single entry representing the master subnet (or make
create_ips detect zero and iterate over the master subnet) and then generate IP
addresses from that master subnet; add a new integration test that creates a
rule with number_of_subnets=0, triggers provisioning (e.g., add the
template/config that calls SubnetDivisionRule.provision or equivalent code
path), and assert IP objects were created with addresses within the master
subnet.
🪄 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: 79cd90dc-2a74-4729-b0e5-d828030efdc5
📒 Files selected for processing (1)
openwisp_controller/subnet_division/tests/test_models.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.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.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
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.py
🧠 Learnings (3)
📚 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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.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/subnet_division/tests/test_models.py
| with self.subTest("Test rule allows zero subnets"): | ||
| options = default_options.copy() | ||
| options["number_of_subnets"] = 0 | ||
| rule = SubnetDivisionRule(**options) | ||
| with self.assertRaises(ValidationError) as context_manager: | ||
| rule.full_clean() | ||
| expected_message_dict = { | ||
| "number_of_subnets": [ | ||
| "Ensure this value is greater than or equal to 1." | ||
| ] | ||
| } | ||
| self.assertDictEqual( | ||
| context_manager.exception.message_dict, expected_message_dict | ||
| ) | ||
| # Should not raise ValidationError for number_of_subnets | ||
| rule.full_clean() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if create_subnets handles number_of_subnets=0 specially
rg -n -A 20 'def create_subnets' openwisp_controller/subnet_division/rule_types/base.py
# Check if there's any special handling for zero subnets in provisioning
rg -n 'number_of_subnets.*==.*0|number_of_subnets.*0' openwisp_controller/subnet_division/Repository: openwisp/openwisp-controller
Length of output: 2452
🏁 Script executed:
# Check the create_ips method to see if it handles empty subnets list
rg -n -B 5 -A 25 'def create_ips' openwisp_controller/subnet_division/rule_types/base.py
# Look for any special handling or comments about number_of_subnets=0
rg -n -B 3 -A 3 'number_of_subnets.*0' openwisp_controller/subnet_division/rule_types/
# Check the SubnetDivisionRule model for constraints or validation
rg -n -A 30 'class SubnetDivisionRule' openwisp_controller/subnet_division/models.pyRepository: openwisp/openwisp-controller
Length of output: 2118
🏁 Script executed:
# Find the AbstractSubnetDivisionRule model definition
fd -t f 'models.py' openwisp_controller/subnet_division/ | head -5
# Read the abstract model to understand field validation
rg -n -B 5 -A 50 'class AbstractSubnetDivisionRule' openwisp_controller/subnet_division/models.py
# Look for any logic or comments about assigning IPs directly from master subnet
rg -n 'master_subnet|direct.*IP|assign.*direct' openwisp_controller/subnet_division/Repository: openwisp/openwisp-controller
Length of output: 14305
🏁 Script executed:
# Get more context on the number_of_subnets field definition
rg -n -B 5 -A 15 'number_of_subnets.*=' openwisp_controller/subnet_division/base/models.py | head -60
# Check if there's any TODO or FIXME comments related to number_of_subnets=0
rg -n 'TODO|FIXME|XXX' openwisp_controller/subnet_division/rule_types/base.py
# Look at the full create_ips method for any number_of_subnets=0 handling
rg -n -A 40 'def create_ips' openwisp_controller/subnet_division/rule_types/base.py | head -70
# Check if there are any other tests about number_of_subnets=0 provisioning
rg -n -B 5 -A 10 'number_of_subnets.*0' openwisp_controller/subnet_division/tests/test_models.pyRepository: openwisp/openwisp-controller
Length of output: 7150
Add integration test to verify IP provisioning behavior when number_of_subnets=0.
The current test validates that number_of_subnets=0 passes model validation, but the documented feature—"assign IP addresses directly from the main subnet"—is not implemented. When number_of_subnets=0, the create_subnets method produces an empty list, causing create_ips to generate zero IP addresses.
Add an integration test that:
- Creates a rule with
number_of_subnets=0 - Triggers provisioning (e.g., adding the template to a config)
- Asserts that IP addresses are assigned directly from the master subnet
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/subnet_division/tests/test_models.py` around lines 132 -
137, The integration test should ensure that when a SubnetDivisionRule is
created with number_of_subnets=0 the provisioning assigns IPs from the master
subnet; update behavior in SubnetDivisionRule by changing create_subnets and/or
create_ips so that if number_of_subnets == 0 create_subnets returns a single
entry representing the master subnet (or make create_ips detect zero and iterate
over the master subnet) and then generate IP addresses from that master subnet;
add a new integration test that creates a rule with number_of_subnets=0,
triggers provisioning (e.g., add the template/config that calls
SubnetDivisionRule.provision or equivalent code path), and assert IP objects
were created with addresses within the master subnet.
CI Failures in OpenWISP ControllerHello @peterishakvr12, There are three distinct failures in the CI logs:
Failure 1: Code Style/QA (Black and Flake8)Explanation: Remediation: openwisp-qa-formatThis command will reformat the code according to the project's style guidelines, resolving the indentation problems. Failure 2: Build/Infrastructure/Other (Migrations Check)Explanation: Remediation: ./manage.py makemigrations openwisp_controller/subnet_divisionThis will create a new migration file (likely Failure 3: Test Failure (Multiple Test Failures)Explanation: Remediation:
|
@peterishakvr12 Hello there. have you joined the Matrix chats general and dev in openwisp, and introduced yourself? And you should talk there for finding new bugs or seeing issues, as well as attempting to meet the community and talk with them, get to know them (with projects and stuff, not personal lives). Also before attempting to fix the issue you should either comment on the currently open issue, asking, "Since no one is working on this, Can I?" for example. and the same in the matrix chats. Please do that and then the maintainers will atleast know of your existence and your PR, and then you can patiently wait and let them review your PR, and help you resolve the issues, and ultimately, merge the PR. |
Black and Flake8 FailuresHello @peterishakvr12, (Analysis for commit a3c6540) There are multiple Python code style and syntax errors detected by Black and Flake8. Fix: The following specific errors require manual attention:
Please review these files and correct the syntax and indentation issues manually. Test FailuresThe CI also failed due to test errors. The logs indicate a Fix: |
Multiple CI Failures DetectedHello @peterishakvr12, The CI pipeline failed due to the following issues:
|
3 participants
Reference to Existing Issue
Closes #843
Description of Changes
Allow specifying 0 in the "Number of Subnets" field so that
the rule does not trigger the creation of a new subnet but
just assigns IP addresses directly from the main subnet.
Updated help_text to document the new behavior.
Screenshot
No UI changes, backend model fix only.