fix(797): persist Folder Settings and repair the User Email lookup

TaskMaster.Test/AppGlobals/AppOlObjectsCoverageTests.cs

@@ -209,6 +209,88 @@ public void BuildFreshStoresWrapper_WhenLiveStoresAvailable_ReturnsInitializedWr
             result.Stores.Single().DisplayName.Should().Be("Mailbox");
         }
 
+        /// <summary>
+        /// A fixed, non-existent AppData-shaped settings path. It is never opened, created or
+        /// probed: both AC1 tests assert on the configuration value the loader carries, so no
+        /// filesystem access occurs and no host path appears in the test.
+        /// </summary>
+        private const string FakeAppDataSettingsPath =
+            @"X:\FakeAppData\TaskMaster\StoresWrapper.json";
+
+        [TestMethod]
+        public async Task LoadStoresAsync_WhenConfigDeserializesToNull_FreshWrapperAdoptsLoaderDiskConfiguration()
+        {
+            // Arrange (issue #797, AC1): the "StoresWrapper" key is present and carries a loader
+            // whose disk configuration holds the resource-defined path, but the deserialize returns
+            // null because the file is absent. The fresh-build branch must adopt that configuration
+            // instead of discarding it, so the first Save has a path to write to.
+            var application = new Mock<OutlookApplication>();
+            var configuration = new ConcurrentDictionary<string, SmartSerializableLoader>();
+            var globals = new StubApplicationGlobals();
+            var loader = new SmartSerializableLoader();
+            loader.Config.Disk.FilePath = FakeAppDataSettingsPath;
+            configuration.TryAdd("StoresWrapper", loader);
+            globals.IntelResInstance = new StubIntelligenceConfig(globals, configuration);
+            var freshWrapper = new StoresWrapper();
+            var smartSerializable = new Mock<ISmartSerializableNonTyped>();
+            smartSerializable
+                .Setup(x =>
+                    x.Deserialize<StoresWrapper, SmartSerializableLoader>(
+                        It.IsAny<SmartSerializable<SmartSerializableLoader>>()
+                    )
+                )
+                .Returns((StoresWrapper)null);
+
+            var sut = new TestableAppOlObjects(
+                application.Object,
+                globals,
+                _ => Task.CompletedTask,
+                freshWrapper
+            )
+            {
+                SmartSerializable = smartSerializable.Object,
+            };
+
+            // Act
+            await sut.LoadStoresAsync();
+
+            // Assert
+            sut.StoresWrapper.Should().BeSameAs(freshWrapper);
+            sut.StoresWrapper.Config.Disk.FilePath.Should()
+                .Be(
+                    FakeAppDataSettingsPath,
+                    "the fresh-build branch must adopt the loader's disk configuration (AC1)."
+                );
+        }
+
+        [TestMethod]
+        public async Task LoadStoresAsync_WhenConfigKeyIsAbsent_FreshWrapperKeepsEmptyDiskPath()
+        {
+            // Arrange (issue #797, AC1 negative case): with no "StoresWrapper" key there is no
+            // loader to adopt, so the fresh build must keep the FilePathHelper default empty path.
+            // That case is deliberately out of AC1's scope and is made visible by AC2's error log.
+            var application = new Mock<OutlookApplication>();
+            var configuration = new ConcurrentDictionary<string, SmartSerializableLoader>();
+            var globals = new StubApplicationGlobals();
+            globals.IntelResInstance = new StubIntelligenceConfig(globals, configuration);
+            var freshWrapper = new StoresWrapper();
+
+            var sut = new TestableAppOlObjects(
+                application.Object,
+                globals,
+                _ => Task.CompletedTask,
+                freshWrapper
+            );
+
+            // Act
+            await sut.LoadStoresAsync();
+
+            // Assert
+            sut.StoresWrapper.Should().BeSameAs(freshWrapper);
+            sut.StoresWrapper.Config.Disk.FilePath.Should()
+                .BeEmpty("there is no loader to adopt on the key-absent branch.");
+        }
+
         private sealed class TestableAppOlObjects : AppOlObjects
         {
             private readonly Func<StoresWrapper, Task> awaitStoreRewireAsync;

TaskMaster/AppGlobals/AppOlObjects.JunkFolders.cs

@@ -16,7 +16,7 @@ namespace TaskMaster
     /// <c>AppOlObjects.cs</c> to bring that file under the 500-line cap. Behavior is unchanged
     /// (move-only).
     /// </summary>
-    public partial class AppOlObjects
+    public partial class AppOlObjects : IJunkFolderSelectionSink
     {
         private Folder _junkPotential;
         public Folder JunkPotential => Initializer.GetOrLoad(ref _junkPotential, LoadJunkPotential);
@@ -44,6 +44,18 @@ string junkPotentialRelativePath
             RefreshJunkFolderSelections();
         }
 
+        /// <summary>
+        /// Explicit implementation of <see cref="IJunkFolderSelectionSink"/> (issue #797, AC5),
+        /// forwarding to the existing internal method with the arguments in the certain-then-
+        /// potential order the interface fixes. The implementation is explicit so the public
+        /// surface of this type does not widen; the internal method keeps its accessibility and its
+        /// body.
+        /// </summary>
+        void IJunkFolderSelectionSink.ApplyJunkFolderSelections(
+            string junkCertainRelativePath,
+            string junkPotentialRelativePath
+        ) => ApplyJunkFolderSelections(junkCertainRelativePath, junkPotentialRelativePath);
+
         internal void RefreshJunkFolderSelections()
         {
             _junkCertain = null;

TaskMaster/AppGlobals/AppOlObjects.StoreLoading.cs

@@ -36,7 +36,11 @@ internal async Task LoadStoresAsync()
         {
             try
             {
-                if (_globals.IntelRes.Config.TryGetValue("StoresWrapper", out var config))
+                var configFound = _globals.IntelRes.Config.TryGetValue(
+                    "StoresWrapper",
+                    out var config
+                );
+                if (configFound)
                 {
                     var deserialized = SmartSerializable.Deserialize<
                         StoresWrapper,
@@ -62,6 +66,17 @@ internal async Task LoadStoresAsync()
                 // source from which a previously disabled-for-future-sessions store could be
                 // recovered, so a store re-enabled here is expected, not a regression in F1/F5.
                 StoresWrapper = BuildFreshStoresWrapper();
+
+                // why: issue #797 AC1. A freshly built wrapper carries the FilePathHelper default
+                // empty file path, so every later Save returned silently and the settings file was
+                // never created. The loader resolved above already carries the resource-defined
+                // path under the local application data TaskMaster directory, so adopt it here with
+                // a deep copy. The key-absent branch has no loader and deliberately keeps the empty
+                // path; AC2's error log makes that case visible rather than silent.
+                if (configFound && StoresWrapper is not null)
+                {
+                    StoresWrapper.Config.CopyFrom(config.Config, true);
+                }
             }
             catch (Exception e)
             {

UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperControllerTests.cs

@@ -2,8 +2,13 @@
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Drawing;
+using System.Linq;
 using System.Threading.Tasks;
 using FluentAssertions;
+using log4net;
+using log4net.Appender;
+using log4net.Core;
+using log4net.Repository.Hierarchy;
 using Microsoft.Office.Interop.Outlook;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Moq;
@@ -123,8 +128,11 @@ public void PopulateWithCurrent_WhenInvokeRequired_DelegatesToViewerInvoke()
         [TestMethod]
         public void PersistJunkFolderSelections_WhenApplyMethodIsMissing_DoesNotThrow()
         {
+            // Retargeted to the typed seam (issue #797, AC5). What is "missing" is no longer a
+            // method discoverable by name but an implementation of IJunkFolderSelectionSink: the
+            // double below declares a matching public method yet does not implement the interface.
             var globals = new Mock<IApplicationGlobals>();
-            globals.SetupGet(x => x.Ol).Returns(new NoApplyOlObjects());
+            globals.SetupGet(x => x.Ol).Returns(new NonSinkOlObjects());
             var controller = new StoreWrapperController(globals.Object)
             {
                 JunkEmail = new FolderMinimalWrapper("Junk", "Inbox\\Junk Email"),
@@ -136,6 +144,121 @@ public void PersistJunkFolderSelections_WhenApplyMethodIsMissing_DoesNotThrow()
             act.Should().NotThrow();
         }
 
+        [TestMethod]
+        public void PersistJunkFolderSelections_PassesJunkCertainPathFirst()
+        {
+            // Arrange (issue #797, AC5): the argument order is enforced by nothing except
+            // positional agreement between the call site and the signature, which is exactly the
+            // fragility the typed seam removes. Pin it with distinguishable values.
+            var olObjects = new RecordingOlObjects();
+            var globals = new Mock<IApplicationGlobals>();
+            globals.SetupGet(x => x.Ol).Returns(olObjects);
+            var controller = new StoreWrapperController(globals.Object)
+            {
+                JunkEmail = new FolderMinimalWrapper("Certain", "Inbox\\Certain Folder"),
+                JunkPotential = new FolderMinimalWrapper("Potential", "Inbox\\Potential Folder"),
+            };
+
+            // Act
+            controller.PersistJunkFolderSelections();
+
+            // Assert
+            olObjects.ApplyCallCount.Should().Be(1);
+            olObjects
+                .AppliedJunkCertainPath.Should()
+                .Be("Inbox\\Certain Folder", "the junk-certain path is supplied first.");
+            olObjects
+                .AppliedJunkPotentialPath.Should()
+                .Be("Inbox\\Potential Folder", "the junk-potential path is supplied second.");
+        }
+
+        [TestMethod]
+        public void PersistJunkFolderSelections_WhenGlobalsAreNotTheTypedSink_LogsErrorAndDoesNotInvoke()
+        {
+            // Arrange (issue #797, AC5): the failure must be loud. The double declares a public
+            // method with the historic name and signature but does not implement the sink
+            // interface, so the reflection lookup would have succeeded while the typed cast fails.
+            var olObjects = new NonSinkOlObjects();
+            var globals = new Mock<IApplicationGlobals>();
+            globals.SetupGet(x => x.Ol).Returns(olObjects);
+            var controller = new StoreWrapperController(globals.Object)
+            {
+                JunkEmail = new FolderMinimalWrapper("Junk", "Inbox\\Junk Email"),
+                JunkPotential = new FolderMinimalWrapper("Potential", "Inbox\\Junk Potential"),
+            };
+
+            var appender = AttachControllerMemoryAppender(out var restore);
+            try
+            {
+                // Act
+                controller.PersistJunkFolderSelections();
+
+                // Assert: existence, not an exact count. The controller's logger is a static field
+                // shared with every other controller test class in this assembly and the run
+                // settings impose a class-level parallel scope, so a sibling class can only add
+                // events. The paired assertion that the double recorded no invocation is what
+                // attributes the event to this test.
+                SinkErrorEvents(appender)
+                    .Should()
+                    .NotBeEmpty(
+                        "a failed cast to the typed sink must be reported at error level (AC5)."
+                    );
+                olObjects
+                    .ApplyCallCount.Should()
+                    .Be(0, "a double that is not the typed sink must never be invoked.");
+            }
+            finally
+            {
+                restore();
+            }
+        }
+
+        /// <summary>
+        /// Attaches an in-memory appender to the logger the controller writes to. The controller is
+        /// not generic, so its logger name is the full name of the controller type and the appender
+        /// can be attached to that named logger in the ordinary way.
+        /// </summary>
+        /// <param name="restore">
+        /// Receives the action that detaches the appender and restores the logger's previous level
+        /// and the repository's previous configured flag.
+        /// </param>
+        private static MemoryAppender AttachControllerMemoryAppender(out System.Action restore)
+        {
+            var appender = new MemoryAppender();
+            appender.ActivateOptions();
+
+            var controllerType = typeof(StoreWrapperController);
+            var hierarchy = (Hierarchy)LogManager.GetRepository(controllerType.Assembly);
+            var logger = (Logger)hierarchy.GetLogger(controllerType.FullName);
+            var previousLevel = logger.Level;
+            var previousConfigured = hierarchy.Configured;
+
+            logger.Level = Level.Debug;
+            hierarchy.Configured = true;
+            logger.AddAppender(appender);
+
+            restore = () =>
+            {
+                logger.RemoveAppender(appender);
+                logger.Level = previousLevel;
+                hierarchy.Configured = previousConfigured;
+            };
+
+            return appender;
+        }
+
+        private static LoggingEvent[] SinkErrorEvents(MemoryAppender appender)
+        {
+            return appender
+                .GetEvents()
+                .Where(loggingEvent =>
+                    loggingEvent.Level >= Level.Error
+                    && loggingEvent.RenderedMessage != null
+                    && loggingEvent.RenderedMessage.Contains(nameof(IJunkFolderSelectionSink))
+                )
+                .ToArray();
+        }
+
         private abstract class OlObjectsStubBase : IOlObjects
         {
             public Application App => null!;
@@ -193,7 +316,11 @@ string junkPotentialRelativePath
             }
         }
 
-        private sealed class RecordingOlObjects : OlObjectsStubBase
+        /// <summary>
+        /// Globals double that implements the typed junk-folder sink (issue #797, AC5) in addition
+        /// to the globals stub base, and records both arguments and the invocation count.
+        /// </summary>
+        private sealed class RecordingOlObjects : OlObjectsStubBase, IJunkFolderSelectionSink
         {
             public string AppliedJunkCertainPath { get; private set; } = string.Empty;
             public string AppliedJunkPotentialPath { get; private set; } = string.Empty;
@@ -211,6 +338,24 @@ string junkPotentialRelativePath
             }
         }
 
-        private sealed class NoApplyOlObjects : OlObjectsStubBase { }
+        /// <summary>
+        /// Globals double that implements only the globals interface while still declaring a public
+        /// method named <c>ApplyJunkFolderSelections</c> with the same two string parameters (issue
+        /// #797, AC5). The historic reflection lookup would bind to that method; the typed cast does
+        /// not, so this double drives the loud-failure branch.
+        /// </summary>
+        private sealed class NonSinkOlObjects : OlObjectsStubBase
+        {
+            public int ApplyCallCount { get; private set; }
+
+            public void ApplyJunkFolderSelections(
+                string junkCertainRelativePath,
+                string junkPotentialRelativePath
+            )
+            {
+                ApplyCallCount++;
+                SetJunkFolders(junkCertainRelativePath, junkPotentialRelativePath);
+            }
+        }
     }
 }

UtilitiesCS.Test/OutlookObjects/Store/StoreWrapperController_Tests.ButtonAndPopulate.cs

@@ -127,11 +127,17 @@ public void PopulateWithCurrent_NullCurrent_SetsErrorLoadingText()
             controller.Current = null;
             controller.FsConverter = (path) => ("", "");
 
-            // PopulateWithCurrent accesses Current which may be null
-            // This would throw NullReferenceException, verifying we need Current set
+            // Declared AC8 expectation inversion under D6 (issue #797): this test's name always
+            // described the fixed behaviour while its assertion codified the defect. The inverted
+            // assertion is stricter than the original, pinning the specific rendered values rather
+            // than merely an exception type.
             var act = () => controller.PopulateWithCurrent();
 
-            act.Should().Throw<NullReferenceException>();
+            act.Should().NotThrow();
+            mockViewer.Object.ArchiveOutlook.Text.Should().Be("Please select an archive");
+            mockViewer.Object.ArchiveFS.Text.Should().Be("Please select an archive");
+            mockViewer.Object.JunkEmail.Text.Should().Be("Please select a folder");
+            mockViewer.Object.JunkPotential.Text.Should().Be("Please select a folder");
         }
 
         [TestMethod]