ACS Chat: Changes for API version: 2020-11-01-preview3 #15455
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add failed participant check in response header for create_thread method * New convention method add_participant(thread_participant: ChatThreadParticipant) - sync and async
Leoaqr
requested review from
ankitarorabit,
RezaJooyandeh,
turalf and
a team
as code owners
Leoaqr
requested review from
sarkar-rajarshi,
fangchen0601,
annatisch and
juancamilor
| @@ -0,0 +1,8 @@ | |||
| # ------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
the _shared folder is shared between all service SDKs, like SMS SDK has the same folder and same files/methods. Which means adding one method/module to this folder, other teams must do the same.
So I suggest:
- Talk to Tural to ask if above is the same case, if yes, ask if this will become a common method that other teams will use, if yes, then your PR is good
- If this module and method is only for Chat team, suggest move it to azure/communication/chat/_util.py after talked to Tural.
There was a problem hiding this comment.
Thanks for the feedback.
Currently, I don't commit the test files in order to reduce the unnecessary review workload. Yes, I will run and upload new test files after review.
For the helper file in shared folder, what do you think? @turalf
There was a problem hiding this comment.
I guess this file is specific to the chat module. If that is the case please keep this file outside of the _shared folder.
Also it seems that this method is used only in 1 file. If you are not planning to use it somewhere else in the future, better to keep in that file.
Leoaqr
force-pushed
the
feature/communication-chat-preview3
branch
from
4b1f836 to
a0e2210
Compare
Leoaqr
changed the base branch from
master
to
feature/communication-chat-preview3
turalf
reviewed
Nov 25, 2020
| @@ -0,0 +1,8 @@ | |||
| # ------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
I guess this file is specific to the chat module. If that is the case please keep this file outside of the _shared folder.
Also it seems that this method is used only in 1 file. If you are not planning to use it somewhere else in the future, better to keep in that file.
Leoaqr
force-pushed
the
feature/communication-chat-preview3
branch
from
9d0e425 to
914bbfe
Compare
annatisch
reviewed
Nov 30, 2020
| :return: SendChatMessageResult, or the result of cls(response) | ||
| :rtype: ~azure.communication.chat.SendChatMessageResult | ||
| :return: str, or the result of cls(response) | ||
| :rtype: str |
There was a problem hiding this comment.
This change should not have been made - could we fix it before the next release goes out?
Instead we should use something like this:
return { 'id': send_chat_message_result.id }
There was a problem hiding this comment.
Will change accordingly in the next iteration
annatisch
reviewed
Nov 30, 2020
| @@ -113,7 +113,7 @@ def thread_id(self): | |||
| return self._thread_id | |||
|
|
|||
| @distributed_trace | |||
| def update_thread( | |||
| def update_topic( | |||
| self, | |||
| topic=None, # type: Optional[str] | |||
There was a problem hiding this comment.
If we are updating the topic- the topic parameter should no longer be optional.
annatisch
reviewed
Nov 30, 2020
| participant, reason = invalid_participant_and_reason.split(',', 1) | ||
| errors.append('participant ' + participant + ' failed to join thread ' | ||
| + create_chat_thread_result.id + ' return statue code ' + reason) | ||
| raise ValueError(errors) |
There was a problem hiding this comment.
This should not be a ValueError
Leoaqr
added a commit
to Leoaqr/azure-sdk-for-python
that referenced
this pull request
Dec 4, 2020
Several changes required by `2020-11-01-preview3` of the API: - Rename `Thread Members` to `Participants` - Rename function `UpdateThread` to `UpdateTopic` - Rename `ReadReceipt` to `ChatMessageReadReceipt` in model - Return message id (string) instead of SendMessageResult (object) when calling `send_message` - Add a convenience method `add_participant` for adding 1 participant - Add failed participants check in response header in `create_thread` function Misc: - Add updated test record files Co-authored-by: Leo Li <jixli@microsoft.com> Co-authored-by: Rajarshi Sarkar <73562869+sarkar-rajarshi@users.noreply.github.com>
sarkar-rajarshi
added a commit
that referenced
this pull request
Feb 2, 2021
* ACS Chat: Changes for API version: 2020-11-01-preview3 (#15455) Several changes required by `2020-11-01-preview3` of the API: - Rename `Thread Members` to `Participants` - Rename function `UpdateThread` to `UpdateTopic` - Rename `ReadReceipt` to `ChatMessageReadReceipt` in model - Return message id (string) instead of SendMessageResult (object) when calling `send_message` - Add a convenience method `add_participant` for adding 1 participant - Add failed participants check in response header in `create_thread` function Misc: - Add updated test record files Co-authored-by: Leo Li <jixli@microsoft.com> Co-authored-by: Rajarshi Sarkar <73562869+sarkar-rajarshi@users.noreply.github.com> * Update ACS Chat Python SDK align with swagger changes (#15640) - Seperate AzureCommunicationChatServiceOperationsMixin into ChatOperations and ChatThreadOperations - Move invalid participants into errors object in reponse body Co-authored-by: Leo Li <jixli@microsoft.com> * Communication chat - paginated results for participants and readreceipts (#15682) * Failed participant check + single member add * Add failed participant check in response header for create_thread method * New convention method add_participant(thread_participant: ChatThreadParticipant) - sync and async * Add empty line at the end of files * Enable pagination for list participants * Enable pagination for read receipts * conflicts resolved after merge with upstream * update README.md * e2e test fix * bkp commit * updated e2e tests * adding test recording Co-authored-by: Leo Li <jixli@microsoft.com> * Updated swagger changes (#16390) * Updated swagger changes - Generate new models from swagger - ChatMessage.content -> ChatMessageContent instead of 'str' - Remove ChatMessagePriority - Introduce ChatMessageType - Add tests around ChatMessageType deserialization - Generate a repeatability ID by default - Add some test scenarios around repeatability ID - Update all relevant tests - Update sample code - Record new test sessions * Rebase with master - Use CommunicationUserIdentifier * pylint fixes Co-authored-by: Jixing (Leo) Li <lijixing3377@gmail.com> Co-authored-by: Leo Li <jixli@microsoft.com>
juancamilor
pushed a commit
that referenced
this pull request
Feb 5, 2021
* Rsarkar/chat preview3 rebased (#16463) * ACS Chat: Changes for API version: 2020-11-01-preview3 (#15455) Several changes required by `2020-11-01-preview3` of the API: - Rename `Thread Members` to `Participants` - Rename function `UpdateThread` to `UpdateTopic` - Rename `ReadReceipt` to `ChatMessageReadReceipt` in model - Return message id (string) instead of SendMessageResult (object) when calling `send_message` - Add a convenience method `add_participant` for adding 1 participant - Add failed participants check in response header in `create_thread` function Misc: - Add updated test record files Co-authored-by: Leo Li <jixli@microsoft.com> Co-authored-by: Rajarshi Sarkar <73562869+sarkar-rajarshi@users.noreply.github.com> * Update ACS Chat Python SDK align with swagger changes (#15640) - Seperate AzureCommunicationChatServiceOperationsMixin into ChatOperations and ChatThreadOperations - Move invalid participants into errors object in reponse body Co-authored-by: Leo Li <jixli@microsoft.com> * Communication chat - paginated results for participants and readreceipts (#15682) * Failed participant check + single member add * Add failed participant check in response header for create_thread method * New convention method add_participant(thread_participant: ChatThreadParticipant) - sync and async * Add empty line at the end of files * Enable pagination for list participants * Enable pagination for read receipts * conflicts resolved after merge with upstream * update README.md * e2e test fix * bkp commit * updated e2e tests * adding test recording Co-authored-by: Leo Li <jixli@microsoft.com> * Updated swagger changes (#16390) * Updated swagger changes - Generate new models from swagger - ChatMessage.content -> ChatMessageContent instead of 'str' - Remove ChatMessagePriority - Introduce ChatMessageType - Add tests around ChatMessageType deserialization - Generate a repeatability ID by default - Add some test scenarios around repeatability ID - Update all relevant tests - Update sample code - Record new test sessions * Rebase with master - Use CommunicationUserIdentifier * pylint fixes Co-authored-by: Jixing (Leo) Li <lijixing3377@gmail.com> Co-authored-by: Leo Li <jixli@microsoft.com> * Apiview changes (#16560) * apiview fixes - Add missing type hints - return ItemPaged or AsyncItemPaged instead of full classpath Co-authored-by: Jixing (Leo) Li <lijixing3377@gmail.com> Co-authored-by: Leo Li <jixli@microsoft.com>
openapi-sdkautomation bot
pushed a commit
to AzureSDKAutomation/azure-sdk-for-python
that referenced
this pull request
Aug 2, 2021
signalr: adding the missing `Trace` value (Azure#15455) Fixes Azure#14923
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Several changes required by
2020-11-01-preview3of the API:Thread MemberstoParticipantsUpdateThreadtoUpdateTopicReadReceipttoChatMessageReadReceiptin modelsend_messageadd_participantfor adding 1 participantcreate_threadfunction