Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New command: List the chat conversations from a Microsoft Teams chat #2860

Closed
arjunumenon opened this issue Dec 7, 2021 · 26 comments
Closed

Comments

@arjunumenon
Copy link
Member

arjunumenon commented Dec 7, 2021

Usage

m365 teams chat message list [options]

Description

List the conversations from a particular chat (1 on 1 chat OR Group chat OR Meeting Chats etc) in Microsoft Teams

Options

Option Description
-i, --chatId<chatId> The chat ID / Conversation ID of the Microsoft Teams Chat.

Additional Info

We hay have to use the Graph API end point - v1.0/chats/ConversationID/messages as per this documentation.

New API Permission for PnP Management Shell AAD App

For us to use the API, the AAD App needs to have the permission Chat.Read, Chat.ReadWrite. Currently the AAD App which is being used by CLI for Microsoft 365 (PnP Management Shell) does not have the permission setup. Once the PR is completed, the permission will have to applied to the AAD App.

@arjunumenon arjunumenon added needs discussion needs peer review Needs second pair of eyes to review the spec or PR new feature labels Dec 7, 2021
@arjunumenon
Copy link
Member Author

arjunumenon commented Dec 7, 2021

Hello @pnp/cli-for-microsoft-365-maintainers - Can you have a look into the specs and let me know if any modification is needed for the same.

I would also request your opinion on the placement of the new Command. Currently we have our Menu items for Teams as Microsoft Teams (teams).
image

So the commands which we have under that section covers about Teams team. If you are all ok with this command, I am assuming we will keep this command also underneath that renaming the section to Microsoft Teams. Do you think it make sense. Appreciate your suggestion on that as well so that I will update the spec accordingly.

@waldekmastykarz
Copy link
Member

Thanks for the spec @arjunumenon, here's some feedback.

The v1.0/chats/ConversationID/messages API would be suitable for a teams chat message list command rather than a get as it returns all messages in the specified conversation. Let's double check the API that we want to use or adjust the command name.

So the commands which we have under that section covers about Teams team. If you are all ok with this command, I am assuming we will keep this command also underneath that renaming the section to Microsoft Teams.

This command is indeed related to Microsoft Teams so it would land under the teams group. Why do you think we should rename the section in the docs?

@arjunumenon
Copy link
Member Author

The v1.0/chats/ConversationID/messages API would be suitable for a teams chat message list command rather than a get as it returns all messages in the specified conversation. Let's double check the API that we want to use or adjust the command name.

Totally with you on that point. Thanks for pointing that out 👍.
My intention of the command was indeed to list the chat messages of the chat rather than getting particular chat. I had not realized get chat do have a different meaning in the context. I will update command spec to be,
m365 teams chat message list [options]

This command is indeed related to Microsoft Teams so it would land under the teams group. Why do you think we should rename the section in the docs?

Here is my thought - currently the commands which are there in the Microsoft Teams (teams) group , majority of the command are related to Microsoft Teams team.
However, there are some commands which are coming outside the scope of Microsoft Teams team. Some of the commands are below.

  1. m365 teams app publish
  2. m365 teams app remove
  3. m365 teams report deviceusagedistributionusercounts [options]
  4. m365 teams user app add

This command which we have spec'd is also not related to Microsoft Teams team, while it is related to chat outside Microsoft Teams team. Hence I was wondering whether it make sense to rename the section in the Navigation since the commands coming under that list of commands which are both related to Microsoft Teams team as well related to commands outside Microsoft Teams team.

Pardon me if it does not make sense to the original idea / thought process

@arjunumenon arjunumenon changed the title New command: Get the chat conversations from Teams Chat New command: List the chat conversations from a Microsoft Teams chat Dec 9, 2021
@waldekmastykarz
Copy link
Member

So the idea behind our naming convention is that the first segment of the command name indicates a workload. In this case it's Microsoft Teams as whole, not just a specific Teams team. As such, both commands that operate on teams, channels and conversations as teams that relate to the broader Teams workload fall under the teams namespace. Does that make sense?

@arjunumenon
Copy link
Member Author

As such, both commands that operate on teams, channels and conversations as teams that relate to the broader Teams workload fall under the teams namespace. Does that make sense?

