netbox-acls
netbox-acls copied to clipboard
Fixes #47 [Housekeeping]: Develop Plugin Tests - Models
Pull Request
Related Issue
Fixes: #47 [Housekeeping] Develop Plugin Tests - Models
New Behavior
- Adds model tests for the NetBox ACLs Plugin to improve test coverage.
Contrast to Current Behavior
- Introduces comprehensive test cases, increasing coverage and helping detect side effects in new development.
Discussion: Benefits and Drawbacks
- Benefits:
- Enhances confidence in changes by ensuring the plugin’s model behavior is verified.
- Prevents regressions by catching issues early during development.
- Drawbacks:
- No significant drawbacks, as this is an improvement to test coverage.
Changes to the Documentation
- Documentation may be updated to outline the extended test coverage for new features.
Proposed Release Note Entry
- Expanded test coverage for NetBox ACLs Plugin models.
Attribution
Some of the code in this PR is based on previous work by @ryanmerolle in PR #134.
Double Check
- [x] I have explained my PR according to the information in the comments or in a linked issue.
- [x] My PR targets the
devbranch.
Note: This is a draft PR.
I rebased the work by @ryanmerolle from PR #134, cleaned up the codebase, and added additional tests. I'll be adding tests for ACLStandardRule and ACLExtendedRule in the coming days.
I included a fix for Bug #258, as it was necessary to properly test the uniqueness of Interface and Direction.
Additionally, I cleaned up the model files access_lists.py and access_list_rules.py and added i18n (internationalization) support.
I appreciate your time reviewing all these changes. Thank you, @cruse1977 !
Hi @cruse1977,
Thank you again for taking the time to review this PR.
I wanted to mention that I’m happy to break out some of the enhancements and housekeeping changes (such as the i18n additions and the model file cleanups) into separate issues or pull requests if that would help streamline the review process or better align with the project's contribution practices.
Please let me know what you prefer, and I’ll adjust accordingly.
Thanks again for your guidance and support!
Hi @cruse1977,
Quick update on this PR:
- I’ve rebased the branch on the current
devto stay up to date. - The TODO change has been dropped to keep the scope focused.
- The internationalization (i18n) changes have been split into their own feature request and a separate PR for clarity and easier review.
- I also removed the model changes that would have required a migration, as they were outside the scope of adding model tests.
- The changes related to issue #258 have been removed to keep this PR focused solely on test coverage.
One note: the following commit is still part of this PR:
refactor(models): Centralize ACL rule validation
Moves ACL rule validation logic from forms to model methods to ensure
consistent enforcement across all usages. This improves maintainability
and eliminates redundant validation code.
These changes are necessary for the tests to pass. I'm happy to extract this into its own issue and PR if you prefer, but in that case, this models test PR would need to wait for those changes to be merged first.
Let me know how you’d like to proceed. Thanks again for your feedback and support!