feat: elaborate TODOs (#227)

* feat: elaborate TODOs

* Feature/remove remainingattendeecapacity from b (#229)

* feat: remove remainingAttendeeCapacity and maximumAttendeeCapacity from B response

* remove from P response too

* Update OpenActive.Server.NET/CustomBookingEngine/CustomBookingEngine.cs

Co-authored-by: Luke Winship <luke.winship@gmail.com>

* elaborate a todo

---------

Co-authored-by: Luke Winship <luke.winship@gmail.com>

Author: civsiv

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/Extensions/BookedOrderItemHelper.cs

@@ -52,5 +52,13 @@ public static void AddPropertiesToBookedOrderItem(IOrderItemContext ctx, BookedO
             }
         }
 
+        public static void RemovePropertiesFromBookedOrderItem(IOrderItemContext ctx)
+        {
+            // Set RemainingAttendeeCapacity and MaximumAttendeeCapacity to null as the do not belong in the B and P responses.
+            // For more information see: https://github.com/openactive/open-booking-api/issues/156#issuecomment-926643733
+            ctx.ResponseOrderItem.OrderedItem.Object.RemainingAttendeeCapacity = null;
+            ctx.ResponseOrderItem.OrderedItem.Object.MaximumAttendeeCapacity = null;
+        }
+
     }
 }

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
@@ -558,6 +560,8 @@ protected override async ValueTask BookOrderItems(List<OrderItemContext<Facility
                             // Set OrderItemId and access properties for each orderItemContext
                             ctx.SetOrderItemId(flowContext, bookedOrderItemInfo.OrderItemId);
                             BookedOrderItemHelper.AddPropertiesToBookedOrderItem(ctx, bookedOrderItemInfo);
+                            // Remove attendee capacity information from the OrderedItem. For more information see: https://github.com/openactive/open-booking-api/issues/156#issuecomment-926643733
+                            BookedOrderItemHelper.RemovePropertiesFromBookedOrderItem(ctx);
                         }
                         break;
                     case ReserveOrderItemsResult.SellerIdMismatch:
