Deal with a bunch of TODOs

src/ServiceControl.Persistence.RavenDb/DocumentIdGenerators/FailedMessageIdGenerator.cs

@@ -2,13 +2,10 @@
 {
     public const string CollectionName = "FailedMessages";
 
-    //[Obsolete("Use Guid.Parse")] TODO: As these are all guids... we don't need these generators.. Unless these are MessageIdentifiers as these can have any string value
     public static string MakeDocumentId(string messageUniqueId)
     {
-        Guard.Assert(!HasPrefix(messageUniqueId), $"value has {CollectionName}/ prefix"); // TODO: Could potentially be removed when all tests are green but no harm as its only included on Debug builds
         return $"{CollectionName}/{messageUniqueId}";
     }
 
     public static string GetMessageIdFromDocumentId(string failedMessageDocumentId) => failedMessageDocumentId.Substring(CollectionName.Length + 1);
-    static bool HasPrefix(string value) => value.StartsWith(CollectionName + "/");
 }

src/ServiceControl.Persistence.RavenDb/DocumentIdGenerators/MessageBodyIdGenerator.cs

@@ -4,9 +4,6 @@
 
     public static string MakeDocumentId(string messageUniqueId)
     {
-        Guard.Assert(!HasPrefix(messageUniqueId), $"value has {CollectionName}/ prefix"); // TODO: Could potentially be removed when all tests are green but no harm as its only included on Debug builds
         return $"{CollectionName}/{messageUniqueId}";
     }
-
-    static bool HasPrefix(string value) => value.StartsWith(CollectionName + "/");
 }

src/ServiceControl.Persistence.RavenDb/Editing/NotificationsManager.cs

@@ -7,7 +7,7 @@
 
     class NotificationsManager : AbstractSessionManager, INotificationsManager
     {
-        static readonly TimeSpan CacheTimeoutDefault = TimeSpan.FromMinutes(5); // TODO: Copied value from SendEmailNotificationHandler.cacheTimeout, Raven requires this to be at least 1 second
+        static readonly TimeSpan CacheTimeoutDefault = TimeSpan.FromMinutes(5); // Raven requires this to be at least 1 second
 
         public NotificationsManager(IAsyncDocumentSession session) : base(session)
         {

src/ServiceControl.Persistence.RavenDb/ErrorMessagesDataStore.cs

@@ -217,7 +217,7 @@ public async Task<QueryResult<FailureGroupView>> GetFailureGroupView(string grou
                     .WhereEquals(group => group.Id, groupId)
                     .FilterByStatusWhere(status)
                     .FilterByLastModifiedRange(modified)
-                    .FirstOrDefaultAsync(); // TODO: Was previously a to list with a linq to object FirstOrDefault, not sure if this works;
+                    .FirstOrDefaultAsync();
 
                 return new QueryResult<FailureGroupView>(document, stats.ToQueryStatsInfo());
             }
@@ -343,8 +343,6 @@ public async Task<IDictionary<string, object>> ErrorsSummary()
                         x => (object)x.Value
                         );
 
-                Guard.Assert(false, "TODO: Check how to convert dictionary item VALUES, currently return object which must be typed");
-
                 return results;
             }
         }
@@ -667,7 +665,8 @@ public Task RemoveFailedMessageRetryDocument(string uniqueMessageId)
             return documentStore.AsyncDatabaseCommands.DeleteAsync(FailedMessageRetry.MakeDocumentId(uniqueMessageId), null);
         }
 
-        public async Task<string[]> GetRetryPendingMessages(DateTime from, DateTime to, string queueAddress) // TODO: Could we use IAsyncEnumerable here as this is an unbounded query?
+        // TODO: Once using .NET, consider using IAsyncEnumerable here as this is an unbounded query
+        public async Task<string[]> GetRetryPendingMessages(DateTime from, DateTime to, string queueAddress)
         {
             var ids = new List<string>();
 
@@ -692,10 +691,9 @@ public async Task<string[]> GetRetryPendingMessages(DateTime from, DateTime to,
                 }
             }
 
-            return ids.ToArray(); // TODO: Currently returning array as all other API's return arrays and not IEnumerable<T>
+            return ids.ToArray();
         }
 
-        // TODO: How is this different than what RavenAttachmentBodyStorage.TryFetch is doing? Is this implemented twice?
         public async Task<byte[]> FetchFromFailedMessage(string uniqueMessageId)
         {
             string documentId = FailedMessageIdGenerator.MakeDocumentId(uniqueMessageId);
@@ -722,7 +720,7 @@ public async Task StoreEventLogItem(EventLogItem logItem)
             }
         }
 
