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

Handle invalid alert address in alert sending #2661

Merged

Conversation

johannaengland
Copy link
Contributor

When we try to send an alert, but the alert address is empty currently no alert is sent, an error is logged, but it is reported back as a successful sending, which can become very confusing.

This PR fixes that and even widens the application. Before sending an alert the address it is to be sent to is validated and in case it is invalid an error is logged and an InvalidAlertAddressError is thrown. That is caught further up and the alert is deleted in that case and the sending is reported back as failed.

This popped up due to #1787.

@johannaengland johannaengland self-assigned this Aug 14, 2023
@johannaengland johannaengland changed the base branch from master to 5.6.x August 14, 2023 14:59
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #2661 (ff071d7) into 5.6.x (92acff9) will increase coverage by 0.04%.
The diff coverage is 92.30%.

❗ Current head ff071d7 differs from pull request most recent head c666afd. Consider uploading reports for the commit c666afd to get more accurate results

@@            Coverage Diff             @@
##            5.6.x    #2661      +/-   ##
==========================================
+ Coverage   54.13%   54.17%   +0.04%     
==========================================
  Files         558      558              
  Lines       40641    40647       +6     
==========================================
+ Hits        22000    22021      +21     
+ Misses      18641    18626      -15     
Files Changed Coverage Δ
python/nav/models/profiles.py 71.44% <90.90%> (+1.91%) ⬆️
python/nav/alertengine/dispatchers/__init__.py 48.78% <100.00%> (+2.62%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Aug 14, 2023

Test results

     12 files       12 suites   11m 30s ⏱️
3 237 tests 3 141 ✔️   96 💤 0
9 186 runs  8 898 ✔️ 288 💤 0

Results for commit c666afd.

♻️ This comment has been updated with latest results.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

I don't like the implementation of the address validation itself - but the rest looks peachy :)

python/nav/models/profiles.py Outdated Show resolved Hide resolved
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

🎉

@johannaengland johannaengland force-pushed the invalid-alert-address-error branch from ff071d7 to c666afd Compare September 6, 2023 06:38
@sonarcloud
Copy link

sonarcloud bot commented Sep 6, 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
No Duplication information No Duplication information

@johannaengland johannaengland merged commit a9536a6 into Uninett:5.6.x Sep 6, 2023
10 checks passed
@johannaengland johannaengland deleted the invalid-alert-address-error branch September 6, 2023 07:15
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