Keep the time zone on save, let guests be saved, stop losing the first click (2.4.x)

src/Tests/Grand.Web.Admin.Tests/Extensions/DateTimeServiceExtensionsTests.cs

@@ -0,0 +1,79 @@
+using Grand.Business.Core.Interfaces.Common.Directory;
+using Grand.Web.AdminShared.Extensions;
+using Microsoft.AspNetCore.Mvc.Rendering;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+using System.Collections.ObjectModel;
+
+namespace Grand.Web.Admin.Tests.Extensions;
+
+[TestClass]
+public class DateTimeServiceExtensionsTests
+{
+    private const string ServerLabel = "Server time zone";
+
+    private readonly TimeZoneInfo _dateline = TimeZoneInfo.CreateCustomTimeZone("Dateline", TimeSpan.FromHours(-12), "Dateline", "Dateline");
+    private readonly TimeZoneInfo _warsaw = TimeZoneInfo.CreateCustomTimeZone("Warsaw", TimeSpan.FromHours(1), "Warsaw", "Warsaw");
+    private readonly TimeZoneInfo _unlisted = TimeZoneInfo.CreateCustomTimeZone("Other/Name", TimeSpan.FromHours(2), "Other", "Other");
+    private IDateTimeService _dateTimeService;
+
+    [TestInitialize]
+    public void Init()
+    {
+        var service = new Mock<IDateTimeService>();
+        service.Setup(x => x.GetSystemTimeZones())
+            .Returns(new ReadOnlyCollection<TimeZoneInfo>([_dateline, _warsaw]));
+        service.Setup(x => x.FindTimeZoneById(It.IsAny<string>())).Throws<TimeZoneNotFoundException>();
+        service.Setup(x => x.FindTimeZoneById("Other/Name")).Returns(_unlisted);
+        _dateTimeService = service.Object;
+    }
+
+    private IList<SelectListItem> Options(string configured) =>
+        _dateTimeService.GetStoreTimeZoneOptions(configured, ServerLabel);
+
+    private static string Selected(IList<SelectListItem> options) => options.Single(x => x.Selected).Value;
+
+    [TestMethod]
+    public void GetStoreTimeZoneOptions_StartWithTheServerZone_WithAnEmptyValue()
+    {
+        var first = Options("Warsaw")[0];
+
+        Assert.AreEqual("", first.Value);
+        StringAssert.StartsWith(first.Text, ServerLabel);
+    }
+
+    [TestMethod]
+    [DataRow(null)]
+    [DataRow("")]
+    public void GetStoreTimeZoneOptions_NothingConfigured_SelectsTheServerZone(string configured)
+    {
+        //so saving the form leaves the store following the server
+        Assert.AreEqual("", Selected(Options(configured)));
+    }
+
+    [TestMethod]
+    public void GetStoreTimeZoneOptions_ConfiguredZoneListed_SelectsIt()
+    {
+        Assert.AreEqual("Warsaw", Selected(Options("Warsaw")));
+    }
+
+    [TestMethod]
+    public void GetStoreTimeZoneOptions_ConfiguredZoneDiffersInCase_SelectsTheListedId()
+    {
+        Assert.AreEqual("Warsaw", Selected(Options("warsaw")));
+    }
+
+    [TestMethod]
+    public void GetStoreTimeZoneOptions_ConfiguredZoneResolvesButIsNotListed_KeepsIt()
+    {
+        //a Windows id on Linux, or the reverse: dates are shown in it, so saving must not change it
+        Assert.AreEqual("Other/Name", Selected(Options("Other/Name")));
+    }
+
+    [TestMethod]
+    public void GetStoreTimeZoneOptions_ConfiguredZoneUnknownOnThisMachine_SelectsTheServerZone()
+    {
+        //DateTimeService.CurrentTimeZone falls back to the server here as well
+        Assert.AreEqual("", Selected(Options("Gone/Zone")));
+    }
+}

