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 82.44% 82.45%
=======================================
Files 132 132
Lines 4866 4884 +18
=======================================
+ Hits 4012 4027 +15
- Misses 854 857 +3 ☔ 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
| if there is only one piece of data you need to send the notification. Among | ||
| other things, it is used to check for duplicate entries, so in a way it acts | ||
| as the primary key for your plugin. For that reason, it must be required in | ||
| the json schema. |
There was a problem hiding this comment.
| the json schema. | |
| the json schema. For example for email it is ``"email_address"`` or for SMS | |
| it is ``"phone_number"``. |
| cls, data: dict, user: User, instance: DestinationConfig = None, exception_class=ValidationError | ||
| ) -> dict: | ||
| if instance: | ||
| if data.get("media", "") != instance.media.slug: |
There was a problem hiding this comment.
But in the case of updating an instance (PATCH) when no media is given then this will raise this error
| if data.get("media", "") != instance.media.slug: | |
| if "media" in data.keys() and data["media"] != instance.media.slug: |
| if self.instance: | ||
| medium = api_safely_get_medium_object(self.instance.media.slug) | ||
| # A PATCH need not contain the media key | ||
| if attrs.get("media", None) != self.instance.media: |
There was a problem hiding this comment.
Also here: If we have PATCH and no media in the attrs, then this will fail since None != EmailMedium for example
| if attrs.get("media", None) != self.instance.media: | |
| if "media" in attrs.keys() and attrs["media"] != self.instance.media: |
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. |
| def validate(cls, instance: RequestDestinationConfigSerializer, dict: dict, user: User) -> dict: | ||
| def validate( | ||
| cls, data: dict, user: User, instance: DestinationConfig = None, exception_class=ValidationError | ||
| ) -> dict: |
There was a problem hiding this comment.
return type should be CommonDestinationConfigForm?
| if not form.is_valid(): | ||
| raise exception_class(form.errors) | ||
|
|
||
| settings_form = cls.validate_settings(data) |
There was a problem hiding this comment.
| settings_form = cls.validate_settings(data) | |
| settings_form = cls.validate_settings(data, user) |
Best reviewed per file.