search

microsoftgraph / aspnet-snippets-sample

A repository of code snippets that use Microsoft Graph to perform common tasks such as sending email, managing groups, and other activities from an ASP.NET Core MVC app. This sample uses the Microsoft Graph .NET Client Library to work with data, and the Microsoft Identity Web Library for authentication on the Microsoft identity platform v2.0 endpoint.
MIT License
189 stars 101 forks source link

Add support for open extensions #31

Closed christophwille closed 7 years ago

christophwille commented 7 years ago

This is a work in progress with request for review.

It implements the open extensions workflow shown in the documentation (https://developer.microsoft.com/en-us/graph/docs/concepts/extensibility_open_users). Notable at this stage:

msftclas commented 7 years ago

@christophwille, Thanks for your contribution. To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects. Thanks, Microsoft Pull Request Bot

msftclas commented 7 years ago

@christophwille, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.
Thanks, Microsoft Pull Request Bot

jamescro commented 7 years ago

Thanks so much for the contribution and for checking in. I'll take a closer look later today or tomorrow. We've settled on using resources for strings as a best practice. Would you object to using them here?

I'll check with the SDK engineers about the null result you're getting on line 55. Sometimes this reflects behavior in the service, but it could be a bug. Could you have that method return a Boolean for success (no exception) as a workaround?

christophwille commented 7 years ago

Resources: I am perfectly fine with those, just didn't want to put them there until the scenario I built is ok'ed.

UpdateAsync: I can work around that, it's just that the SDK behavior / method signature is strange. Because according to (REST API) documentation, the result should be 204 with no content at all. So UpdateAsync should actually have a return of void instead of Extension.

jamescro commented 7 years ago

I followed up with one of our SDK developers. He pointed out that our SDKs are generated by a build that works from a template. Some of the update operations do return the updated items, and some do not. The template has to account for both scenarios, so occasionally the signature will indicate that an item will be returned when, in fact, the underlying service doesn't return anything.

jamescro commented 7 years ago

I've also looked more closely at your code, and this looks great. Thanks so much for doing this work. When you have your strings in resources, I'll run through it end-to-end.

christophwille commented 7 years ago

I have amended my pull request as per the requests here. I kept a comment regarding the return value of UpdateAsync.

jamescro commented 7 years ago

Just merged your PR. Thanks so much for doing this work!

christophwille commented 7 years ago

If I have time I'll look into schemaExtensions. But that needs app permissions.