Yes it does. I perceived the naming convention in a different way, that the commands are related to Teams team alone.
Your explanation make sense. All good for now 👍.

@arjunumenon arjunumenon added good first issue help wanted and removed needs discussion needs peer review Needs second pair of eyes to review the spec or PR labels Dec 9, 2021
@martinlingstuyl
Copy link
Contributor

Hi guys,
I'll see if I can pick this up the coming days. It would be my first contribution, so I'll need time to look into the setup.

@appieschot
Copy link
Member

Woohoo, awesome! Let us know if you need anything and happy hacking 🔥

@waldekmastykarz
Copy link
Member

Awesome! We appreciate your help and please let us know if you need any help.

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented Dec 22, 2021

OK, just one question: would it be beneficial to also add the $top query parameter as optional argument?
https://docs.microsoft.com/en-us/graph/api/chat-list-messages?view=graph-rest-1.0&tabs=http#optional-query-parameters

For example named pageSize (or just top)

@appieschot
Copy link
Member

@martinlingstuyl we do not have a top or pageSize parameter anywhere; the CLI assumes you want to retrieve all data and you can use the query parameter to filter the locally if you want. Lets stay consistent and not implement this for now.

@martinlingstuyl
Copy link
Contributor

👆 I may be able to pick up some more issues. It bugs me that teams chat list is not available.

@waldekmastykarz
Copy link
Member

👆 I may be able to pick up some more issues. It bugs me that teams chat list is not available.

Take as many as you'd like! We appreciate your help 👏

@martinlingstuyl
Copy link
Contributor

You have to forge the iron while it's hot 🐱‍🏍

...plus you have a fantastic getting started guide, and I could copy paste a lot. (Of course I changed and checked if it worked as well)

@waldekmastykarz
Copy link
Member

Thank you for the kind words! Awesome to hear that you could get started so quickly.

@martinlingstuyl
Copy link
Contributor

'Remote container: clone in unique volume' --> no fuss 😎

@waldekmastykarz
Copy link
Member

Have you by any chance written about the steps you took to contribute? I think it would be invaluable for new contributors 👏

@martinlingstuyl
Copy link
Contributor

I haven't yet. But good I idea, I could write a short blog about it for my website https://www.blimped.nl

@waldekmastykarz
Copy link
Member

Awesome! I'd be happy to help share it with others in our community 😊

@martinlingstuyl
Copy link
Contributor

Ok, point for discussion! I'm seeing system chat messages in the list. They look like this: <systemEventMessage/> and have a eventDetail.@odata.type property, like #microsoft.graph.chatRenamedEventMessageDetail. In this case it concerns the message:
image

But this might include any type of system message.

We could transform these in the chat list output. Any ideas about this?

@appieschot
Copy link
Member

For me it makes sense to to include those by default (as that is what the API is returning and it might have value if you want to retrieve all messages). But an optional flag to exclude any systemEvent from the response ( "messageType": "systemEventMessage", according to the docs would make sense as well --excludeSystemEvents perhaps?

@arjunumenon
Copy link
Member Author

I am also in an opinion to have the complete result retrieved along with systemEventMessage.
Even if we want to, If I am not mistaken, I don't think the endpoint supports filter so that we can retrieve the results without systemEventMessage in the results.

That being the case, we may have to filter after getting the results in our command code.
Even if the user wants the result without systemEventMessage they can filter it using JMESPath Queries which CLI already supports.
But it will be a good idea to have this added in the command documentation if needed.

@martinlingstuyl
Copy link
Contributor

I'll check if I can $filter serverside

@martinlingstuyl
Copy link
Contributor

You where correct @arjunumenon : no $filter option on this endpoint as of yet

@appieschot
Copy link
Member

Let us keep it then; we can always write a nice sample with a JMESPath query to exclude all those sytemEvents.

@martinlingstuyl
Copy link
Contributor

Awesome! I'd be happy to help share it with others in our community 😊

Hi Waldek,

I just published it!
https://www.blimped.nl/contributing-as-a-holiday-season-present/

@waldekmastykarz
Copy link
Member

Awesome and a great read! Thank you so much for contributing and taking the time to share your experiences. I'm sure it'll be invaluable for new contributors and it got us a few ideas to include in our docs as well.

@waldekmastykarz waldekmastykarz added this to the v4.4 milestone Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants