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

Avoid confusion about TripSummary #109

Merged
merged 2 commits into from
Nov 11, 2020
Merged

Conversation

pvgrumbkow
Copy link
Contributor

Removed AcceptDeferredDelivery from TripPolicyFilterGroup. Instead added TripSummaryOnly into TripContentFilterGroup as the parameter to control whether complete trips or only trip summaries shall be delivered. There is no relation to a deferred delivery anymore.
Removed IncludeLegs from MultiPointTripContentFilterGroup, as legs are mandatory within trip results. Removed MultiPointTripContentFilterGroup, as there is no need have different content filters for MultiPointTrip or Trip respectively.

Removed AcceptDeferredDelivery from TripPolicyFilterGroup. Instead added TripSummaryOnly into TripContentFilterGroup as the parameter to control whether complete trips or only trip summaries shall be delivered. There is no relation to a deferred delivery anymore.
Removed IncludeLegs from MultiPointTripContentFilterGroup, as legs are mandatory within trip results. Removed MultiPointTripContentFilterGroup, as there is no need have different content filters for MultiPointTrip or Trip respectively.
@pvgrumbkow pvgrumbkow added the bug Something isn't working label Oct 22, 2020
@pvgrumbkow pvgrumbkow added this to the v1.1 milestone Oct 22, 2020
Copy link
Contributor

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

In general I think this change is a good idea, as it simplifies request handling, but it was purposefully added:

Full trip information is sent actively by the server to the Address stated in RequestorEndpointGroup.

This option was not part of TRIAS and has been added to OJP in the initial release - do you know by whom this feature was requested/used?

This once again is a breaking change which must be thoroughly documented.

The documentation in https://github.com/VDVde/OJP/blob/ResponseContextInRequestSupport/OJP_Trips.xsd#L248 needs to be updated too:

				<xs:element name="TripSummary" type="TripSummaryStructure">
					<xs:annotation>
						<xs:documentation>Summary on trip. Only if requestor accepts deferrred delivery of trip details.</xs:documentation>
					</xs:annotation>
				</xs:element>

@pvgrumbkow
Copy link
Contributor Author

Actually, I don't know the detailed reasons for this design decision. However, I assume, that something similar to the once existing DELFI partial connection service was planned (which computed something similar to a TripSummary), but with the mechanisms of SIRI messages. Such a "TripSummary only" service is not part of TRIAS and had to be invented for OJP. But I assume, that the usage of deferred delivery for this was not intension but chance.
Even if somebody is really interested in using the mechanism "deferred delivery" (without choosing the content), this would have to be modeled differently IMHO. Choosing content or functionality should not be mixed with choosing transport mechanism.
Therefore I would still prefer to simply remove AcceptDeferredDelivery. If there is in future really a need for asynchronous Trip requests, we can still add it.
I will upload a new version with changed comment for TripSummary.

@pvgrumbkow pvgrumbkow merged commit 23c2d68 into changes_for_v1.1 Nov 11, 2020
@pvgrumbkow pvgrumbkow deleted the ControlTripSummary branch November 11, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusion about Trip/TripSummary, IncludeLegs, and AcceptDeferredDelivery
3 participants