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.

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

Conversation

johannaengland
Copy link
Contributor

@johannaengland johannaengland commented Nov 17, 2023

This was caused by #2667. It changed how the values are saved (as ['AD'] for example instead of 'AD'). This shows again, that we need even more tests for the alert profiles views.

Thanks to @runeki for finding this bug!

@johannaengland johannaengland requested a review from hmpf November 17, 2023 11:04
@johannaengland johannaengland self-assigned this Nov 17, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9dbc1d0) 55.52% compared to head (4a61445) 55.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2750   +/-   ##
=======================================
  Coverage   55.52%   55.52%           
=======================================
  Files         567      567           
  Lines       41172    41175    +3     
=======================================
+ Hits        22861    22864    +3     
  Misses      18311    18311           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Nov 17, 2023

Test results

     12 files       12 suites   11m 34s ⏱️
3 260 tests 3 260 ✔️ 0 💤 0
9 255 runs  9 255 ✔️ 0 💤 0

Results for commit 4a61445.

♻️ This comment has been updated with latest results.

python/nav/web/alertprofiles/forms.py Outdated Show resolved Hide resolved
Comment on lines +670 to +672
validated_data["value"] = "|".join(value)
elif operator_type == Operator.EQUALS:
validated_data["value"] = value[0]
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!

tests/integration/web/alertprofiles_test.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Nov 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johannaengland johannaengland requested a review from hmpf November 17, 2023 14:18
@johannaengland johannaengland merged commit d0f3a5b into Uninett:master Nov 17, 2023
13 checks passed
@johannaengland johannaengland deleted the bugfix/alertprofiles/add-group-to-expression branch November 17, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants