Open lukehesluke opened 3 years ago
This looks sounds great, and very consistent with the existing framework too.
Thinking about the broker microservice and the buckets it maintains, it’s probably cleaner to subdivide the buckets between flows (as they are already subdivided between opportunity types) - in doing so the offer constraint will need be be inspected to resolve the bucket. Though it feels on the one hand like we’re breaking an abstraction here - this actually might be ok. The other option would be to also add a flow property to the test interface, such that the shape expressions could theoretically be completely ignored, and just the flow properly used to determine the opportunity to return? This way the shape expressions become a concrete representation of the constraints specified by the criteria and flow combination?
I also wonder if it would be helpful to formally define “SimpleOpenBookingFlow” and “ApprovalOpenBookingFlow” or similar in the spec so that we can use that language consistently? We could use these values for the flow property above?
Yes I agree with having a new TestInterface property. Let's call it test:testOpenBookingFlowRequirement
? This can have, for now, one of the following values:
https://openactive.io/OpenBookingApproval
https://openactive.io/OpenBookingSimple
(*)(*) OpenBookingSimple is a new value we'll have to suggest in the Booking Spec. I'll raise an issue for that suggesting that it be the default value for openBookingFlowRequirement
even when this field is absent.
@civsiv, @nickevansuk and I had a call about how long the Test Suite tests are going to take to run now that we're supporting combinations of opportunity type and flow requirement. We agree that it's not too much of a problem for individuals running tests against their Booking Systems as they likely support only a small subset of all features and combinations. HOWEVER, it could make the CI take a very long time.
A suggested approach would be to have the CI run different combinations in parallel e.g. four configs which test each of the combinations of events/facilities & simple/approval flow.
Actually, I'm not sure that the test:testOpenBookingFlowRequirement
TestInterface property above works. By specifying that something be only one thing - OpenBookingSimple
, this means we can't run tests that require some combination of flow requirements e.g. OpenBookingIntakeForm
AND OpenBookingApproval
(or IntakeForm but not Approval).
Perhaps instead we should just have a field "test:testRequiresApproval": true | false
?
And then later we could add a test:testRequiresNegotiation
when needed.
I also toyed with this idea, but it doesn't work with Broker's sub-bucketing at the moment (See the bottom section "RE Broker sub-bucketing"):
So perhaps instead it should just match the openBookingFlowRequirement
field in the Booking spec and be an array that doesn't need a value for "Simple"
e.g.
{
"@context": [
"https://openactive.io/",
"https://openactive.io/test-interface"
],
"@type": "ScheduledSession",
"superEvent": {
"@type": "SessionSeries",
"organizer": { /* .. */ }
},
"test:testOpportunityCriteria": "https://openactive.io/test-interface#TestOpportunityBookable",
"test:testOpenBookingFlowRequirement": [ "https://openactive.io/OpenBookingApproval" ]
}
For a test that requires approval. Some more examples:
Simple Booking Flow only:
// no value for `test:testOpenBookingFlowRequirement`
IntakeForm but not Approval:
"test:testOpenBookingFlowRequirement": [ "https://openactive.io/OpenBookingIntakeForm" ]
Attendee Details and Approval:
"test:testOpenBookingFlowRequirement": [ "https://openactive.io/OpenBookingApproval", "https://openactive.io/OpenBookingAttendeeDetails" ]
...etc
RE Broker sub-bucketing.
Possible solutions:
numCombinations = TriangleNumber(numFlowRequirements) + 1
[as we include the combination in which no flow requirements are selected]
Config
A config var:
Similar to
integrationTests.bookableOpportunityTypesInScope
, this can be used to combinatorially test all selected features with all combinations of opportunity types and booking flows.Some tests will only work for one of the booking flows. For this, FeatureHelper can be set up to allow a test to only work for certain booking flows, similar to its current
skipOpportunityTypes
parameter.Constraints / Criteria
The constraints generated by test-data-generator and Test Suite's
createTestInterfaceOpportunity(..)
call will include atestOfferDataShapeExpression
specifying theopenBookingFlowRequirement
. [This does mean that a given TestInterfaceCriteria identifier (likeTestOpportunityBookableFreePrepaymentRequired
) represents a class of constraints rather than a specific set of constraints. That class of constraints can include varying constraints for theopenBookingFlowRequirement
. I think this is fine - this already happens for datetime-related constraints]This means that every constraints JSON will include a
test:testOfferDataShapeExpression
constraint foropenBookingFlowRequirement
The Test Interface docs should be updated to make it clear that this will happen. It's currently out of date - lacking info about the shape expressions, so these can both be done at the same time.
Existing Criteria like
TestOpportunityBookable
will have their openBookingFlowRequirement constraints removed (as these no longer belong to the criteria themselves). Additionally, theTestOpportunityBookableFlowRequirementOnlyApproval
criteria can be removed as obsoleteFlow Stages
A recipe will be created that uses either B [for simple] or P, AwaitOrderFeedUpdate(A), B [for approval]. This will depend on the value of
bookingFlow
supplied by FeatureHelperQuestions
1. What about testing Orders which contain both Simple & Minimal Proposal Offers?
Without rocking the above design, we could make a special test within a special feature. This test will assert that simple & minimal proposal flows are both supported. It will include a custom criteria template with new field
bookingFlowOverride
as such:2. What if a Booking System only supports approval for Events and not Facilities?
They'll have to run the test once with config: "events only + simple & approval" and another time with "config: facilities only + simple only". They'll get 2 certificates rather than 1.