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

Debug notification sending #760

Merged
merged 6 commits into from
Apr 19, 2024
Merged

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Apr 16, 2024

Merge after #690, might need to be adapted.

@hmpf hmpf added debug Changes that makes debugging easier notification Affects the notification system labels Apr 16, 2024
Copy link

github-actions bot commented Apr 16, 2024

Test results

       7 files     511 suites   19m 37s ⏱️
   409 tests    408 ✔️ 1 💤 0
2 863 runs  2 856 ✔️ 7 💤 0

Results for commit 582e82c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

I also really would like some more tests for the send method to also document that way when we expect it to return False and when to return True.

src/argus/notificationprofile/media/email.py Show resolved Hide resolved
src/argus/notificationprofile/media/email.py Outdated Show resolved Hide resolved
@hmpf hmpf force-pushed the debug-notification-sending branch from a89651c to a3d4c3a Compare April 17, 2024 11:57
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 56.75676% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 78.78%. Comparing base (cce5eed) to head (582e82c).
Report is 1 commits behind head on master.

Files Patch % Lines
src/argus/notificationprofile/media/email.py 53.33% 7 Missing ⚠️
...rc/argus/notificationprofile/media/sms_as_email.py 22.22% 7 Missing ⚠️
src/argus/notificationprofile/media/__init__.py 0.00% 1 Missing ⚠️
src/argus/notificationprofile/media/base.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
- Coverage   79.00%   78.78%   -0.23%     
==========================================
  Files          74       74              
  Lines        3635     3653      +18     
==========================================
+ Hits         2872     2878       +6     
- Misses        763      775      +12     

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

@hmpf hmpf force-pushed the debug-notification-sending branch from a3d4c3a to c5198ee Compare April 18, 2024 06:49
@hmpf hmpf force-pushed the debug-notification-sending branch from c5198ee to a5c8c70 Compare April 18, 2024 06:49
@hmpf hmpf requested a review from johannaengland April 18, 2024 06:49
@johannaengland
Copy link
Contributor

And I would like a changelog entry, because now we're actually changing the way that notifications are sent (since we break in sms if we encounter an error)

@hmpf
Copy link
Contributor Author

hmpf commented Apr 18, 2024

This'll actually take multiple changelogs, which means this should have been multiple PRs.

@hmpf hmpf requested a review from johannaengland April 18, 2024 10:53
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hmpf hmpf merged commit 91c4fea into Uninett:master Apr 19, 2024
10 checks passed
@hmpf hmpf deleted the debug-notification-sending branch April 19, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug Changes that makes debugging easier notification Affects the notification system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants