Fix saving personal info on the Admin User page

src/DevBetterWeb.Web/Pages/Admin/User.cshtml

@@ -119,7 +119,7 @@
 <div class="row" style="margin-top: 20px;">
   <div class="col-12">
     <form asp-page-handler="UpdatePersonalInfo">
-      <input type="hidden" name="userId" value="@Model.UserPersonalUpdateModel.UserId" />
+      <input type="hidden" name="userId" value="@Model.UserId" />
       <div class="card">
         <div class="card-header">
           <b>Personal</b>
@@ -128,6 +128,7 @@
           </div>
         </div>
         <div class="card-body">
+          <div asp-validation-summary="ModelOnly" class="alert alert-danger" role="alert"></div>
           <div class="form-group">
             <label class="font-weight-bold">Email</label>
             <input asp-for="UserPersonalUpdateModel.Email" type="text" value="@Model.UserPersonalUpdateModel.Email" class="form-control form-control-sm" />
@@ -213,7 +214,7 @@
 <div class="row" style="margin-top: 20px;">
   <div class="col-12">
     <form asp-page-handler="UpdateLinks">
-      <input type="hidden" name="userId" value="@Model.UserLinksUpdateModel.UserId" />
+      <input type="hidden" name="userId" value="@Model.UserId" />
       <div class="card">
         <div class="card-header">
           <b>Links</b>

src/DevBetterWeb.Web/Pages/Admin/User.cshtml.cs

@@ -69,6 +69,9 @@ public UserModel(ILogger<UserModel> 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<IdentityRole> Roles { get; set; } = new List<IdentityRole>();
 	public List<SelectListItem> RolesNotAssignedToUser { get; set; } = new List<SelectListItem>();
@@ -81,6 +84,8 @@ public UserModel(ILogger<UserModel> logger,
 
 	public async Task<IActionResult> OnGetAsync(string userId)
 	{
+		UserId = userId;
+
 		try
 		{
 			if (string.IsNullOrEmpty(userId))
@@ -95,8 +100,19 @@ public async Task<IActionResult> OnGetAsync(string userId)
 				return BadRequest();
 			}
 
-			var invoices = await _invoiceHandlerListService.SearchByEmailAsync(currentUser!.Email!);
-			Invoices = _mapper.Map<List<StripeInvoiceDto>>(invoices);
+			try
+			{
+				var invoices = await _invoiceHandlerListService.SearchByEmailAsync(currentUser!.Email!);
+				Invoices = _mapper.Map<List<StripeInvoiceDto>>(invoices);
+			}
+			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));
+			}
 
 			var roles = await _roleManager.Roles.ToListAsync();
 
@@ -249,6 +265,11 @@ public async Task<IActionResult> 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<IActionResult> OnPostUpdateEmailConfirmationAsync(string userId, bool isEmailConfirmed)
 	{
 		await _userEmailConfirmationService.UpdateUserEmailConfirmationAsync(userId, !isEmailConfirmed);
@@ -258,21 +279,38 @@ public async Task<IActionResult> OnPostUpdateEmailConfirmationAsync(string userI
 
 	public async Task<IActionResult> 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 +325,32 @@ public async Task<IActionResult> 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<IActionResult> RedisplayPersonalInfoFormAsync(string userId)
+	{
+		var submitted = UserPersonalUpdateModel;
+		var result = await OnGetAsync(userId);
+		UserPersonalUpdateModel = submitted;
+		return result;
+	}
+
 	public async Task<IActionResult> OnPostUpdateLinksAsync(string userId)
 	{
 		var spec = new MemberByUserIdSpec(userId);

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<Member> _memberRepository = Substitute.For<IRepository<Member>>();
+  private readonly UserManager<ApplicationUser> _userManager = UserManagerHelpers.CreateSubstitute();
+  private readonly Member _member = MemberHelpers.CreateWithInternalConstructor();
+  private readonly UserModel _pageModel;
+
+  public OnPostUpdatePersonalInfoAsync()
+  {
+    _memberRepository.FirstOrDefaultAsync(Arg.Any<MemberByUserIdSpec>(), Arg.Any<CancellationToken>())
+      .Returns(_member);
+    _userManager.FindByIdAsync(UserId).Returns(new ApplicationUser { Id = UserId });
+
+    var roleManager = Substitute.For<RoleManager<IdentityRole>>(
+      Substitute.For<IRoleStore<IdentityRole>>(), null!, null!, null!, null!);
+
+    _pageModel = new UserModel(NullLogger<UserModel>.Instance,
+      _userManager,
+      roleManager,
+      Substitute.For<IUserRoleMembershipService>(),
+      Substitute.For<IMemberRegistrationService>(),
+      _memberRepository,
+      Substitute.For<IRepository<MemberSubscription>>(),
+      Substitute.For<IRepository<MemberSubscriptionPlan>>(),
+      Substitute.For<IUserEmailConfirmationService>(),
+      Substitute.For<IInvoiceHandlerListService>(),
+      Substitute.For<IMapper>());
+    _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<RedirectToPageResult>(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<CancellationToken>());
+    await _userManager.Received(1).UpdateAsync(Arg.Is<ApplicationUser>(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<PageResult>(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<PageResult>(result);
+    await _memberRepository.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default);
+  }
+
+  [Fact]
+  public async Task RedisplaysFormWithErrorGivenNoMemberRecord()
+  {
+    _memberRepository.FirstOrDefaultAsync(Arg.Any<MemberByUserIdSpec>(), Arg.Any<CancellationToken>())
+      .Returns(null);
+    _pageModel.UserPersonalUpdateModel = new UserPersonalUpdateModel { FirstName = "Kajan", LastName = "Smith" };
+
+    var result = await _pageModel.OnPostUpdatePersonalInfoAsync(UserId);
+
+    Assert.IsType<PageResult>(result);
+    Assert.False(_pageModel.ModelState.IsValid);
+  }
+}