-        public async Task StoreFailedMessages(params FailedMessage[] failedMessages)
+        public async Task StoreFailedMessagesForTestsOnly(params FailedMessage[] failedMessages)
         {
             using (var session = documentStore.OpenAsyncSession())
             {

src/ServiceControl.Persistence.RavenDb/FailedMessageViewIndexNotifications.cs

@@ -9,7 +9,7 @@
     using Persistence;
     using Raven.Client;
 
-    class FailedMessageViewIndexNotifications  // TODO: Must be registered as single and as hosted service
+    class FailedMessageViewIndexNotifications
         : IFailedMessageViewIndexNotifications
         , IDisposable
         , IHostedService

src/ServiceControl.Persistence.RavenDb/RavenDbInstaller.cs

@@ -4,6 +4,7 @@
     using System.Threading.Tasks;
     using NServiceBus.Logging;
     using Raven.Client.Embedded;
+    using ServiceControl.Infrastructure.RavenDB;
 
     class RavenDbInstaller : IPersistenceInstaller
     {
@@ -19,22 +20,13 @@ public async Task Install(CancellationToken cancellationToken = default)
             documentStore.Initialize();
             Logger.Info("Database initialization complete");
 
+            Logger.Info("Index creation started");
             await ravenStartup.CreateIndexesAsync(documentStore);
+            Logger.Info("Index creation complete");
 
-            Logger.Info("Data migrations starting **TODO NOT IMPLEMENTED YET**");
-
-            // TODO: Figure out migrations
-            // This was copied from audit code:
-            ////var endpointMigrations = new MigrateKnownEndpoints(documentStore);
-            ////await endpointMigrations.Migrate(cancellationToken: cancellationToken)
-            ////    ;
-            // While this was copied from EmbeddedRavenDbHostedService in main ServiceControl project, but list was injected
-            ////foreach (var migration in dataMigrations)
-            ////{
-            ////    await migration.Migrate(documentStore)
-            ////        ;
-            ////}
-
+            Logger.Info("Data migrations starting");
+            var purgeTempIdKnownEndpoints = new PurgeKnownEndpointsWithTemporaryIdsThatAreDuplicateDataMigration();
+            await purgeTempIdKnownEndpoints.Migrate(documentStore);
             Logger.Info("Data migrations complete");
 
         }

src/ServiceControl.Persistence.RavenDb/RavenDbPersistence.cs

@@ -78,21 +78,6 @@ public IPersistenceLifecycle CreateLifecycle()
             return new RavenDbPersistenceLifecycle(ravenStartup, documentStore);
         }
 
-        // TODO: Make sure this stuff from PersistenceHostBuilderExtensions is accounted for here
-
-        //var documentStore = new EmbeddableDocumentStore();
-        //RavenBootstrapper.Configure(documentStore, settings);
-
-        //hostBuilder.ConfigureServices(serviceCollection =>
-        //{
-        //    serviceCollection.AddSingleton<IDocumentStore>(documentStore);
-        //    serviceCollection.AddHostedService<EmbeddedRavenDbHostedService>();
-        //    serviceCollection.AddCustomCheck<CheckRavenDBIndexErrors>();
-        //    serviceCollection.AddCustomCheck<CheckRavenDBIndexLag>();
-
-        //    serviceCollection.AddServiceControlPersistence(settings.DataStoreType);
-        //});
-
         public IPersistenceInstaller CreateInstaller()
         {
             return new RavenDbInstaller(documentStore, ravenStartup);

src/ServiceControl.Persistence.Tests.RavenDb/Expiration/ProcessedMessageExpirationTests.cs

@@ -18,7 +18,7 @@
 
     sealed class ProcessedMessageExpirationTests : PersistenceTestBase
     {
-        IDocumentStore DocumentStore => GetRequiredService<IDocumentStore>(); // TODO: Should primary use abstractions and only have native code for triggering the cleanup
+        IDocumentStore DocumentStore => GetRequiredService<IDocumentStore>();
 
         [Test]
         public void Old_documents_are_being_expired()

src/ServiceControl.Persistence.Tests.RavenDb/Recoverability/RetryConfirmationProcessorTests.cs

@@ -11,10 +11,9 @@
     using NServiceBus.Testing;
     using NServiceBus.Transport;
     using NUnit.Framework;
-    using ServiceControl.Operations;
     using PersistenceTests;
+    using ServiceControl.Operations;
 
-    // TODO: Moved by Ramon to RavenDB specific tests, has a lot of RavenDB dependencies
     class RetryConfirmationProcessorTests : PersistenceTestBase
     {
         RetryConfirmationProcessor Processor { get; set; }
@@ -28,7 +27,7 @@ class RetryConfirmationProcessorTests : PersistenceTestBase
 
             Handler = new LegacyMessageFailureResolvedHandler(ErrorMessageDataStore, domainEvents);
 
-            await ErrorMessageDataStore.StoreFailedMessages(
+            await ErrorMessageDataStore.StoreFailedMessagesForTestsOnly(
                 new FailedMessage
                 {
                     Id = MessageId,

src/ServiceControl.Persistence.Tests.RavenDb/Recoverability/ReturnToSenderDequeuerTests.cs

@@ -101,7 +101,7 @@ public async Task It_fetches_the_body_from_index_if_provided()
                 }
             };
 
-            await ErrorMessageDataStore.StoreFailedMessages(failedMessage);
+            await ErrorMessageDataStore.StoreFailedMessagesForTestsOnly(failedMessage);
 
             var transformer = new MessagesBodyTransformer();
             await transformer.ExecuteAsync(documentStore);

src/ServiceControl.Persistence.Tests/PersistenceTestBase.cs

@@ -39,7 +39,6 @@ protected Task CompleteDatabaseOperation()
             return testPersistence.CompleteDatabaseOperation();
         }
 
-        // TODO: Evaluate if BlockToInspectDatabase() concept should stay in codebase long-term
         [Conditional("DEBUG")]
         protected void BlockToInspectDatabase() => testPersistence.BlockToInspectDatabase();
 

src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs

@@ -241,7 +241,7 @@ async Task<FailedMessage> CreateAndStoreFailedMessage(string failedMessageId = n
                         }
                     }
             };
-            await ErrorMessageDataStore.StoreFailedMessages(new[] { failedMessage });
+            await ErrorMessageDataStore.StoreFailedMessagesForTestsOnly(new[] { failedMessage });
             return failedMessage;
         }
     }

src/ServiceControl.Persistence.Tests/RetryStateTests.cs

@@ -203,7 +203,7 @@ async Task CreateAFailedMessageAndMarkAsPartOfRetryBatch(RetryingManager retryMa
                 }
             }).ToArray();
 
-            await ErrorStore.StoreFailedMessages(messages);
+            await ErrorStore.StoreFailedMessagesForTestsOnly(messages);
 
             // Needs index FailedMessages_ByGroup
             // Needs index FailedMessages_UniqueMessageIdAndTimeOfFailures
@@ -212,7 +212,6 @@ async Task CreateAFailedMessageAndMarkAsPartOfRetryBatch(RetryingManager retryMa
             var documentManager = new CustomRetryDocumentManager(progressToStaged, RetryStore, retryManager);
             var gateway = new CustomRetriesGateway(progressToStaged, RetryStore, retryManager);
 
-            // TODO: groupType appears to be the same as classifier, which was null in the previous StartRetryForIndex call - make sure that's true
             gateway.EnqueueRetryForFailureGroup(new RetriesGateway.RetryForFailureGroup(groupId, "Test-Context", groupType: null, DateTime.UtcNow));
 
             await CompleteDatabaseOperation();

src/ServiceControl.Persistence/IErrorMessageDatastore.cs

@@ -76,7 +76,6 @@ public interface IErrorMessageDataStore
 
         Task<QueryResult<IList<MessagesView>>> SearchEndpointMessages(string endpointName, string searchKeyword, PagingInfo pagingInfo, SortInfo sortInfo);
 
-        // TODO: So far only used in a persistence test: RetryStateTests
-        Task StoreFailedMessages(params FailedMessage[] failedMessages);
+        Task StoreFailedMessagesForTestsOnly(params FailedMessage[] failedMessages);
     }
 }

src/ServiceControl.Persistence/RetryBatchGroup.cs

@@ -8,6 +8,7 @@ public class RetryBatchGroup
 
         // [Raven.Imports.Newtonsoft.Json.JsonProperty(NullValueHandling = NullValueHandling.Ignore)] //default to RetryType.Unknown for backwards compatability
         // TODO: Need to fix ethe JsonProperty, maybe RavenDB has a method to specify metatdata or use a mapper/transformation
