Skip to content

Conversation

@itdependsnetworks
Copy link
Contributor

@itdependsnetworks itdependsnetworks commented Sep 27, 2025

@github-actions
Copy link

github-actions bot commented Sep 28, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  nautobot_firewall_models
  details.py 21, 23, 27-28, 30
  nautobot_firewall_models/models
  nat_policy.py
  security_policy.py
  zone.py
  nautobot_firewall_models/views
  __init__.py
  address.py
  capirca_policy.py 42-44
  nat_policy.py 71, 83
  security_policy.py 77, 89
  service.py
  user.py
  zone.py
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Contributor

@gsnider2195 gsnider2195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this looks good! Just found a couple of small issues.

pyproject.toml Outdated
python = ">=3.9.2,<3.13"
# Used for local development
nautobot = ">=2.4.2,<3.0.0"
nautobot = ">=2.4.19,<3.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should bump the minor version of firewall models with this change and update the compatibility matrix.

{% include 'nautobot_firewall_models/inc/policy_rules_tablerow.html' with rule=rule parent_policy=True %}
{% endfor %}
{% include "nautobot_firewall_models/inc/policyrule_tablehead.html" %}
{% if object.policy_rules %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be object.policy_rules.exists? I made this change locally.

Before:

image

After:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, something is odd, I am seeing the inverse. With .exists it looks like your after.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My "after" screenshot above was after changing it to use .exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confusing myself.. will re-look at it tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, looking at it, I presume the screenshot is where there is no rule in a set of policies, thus there should be no row. How it is, does currently meets my expectation.

  • What this does: it a single template that normalizes policy_rules from policy
  • What it does not do: Set a default of empty rule

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can evaluate a model manager (Policy.policy_rules) as boolean.

>>> Policy.objects.all()
<RestrictedQuerySet [<Policy: Policy 1>, <Policy: Policy 2>, <Policy: Policy 3>]>
>>> type(Policy.objects.all()[0].policy_rules)
>>> django.db.models.fields.related_descriptors.create_forward_many_to_many_manager.<locals>.ManyRelatedManager
>>> Policy.objects.all()[0].policy_rules.all()
<RestrictedQuerySet [<PolicyRule: Policy Rule 1 - req1>]>
>>> Policy.objects.all()[1].policy_rules.all()
<RestrictedQuerySet [<PolicyRule: Policy Rule 1 - req1>, <PolicyRule: Policy Rule 2 - req2>]>
>>> Policy.objects.all()[2].policy_rules.all()
<RestrictedQuerySet []>
>>> bool(Policy.objects.all()[0].policy_rules)
True
>>> bool(Policy.objects.all()[1].policy_rules)
True
>>> bool(Policy.objects.all()[2].policy_rules)
True

{% if object.policy_rules %} will never evaluate to False so you'll never hit the else in this if/else. The screenshot I'm showing above is what it looks like when the else gets rendered correctly with nautobot_firewall_models/inc/policy_rules_tablerow.html

@itdependsnetworks
Copy link
Contributor Author

Not going to worry about poetry.lock until 2.4.20 is published.

@itdependsnetworks itdependsnetworks merged commit de6455c into nautobot:develop Oct 14, 2025
16 checks passed
@smk4664 smk4664 mentioned this pull request Nov 14, 2025
@gsnider2195 gsnider2195 mentioned this pull request Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants