From dcbb9676af89a28e3b497ccd60172d9c1a5fecb3 Mon Sep 17 00:00:00 2001 From: Steve Smith Date: Mon, 21 Sep 2026 14:56:20 -0400 Subject: [PATCH 1/4] Fix saving personal info on the Admin User page Saving name/email on /Admin/User failed with a raw JSON 400: - The form's hidden userId came from UserPersonalUpdateModel.UserId, which is only set once OnGetAsync reaches the member load. Anything failing earlier (e.g. the Stripe invoice search, swallowed by the catch-all) left it blank, so every save failed "userId is required". The page now captures UserId from the route first, and invoice lookup failures are isolated and logged. - Address/City/Country/PostalCode are [Required], so members without a shipping address could not be edited. On the admin page they are now only required once any address field is filled in, and the shipping address is left untouched when none is. - Validation failures redisplay the form with the submitted values and inline errors instead of returning BadRequest(ModelState). A missing member record is reported on the page instead of throwing. Supersedes draft #1423, which changed the shared model and would also have relaxed validation on members' own profile page. Co-Authored-By: Claude Opus 5 (1M context) --- src/DevBetterWeb.Web/Pages/Admin/User.cshtml | 5 +- .../Pages/Admin/User.cshtml.cs | 67 ++++++++- .../OnPostUpdatePersonalInfoAsync.cs | 127 ++++++++++++++++++ 3 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs diff --git a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml index 012b135ca..767fb0e1b 100644 --- a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml +++ b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml @@ -119,7 +119,7 @@
- +
Personal @@ -128,6 +128,7 @@
+
@@ -213,7 +214,7 @@
- +
Links diff --git a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs index bf9e48aac..4eb690afc 100644 --- a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs +++ b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs @@ -69,6 +69,9 @@ public UserModel(ILogger logger, } + // Set from the route before anything else loads so the page's forms always post a userId, + // even if a later part of OnGetAsync (e.g. the Stripe invoice lookup) fails. + public string UserId { get; set; } = string.Empty; public IdentityUser? IdentityUser { get; set; } public List Roles { get; set; } = new List(); public List RolesNotAssignedToUser { get; set; } = new List(); @@ -81,6 +84,8 @@ public UserModel(ILogger logger, public async Task OnGetAsync(string userId) { + UserId = userId; + try { if (string.IsNullOrEmpty(userId)) @@ -95,8 +100,15 @@ public async Task OnGetAsync(string userId) return BadRequest(); } - var invoices = await _invoiceHandlerListService.SearchByEmailAsync(currentUser!.Email!); - Invoices = _mapper.Map>(invoices); + try + { + var invoices = await _invoiceHandlerListService.SearchByEmailAsync(currentUser!.Email!); + Invoices = _mapper.Map>(invoices); + } + catch (Exception exception) + { + _logger.LogError(exception, "Unable to load Stripe invoices for userId {UserId}", userId); + } var roles = await _roleManager.Roles.ToListAsync(); @@ -258,21 +270,38 @@ public async Task OnPostUpdateEmailConfirmationAsync(string userI public async Task OnPostUpdatePersonalInfoAsync(string userId) { + // Admins often edit members who have not entered a shipping address yet, so the address + // fields are only required once any of them is filled in. + bool hasShippingAddress = HasAnyShippingAddressField(UserPersonalUpdateModel); + if (!hasShippingAddress) + { + foreach (var field in ShippingAddressFields) + { + ModelState.Remove($"{nameof(UserPersonalUpdateModel)}.{field}"); + } + } + if (!ModelState.IsValid) { - ModelState.AddModelError("InvalidUserId", "Bad Data"); - return BadRequest(ModelState); + return await RedisplayPersonalInfoFormAsync(userId); } var spec = new MemberByUserIdSpec(userId); var member = await _memberRepository.FirstOrDefaultAsync(spec); - if (member is null) throw new MemberNotFoundException(userId); + if (member is null) + { + ModelState.AddModelError(string.Empty, $"No member record exists for user {userId}."); + return await RedisplayPersonalInfoFormAsync(userId); + } member.UpdateName(UserPersonalUpdateModel.FirstName, UserPersonalUpdateModel.LastName, false); member.UpdatePEInfo(UserPersonalUpdateModel.PEFriendCode, UserPersonalUpdateModel.PEUsername, false); member.UpdateAboutInfo(UserPersonalUpdateModel.AboutInfo, false); member.UpdateAddress(UserPersonalUpdateModel.Address, false); - member.UpdateShippingAddress(UserPersonalUpdateModel.Address!, UserPersonalUpdateModel.City!, UserPersonalUpdateModel.State!, UserPersonalUpdateModel.PostalCode!, UserPersonalUpdateModel.Country!, false); + if (hasShippingAddress) + { + member.UpdateShippingAddress(UserPersonalUpdateModel.Address!, UserPersonalUpdateModel.City!, UserPersonalUpdateModel.State!, UserPersonalUpdateModel.PostalCode!, UserPersonalUpdateModel.Country!, false); + } member.UpdateDiscord(UserPersonalUpdateModel.DiscordUsername, false); member.UpdateEmail(UserPersonalUpdateModel.Email, false); @@ -287,6 +316,32 @@ public async Task OnPostUpdatePersonalInfoAsync(string userId) return RedirectToPage("./User", new { userId }); } + + private static readonly string[] ShippingAddressFields = + { + nameof(UserPersonalUpdateModel.Address), + nameof(UserPersonalUpdateModel.City), + nameof(UserPersonalUpdateModel.State), + nameof(UserPersonalUpdateModel.Country), + nameof(UserPersonalUpdateModel.PostalCode), + }; + + private static bool HasAnyShippingAddressField(UserPersonalUpdateModel model) => + !string.IsNullOrWhiteSpace(model.Address) || + !string.IsNullOrWhiteSpace(model.City) || + !string.IsNullOrWhiteSpace(model.State) || + !string.IsNullOrWhiteSpace(model.Country) || + !string.IsNullOrWhiteSpace(model.PostalCode); + + // Reloads the page data but keeps the admin's submitted values so validation errors show next to the fields. + private async Task RedisplayPersonalInfoFormAsync(string userId) + { + var submitted = UserPersonalUpdateModel; + var result = await OnGetAsync(userId); + UserPersonalUpdateModel = submitted; + return result; + } + public async Task OnPostUpdateLinksAsync(string userId) { var spec = new MemberByUserIdSpec(userId); diff --git a/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs b/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs new file mode 100644 index 000000000..f55ebffda --- /dev/null +++ b/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs @@ -0,0 +1,127 @@ +using System.Threading; +using System.Threading.Tasks; +using AutoMapper; +using DevBetterWeb.Core.Entities; +using DevBetterWeb.Core.Interfaces; +using DevBetterWeb.Core.Specs; +using DevBetterWeb.Infrastructure.Identity.Data; +using DevBetterWeb.Infrastructure.Interfaces; +using DevBetterWeb.Web.Pages.Admin; +using DevBetterWeb.Web.Pages.User; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.RazorPages; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using Xunit; + +namespace DevBetterWeb.Tests.Pages.AdminUserModelTests; + +public class OnPostUpdatePersonalInfoAsync +{ + private const string UserId = "user-123"; + + private readonly IRepository _memberRepository = Substitute.For>(); + private readonly UserManager _userManager = UserManagerHelpers.CreateSubstitute(); + private readonly Member _member = MemberHelpers.CreateWithInternalConstructor(); + private readonly UserModel _pageModel; + + public OnPostUpdatePersonalInfoAsync() + { + _memberRepository.FirstOrDefaultAsync(Arg.Any(), Arg.Any()) + .Returns(_member); + _userManager.FindByIdAsync(UserId).Returns(new ApplicationUser { Id = UserId }); + + var roleManager = Substitute.For>( + Substitute.For>(), null!, null!, null!, null!); + + _pageModel = new UserModel(NullLogger.Instance, + _userManager, + roleManager, + Substitute.For(), + Substitute.For(), + _memberRepository, + Substitute.For>(), + Substitute.For>(), + Substitute.For(), + Substitute.For(), + Substitute.For()); + _pageModel.PageContext = new PageContext { HttpContext = new DefaultHttpContext() }; + } + + private void GivenBindingErrorsForEmptyAddressFields() + { + // Mirrors what model binding reports for the [Required] address fields when they are left blank. + foreach (var field in new[] { "Address", "City", "Country", "PostalCode" }) + { + _pageModel.ModelState.AddModelError($"UserPersonalUpdateModel.{field}", $"The {field} field is required."); + } + } + + [Fact] + public async Task SavesNameAndEmailGivenNoShippingAddress() + { + _pageModel.UserPersonalUpdateModel = new UserPersonalUpdateModel + { + FirstName = "Kajan", + LastName = "Smith", + Email = "kajan@example.com" + }; + GivenBindingErrorsForEmptyAddressFields(); + + var result = await _pageModel.OnPostUpdatePersonalInfoAsync(UserId); + + Assert.IsType(result); + Assert.Equal("Kajan", _member.FirstName); + Assert.Equal("kajan@example.com", _member.Email); + Assert.Null(_member.ShippingAddress); + await _memberRepository.Received(1).UpdateAsync(_member, Arg.Any()); + await _userManager.Received(1).UpdateAsync(Arg.Is(u => u.Email == "kajan@example.com")); + } + + [Fact] + public async Task RedisplaysFormWithSubmittedValuesGivenPartialShippingAddress() + { + _pageModel.UserPersonalUpdateModel = new UserPersonalUpdateModel + { + FirstName = "Kajan", + LastName = "Smith", + City = "Vavuniya" + }; + GivenBindingErrorsForEmptyAddressFields(); + + var result = await _pageModel.OnPostUpdatePersonalInfoAsync(UserId); + + Assert.IsType(result); + Assert.False(_pageModel.ModelState.IsValid); + Assert.Equal("Vavuniya", _pageModel.UserPersonalUpdateModel.City); + Assert.Equal(UserId, _pageModel.UserId); + await _memberRepository.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default); + } + + [Fact] + public async Task RedisplaysFormInsteadOfBadRequestGivenMissingLastName() + { + _pageModel.UserPersonalUpdateModel = new UserPersonalUpdateModel { FirstName = "Kajan" }; + _pageModel.ModelState.AddModelError("UserPersonalUpdateModel.LastName", "The LastName field is required."); + + var result = await _pageModel.OnPostUpdatePersonalInfoAsync(UserId); + + Assert.IsType(result); + await _memberRepository.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default); + } + + [Fact] + public async Task RedisplaysFormWithErrorGivenNoMemberRecord() + { + _memberRepository.FirstOrDefaultAsync(Arg.Any(), Arg.Any()) + .Returns((Member?)null); + _pageModel.UserPersonalUpdateModel = new UserPersonalUpdateModel { FirstName = "Kajan", LastName = "Smith" }; + + var result = await _pageModel.OnPostUpdatePersonalInfoAsync(UserId); + + Assert.IsType(result); + Assert.False(_pageModel.ModelState.IsValid); + } +} From 803748443eb3d568f618bf39ff06e798ccf36813 Mon Sep 17 00:00:00 2001 From: "Steve \"Ardalis\" Smith" Date: Mon, 21 Sep 2026 15:14:16 -0400 Subject: [PATCH 2/4] Potential fix for pull request finding 'CodeQL / Log entries created from user input' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs index 4eb690afc..450d8b987 100644 --- a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs +++ b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs @@ -107,7 +107,7 @@ public async Task OnGetAsync(string userId) } catch (Exception exception) { - _logger.LogError(exception, "Unable to load Stripe invoices for userId {UserId}", userId); + _logger.LogError(exception, "Unable to load Stripe invoices for userId {UserId}", SanitizeForLog(userId)); } var roles = await _roleManager.Roles.ToListAsync(); @@ -261,6 +261,11 @@ public async Task OnPostEditSubscriptionAsync(string userId, int return RedirectToPage("./User", new { userId = userId }); } + private static string SanitizeForLog(string? value) + { + return value?.Replace("\r", string.Empty).Replace("\n", string.Empty) ?? string.Empty; + } + public async Task OnPostUpdateEmailConfirmationAsync(string userId, bool isEmailConfirmed) { await _userEmailConfirmationService.UpdateUserEmailConfirmationAsync(userId, !isEmailConfirmed); From f936ac33c28ac20a0bd0dbcd0cd753173d96bd89 Mon Sep 17 00:00:00 2001 From: "Steve \"Ardalis\" Smith" Date: Mon, 21 Sep 2026 15:14:28 -0400 Subject: [PATCH 3/4] Potential fix for pull request finding 'CodeQL / Generic catch clause' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs index 450d8b987..33f33c2c0 100644 --- a/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs +++ b/src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs @@ -105,7 +105,11 @@ public async Task OnGetAsync(string userId) var invoices = await _invoiceHandlerListService.SearchByEmailAsync(currentUser!.Email!); Invoices = _mapper.Map>(invoices); } - catch (Exception exception) + catch (InvalidOperationException exception) + { + _logger.LogError(exception, "Unable to load Stripe invoices for userId {UserId}", userId); + } + catch (DbUpdateException exception) { _logger.LogError(exception, "Unable to load Stripe invoices for userId {UserId}", SanitizeForLog(userId)); } From cea4dc6b404c1745030052b24705c0f05b2a6326 Mon Sep 17 00:00:00 2001 From: "Steve \"Ardalis\" Smith" Date: Mon, 21 Sep 2026 15:14:37 -0400 Subject: [PATCH 4/4] Potential fix for pull request finding 'CodeQL / Useless upcast' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs b/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs index f55ebffda..73aee4f77 100644 --- a/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs +++ b/tests/DevBetterWeb.Tests/Pages/AdminUserModelTests/OnPostUpdatePersonalInfoAsync.cs @@ -116,7 +116,7 @@ public async Task RedisplaysFormInsteadOfBadRequestGivenMissingLastName() public async Task RedisplaysFormWithErrorGivenNoMemberRecord() { _memberRepository.FirstOrDefaultAsync(Arg.Any(), Arg.Any()) - .Returns((Member?)null); + .Returns(null); _pageModel.UserPersonalUpdateModel = new UserPersonalUpdateModel { FirstName = "Kajan", LastName = "Smith" }; var result = await _pageModel.OnPostUpdatePersonalInfoAsync(UserId);