DEVEXP-794: Conversation API - Messages (E2E - delete/get/update) #109
Conversation
| 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.
You're right, refactored this file
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.
Deleted. I haven't deleted before and ruff reformatted 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?
There was a problem hiding this comment.
Updated the description and merged GetMessageRequest and DeleteMessageRequest into MessageIdRequest. While internal, these models are exposed via messages_apis.py, so clearer descriptions improve DX.
| 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": "asdf@example.com",
"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?
There was a problem hiding this comment.
Unfortunately yes, I had circular imports :(
| from pydantic import StrictStr | ||
|
|
||
|
|
||
| ReasonCode = Union[ |
There was a problem hiding this comment.
Why is this union in shared and not in types ?
There was a problem hiding this comment.
Fixed. Also renamed to ReasonCodeType
| def build_url(self, sinch) -> str: | ||
| if not self.ENDPOINT_URL: | ||
| raise NotImplementedError( | ||
| "ENDPOINT_URL must be defined in the subclass." |
There was a problem hiding this comment.
Is the error message will contains information about Conversation domain raising the issue ?
Without more context "ENDPOINT_URL must be defined in the subclass" won't help user to locate what is wrong/how to fix it.
| # For Union types, try to validate against each type in the Union sequentially | ||
| # This handles cases where TypeAdapter might not be fully defined | ||
| union_args = get_args(response_model) | ||
| last_error = None |
There was a problem hiding this comment.
Seems this variable is not used (last_error)
| AppMessageConversationMessageInternal, | ||
| ContactMessageConversationMessageInternal, |
There was a problem hiding this comment.
A lot of public model name contains the Internal suffix.
There was a problem hiding this comment.
Ideally, due to API complexity, it could be useful to triage the classes into dedicated directories (vs flatten all files in same one)
It will simplify namespace ease of use/access
There was a problem hiding this comment.
File, or part of file, is already existing from Numbers and SMS:
- https://github.com/sinch/sinch-sdk-python/blob/feat/conversation-messages/sinch/domains/sms/models/v1/internal/base/base_model_configuration.py
- https://github.com/sinch/sinch-sdk-python/blob/feat/conversation-messages/sinch/domains/numbers/models/v1/internal/base/base_model_configuration.py
Is it possible/does it have sense to define common content them once ?
| :param messages_source: Specifies the message source for which the request will be processed. Used for | ||
| operations on messages in Dispatch Mode. For more information, | ||
| see [Processing Modes](https://developers.sinch.com/docs/conversation/processing-modes/). | ||
| (optional) |
There was a problem hiding this comment.
Ideally, comment should reflect the default value in use when not set
| ..., | ||
| description="The title is shown close to the button that leads to open a user calendar.", | ||
| ) | ||
| event_start: StrictStr = Field( |
| event_start: StrictStr = Field( | ||
| ..., description="The timestamp defines start of a calendar event." | ||
| ) | ||
| event_end: StrictStr = Field( |
| default=None, | ||
| description="Unique ID of the Facebook catalog being used by the business.", | ||
| ) | ||
| expiration_time: Optional[StrictStr] = Field( |
No description provided.