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.

Sign up for GitHub

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

Jump to bottom

Jmprieur/separate graph #534

Merged
merged 7 commits into from
Sep 2, 2020
Merged

Jmprieur/separate graph #534

merged 7 commits into from
Sep 2, 2020

Conversation

jmprieur
Copy link
Collaborator

@jmprieur jmprieur commented Sep 2, 2020 •
edited
Loading

Fix for #506:

  • Moves the classes which were under the MicrosoftGraph folder into a new assembly:Microsoft.Identity.Web.MicrosoftGraph
  • Updated the samples calling Graph
  • Updated the templates to conditionnaly reference Microsoft.Identity.Web.MicrosoftGraph*
  • Also adds Microsoft.Identity.Web.MicrosoftGraphBeta

@jmprieur
Fix for #506
080d112 
Separate Microsoft Graph support from Microsoft.Identity.web

Needs more work on templates.
@jmprieur
Udapting the templates
52b65a9
@jmprieur jmprieur requested review from jennyf19 and pmaytak September 2, 2020 15:27
@jmprieur
Copy link
Collaborator Author

jmprieur commented Sep 2, 2020 •
edited by jennyf19
Loading

@darrelmiller : FYI and in case you want to provide feedback

@schmid37, @gavinmnz, I think this should solve your issue. Please provide feedback (and try out the branch jmprieur/SeparateGraph
#Resolved

pmaytak
pmaytak approved these changes Sep 2, 2020
View reviewed changes
src/Microsoft.Identity.Web.MicrosoftGraph/Microsoft.Identity.Web.MicrosoftGraph.csproj Outdated
<Product>Microsoft Identity Web</Product>
<Description>
This package enables ASP.NET Core Web apps and Web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).
This package is specifically used for web applications, which sign-in users, and protected web APIs, call Microsoft Graph.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This package is specifically used for web applications, which sign-in users, and protected web APIs, call Microsoft Graph.
This package is specifically used for web applications, which sign-in users, and protected web APIs, which call Microsoft Graph.

src/Microsoft.Identity.Web.MicrosoftGraph/Microsoft.Identity.Web.MicrosoftGraph.csproj Outdated
<Company>Microsoft Corporation</Company>
<Product>Microsoft Identity Web</Product>
<Description>
This package enables ASP.NET Core Web apps and Web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This package enables ASP.NET Core Web apps and Web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).
This package enables ASP.NET Core web apps and web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmprieur i'll go through the files and fix this everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

after you merge

src/Microsoft.Identity.Web.MicrosoftGraphBeta/Microsoft.Identity.Web.MicrosoftGraphBeta.csproj Outdated
<Company>Microsoft Corporation</Company>
<Product>Microsoft Identity Web</Product>
<Description>
This package enables ASP.NET Core Web apps and Web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This package enables ASP.NET Core Web apps and Web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).
This package enables ASP.NET Core web apps and web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).

src/Microsoft.Identity.Web.MicrosoftGraphBeta/Microsoft.Identity.Web.MicrosoftGraphBeta.csproj Outdated
<Product>Microsoft Identity Web</Product>
<Description>
This package enables ASP.NET Core Web apps and Web APIs to use the Microsoft identity platform (formerly Azure AD v2.0).
This package is specifically used for web applications, which sign-in users, and protected web APIs, call Microsoft Graph Beta API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This package is specifically used for web applications, which sign-in users, and protected web APIs, call Microsoft Graph Beta API.
This package is specifically used for web applications, which sign-in users, and protected web APIs, which call Microsoft Graph Beta API.

jennyf19
jennyf19 reviewed Sep 2, 2020
View reviewed changes
src/Microsoft.Identity.Web.MicrosoftGraph/Constants.cs Outdated
public const string UserReadScope = "user.read";
public const string GraphBaseUrlV1 = "https://graph.microsoft.com/v1.0";
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmmm...not a fan of repeating code. especially for maintainability and fixes. i can look into how to resolve this.

Copy link
Contributor

@pmaytak pmaytak Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Constants class in Identity.Web is internal. Perhaps set InterntalsVisibleTo to Identity.Web.Graph?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks you for the PR comments. Addressed.

jennyf19
jennyf19 requested changes Sep 2, 2020
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

🕐

@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 2, 2020

looks great. one comment on the re-using of some of the constant classes. let's try to fix that before merging.

In reply to: 685816103 [](ancestors = 685816103)

@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 2, 2020

also...did you check the dependency across packages?

In reply to: 685852275 [](ancestors = 685852275,685816103)

jennyf19
jennyf19 approved these changes Sep 2, 2020
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

@jmprieur jmprieur merged commit fde3687 into master Sep 2, 2020
@jmprieur jmprieur deleted the jmprieur/SeparateGraph branch September 4, 2020 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmaytak pmaytak pmaytak approved these changes

@jennyf19 jennyf19 jennyf19 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmprieur @jennyf19 @pmaytak