+        // THEORY: RetryType.Unknown is value 0 so it should default to that anyway
         public RetryType RetryType { get; set; }
 
         public bool HasStagingBatches { get; set; }

src/ServiceControl.UnitTests/API/APIApprovals.cs

@@ -6,11 +6,9 @@
     using System.Net.Http;
     using System.Reflection;
     using System.Text;
-    using System.Text.Json;
     using System.Web.Http.Controllers;
     using System.Web.Http.Hosting;
     using System.Web.Http.Routing;
-    using Newtonsoft.Json.Linq;
     using NServiceBus.CustomChecks;
     using NUnit.Framework;
     using Particular.Approvals;
@@ -134,7 +132,7 @@ public void TransportNames()
             Approver.Verify(publicTransportNames);
         }
 
-        [Test, Ignore("TODO: Deal with this once persister settings are properly managed")] // 
+        [Test, Ignore("TODO: Deal with this once persister settings are properly managed")]
         public void PlatformSampleSettings()
         {
             //HINT: Particular.PlatformSample includes a parameterized version of the ServiceControl.exe.config file.

src/ServiceControl/Bootstrapper.cs

@@ -93,11 +93,6 @@ void CreateHost()
 
             HostBuilder = new HostBuilder();
             HostBuilder
-                .UseDefaultServiceProvider(c => // TODO: Remove when done testing
-                {
-                    c.ValidateOnBuild = false;
-                    c.ValidateScopes = false;
-                })
                 .ConfigureLogging(builder =>
                 {
                     builder.ClearProviders();

src/ServiceControl/MessageFailures/InternalMessages/UnArchiveMessages.cs

@@ -5,6 +5,6 @@
 
     class UnArchiveMessages : ICommand
     {
-        public List<string> FailedMessageIds { get; set; } // TODO: 
+        public List<string> FailedMessageIds { get; set; }
     }
 }

src/ServiceControl/Operations/ErrorProcessor.cs

@@ -117,7 +117,7 @@ async Task ProcessMessage(MessageContext context, IIngestionUnitOfWork unitOfWor
 
                 var processingAttempt = failedMessageFactory.CreateProcessingAttempt(
                     context.Headers,
-                    new Dictionary<string, object>(metadata), // TODO: metadata is already a dictionary, it this really needed?
+                    new Dictionary<string, object>(metadata),
                     failureDetails);
 
                 await bodyStorageEnricher.StoreErrorMessageBody(context.Body, processingAttempt);

src/ServiceControl/Recoverability/API/FailureGroupsController.cs

@@ -85,7 +85,7 @@ public async Task<HttpResponseMessage> GetAllGroups(string classifier = "Excepti
                 classifierFilter = null;
             }
 
-            var results = await groupFetcher.GetGroups(classifier, classifierFilter); // TODO: Analyze what to do with the GroupFetcher dependency
+            var results = await groupFetcher.GetGroups(classifier, classifierFilter);
 
             return Negotiator.FromModel(Request, results)
                 .WithDeterministicEtag(EtagHelper.CalculateEtag(results));
@@ -137,7 +137,6 @@ public async Task<HttpResponseMessage> GetGroup(string groupId)
             string status = Request.GetStatus();
             string modified = Request.GetModified();
 
-            // TODO: Migrated as previous behavior but can be optimized as http api will return at most 1 item
             var result = await dataStore.GetGroup(groupId, status, modified);
 
             return Negotiator

src/ServiceControl/Recoverability/Retrying/RetryDocumentManager.cs

@@ -71,7 +71,8 @@ public async Task RebuildRetryOperationState()
         readonly RetryingManager operationManager;
         readonly IRetryDocumentDataStore store;
         bool abort;
-        public static string RetrySessionId = Guid.NewGuid().ToString(); // TODO: Uplift this into DI?
+        // TODO: Uplift this into DI? Meant to differentiate between ServiceControl.exe process runs, so likely doesn't matter.
+        public static string RetrySessionId = Guid.NewGuid().ToString();
 
         static readonly ILog log = LogManager.GetLogger(typeof(RetryDocumentManager));
     }

src/ServiceControl/SagaAudit/SagaAuditComponent.cs

@@ -9,6 +9,7 @@ class SagaAuditComponent : ServiceControlComponent
         public override void Configure(Settings settings, IHostBuilder hostBuilder)
         {
             // TODO: If this component doesn't do anything, should it even exist?
+            // THEORY: Remove in V5, since then there will be no audit capabilities left in the primary instance
         }
 
         public override void Setup(Settings settings, IComponentInstallationContext context)