@@ -574,7 +578,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
@@ -619,6 +623,8 @@ protected override async ValueTask ProposeOrderItems(List<OrderItemContext<Facil
                         foreach (var (ctx, bookedOrderItemInfo) in ctxGroup.Zip(bookedOrderItemInfos, (ctx, bookedOrderItemInfo) => (ctx, bookedOrderItemInfo)))
                         {
                             ctx.SetOrderItemId(flowContext, bookedOrderItemInfo.OrderItemId);
+                            // Remove attendee capacity information from the OrderedItem. For more information see: https://github.com/openactive/open-booking-api/issues/156#issuecomment-926643733
+                            BookedOrderItemHelper.RemovePropertiesFromBookedOrderItem(ctx);
                         }
                         break;
                     case ReserveOrderItemsResult.SellerIdMismatch:

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
@@ -580,6 +581,8 @@ protected override async ValueTask BookOrderItems(List<OrderItemContext<SessionO
                             // Set OrderItemId and access properties for each orderItemContext
                             ctx.SetOrderItemId(flowContext, bookedOrderItemInfo.OrderItemId);
                             BookedOrderItemHelper.AddPropertiesToBookedOrderItem(ctx, bookedOrderItemInfo);
+                            // Remove attendee capacity information from the OrderedItem. For more information see: https://github.com/openactive/open-booking-api/issues/156#issuecomment-926643733
+                            BookedOrderItemHelper.RemovePropertiesFromBookedOrderItem(ctx);
                         }
                         break;
                     case ReserveOrderItemsResult.SellerIdMismatch:
@@ -596,7 +599,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
@@ -642,6 +645,8 @@ protected override async ValueTask ProposeOrderItems(List<OrderItemContext<Sessi
                         foreach (var (ctx, bookedOrderItemInfo) in ctxGroup.Zip(bookedOrderItemInfos, (ctx, bookedOrderItemInfo) => (ctx, bookedOrderItemInfo)))
                         {
                             ctx.SetOrderItemId(flowContext, bookedOrderItemInfo.OrderItemId);
+                            // Remove attendee capacity information from the OrderedItem. For more information see: https://github.com/openactive/open-booking-api/issues/156#issuecomment-926643733
+                            BookedOrderItemHelper.RemovePropertiesFromBookedOrderItem(ctx);
                         }
                         break;
                     case ReserveOrderItemsResult.SellerIdMismatch:

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

@@ -70,8 +70,7 @@ public CustomBookingEngine(BookingEngineSettings settings, Uri openBookingAPIBas
             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,
@@ -375,7 +374,8 @@ private Guid ConvertToGuid(string uuidString)
             if (Guid.TryParse(uuidString, out Guid result))
             {
                 return result;
-            } else
+            }
+            else
             {
                 throw new OpenBookingException(new OpenBookingError(), "Invalid format for Order UUID");
             }
@@ -424,7 +424,7 @@ public async Task<ResponseContent> ProcessOrderCreationB(string clientId, Uri se
             {
                 // Attempt to use idempotency cache if it exists
                 var cachedResponse = await GetResponseFromIdempotencyStoreIfExists(settings, orderId, orderJson);
-                if (cachedResponse != null) 
+                if (cachedResponse != null)
                 {
                     return cachedResponse;
                 }
@@ -449,7 +449,7 @@ public async Task<ResponseContent> ProcessOrderProposalCreationP(string clientId
             {
                 // Attempt to use idempotency cache if it exists
                 var cachedResponse = await GetResponseFromIdempotencyStoreIfExists(settings, orderId, orderJson);
-                if (cachedResponse != null) 
+                if (cachedResponse != null)
                 {
                     return cachedResponse;
                 }
@@ -462,7 +462,7 @@ public async Task<ResponseContent> ProcessOrderProposalCreationP(string clientId
 
         private async Task<ResponseContent> GetResponseFromIdempotencyStoreIfExists(BookingEngineSettings settings, OrderIdComponents orderId, string orderJson)
         {
-             // Attempt to use idempotency cache if it exists
+            // Attempt to use idempotency cache if it exists
             if (settings.IdempotencyStore != null)
             {
                 var cachedResponse = await settings.IdempotencyStore.GetSuccessfulOrderCreationResponse(orderId, orderJson);
@@ -474,7 +474,8 @@ private async Task<ResponseContent> GetResponseFromIdempotencyStoreIfExists(Book
             return null;
         }
 
-        private async Task<ResponseContent> CreateResponseViaIdempotencyStoreIfExists(BookingEngineSettings settings, OrderIdComponents orderId, string orderJson, Order response) {
+        private async Task<ResponseContent> CreateResponseViaIdempotencyStoreIfExists(BookingEngineSettings settings, OrderIdComponents orderId, string orderJson, Order response)
+        {
             // Return a 409 status code if any OrderItem level errors exist
             var httpStatusCode = response.OrderedItem.Exists(x => x.Error?.Count > 0) ? HttpStatusCode.Conflict : HttpStatusCode.Created;
             var responseJson = OpenActiveSerializer.Serialize(response);
@@ -730,6 +731,7 @@ async Task<ResponseContent> IBookingEngine.InsertTestOpportunity(string testData
                     throw new OpenBookingException(new OpenBookingError(), "Only bookable opportunities are permitted in the test interface");
 
                     // TODO: add this error class to the library
+                    // CS: Does this mean add a new specific error to OpenActive.Net, something like OnlyBookableOpportunitesPermittedInTestInterfaceError
             }
 
             if (!genericEvent.TestOpportunityCriteria.HasValue)
@@ -868,7 +870,7 @@ async Task<ResponseContent> IBookingEngine.TriggerTestAction(string actionJson)
 
             // 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 OpenBookingException 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):