Move common functionality to destination base class #936
Conversation
hmpf
changed the title
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
c39d564 to
55a2ff0
Compare
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
55a2ff0 to
5989b0a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #936 +/- ##
==========================================
- Coverage 78.48% 78.26% -0.22%
==========================================
Files 141 141
Lines 5442 5512 +70
==========================================
+ Hits 4271 4314 +43
- Misses 1171 1198 +27 ☔ View full report in Codecov by Sentry. |
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
3b78212 to
7b8826c
Compare
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
0e23752 to
3131826
Compare
|
There was a problem hiding this comment.
I just checked and we don't have a test for trying to PATCH update with medium being empty, that should be added, I think then it would be clearer if the changes work or not
We do have tests for when trying to change the medium that that will lead to an error
docs/integrations/notifications/writing-notification-plugins.rst
Outdated
Show resolved
Hide resolved
We are now way out of scope for a refactor. I guess the underlying problem is that we cannot lookup and change destinations of a specific media type directly, as part of the url of the endpoint. If we could, we would never need bother with media in the update-serializer. |
Yes, I agree, I can do this in another PR, but that would have to merged before this then, because it would show if the functionality is staying the same |
|
I don't think it is wise to stress with finishing this PR this week anymore, so start on the patch-PR next week. |
hmpf
force-pushed
the
cleanup-destination-plugins
branch
2 times, most recently
from
d1eb78c to
97cc505
Compare
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
8a84843 to
d329536
Compare
|
I decided to pass in the DestinationConfig instance (if any) to the clean-method so that it will have access to the currently stored settings. |
Originally posted by @johannaengland in #1118 (comment) |
|
I got stuff mostly working with this but the main problem is extracting errors to show in the related fields |
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
d329536 to
ee9eb86
Compare
hmpf
force-pushed
the
cleanup-destination-plugins
branch
3 times, most recently
from
38130e1 to
282b4d2
Compare
.. also make it easioer to validate the settings-field with a django form.
hmpf
force-pushed
the
cleanup-destination-plugins
branch
from
282b4d2 to
304367b
Compare
|
| if cls.has_duplicate(user.destinations, form.cleaned_data): | ||
| form.add_error( | ||
| None, | ||
| DjangoValidationError( | ||
| "This %(media)s destination already exists", | ||
| code="duplicate", | ||
| params={"media": cls.MEDIA_SLUG}, | ||
| ), | ||
| ) | ||
| return form |
There was a problem hiding this comment.
This error message does not specify what is a duplicate: the label, the settings value or both. When testing #1161, it was not clear what exactly the problem was when this message was shown. If this is a problem with this part of the code, or a problem the frontend elements in #1161 should fix I am not sure, but I'll just leave the comment here
| @@ -67,23 +185,52 @@ def send(cls, event: Event, destinations: Iterable[DestinationConfig], **kwargs) | |||
| pass | |||
|
|
|||
| @classmethod | |||
| def raise_if_not_deletable(cls, destination: DestinationConfig) -> NoneType: | |||
| def is_not_deletable(cls, destination: DestinationConfig) -> dict[str, Any]: | |||
There was a problem hiding this comment.
why is the value type Any? Seems like it can only ever be a str
| return {} | ||
|
|
||
| @classmethod | ||
| def raise_if_not_deletable(cls, destination: DestinationConfig) -> NoneType: |
There was a problem hiding this comment.
Pretty sure None should be used as return type over NoneType, but this seems to be a relic from the past and not really introduced in this PR
Best reviewed per file.