Skip to content

Fix resend email confirmation #14118

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

Merged
merged 10 commits into from
Nov 5, 2019
Merged

Fix resend email confirmation #14118

merged 10 commits into from
Nov 5, 2019

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Sep 18, 2019

Fix for #13462

@HaoK HaoK requested a review from javiercn September 18, 2019 21:16
@Eilon Eilon added the area-identity Includes: Identity and providers label Sep 19, 2019
@@ -42,7 +42,7 @@
<a asp-page="./Register" asp-route-returnUrl="@Model.ReturnUrl">Register as a new user</a>
</p>
<p>
<button id="resend-confirmation" type="submit" asp-page-handler="SendVerificationEmail" class="btn-link" style="padding:0px;margin:0px;border:0px">Resend email confirmation</button>
Copy link
Member

Choose a reason for hiding this comment

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

nit: Do we still have inline styles anywhere? We should avoid those.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a few class="btn btn-default" on buttons, safe to just remove them everywhere?

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.

Looks good to me, although to be fair is really hard to validate this just by looking at the code. I saw you added a test, so I'm trusting the tests 😄.

Update src/Identity/UI/src/Areas/Identity/Pages/V3/Account/ResendEmailConfirmation.cshtml

Co-Authored-By: campersau <[email protected]>

Resend not reset

Update ref

Add new email confirmation link to templates test

Fix test

Revert website change
@HaoK HaoK force-pushed the identity-confirm branch from 686d0ef to 32540d5 Compare October 3, 2019 05:28
@HaoK HaoK changed the base branch from release/3.1 to master October 3, 2019 05:28
HaoK added 4 commits October 2, 2019 22:29
Update src/Identity/UI/src/Areas/Identity/Pages/V3/Account/ResendEmailConfirmation.cshtml

Co-Authored-By: campersau <[email protected]>

Resend not reset

Update ref

Add new email confirmation link to templates test

Fix test

Revert website change
@github-actions github-actions bot added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Nov 4, 2019
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Two concerns, I'm only really worried about the user == null one, but as long as there's a good reason for that behavior I won't block on it.

/// </summary>
[AllowAnonymous]
[IdentityDefaultUI(typeof(ResendEmailConfirmationModel<>))]
public abstract class ResendEmailConfirmationModel : PageModel
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could unify this considering that it seems to just be a copy of the V3 one, but if there's not an existing structure for making those two versions share code I can see it being not worth the effort.

@HaoK HaoK merged commit 4119513 into master Nov 5, 2019
@HaoK HaoK deleted the identity-confirm branch November 5, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-identity Includes: Identity and providers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants