Stop using media_slugs verbatim - #1953
Conversation
Test results 8 files 1 744 suites 3m 12s ⏱️ Results for commit c44d281. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1953 +/- ##
==========================================
+ Coverage 88.52% 88.61% +0.08%
==========================================
Files 148 148
Lines 7135 7173 +38
==========================================
+ Hits 6316 6356 +40
+ Misses 819 817 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
There is such a thing as too much consistency.
I'm thinking we should instead have a constant "EMAIL_DESTINATION_SLUG", as nothing not in the tree below argus.notificationprofile.media should be imported out. But where to put it? It could be on argus.notificationprofile.media, it could be on argus.notificationprofile too maybe.
None of the other media types are special cased like this.
Another possibility is moving the signal and only attaching it if the email plugin is in use but that's a lot more work and takes more thinking.
There was a problem hiding this comment.
It is more okay with the v2 API since that is deprecated anyway.
There was a problem hiding this comment.
I added the constant to media since that seemed most natural. It's not ideal but it works.
There was a problem hiding this comment.
See the comment on signals. I could have said the same here but this serializer is after all deprecated so when it is deleted all sins will be forgotten =)
994dc73 to
c44d281
Compare
|



Scope and purpose
Fixes #1923.
Add a description of what this PR does and why it is needed. If a linked ticket(s) fully
cover it, you can omit this.
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.