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

Chat Features: Create, Get, List, Update #219

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Threpio
Copy link
Contributor

@Threpio Threpio commented Feb 14, 2023

A simplified PR that contains most of the object modelling AND the basic Chat feature functionalities. #195

Other 'Message handling' features will be included in a future PR

Other implemented valuetypes are not in alphabetical order - I think we need to fix this.
@Threpio Threpio mentioned this pull request Feb 14, 2023
@Threpio Threpio changed the title Chat Features: create get list update Chat Features: Create, Get, List, Update Feb 14, 2023
@Threpio
Copy link
Contributor Author

Threpio commented Feb 20, 2023

I cannot see an example in this where the tests would destroy the resources created as a check?
A chat resource cannot be deleted only hidden.
Sorry for the Ping: @manicminer Please advise or if the PR can be reviewed I can work on the next section!

Copy link
Owner

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Threpio, thanks for working on this, it is looking good so far! I have made some suggestions/comments below - if you can looka t these I'll be happy to review again. Thanks!

msgraph/chat.go Outdated Show resolved Hide resolved
msgraph/chat_test.go Outdated Show resolved Hide resolved
internal/test/testing.go Outdated Show resolved Hide resolved
msgraph/chat_test.go Outdated Show resolved Hide resolved
ChatType: utils.StringPtr(msgraph.ChatTypeGroup),
Members: &[]msgraph.ConversationMember{
{
ID: user1.Id,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are bind records, it looks like the odata type is required here, and also the ID needs to be sent as a full odata ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for taking this PR on without understanding this part.

Can you link an example of where the OdataID is passed in a full context? I must be missing the apparent examples elsewhere in the code.

My understanding from your first part is that it needs to be:

			{
                                 ODataType: odata.TypeConversationMember
				ID:    user1.Id,
				Roles: &[]string{"owner"},
			},

And then the odata.TypeConversationMember be added as a type?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Threpio No apologies are needed - thank you for working on this, it is very appreciated.

Perhaps it is also possible to specify the regular ID and a type instead of the OData ID?

I noted my original comment from the docs where there is an example showing members being set using an OData bind field:

Screenshot 2023-02-27 at 22 36 14

There are other places in the SDK where we handle this type of field, for example see the Group model:

Screenshot 2023-02-27 at 22 38 36

The Members field marshals to members@odata.bind and it should be populated by the user with OData IDs, which take the form: https://graph.microsoft.com/v1/users/00000000-0000-0000-0000-000000000000

You should be able to re-use the Members type here too, as this has its own marshaler and unmarshaler funcs to help with parsing values.

Hope this helps!

Threpio and others added 4 commits February 21, 2023 14:06
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
Co-authored-by: Tom Bamford <tom@bamford.io>
@Threpio
Copy link
Contributor Author

Threpio commented Feb 23, 2023

@manicminer - I have added the Odata type to the conversation member to be requested in tests.

I am fairly certain my answer to the binding of the ID is completely wrong. Let me know :))

@manicminer
Copy link
Owner

Hi @Threpio, sorry about letting this PR slip. I realise it's been some time, but I have merged in main and updated the ChatClient to use go-azure-sdk as well as some minor fixes based on the latest API documentation. However I still can't get it to create a chat, I'm getting the following error (even after double checking permissions, and retrying for some time):

{
  "error": {
    "code": "Forbidden",
    "message": "OperationFailed",
    "innerError": {
      "code": "200",
      "message": "DisablePartialSuccess-Create Thread: One or more members cannot be added to the thread roster",
      "date": "2024-06-05T23:43:44",
      "request-id": "bd004a22-dc4c-4d31-aa76-cf3135c4435b",
      "client-request-id": "bd004a22-dc4c-4d31-aa76-cf3135c4435b"
    }
  }
}

If you'd like to take another look at this, I'll be happy to get this in. But if not, no worries at all, we can close this off and hopefully support this in our upcoming newer SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants