feat: elaborate TODOs

Examples/BookingSystem.AspNetCore.IdentityServer/Custom/Users/UserRepository.cs

@@ -33,7 +33,7 @@ public async Task<User> FindByUsername(string username)
             return GetUserFromSellerUser(await _fakeBookingSystem.Database.GetSellerUser(username));
         }
 
-        // TODO: Make this an extension method
+        // TODO: Make this an extension method to Claim class
         private static void AddClaimIfNotNull(List<Claim> claims, string key, string value)
         {
             if (!string.IsNullOrEmpty(value))

Examples/BookingSystem.AspNetCore/Stores/FacilityStore.cs

@@ -358,6 +358,7 @@ protected override async Task GetOrderItems(List<OrderItemContext<FacilityOpport
                     OrderItem = new OrderItem
                     {
                         // TODO: The static example below should come from the database (which doesn't currently support tax)
+                        // This is because it can be different for each Seller and needs to calculated at the time of booking
                         UnitTaxSpecification = GetUnitTaxSpecification(flowContext, slot),
                         AcceptedOffer = new Offer
                         {
@@ -512,7 +513,8 @@ protected override async ValueTask LeaseOrderItems(Lease lease, List<OrderItemCo
             }
         }
 
-        //TODO: This should reuse code of LeaseOrderItem
+        // TODO: This should reuse code of LeaseOrderItem to be DRY. Similar logic is also used in ProposeOrderItems as well as
+        // in LeaseOrderItems, BookOrderItems, and ProposeOrderItems in the FacilityStore. The issue for this is: https://github.com/openactive/OpenActive.Server.NET/issues/226
         protected override async ValueTask BookOrderItems(List<OrderItemContext<FacilityOpportunity>> orderItemContexts, StoreBookingFlowContext flowContext, OrderStateContext stateContext, OrderTransaction databaseTransaction)
         {
             // Check that there are no conflicts between the supplied opportunities
@@ -574,7 +576,7 @@ protected override async ValueTask BookOrderItems(List<OrderItemContext<Facility
             }
         }
 
-        // TODO check logic here, it's just been copied from BookOrderItems. Possibly could remove duplication here.
+        // TODO check logic here, it's just been copied from BookOrderItems. Possibly could remove duplication here. Common logic between this, BookOrderItems, and LeaseOrderItems should be pulled out.
         protected override async ValueTask ProposeOrderItems(List<OrderItemContext<FacilityOpportunity>> orderItemContexts, StoreBookingFlowContext flowContext, OrderStateContext stateContext, OrderTransaction databaseTransaction)
         {
             // Check that there are no conflicts between the supplied opportunities

Examples/BookingSystem.AspNetCore/Stores/OrderStore.cs

@@ -389,7 +389,7 @@ public override async Task<bool> CreateOrderFromOrderProposal(OrderIdComponents
                 sellerId.IdLong ?? null /* Hack to allow this to work in Single Seller mode too */,
                 orderId.uuid,
                 version);
-            // TODO return enum to allow errors cases to be handled in the engine
+            // TODO return enum (something like CreateOrderStatus) to allow errors cases to be handled in the engine
             switch (result)
             {
                 case FakeDatabaseBookOrderProposalResult.OrderSuccessfullyBooked:

Examples/BookingSystem.AspNetCore/Stores/SessionStore.cs

@@ -331,6 +331,7 @@ protected override async Task GetOrderItems(List<OrderItemContext<SessionOpportu
                     OrderItem = new OrderItem
                     {
                         // TODO: The static example below should come from the database (which doesn't currently support tax)
+                        // This is because it can be different for each Seller and needs to calculated at the time of booking
                         UnitTaxSpecification = GetUnitTaxSpecification(flowContext, @class),
                         AcceptedOffer = new Offer
                         {
@@ -523,8 +524,8 @@ protected override async ValueTask LeaseOrderItems(
                 }
             }
         }
-
-        //TODO: This should reuse code of LeaseOrderItem
+       // TODO: This should reuse code of LeaseOrderItem to be DRY. Similar logic is also used in ProposeOrderItems as well as
+        // in LeaseOrderItems, BookOrderItems, and ProposeOrderItems in the SessionStore. The issue for this is: https://github.com/openactive/OpenActive.Server.NET/issues/226
         protected override async ValueTask BookOrderItems(List<OrderItemContext<SessionOpportunity>> orderItemContexts, StoreBookingFlowContext flowContext, OrderStateContext stateContext, OrderTransaction databaseTransaction)
         {
             // Check that there are no conflicts between the supplied opportunities
@@ -596,7 +597,7 @@ protected override async ValueTask BookOrderItems(List<OrderItemContext<SessionO
             }
         }
 
-        // TODO check logic here, it's just been copied from BookOrderItems. Possibly could remove duplication here.
+        // TODO check logic here, it's just been copied from BookOrderItems. Possibly could remove duplication here. Common logic between this, BookOrderItems, and LeaseOrderItems should be pulled out.
         protected override async ValueTask ProposeOrderItems(List<OrderItemContext<SessionOpportunity>> orderItemContexts, StoreBookingFlowContext flowContext, OrderStateContext stateContext, OrderTransaction databaseTransaction)
         {
             // Check that there are no conflicts between the supplied opportunities

Fakes/OpenActive.FakeDatabase.NET/FakeBookingSystem.cs

@@ -165,7 +165,7 @@ static FakeDatabase()
             int.TryParse(Environment.GetEnvironmentVariable("OPPORTUNITY_COUNT"), out var opportunityCount) ? opportunityCount : 2000;
 
         /// <summary>
-        /// TODO: Call this on a schedule from both .NET Core and .NET Framework reference implementations
+        /// TODO: Call this on a schedule from both .NET Core and .NET Framework reference implementations to ensure database is not cluttered
         /// </summary>
         public async Task CleanupExpiredLeases()
         {
@@ -1316,7 +1316,6 @@ public static async Task RecalculateSlotUses(IDbConnection db, SlotTable slot)
             slot.RemainingUses = slot.MaximumUses - totalUsesTaken;
 
             // Push the change into the future to avoid it getting lost in the feed (see race condition transaction challenges https://developer.openactive.io/publishing-data/data-feeds/implementing-rpde-feeds#preventing-the-race-condition)
-            // TODO: Document this!
             slot.Modified = DateTimeOffset.Now.UtcTicks;
             await db.UpdateAsync(slot);
         }
@@ -1343,7 +1342,7 @@ public static async Task RecalculateSpaces(IDbConnection db, OccurrenceTable occ
             var totalSpacesTaken = (await db.LoadSelectAsync<OrderItemsTable>(x => x.OrderTable.OrderMode == OrderMode.Booking && x.OccurrenceId == occurrence.Id && (x.Status == BookingStatus.Confirmed || x.Status == BookingStatus.Attended || x.Status == BookingStatus.Absent))).Count();
             occurrence.RemainingSpaces = occurrence.TotalSpaces - totalSpacesTaken;
 
-            // Push the change into the future to avoid it getting lost in the feed (see race condition transaction challenges https://developer.openactive.io/publishing-data/data-feeds/implementing-rpde-feeds#preventing-the-race-condition) // TODO: Document this!
+            // Push the change into the future to avoid it getting lost in the feed (see race condition transaction challenges https://developer.openactive.io/publishing-data/data-feeds/implementing-rpde-feeds#preventing-the-race-condition)
             occurrence.Modified = DateTimeOffset.Now.UtcTicks;
             await db.UpdateAsync(occurrence);
         }

OpenActive.Server.NET/CustomBookingEngine/CustomBookingEngine.cs

@@ -1,4 +1,4 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Net;
@@ -70,8 +70,7 @@
             if (settings.CustomerAccountIdTemplate != null) settings.CustomerAccountIdTemplate.RequiredBaseUrl = settings.CustomerAccountIdBaseUrl;
 
             // Create a lookup of each IdTemplate to pass into the appropriate RpdeGenerator
-            // TODO: Output better error if there is a feed assigned across two templates
-            // (there should never be, as each template represents everyting you need in one feed)
+            // TODO: Output better error if there is a feed assigned across two templates (there should never be, as each template represents everyting you need in one feed)
             this.feedAssignedTemplates = settings.IdConfiguration.SelectMany(t => t.IdConfigurations.Select(x => new
             {
                 assignedFeed = x.AssignedFeed,
@@ -151,7 +150,7 @@
         /// Handler for Dataset Site endpoint
         /// </summary>
         /// <returns></returns>
         public async Task<ResponseContent> RenderDatasetSite()
         {
             if (datasetSettings == null || supportedFeeds == null) throw new NotSupportedException("RenderDatasetSite is only supported if DatasetSiteGeneratorSettings are supplied to the IBookingEngine");
             // TODO add caching layer in front of dataset site rendering
@@ -730,6 +729,7 @@
                     throw new OpenBookingException(new OpenBookingError(), "Only bookable opportunities are permitted in the test interface");
 
                     // TODO: add this error class to the library
+                    // CS: Is this still the case? Unclear where this library is, and hard to believe OpenBookingError would not be in the libary
             }
 
             if (!genericEvent.TestOpportunityCriteria.HasValue)
@@ -868,7 +868,7 @@
 
             // Throw error if TotalPaymentDue is not specified at B or P
             if (order.TotalPaymentDue?.Price.HasValue != true && (stage == FlowStage.B || stage == FlowStage.P))
-                // TODO replace this with a more specific error
+                // TODO replace this with a more specific error ie a subclass of OpenBookingError like TotalPaymentMissingAtBOrPError
                 throw new OpenBookingException(new OpenBookingError(), "TotalPaymentDue must have a price set");
 
             var payer = order.BrokerRole == BrokerType.ResellerBroker ? order.Broker : order.Customer;

OpenActive.Server.NET/OpenBookingHelper/IdTransforms/IdTemplate.cs

@@ -417,20 +417,24 @@ public Uri RenderOrderId(OrderType orderType, Guid uuid)
             return this.RenderOrderId(new OrderIdComponents { OrderType = orderType, uuid = uuid });
         }
 
-        //TODO reduce duplication of the strings / logic below
-        public Uri RenderOrderItemId(OrderType orderType, Guid uuid, Guid orderItemId)
+        private void ValidateOrderType(OrderType orderType)
         {
             if (orderType != OrderType.Order) throw new ArgumentOutOfRangeException(nameof(orderType), "The Open Booking API 1.0 specification only permits OrderItem Ids to exist within Orders, not OrderQuotes or OrderProposals.");
+        }
+
+        public Uri RenderOrderItemId(OrderType orderType, Guid uuid, Guid orderItemId)
+        {
+            ValidateOrderType(orderType);
             return this.RenderOrderItemId(new OrderIdComponents { OrderType = orderType, uuid = uuid, OrderItemIdGuid = orderItemId });
         }
         public Uri RenderOrderItemId(OrderType orderType, Guid uuid, string orderItemId)
         {
-            if (orderType != OrderType.Order) throw new ArgumentOutOfRangeException(nameof(orderType), "The Open Booking API 1.0 specification only permits OrderItem Ids to exist within Orders, not OrderQuotes or OrderProposals.");
+            ValidateOrderType(orderType);
             return this.RenderOrderItemId(new OrderIdComponents { OrderType = orderType, uuid = uuid, OrderItemIdString = orderItemId });
         }
         public Uri RenderOrderItemId(OrderType orderType, Guid uuid, long orderItemId)
         {
-            if (orderType != OrderType.Order) throw new ArgumentOutOfRangeException(nameof(orderType), "The Open Booking API 1.0 specification only permits OrderItem Ids to exist within Orders, not OrderQuotes or OrderProposals.");
+            ValidateOrderType(orderType);
             return this.RenderOrderItemId(new OrderIdComponents { OrderType = orderType, uuid = uuid, OrderItemIdLong = orderItemId });
         }
 

OpenActive.Server.NET/OpenBookingHelper/Model/OrderModelSupport.cs

@@ -18,7 +18,6 @@ protected Uri RenderOrderId(OrderType orderType, Guid uuid)
             return this.OrderIdTemplate.RenderOrderId(orderType, uuid);
         }
 
-        //TODO reduce duplication of the strings / logic below
         protected Uri RenderOrderItemId(OrderType orderType, Guid uuid, Guid orderItemId)
         {
             return this.OrderIdTemplate.RenderOrderItemId(orderType, uuid, orderItemId);

OpenActive.Server.NET/OpenBookingHelper/OrderCalculations/OrderCalculations.cs

@@ -145,7 +145,7 @@ public static List<OpenBookingError> ValidateAdditionalDetails(OrderItem respons
 
         public static Event RenderOpportunityWithOnlyId(OpportunityType opportunityType, Uri id)
         {
-            // TODO: Create an extra prop in DatasetSite lib so that we don't need to parse the URL here
+            // TODO: Create an extra property in DatasetSite library called DatasetOpportunityType so that we don't need to parse the URL here
             switch (OpportunityTypes.Configurations[opportunityType].SameAs.AbsolutePath.Trim('/'))
             {
                 case nameof(Event):

OpenActive.Server.NET/StoreBookingEngine/Context/StoreBookingFlowContext.cs

@@ -5,7 +5,7 @@
 
 namespace OpenActive.Server.NET.StoreBooking
 {
-    //TODO: Refactor to inherrit from BookingFlowContext (using constructor to copy params? Use Automapper?)
+    //TODO: Refactor to inherit from BookingFlowContext (using constructor to copy params? Use Automapper?)
     public class StoreBookingFlowContext : BookingFlowContext
     {
         public StoreBookingFlowContext(BookingFlowContext bookingFlowContext)