Communication chat preview4

[sarkar-rajarshi]

• CommunicationUserIdentifier models added
• create_chat_thread - returns CreateChatThreadResult instead of ChatThreadClient
• add_participant - AddChatParticipantsResult -> tuple(ChatThreadParticipant, CommunicationError)
• add_participants - AddChatParticipantsResult -> list(tuple(ChatThreadParticipant, CommunicationError))
• CommunicationErrorResponseConverter added to cosolidate list(ChatThreadParticipant) and list(CommunicationError) into list(tuple(ChatThreadParticipant, CommunicationError))
• CreateChatThreadResult -> attributes changed to
  • chat_thread -> ChatThread (no change)
  • Errors -> CreateChatThreadErrors -> list(tuple(ChatThreadParticipant, CommunicationError))
• create_chat_thread -> thread_participants and repeatability_request_id changed to keyword arguments
• Modify unit tests to capture method signature modifications
• Modify e2e tests to capture method signature modifications
• Update README.md
• Add new test recordings

[sarkar-rajarshi]

PR includes changes from https://github.com/Azure/azure-sdk-for-python/tree/feature/preview4 as well

[annatisch]

We should unpackage this result type. It's currently pretty hard to use...
If you could write a sample demonstrating what a customer has to do to:

• Add 5 participants
• Determine which 2 our of those 5 failed.
• Re-attempt to add those 2 failures.

From their we can figure out the best response value here.

[sarkar-rajarshi]

@annatisch something like this?

def _util_create_thread_participant(user, **kwargs) -> ChatThreadParticipant:
    """
    Create util or call the constructor of ChatThreadParticipant - same thing
    """
    return ChatThreadParticipant(
    user=user_id,
    display_name = kwargs.get('display_name', None),
    share_history_time = kwargs.get('share_history_time', None)
    )
	

def decide_if_needs_to_retried(invalid_participant) -> bool:
    """
    some logic to determine if this is needed to be retried or not
    """
    return True


if __name__=='__main__':
    # intialize identity client
    identity_client = CommunicationIdentityClient.from_connection_string(connection_string)
    
    # initialize and retrieve chat_thread_client
    refresh_options = CommunicationTokenRefreshOptions(token)
    chat_client = ChatClient(endpoint, CommunicationTokenCredential(refresh_options))
    topic = "some topic"
    participants = [ChatThreadParticipant(
            user=user, # whoever is creating the chat_thread
            display_name='name',
            share_history_time=datetime.utcnow()
        )]
    create_chat_thread_result = chat_client.create_chat_thread(topic, participants)
    chat_thread_client = chat_client.get_chat_thread_client(create_chat_thread_result.chat_thread.id)
    
    
    # initialize 3+2 participants
    users_for_sucesss = [identity_client.create_user() for i in range(3)] # create 3 users for success - type: List[CommunicationUserIdentifier]
    users_for_failures = [CommunicationUserIdentifier("some random id 1"), CommunicationUserIdentifier("some random id 2")] # create 2 users for failures - type: List[CommunicationUserIdentifier]
    
    # create ChatThreadParticipant objects
    participants_success = [_util_create_thread_participant(user) for user in users_for_sucesss]
    participants_failures = [_util_create_thread_participant(user) for user in users_for_failures]
    
    # add participants to chat thread
    thread_participants = [participants_success + participants_failures]
    add_chat_participants_result = chat_thread_client.add_participants(thread_participants)
    
    # verify if all participants have been added or not
    participants_to_retry = [] # in case of failure store participants to be retried - type: List[CommunicationUserIdentifier]
    
    if hasattr(add_chat_participants_result, 'errors') and \
            add_chat_participants_result.errors is not None:
        print("Error encountered in adding ", len(thread_participants), " participants")
        participant_failed_to_add = add_chat_participants_result.errors.invalid_participants
        
        for failed_participant in participant_failed_to_add:
            # failed_participant.target contains the user_id - type: str
            # For example, in this scenario the values would be either 'some random id 1' or 'some random id 2'
            print("Failed to add participant", failed_participant.target)
            
            if decide_if_needs_to_retried(failed_participant):
                participants_to_retry.append(CommunicationUserIdentifier(failed_participant.target))
    
    # if any user has been failed to be added, then retry
    if len(participants_to_retry) != 0:
        thread_participants = [_util_create_thread_participant(retry_user) for retry_user in participants_to_retry]
        add_chat_participants_result_retry = chat_thread_client.add_participants(thread_participants)
		

