Skip to content

Conversation

@JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Aug 31, 2020

Fixes #25353

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 31, 2020
@JunTaoLuo JunTaoLuo added the area-blazor Includes: Blazor, Razor Components label Aug 31, 2020
@JunTaoLuo JunTaoLuo added this to the 5.0.0-rc1 milestone Aug 31, 2020
@JunTaoLuo
Copy link
Contributor Author

FYI @jmprieur @javiercn

Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @JunTaoLuo

@JunTaoLuo JunTaoLuo requested a review from captainsafia August 31, 2020 21:06
@JunTaoLuo JunTaoLuo marked this pull request as ready for review September 1, 2020 07:13
@JunTaoLuo JunTaoLuo requested a review from a team as a code owner September 1, 2020 07:13
@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Sep 1, 2020

A few notes for reviewers:

  1. I verified manually that a blazorserver template with individual auth using local db has now resolved the original issue as described in https://github.com/aspnet/AspNetCore-ManualTests/issues/140. Specifically, now at the email confirmation page:
    image
    The links Register and Login now work. For example, after I click Login it directs me to this page:
    image

  2. In verifying the fix I think I uncovered an issue that current breaks blazorserver template for any configuration (Auth, no auth, etc). Specifically, it seems like @using Microsoft.AspNetCore.Components.Web.Virtualization was added in https://github.com/dotnet/aspnetcore/blame/release/5.0/src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/_Imports.razor#L7. To my understanding, Virtualization is a purely blazor wasm concern and this using does not belong in a blazor server template. The effect of having such a using statement leads to errors:

C:\gh\tp\BlazorServer\_Imports.razor(7,43): error CS0234: The type or namespace name 'Virtualization' does not exist in the namespace 'Microsoft.AspNetCore.Components.Web' (are you missing an assembly reference?) [C:\gh\tp\BlazorServer\BlazorServer.csproj]

If my understanding is correct, I'm very concerned that this issue wasn't caught earlier

  1. I'm a little perplexed by the recommendation in the linked issue:
    The fix would be to use a different set of _LoginPartial files, one for OrgAuth and for IndividualAuth similar to the Razor Pages template: https://github.com/dotnet/aspnetcore/blob/master/src/ProjectTemplates/Web.ProjectTemplates/content/RazorPagesWeb-CSharp/Pages/Shared/_LoginPartial.OrgAuth.cshtml.
    Specifically, the contents of RazorPagesWeb-CSharp/Pages/Shared/_LoginPartial.OrgAuth.cshtml and the original contents of BlazorServerWeb-CSharp/Identity/Pages/Shared/_LoginPartial.cshtml are quite different, such as the content generated for IndividualB2CAuth scenarios. I'm choosing to make the minimum changes to resolve the issue outlined in the original issue https://github.com/aspnet/AspNetCore-ManualTests/issues/140. I'm leaving the option to make further changes to BlazorServerWeb-CSharp/Identity/Pages/Shared/_LoginPartial.cshtml for the next milestone after folks with more context can elaborate on what needs to be updated if any.

    Nevermind, it seems like _LoginPartial is only relevant in IndividualLocalAuth scenarios.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM.

Since areas is only used in the IndividualLocalAuth case, maybe just the content of content/BlazorServerWeb-CSharp/Areas/Identity/Pages/Shared/_LoginPartial.cshtml with the content you've proposed for .../content/BlazorServerWeb-CSharp/Areas/Identity/Pages/Shared/_LoginPartial.Identity.cshtml would work?

@JunTaoLuo
Copy link
Contributor Author

JunTaoLuo commented Sep 1, 2020

@jmprieur you're absolutely right, It didn't occur to me that we don't need the _LoginPartial.OrgAuth.cshtml at all and I took the recommendation in the issue too literally. However, I'd like to address that for rc2 instead of re-triggering the CI checks again since there's a chance they might not complete before the 10AM deadline. Given the current code is correct but just more verbose than necessary, I'd like to get this change merged as-is.

@Pilchie
Copy link
Member

Pilchie commented Sep 1, 2020

Approved for RC1 if merged before 10am Pacific on 2020-09-01.

@jmprieur
Copy link
Contributor

jmprieur commented Sep 1, 2020

That sounds good to me, @JunTaoLuo
I tested the patch on my templates, and this works (that's the way I discovered it was not needed)

Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
Tested

@using Microsoft.AspNetCore.Components.Forms
@using Microsoft.AspNetCore.Components.Routing
@using Microsoft.AspNetCore.Components.Web
@using Microsoft.AspNetCore.Components.Web.Virtualization
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why was this added in the first place? FYI, I discussed this a little more in #25456 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Virtualization isn't wasm specific, it's part of the core component experience: https://github.com/dotnet/aspnetcore/blob/master/src/Components/Web/src/Virtualization/Virtualize.cs#L12-L18.

It's weird that you're seeing the error. Perhaps you are using an an old targeting pack?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I believe the template is fine here and this is some environment-related issue. I see this PR is already merged but we should consider bringing it back in another PR. Perhaps @MackinnonBuck can resolve this in his virtualization improvements PR?

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'm making a followup PR to this one to rc2 so I'll do it there. Can one of you help me figure out why I was running into the error during verification?

@Pilchie Pilchie added the Servicing-approved Shiproom has approved the issue label Sep 1, 2020
@JunTaoLuo JunTaoLuo merged commit 6bdb4b9 into release/5.0 Sep 1, 2020
@JunTaoLuo JunTaoLuo deleted the johluo/identity-templates branch September 1, 2020 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-blazor Includes: Blazor, Razor Components area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants