Conversation
465ab41 to
d30ef5e
Compare
652c622 to
cfa79b6
Compare
So do not allow 'everything' if there are no limitations
|
Triggered a build yesterday, so build 1763997892 is testable with bike ticket purchase. Cc @tormoseng et al |
Seems to be working fine! But, as we talked about @cbrevik , only the 1 adult part of the single ticket is shown under the "Repeat purchase" section in the app. And the single bike doesn't show at all. Maybe we need to talk to @Sebstorvik about how we like this to be presented (do we want bike tickets to appear or not). |
| buildState.userProfilesWithCount = userProfilesWithCount; | ||
| return builder; | ||
| }, | ||
| baggageProducts: (baggageProductsWithCount) => { | ||
| buildState.baggageProductsWithCount = baggageProductsWithCount; |
There was a problem hiding this comment.
Why are we adding the state to buildState instead of using selection here? I also don't quite understand why so much logic has been put into the build function 🤔
There was a problem hiding this comment.
This is a bit complex, and may be easier to explain in person. But the problem is that if we don't do this, the building ends up in an inconsistent state. And I'd argue you would want to put even more logic in the build function for this builder to not have bugs.
What we are solving here specifically is the business rule that you have to have either one or more useprofiles, or one or more baggages. If you set them separately, it is difficult to validate that business rule.
There was a problem hiding this comment.
There are more things that needs to be fixed in the builder, but this was sort of a minimum change that at least does not introduce more inconsistent state 😅
There was a problem hiding this comment.
Had a talk with @strandlie, and explained why this was confusing to me. I'm wondering if we can avoid the buildState concept, and move some logic from the build step back here.
Håkon suggested we all could have a talk tomorrow? Might be easier to talk about in person indeed 😄
There was a problem hiding this comment.
The pricing calculations are getting hard to reason about, and it's important to get correct. Is it time to extract it out of use-offer-state and add some tests?
rosvik
left a comment
There was a problem hiding this comment.
Good work 🙌 Talked with @cbrevik about my remaining comments, and it makes sense to handle those as separate PRs:
- Extract pricing calculations from use-offers-state, and add tests.
- Update the purchase selection builder, and maybe handle invalid states better.
…te.ts Co-authored-by: Johannes Røsvik <j.rosvik@gmail.com>
Closes https://github.com/AtB-AS/kundevendt/issues/4282
Adds a complete bike ticket purchase flow where you can select a bike on an equal footing with user profiles in the bottom sheet for travellers. Also introduces support for "baggage products" as a specific type of supplement product.
Changes
use-offer-state.ts.use-offer-state.ts.BaggageTicketOfferZod-schema, based on theTicketOffer-schema. This new schema assumes that there is only one supplement product in thesupplementProducts-array, and thefareProduct(ref) is null/undefined, if it is a baggage-type offer. This is something we will have to change in the future when we want to support more complex offers. Backend are aware of this, and will ensure that this assumption/requirement is held until we decide to rewrite the ticket data structure.BaggageProducttype (a subtype ofSupplementProductwith specific properties) and corresponding runtime validation using zod insrc/modules/configuration/types.ts. Use this type tosafeParsebaggage products ingetBaggageProductsanduse-offer-stateand friends.baggageTypeonSupplementProductwhich is used in the OfferSearch-request.supplementProductsonTicketOffer.fareProductonTicketOffernullable (it will always benullon baggage-type offers).Test changes
.toBe()with.toStrictEqual()for deep object comparison in several tests.Dependent on: