DEVEXP-794: Conversation API - Messages (E2E - delete/get/update) #109
Conversation
| #from sinch.domains.conversation.endpoints.app.delete_app import DeleteConversationAppEndpoint | ||
| #from sinch.domains.conversation.models.app.requests import DeleteConversationAppRequest | ||
|
|
||
| # TODO: Reimplement test when DeleteConversationAppEndpoint is functional |
There was a problem hiding this comment.
Ideally, this test should not depend on a domain implementation.
=> Keep it like this in this PR and create a ticket to rewrite the test.
| def build_query_params(self) -> dict: | ||
| query_params = self.request_data.model_dump( | ||
| include=self.QUERY_PARAM_FIELDS, exclude_none=True, by_alias=True | ||
| ) | ||
| return query_params |
There was a problem hiding this comment.
I see a different behavior between the 3 methods: for update, only the messages_source parameter will be added as a query parameter. But for get and delete it will be ALL the parameters except the ones labelled as path parameters.
I understand this is done to segregate the request parameters from the path parameters, but why would the extra parameters be included in the body for this method?
There was a problem hiding this comment.
Shouldn't you delete the whole content of this file instead of editing it?
There was a problem hiding this comment.
Can you double-check this class? It seems to be duplicated with sinch/domains/conversation/models/v1/messages/fields/text_message_field_internal.py
| class GetMessageRequest(BaseModelConfigurationRequest): | ||
| message_id: str = Field(...) | ||
| messages_source: Optional[MessagesSourceType] = None |
There was a problem hiding this comment.
For the UpdateMessageMetadataRequest, descriptions are provided but not here (and not for DeleteMessageRequest either). As there are "internal" classes, is it useful to set the descriptions?
| description="Integer representing the total amount of the transaction.", | ||
| ) | ||
| order: PaymentOrderInternal = Field(..., description="The payment order.") | ||
| payment_settings: Optional[PaymentSettingsInternal] = None |
There was a problem hiding this comment.
This may change: PaymentSettingsInternal may not be a oneOf anymore and should support a data structure such as
"payment_settings": {
"dynamic_pix": {
"code": "1234",
"merchant_name": "Test merchant",
"key": "[email protected]",
"key_type": "EMAIL"
},
"payment_link": {
"uri": "https://www.example.com/payment_link"
}
},
Check MR https://gitlab.com/sinch/sinch-projects/enterprise-and-messaging/documentation/developer-experience/oas-documentation/-/merge_requests/521/diffs#e8e382f867c6543f76cefa09122b39e962ba074b for details
| ) | ||
|
|
||
|
|
||
| class TemplateReferenceField(BaseModelConfigurationResponse): |
There was a problem hiding this comment.
Why is this class declared in this package and not in the fields one? And also, what is the meaning of this specific fields package?
| message: Annotated[ | ||
| Union[WhatsAppInteractiveNfmReplyChannelSpecificContactMessage], | ||
| Field(discriminator="type"), | ||
| ] = Field(..., description="The message content.") |
There was a problem hiding this comment.
I now this is coming from the specification, but does it make sense to keep a single element in a Union?
| from sinch.domains.conversation.models.v1.messages.shared.media_properties_internal import ( | ||
| MediaPropertiesInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.shared.card_message_internal import ( | ||
| CardMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.types.carousel_message_internal import ( | ||
| CarouselMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.types.choice_message_internal import ( | ||
| ChoiceMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.types.list_message_internal import ( | ||
| ListMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.types.location_message_internal import ( | ||
| LocationMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.types.template_message_internal import ( | ||
| TemplateMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.internal.base import ( | ||
| BaseModelConfigurationResponse, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.shared.text_message_internal import ( | ||
| TextMessageInternal, | ||
| ) | ||
| from sinch.domains.conversation.models.v1.messages.types.contact_info_message_internal import ( | ||
| ContactInfoMessageInternal, | ||
| ) |
There was a problem hiding this comment.
From my perspective, it feels strange to import the classes from their class package. Did you stumble on a cycling dependencies issue?
| from pydantic import StrictStr | ||
|
|
||
|
|
||
| ReasonCode = Union[ |
There was a problem hiding this comment.
Why is this union in shared and not in types ?
No description provided.