src/Tests/Grand.Web.Admin.Tests/Services/CustomerViewModelServiceTests.cs

@@ -172,6 +172,17 @@ public async Task InsertCustomerModel_MapsStoreIdFromModel()
         _customerServiceMock.Verify(c => c.InsertCustomer(It.Is<Customer>(x => x.StoreId == "store-1")), Times.Once);
     }
 
+    [TestMethod]
+    public async Task UpdateCustomerModel_GuestWithoutEmail_StaysWithoutOne()
+    {
+        var customer = await _customerViewModelService.UpdateCustomerModel(
+            new Customer { Id = "c1" }, new CustomerModel { Id = "c1", AdminComment = "called in" });
+
+        Assert.AreEqual("called in", customer.AdminComment);
+        Assert.IsTrue(string.IsNullOrEmpty(customer.Email));
+        Assert.IsTrue(string.IsNullOrEmpty(customer.Username));
+    }
+
     [TestMethod]
     public async Task PrepareCustomerModel_StoreManager_PresetsCurrentStoreId()
     {

src/Tests/Grand.Web.Admin.Tests/Validators/CustomerValidatorTests.cs

@@ -20,7 +20,9 @@ public class CustomerValidatorTests
     private const string CurrentStoreId = "store-current";
     private const string RequiredMessage = "Admin.Customers.Customers.Fields.Store.Required";
     private const string MustBeCurrentStoreMessage = "Admin.Customers.Customers.Fields.Store.MustBeCurrentStore";
+    private const string EmailRequiredMessage = "Admin.Customers.Customers.Fields.Email.Required";
 
+    private Mock<ICustomerService> _customerServiceMock;
     private Mock<IGroupService> _groupServiceMock;
     private CustomerValidator _validator;
 
@@ -30,6 +32,7 @@ public void Setup()
         var translationServiceMock = new Mock<ITranslationService>();
         translationServiceMock.Setup(t => t.GetResource(It.IsAny<string>())).Returns<string>(k => k);
 
+        _customerServiceMock = new Mock<ICustomerService>();
         _groupServiceMock = new Mock<IGroupService>();
         _groupServiceMock
             .Setup(g => g.GetAllCustomerGroups(It.IsAny<string>(), It.IsAny<int>(), It.IsAny<int>(),
@@ -49,7 +52,7 @@ public void Setup()
             translationServiceMock.Object,
             new Mock<ICountryService>().Object,
             contextAccessorMock.Object,
-            new Mock<ICustomerService>().Object,
+            _customerServiceMock.Object,
             _groupServiceMock.Object,
             new CustomerSettings(),
             new CustomerConfig());
@@ -58,6 +61,79 @@ public void Setup()
     private static CustomerModel BuildModel(string storeId) =>
         new() { Id = "customer-1", Email = "customer@example.com", StoreId = storeId };
 
+    [TestMethod]
+    [DataRow(null)]
+    [DataRow("")]
+    public async Task Email_Empty_GuestWithoutEmail_IsAccepted(string email)
+    {
+        _customerServiceMock.Setup(c => c.GetCustomerById("customer-1"))
+            .ReturnsAsync(new Customer { Id = "customer-1", Email = null });
+
+        var result = await _validator.ValidateAsync(new CustomerModel { Id = "customer-1", Email = email, StoreId = CurrentStoreId });
+
+        Assert.IsFalse(result.Errors.Any(e => e.PropertyName == nameof(CustomerModel.Email)));
+    }
+
+    [TestMethod]
+    public async Task Email_Empty_GuestBecomingRegistered_ReportsRequired()
+    {
+        _customerServiceMock.Setup(c => c.GetCustomerById("customer-1"))
+            .ReturnsAsync(new Customer { Id = "customer-1", Email = null });
+        _groupServiceMock.Setup(g => g.GetCustomerGroupBySystemName(SystemCustomerGroupNames.Registered))
+            .ReturnsAsync(new CustomerGroup { Id = "group-registered" });
+
+        var result = await _validator.ValidateAsync(new CustomerModel {
+            Id = "customer-1", StoreId = CurrentStoreId, CustomerGroups = ["group-registered"]
+        });
+
+        Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == EmailRequiredMessage));
+    }
+
+    [TestMethod]
+    public async Task Email_Invalid_GuestWithoutEmail_IsStillChecked()
+    {
+        _customerServiceMock.Setup(c => c.GetCustomerById("customer-1"))
+            .ReturnsAsync(new Customer { Id = "customer-1", Email = null });
+
+        var result = await _validator.ValidateAsync(new CustomerModel { Id = "customer-1", Email = "not-an-email", StoreId = CurrentStoreId });
+
+        Assert.IsTrue(result.Errors.Any(e => e.PropertyName == nameof(CustomerModel.Email)));
+    }
+
+    [TestMethod]
+    [DataRow(null)]
+    [DataRow("")]
+    public async Task Email_Cleared_RegisteredCustomer_ReportsRequired(string email)
+    {
+        _customerServiceMock.Setup(c => c.GetCustomerById("customer-1"))
+            .ReturnsAsync(new Customer { Id = "customer-1", Email = "customer@example.com" });
+
+        var result = await _validator.ValidateAsync(new CustomerModel { Id = "customer-1", Email = email, StoreId = CurrentStoreId });
+
+        Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == EmailRequiredMessage));
+    }
+
+    [TestMethod]
+    public async Task Email_Changed_IsStillCheckedForDuplicates()
+    {
+        _customerServiceMock.Setup(c => c.GetCustomerById("customer-1"))
+            .ReturnsAsync(new Customer { Id = "customer-1", Email = "customer@example.com" });
+        _customerServiceMock.Setup(c => c.GetCustomerByEmail("taken@example.com", It.IsAny<string>()))
+            .ReturnsAsync(new Customer { Id = "customer-2", Email = "taken@example.com" });
+
+        var result = await _validator.ValidateAsync(new CustomerModel { Id = "customer-1", Email = "taken@example.com", StoreId = CurrentStoreId });
+
+        Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == "Account.EmailUsernameErrors.EmailAlreadyExists"));
+    }
+
+    [TestMethod]
+    public async Task Email_Empty_NewCustomer_ReportsRequired()
+    {
+        var result = await _validator.ValidateAsync(new CustomerModel { StoreId = CurrentStoreId });
+
+        Assert.IsTrue(result.Errors.Any(e => e.ErrorMessage == EmailRequiredMessage));
+    }
+
     [TestMethod]
     public async Task StoreId_Empty_FailsRequired()
     {

src/Web/Grand.Web.Admin/Controllers/SettingController.cs

@@ -23,8 +23,10 @@
 using Grand.Domain.Seo;
 using Grand.Domain.Stores;
 using Grand.Domain.Vendors;
+using Grand.Infrastructure;
 using Grand.Infrastructure.Caching;
 using Grand.SharedKernel.Extensions;
+using Grand.Web.AdminShared.Extensions;
 using Grand.Web.AdminShared.Extensions.Mapping;
 using Grand.Web.AdminShared.Extensions.Mapping.Settings;
 using Grand.Web.AdminShared.Models.Settings;
@@ -37,7 +39,6 @@
 using Grand.Mediator;
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Mvc.Rendering;
-using OperatingSystem = Grand.Infrastructure.OperatingSystem;
 
 namespace Grand.Web.Admin.Controllers;
 
@@ -596,25 +597,21 @@ public async Task<IActionResult> Customer(CustomerSettingsModel model)
         return RedirectToAction("Customer");
     }
 
