Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix form validation with equal and in operator for adding expression with group to filter #2750

Merged
merged 4 commits into from
Nov 17, 2023
Merged

Fix form validation with equal and in operator for adding expression with group to filter #2750

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4af77bb
Fix form validation with equal and in operator
johannaengland Nov 17, 2023
f33204c
Add more tests for expression validation
johannaengland Nov 17, 2023
e045c42
Break out ip validation in seperate function
johannaengland Nov 17, 2023
4a61445
Rename expression tests to focus on operator
johannaengland Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions python/nav/web/alertprofiles/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,16 +663,12 @@ def clean(self) -> Dict[str, Any]:
)
else:
validated_ip_addresses.append(str(ip))
# Bring ip address back into original format to be processed below
value = " ".join(validated_ip_addresses)
validated_data["value"] = "|".join(validated_ip_addresses)
return validated_data
johannaengland marked this conversation as resolved.
Show resolved Hide resolved

if operator_type == Operator.IN:
"""If input was a multiple choice list we have to join each option in one
string, where each option is separated by a | (pipe).
If input was a IP adress we should replace space with | (pipe)."""
if match_field.data_type == MatchField.IP:
validated_data["value"] = value.replace(' ', '|')
else:
validated_data["value"] = "|".join(value)
validated_data["value"] = "|".join(value)
elif operator_type == Operator.EQUALS:
validated_data["value"] = value[0]
Comment on lines +659 to +661
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do early returns. If you plan one for every operator_type this order is good, since EQUALS and IN are the most common (exists for the most match_fields).

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are other cases of match_fields behaving oddly you can split out handling of those too, like for ip-addresses. Isolate 'em!

Copy link
Contributor

Choose a reason for hiding this comment

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

The odd ones are "ip_address", "severity" and "sysname". "sysname" can do partial string matching, "severity" does numerical comparison. They're both candidates for their own methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look to me that the actual cleaning for "sysname" and "severity" will look identical to EQUALS. If you do an early return after IN you could remove the "if" for EQUALS and just do value = value[0]. "severity" might need casting to int but that should be it!


return validated_data
24 changes: 24 additions & 0 deletions tests/integration/web/alertprofiles_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,30 @@ def test_alertprofiles_add_expression_with_multiple_alert_types_should_succeed(
response.content
)

def test_alertprofiles_add_expression_with_valid_group_should_succeed(
johannaengland marked this conversation as resolved.
Show resolved Hide resolved
self, client, dummy_filter
):
"""Tests that an expression with a valid group can be added"""
group_match_field = MatchField.objects.get(name="Group")
url = reverse("alertprofiles-filters-saveexpression")
data = {
"filter": dummy_filter.pk,
"match_field": group_match_field.pk,
"operator": Operator.EQUALS,
"value": "AD",
}
response = client.post(url, data=data, follow=True)
assert response.status_code == 200
assert Expression.objects.filter(
filter=dummy_filter,
match_field=group_match_field,
operator=Operator.EQUALS,
value=data["value"],
).exists()
assert f"Added expression to filter {dummy_filter}" in smart_str(
response.content
)


class TestsPermissions:
def test_set_accountgroup_permissions_should_not_crash(self, db, client):
Expand Down
Loading