[annatisch]

Wow that's lots of code... and a couple of red flags. If a customer ever has to call hasattr on the response of call, then we did something wrong :)

If we change the return type to: List[Tuple[ChatThreadParticipant, Optional[AddParticipantError]]
Then we could replace all this:

    add_chat_participants_result = chat_thread_client.add_participants(thread_participants)
    
    # verify if all participants have been added or not
    participants_to_retry = [] # in case of failure store participants to be retried - type: List[CommunicationUserIdentifier]
    
    if hasattr(add_chat_participants_result, 'errors') and \
            add_chat_participants_result.errors is not None:
        print("Error encountered in adding ", len(thread_participants), " participants")
        participant_failed_to_add = add_chat_participants_result.errors.invalid_participants
        
        for failed_participant in participant_failed_to_add:
            # failed_participant.target contains the user_id - type: str
            # For example, in this scenario the values would be either 'some random id 1' or 'some random id 2'
            print("Failed to add participant", failed_participant.target)
            
            if decide_if_needs_to_retried(failed_participant):
                participants_to_retry.append(CommunicationUserIdentifier(failed_participant.target))
    
    # if any user has been failed to be added, then retry
    if len(participants_to_retry) != 0:
        thread_participants = [_util_create_thread_participant(retry_user) for retry_user in participants_to_retry]
        add_chat_participants_result_retry = chat_thread_client.add_participants(thread_participants)

With just a couple of lines:

result = chat_thread_client.add_participants(thread_participants)
failed = [p for p, error in result if decide_if_needs_to_retried(error)]
retry = chat_thread_client.add_participants(failed)

[sarkar-rajarshi]

@annatisch The following changes have been made

• add_participant - AddChatParticipantsResult -> tuple(ChatThreadParticipant, CommunicationError)
• add_participants - AddChatParticipantsResult -> list(tuple(ChatThreadParticipant, CommunicationError))
• CreateChatThreadResult -> attributes changed to
  • chat_thread -> ChatThread (no change)
  • Errors -> CreateChatThreadErrors -> list(tuple(ChatThreadParticipant, CommunicationError))

[annatisch]

Looking good - I think we're nearly there :)

[annatisch]

In the case of the single add_participant, we shouldn't return the error object - we should just raise an error.

[juancamilor]

@annatisch we mentioned this one on the .NET review:
https://apiview.dev/Assemblies/Review/a60a130cc1e24d65a74c20e42887b61f#74d339479f13486ebababaee478f8990

[sarkar-rajarshi]

@annatisch add_participant raises error - change implemented

[juancamilor]

@annatisch are we ok with having this diverge in python ? I am ok if you are.
When we run this in that meeting, the answer was to leave it as is in case in the future any additional data is required back from that call. I don't see that coming but that was the decision.

[annatisch]

@juancamilor - raising errors rather than returning them is Pythonic behaviour that I would expect to differ from .Net.
We can still return the ChatThreadParticipant object - when you say additional data, you mean other information outside of these two fields?

[juancamilor]

yes, something like extra headers is what Ted G mentioned, but I don't have any requirements yet to back it up.

tg-msft1
There aren't today, but maybe it'll come back with extra headers in the future?

[juancamilor]

@annatisch after today's review we will address all the changes on the next iteration (including the removal of this operation).