-    public async Task<IActionResult> GeneralCommon([FromServices] IEnumerable<IThemeView> themes, [FromServices] IDateTimeService dateTimeService)
+    public async Task<IActionResult> GeneralCommon([FromServices] IEnumerable<IThemeView> themes, [FromServices] IDateTimeService dateTimeService,
+        [FromServices] IContextAccessor contextAccessor)
     {
         var model = new GeneralCommonSettingsModel();
         var storeScope = await GetActiveStore();
         model.ActiveStore = storeScope;
         //datettime settings
         var dateTimeSettings = await settingService.LoadSetting<DateTimeSettings>(storeScope);
-        model.DateTimeSettings.DefaultStoreTimeZoneId = dateTimeSettings.DefaultStoreTimeZoneId;
-        var iswindows = OperatingSystem.IsWindows();
-        foreach (var timeZone in dateTimeService.GetSystemTimeZones())
-        {
-            var name = iswindows ? timeZone.DisplayName : $"{timeZone.StandardName} ({timeZone.Id})";
-            model.DateTimeSettings.AvailableTimeZones.Add(new SelectListItem {
-                Text = name,
-                Value = timeZone.Id,
-                Selected = timeZone.Id.Equals(dateTimeSettings.DefaultStoreTimeZoneId,
-                    StringComparison.OrdinalIgnoreCase)
-            });
-        }
+        //the default text is for a database already on 2.4, where the resources of 2.4 are not imported again
+        model.DateTimeSettings.AvailableTimeZones = dateTimeService.GetStoreTimeZoneOptions(
+            dateTimeSettings.DefaultStoreTimeZoneId,
+            translationService.GetResource("Admin.Settings.GeneralCommon.DefaultStoreTimeZone.Server",
+                contextAccessor.WorkContext.WorkingLanguage.Id, "Server time zone"));
+        model.DateTimeSettings.DefaultStoreTimeZoneId =
+            model.DateTimeSettings.AvailableTimeZones.First(x => x.Selected).Value;
 
         //store information
         var storeInformationSettings = await settingService.LoadSetting<StoreInformationSettings>(storeScope);

src/Web/Grand.Web.AdminShared/Extensions/DateTimeServiceExtensions.cs

@@ -0,0 +1,56 @@
+using Grand.Business.Core.Interfaces.Common.Directory;
+using Microsoft.AspNetCore.Mvc.Rendering;
+
+namespace Grand.Web.AdminShared.Extensions;
+
+public static class DateTimeServiceExtensions
+{
+    /// <summary>
+    ///     The choices for a store's time zone, with the one dates are really shown in selected
+    ///     (see DateTimeService.CurrentTimeZone). The empty value means the store follows the server, as it does
+    ///     after installation, so saving the form never turns that into a fixed zone. A configured zone that
+    ///     resolves but is not listed - a Windows id on Linux, or the reverse - keeps an option of its own.
+    /// </summary>
+    public static IList<SelectListItem> GetStoreTimeZoneOptions(this IDateTimeService dateTimeService,
+        string configuredTimeZoneId, string serverTimeZoneLabel)
+    {
+        var options = new List<SelectListItem> {
+            new() { Text = $"{serverTimeZoneLabel}: {DisplayName(TimeZoneInfo.Local)}", Value = "" }
+        };
+        options.AddRange(dateTimeService.GetSystemTimeZones()
+            .Select(zone => new SelectListItem { Text = DisplayName(zone), Value = zone.Id }));
+
+        var selected = options.FirstOrDefault(x => x.Value.Equals(configuredTimeZoneId, StringComparison.OrdinalIgnoreCase))
+                       ?? AddUnlistedZone(dateTimeService, configuredTimeZoneId, options)
+                       ?? options[0];
+        selected.Selected = true;
+
+        return options;
+    }
+
+    private static SelectListItem AddUnlistedZone(IDateTimeService dateTimeService, string timeZoneId,
+        List<SelectListItem> options)
+    {
+        if (string.IsNullOrEmpty(timeZoneId)) return null;
+        try
+        {
+            var option = new SelectListItem {
+                Text = DisplayName(dateTimeService.FindTimeZoneById(timeZoneId)), Value = timeZoneId
+            };
+            options.Add(option);
+            return option;
+        }
+        catch (Exception exc) when (exc is TimeZoneNotFoundException or InvalidTimeZoneException)
+        {
+            //unknown on this machine - DateTimeService.CurrentTimeZone shows dates in the server's zone then
+            return null;
+        }
+    }
+
+    private static string DisplayName(TimeZoneInfo zone)
+    {
+        return Grand.Infrastructure.OperatingSystem.IsWindows()
+            ? zone.DisplayName
+            : $"{zone.StandardName} ({zone.Id})";
+    }
+}

src/Web/Grand.Web.AdminShared/Services/CustomerViewModelService.cs

@@ -506,8 +506,8 @@ await _customerService.UpdateUserField(customer, SystemCustomerFieldNames.TwoFac
         customer.Email = model.Email;
 
         customer.Username = _customerSettings.UsernamesEnabled switch {
-            true when _customerSettings.AllowUsersToChangeUsernames => model.Username.Trim(),
-            false => model.Email.Trim(),
+            true when _customerSettings.AllowUsersToChangeUsernames => model.Username?.Trim(),
+            false => model.Email?.Trim(),
             _ => customer.Username
         };
 

src/Web/Grand.Web.AdminShared/Validators/Customers/CustomerValidator.cs

@@ -31,9 +31,11 @@ public CustomerValidator(
         CustomerCreateValidator();
         CustomerEditValidator();
 
-        //customer email
-        RuleFor(x => x.Email).NotEmpty().EmailAddress()
-            .WithMessage(translationService.GetResource("Admin.Customers.Customers.Fields.Email.Required"));
+        //customer email - a guest has none, and staying a guest must not take inventing one
+        RuleFor(x => x.Email)
+            .NotEmpty().WithMessage(translationService.GetResource("Admin.Customers.Customers.Fields.Email.Required"))
+            .EmailAddress().WithMessage(translationService.GetResource("Admin.Customers.Customers.Fields.Email.Required"))
+            .WhenAsync(async (x, _) => !string.IsNullOrEmpty(x.Email) || !await StaysGuestWithoutEmail(x));
 
         //store - required only when the editor is not an administrator (admins may manage store-independent
         //back-office/system accounts that have no store)
@@ -107,6 +109,17 @@ public CustomerValidator(
                 context.AddFailure("The username cannot be empty");
         });
 
+        async Task<bool> StaysGuestWithoutEmail(CustomerModel model)
+        {
+            if (string.IsNullOrEmpty(model.Id)) return false;
+
+            var customer = await customerService.GetCustomerById(model.Id);
+            if (customer == null || !string.IsNullOrEmpty(customer.Email)) return false;
+
+            var registered = await groupService.GetCustomerGroupBySystemName(SystemCustomerGroupNames.Registered);
+            return registered == null || model.CustomerGroups == null || !model.CustomerGroups.Contains(registered.Id);
+        }
+
         async Task<string> ValidateCustomerGroups(IList<CustomerGroup> customerGroups, CustomerModel customerModel)
         {
             ArgumentNullException.ThrowIfNull(customerGroups);
@@ -250,7 +263,8 @@ void CustomerEditValidator()
                         customer?.Id == contextAccessor.WorkContext.CurrentCustomer.Id)
                         context.AddFailure("You can't edit own data from admin panel");
 
-                    if (customer != null && customer.Email != x.Email.ToLower())
+                    //an empty email is for the required rule above to judge: guests have none
+                    if (customer != null && !string.IsNullOrWhiteSpace(x.Email) && customer.Email != x.Email.ToLower())
                     {
                         if (!CommonHelper.IsValidEmail(x.Email))
                             context.AddFailure(