chore: Elaborate on all TODOs (#672)

packages/openactive-broker-microservice/src/models/core.d.ts

@@ -3,11 +3,14 @@ export type OrderFeedIdentifier = 'OrdersFeed' | 'OrderProposalsFeed';
 export type BookingPartnerIdentifier = string;
 
 /*
-TODO in future we should put more thorough typing on this object (especially
-given that it has been validated [Could we couple TS with validator I wonder 🤔])
-and that would then immediately improve type-checking on all functions that
-act on opportunity data
- */
+TODO in future we should put more thorough typing on this object and that would
+then immediately improve type-checking on all functions that act on opportunity
+data.
+
+Since opportunities have (mostly) already been validated, we could make use of
+this, if it gets implemented, to automatically get a confirmed valid type for
+the Opportunity: https://github.com/openactive/data-model-validator/issues/448.
+*/
 export type Opportunity = Record<string, any>;
 export type RpdeItem = {
   state: string;

packages/openactive-integration-tests/.eslintignore

@@ -7,8 +7,6 @@ test/global-setup.js
 test/helpers/flow-helper.js
 test/helpers/index.js
 test/helpers/mapping.js
-test/reporter.js
 test/setup.js
 test/shared-behaviours/common.js
-test/shared-behaviours/validation.js
 test/test-framework/jasmine-state-reporter.js

packages/openactive-integration-tests/documentation/generator.js

@@ -1,4 +1,5 @@
-// TODO fix this file so that it no longer needs to disable these rules
+/* TODO fix this file so that it no longer needs to disable these rules:
+https://github.com/openactive/openactive-test-suite/issues/648 */
 /* eslint-disable no-use-before-define */
 /* eslint-disable global-require */
 /* eslint-disable import/no-dynamic-require */
@@ -94,7 +95,8 @@ global.documentationGenerationMode = true;
 // Load metadata from all tests
 const testMetadata = fg.sync(jestConfig.testMatch, { cwd: rootDirectory }).map(function (file) {
   console.log(`Reading: ${file}`);
-  // TODO: Verify that the data actually conforms to the type.
+  // TODO: Use code validation (e.g. with zod) to ensure that the data actually
+  // conforms to the TestModuleExports type
   // ## Load the test
   const data = /** @type {TestModuleExports} */(require(`${rootDirectory}${file}`));
   // ## Validate the test metadata
@@ -223,19 +225,6 @@ function renderFeatureIndexFeatureFragment(f) {
 `;
 }
 
-// TODO - unused function - delete if not needed
-// function renderFeatureIndexFeatureFragmentOld(f) {
-//   return `
-// #### ${f.name} ([${f.identifier}](./${f.category}/${f.identifier}/README.md))
-
-// ${f.description}
-
-// ${f.specificationReference}
-// ${renderCriteriaRequired(f.criteriaRequirement)}
-
-// `;
-// }
-
 /**
  * @param {FeatureMetadataItem} f
  */

packages/openactive-integration-tests/package.json

@@ -19,6 +19,7 @@
   "dependencies": {
     "@openactive/data-model-validator": "^2.0.78",
     "@openactive/data-models": "^2.0.316",
+    "@openactive/models-ts": "^1.0.4",
     "@openactive/openactive-openid-client": "file:../openactive-openid-client",
     "@openactive/test-interface-criteria": "file:../test-interface-criteria",
     "async-mutex": "^0.3.1",

packages/openactive-integration-tests/test-data-generator/test-data-generator.js

@@ -1,7 +1,7 @@
-// TODO Support specifying category in the CLI args e.g. "authorization"
-// This should be supported as well as feature, which is currently supported
-// This should look something like `npm run test-data-generator -- [category-or-feature]`
-// e.g. `npm run test-data-generator -- authorization`
+/* TODO Support specifying category in the CLI args e.g. "authorization"
+This should be supported as well as feature, which is currently supported
+This should look something like `npm run test-data-generator -- [category-or-feature]`
+e.g. `npm run test-data-generator -- authorization` */
 const fs = require('fs').promises;
 const path = require('path');
 const { DateTime } = require('luxon');
@@ -139,7 +139,14 @@ function createOutputOpportunityTestData(sellerRequirements) {
     for (const [opportunityCriteria, numOpportunitiesRequired] of opportunityCriteriaRequirements) {
       for (const bookingFlow of IMPLEMENTED_BOOKING_FLOWS) {
         for (const opportunityType of IMPLEMENTED_OPPORTUNITY_TYPES) {
-          // TODO this needs to take into account FeatureHelper.skipOpportunityTypes/skipBookingFlows
+          /* TODO: This currently over-counts the number of Opportunities
+          required because some tests implement `skipOpportunityTypes` or
+          `skipBookingFlows` in FeatureHelper. This means that, for these tests,
+          some opportunity types and/or booking flows should be ignored in the
+          count. Doing this would require adding more information to the
+          `feature-requirements.json` file that is output by documentation
+          generation.
+          It's not a huge issue as it's an over-count */
           numberOfItems += numOpportunitiesRequired;
           const testInterfaceOpportunity = createTestInterfaceOpportunity({
             opportunityType,

packages/openactive-integration-tests/test/features/approval/minimal-proposal/implemented/accept-proposal-book-test.js

@@ -70,13 +70,12 @@ FeatureHelper.describeFeature(module, {
     itShouldReturnOrderRequiresApprovalTrue(() => c2.getStage('c2').getOutput().httpResponse);
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(bookRecipe, () => {
-    // TODO does validator already check that orderProposalVersion is of form {orderId}/versions/{versionUuid}?
+    // TODO Validator should check this: https://github.com/openactive/data-model-validator/issues/449
     it('should include an orderProposalVersion, of the form {orderId}/versions/{versionUuid}', () => {
       const { uuid } = defaultFlowStageParams;
       expect(bookRecipe.p.getOutput().httpResponse.body).to.have.property('orderProposalVersion')
         .which.matches(RegExp(`${uuid}/versions/.+`));
     });
-    // TODO does validator check that full Seller details are included in the seller response?
     it('should have orderProposalStatus: SellerAccepted', () => {
       expect(bookRecipe.orderFeedUpdateCollector.getOutput().httpResponse.body)
         .to.have.nested.property('data.orderProposalStatus', 'https://openactive.io/SellerAccepted');

packages/openactive-integration-tests/test/features/approval/minimal-proposal/implemented/customer-reject-proposal-test.js

@@ -87,12 +87,11 @@ FeatureHelper.describeFeature(module, {
     itShouldReturnOrderRequiresApprovalTrue(() => c2.getStage('c2').getOutput().httpResponse);
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(p, () => {
-    // TODO does validator check that orderProposalVersion is of form {orderId}/versions/{versionUuid}
+    // TODO Validator should check this: https://github.com/openactive/data-model-validator/issues/449
     it('should include an orderProposalVersion, of the form {orderId}/versions/{versionUuid}', () => {
       expect(p.getOutput().httpResponse.body).to.have.property('orderProposalVersion')
         .which.matches(RegExp(`${defaultFlowStageParams.uuid}/versions/.+`));
     });
-    // TODO does validator check that full Seller details are included in the seller response?
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(customerRejection);
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(orderFeedUpdate, () => {

packages/openactive-integration-tests/test/features/approval/minimal-proposal/implemented/not-accept-proposal-book-test.js

@@ -69,12 +69,11 @@ FeatureHelper.describeFeature(module, {
     itShouldReturnOrderRequiresApprovalTrue(() => c2.getStage('c2').getOutput().httpResponse);
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(p, () => {
-    // TODO does validator check that orderProposalVersion is of form {orderId}/versions/{versionUuid}
+    // TODO Validator should check this: https://github.com/openactive/data-model-validator/issues/449
     it('should include an orderProposalVersion, of the form {orderId}/versions/{versionUuid}', () => {
       expect(p.getOutput().httpResponse.body).to.have.property('orderProposalVersion')
         .which.matches(RegExp(`${defaultFlowStageParams.uuid}/versions/.+`));
     });
-    // TODO does validator check that full Seller details are included in the seller response?
   });
   FlowStageUtils.describeRunAndCheckIsValid(b, () => {
     itShouldReturnAnOpenBookingError('OrderCreationFailedError', 500, () => b.getStage('b').getOutput().httpResponse);

packages/openactive-integration-tests/test/features/approval/minimal-proposal/implemented/seller-reject-proposal-test.js

@@ -94,12 +94,11 @@ FeatureHelper.describeFeature(module, {
     itShouldReturnOrderRequiresApprovalTrue(() => c2.getStage('c2').getOutput().httpResponse);
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(p, () => {
-    // TODO does validator check that orderProposalVersion is of form {orderId}/versions/{versionUuid}
+    // TODO Validator should check this: https://github.com/openactive/data-model-validator/issues/449
     it('should include an orderProposalVersion, of the form {orderId}/versions/{versionUuid}', () => {
       expect(p.getOutput().httpResponse.body).to.have.property('orderProposalVersion')
         .which.matches(RegExp(`${defaultFlowStageParams.uuid}/versions/.+`));
     });
-    // TODO does validator check that full Seller details are included in the seller response?
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(simulateSellerRejection);
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(orderFeedUpdate, () => {

packages/openactive-integration-tests/test/features/approval/proposal-amendment/implemented/proposal-amendment-book-test.js

@@ -134,13 +134,12 @@ FeatureHelper.describeFeature(module, {
     itShouldReturnOrderRequiresApprovalTrue(() => c2.getStage('c2').getOutput().httpResponse);
   });
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(p, () => {
-    // TODO does validator already check that orderProposalVersion is of form {orderId}/versions/{versionUuid}?
+    // TODO Validator should check this: https://github.com/openactive/data-model-validator/issues/449
     it('should include an orderProposalVersion, of the form {orderId}/versions/{versionUuid}', () => {
       const { uuid } = defaultFlowStageParams;
       expect(p.getOutput().httpResponse.body).to.have.property('orderProposalVersion')
         .which.matches(RegExp(`${uuid}/versions/.+`));
     });
-    // TODO does validator check that full Seller details are included in the seller response?
   });
 
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(simulateSellerAmendment);

packages/openactive-integration-tests/test/features/authentication/booking-partner-authentication/implemented/authorization-code-flow-test.js

@@ -27,7 +27,10 @@ function (configuration, orderItemCriteria, featureIsImplemented, logger) {
       .setClientCredentials(false, 'primary')
       .authorizeAuthorizationCodeFlow({
         loginCredentialsAccessor: () => SELLER_CONFIG.primary.authentication.loginCredentials,
-        assertFlowRequiredConsent: null, // TODO: Add a test interface Action that resets consent for the specified user, and call it before this flow starts. Then this flow should assert consent.
+        /* TODO: Add a test interface Action that resets consent for the
+        specified user, and call it before this flow starts. Then this flow
+        should assert consent. */
+        assertFlowRequiredConsent: null,
         assertSellerIdClaim: SELLER_CONFIG.primary['@id'],
       })
       .refresh();

packages/openactive-integration-tests/test/features/cancellation/seller-requested-replacement/implemented/book-and-seller-replace-items-test.js

@@ -32,7 +32,7 @@ FeatureHelper.describeFeature(module, {
   /* TODO in order to do proper capacity assertions for this test:
   - uncomment the below stuff, which will assert capacity for items that were in the original Order (before replacement)
   - add logic which will assert capacity for items that have newly been added to Order
-  */
+  This will resolve this issue: https://github.com/openactive/openactive-test-suite/issues/671. */
   // const assertOpportunityCapacity = new AssertOpportunityCapacityFlowStage({
   //   ...defaultFlowStageParams,
   //   prerequisite: orderFeedUpdate,

packages/openactive-integration-tests/test/features/core/availability-check/implemented/availability-confirmed-test.js

@@ -7,8 +7,10 @@ const { FlowStageRecipes, FlowStageUtils } = require('../../../../helpers/flow-s
  * @typedef {import('../../../../helpers/flow-stages/c2').C2FlowStageType} C2FlowStageType
  */
 
-/* TODO These tests (Common.itForEachOrderItemShouldHaveUnchangedCapacity) are now automatically run whenever C1 / C2
-are called, so this test is redundant. Might be useful to keep it for documentational purposes? */
+/* NOTE: These tests (Common.itForEachOrderItemShouldHaveUnchangedCapacity) are
+now automatically run whenever C1 / C2 are called, so this test is redundant,
+but it is kept for documentational purposes — it makes it clear that this is an
+important aspect that must be adhered to */
 FeatureHelper.describeFeature(module, {
   testCategory: 'core',
   testFeature: 'availability-check',

packages/openactive-integration-tests/test/features/core/dataset-site/implemented/dataset-site-jsonld-valid-test.js

@@ -25,7 +25,10 @@ function (configuration, orderItemCriteria, featureIsImplemented, logger) {
       });
     });
 
-    // TODO does validator check that endpointUrl does not end in a `/` (as per Open API 3 Base URL https://swagger.io/docs/specification/api-host-and-base-path/)
+    /* TODO have validator check that endpointUrl does not end in a `/` (as per
+    Open API 3 Base URL
+    https://swagger.io/docs/specification/api-host-and-base-path/). See GitHub
+    issue: https://github.com/openactive/data-model-validator/issues/450 */
     it('should include accessService.endpointUrl that does not end in a trailing "/"', () => {
       chai.expect(getDatasetSite.datasetSite.body.accessService.endpointUrl).not.to.match(/\/$/g, 'a trailing /');
     });

packages/openactive-integration-tests/test/features/details-capture/attendee-details-capture/implemented/attendee-details-included-test.js

@@ -19,7 +19,9 @@ FeatureHelper.describeFeature(module, {
     bookReqTemplateRef: 'attendeeDetails',
   });
 
-  // TODO: Check that attendee details are reflected back at B and P, and included in the Orders feed for A, as per https://openactive.io/open-booking-api/EditorsDraft/1.0CR3/#attendee-details-capture
+  /* TODO: Check that attendee details are reflected back at B and P, and
+  included in the Orders feed for Approval, as per
+  https://openactive.io/open-booking-api/EditorsDraft/1.0CR3/#attendee-details-capture */
 
   // # Set up Tests
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(fetchOpportunities);

packages/openactive-integration-tests/test/features/details-capture/customer-details-capture-non-essential/implemented/non-essential-customer-details-reflected-test.js

@@ -26,6 +26,9 @@ function (configuration, orderItemCriteriaList, featureIsImplemented, logger, de
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(c2, () => {
     it('should include expected customer details', () => {
       const apiResponseJson = c2.getStage('c2').getOutput().httpResponse.body;
+      if (defaultFlowStageParams.customer['@type'] !== 'Person') {
+        throw new Error(`Tests expect customer to be a Person. Customer: ${JSON.stringify(defaultFlowStageParams.customer)}`);
+      }
       expect(apiResponseJson).to.have.nested.property('customer.email').that.match(new RegExp(defaultFlowStageParams.customer.email, 'i'));
       expect(apiResponseJson).to.have.nested.property('customer.telephone', defaultFlowStageParams.customer.telephone);
       expect(apiResponseJson).to.have.nested.property('customer.givenName', defaultFlowStageParams.customer.givenName);
@@ -35,6 +38,9 @@ function (configuration, orderItemCriteriaList, featureIsImplemented, logger, de
   FlowStageUtils.describeRunAndCheckIsSuccessfulAndValid(bookRecipe, () => {
     it('should include expected customer details', () => {
       const apiResponseJson = bookRecipe.b.getOutput().httpResponse.body;
+      if (defaultFlowStageParams.customer['@type'] !== 'Person') {
+        throw new Error(`Tests expect customer to be a Person. Customer: ${JSON.stringify(defaultFlowStageParams.customer)}`);
+      }
       expect(apiResponseJson).to.have.nested.property('customer.email').that.match(new RegExp(defaultFlowStageParams.customer.email, 'i'));
       expect(apiResponseJson).to.have.nested.property('customer.telephone', defaultFlowStageParams.customer.telephone);
       expect(apiResponseJson).to.have.nested.property('customer.givenName', defaultFlowStageParams.customer.givenName);

packages/openactive-integration-tests/test/features/payment/non-free-opportunities/not-implemented/no-paid-bookable-sessions-test.js

@@ -1,4 +1,5 @@
-// TODO TODO think about this pattern of having a shell not-implemented test for non-free-opportunities but the main tests are in prepayment-* features
+/* TODO consider using this pattern of having a shell not-implemented test for
+non-free-opportunities but the main tests are in prepayment-* features */
 const { FeatureHelper } = require('../../../../helpers/feature-helper');
 
 FeatureHelper.describeUnmatchedCriteriaFeature(module, {

packages/openactive-integration-tests/test/helpers/flow-stages/book-recipe.js

@@ -6,7 +6,6 @@
  * @typedef {import('./assert-opportunity-capacity').AssertOpportunityCapacityFlowStageType} AssertOpportunityCapacityFlowStageType
  */
 
-// TODO construct this class using the generic FlowStageRun (e.g. with inheritance), rather than have it be a bespoke thing
 /**
  * The Flow Stages required to complete booking in either Simple or Approval flow.
  *