Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dt 615 booking conformance uc3 shipper submit updated booking request #11

Conversation

preetamnpr
Copy link
Collaborator

No description provided.

@preetamnpr preetamnpr requested review from nt-gt and gj0dcsa November 24, 2023 08:20

@Override
public String getHumanReadablePrompt() {
return ("UC3 Submit an Updated booking request using the following parameters:");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to consistently be using UCX: (that is, have a colon after the use-case name). Please align.

Comment on lines 49 to 58
@Override
public void doHandleExchange(ConformanceExchange exchange) {
JsonNode responseJsonNode = exchange.getResponse().message().body().getJsonBody();
// FIXME: Guard against non-conformant parties
getDspConsumer()
.accept(
new DynamicScenarioParameters(
responseJsonNode.get("carrierBookingRequestReference").asText(),
getDspSupplier().get().carrierBookingReference()));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded for this use-case (the CBRR is already recorded in UC1), so please remove this code snippet

Comment on lines 560 to 564
var bookingReference = lastUrlSegment(request.url());
// bookingReference can either be a CBR or CBRR.
BookingState bookingState = BookingState.PENDING_UPDATE_CONFIRMATION;
var cbrr = cbrToCbrr.getOrDefault(bookingReference, bookingReference);
var bookingData = persistentMap.load(cbrr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit pick: The BookingState bookingState = BookingState.PENDING_UPDATE_CONFIRMATION; line is placed confusing relative to the comment.

I would move the BookingState bookingState = BookingState.PENDING_UPDATE_CONFIRMATION; above the var bookingReference = lastUrlSegment(request.url()); line to avoid this (or below var bookingData = persistentMap.load(cbrr);)

if (bookingData == null || bookingData.isMissingNode()) {
return return404(request);
}
booking.put("carrierBookingRequestReference", cbrr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded: The payload should already have a CBRR at this point; there is no need to set it again as PUT is not supposed to be able to change it. Therefore it is confusing at best.

"/v2/bookings/%s".formatted(cbrr),bookingData);

addOperatorLogEntry(
"Sent a Updated booking request with the parameters: %s"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Sent a Updated booking request with the parameters: %s"
"Sent an updated booking request with the parameters: %s"

@preetamnpr preetamnpr requested a review from nt-gt November 24, 2023 09:41
Comment on lines +37 to +40
@Override
public JsonNode getJsonForHumanReadablePrompt() {
return getCspSupplier().get().toJson();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UC3 has no JSON associated with the human-readable prompt, and in fact neither does UC1, in both the override should IMO be deleted. Only the initial action that prompts for scenario parameters needs to override this and provide a JSON to go with the text for the user.

@preetamnpr preetamnpr merged commit 5e8f981 into dev Nov 24, 2023
1 check passed
@nt-gt nt-gt deleted the DT-615-Booking-conformance-UC3-shipper-submit-updated-booking-request branch December 5, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants