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

Return empty list if there is no siriUrls in situations/infoLinks #6232

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Nov 6, 2024

Summary

In the Transmodel API the /stopPlace/quays[]/situations[]/infoLinks is none-null, but the current code returns nullif no infoLinks exist. This PR return an empty list instead.

Issue

There is no issue for this. This bug was reported by T. Sverdvik, Ruter yesterday in \#shared-entur-ruter-utvikling at Entur.

Unit tests

No test is added.

Documentation

Not changed.

Changelog

Probably to small of a fix to be notable.

Bumping the serialization version id

Not needed.

@t2gran t2gran added the Bug label Nov 6, 2024
@t2gran t2gran added this to the 2.7 (next release) milestone Nov 6, 2024
@t2gran t2gran requested a review from lassetyr November 6, 2024 09:20
@t2gran t2gran requested a review from a team as a code owner November 6, 2024 09:20
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 56.66667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.94%. Comparing base (ee53e50) to head (f492446).
Report is 63 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...pplanner/updater/siri/SiriAlertsUpdateHandler.java 57.14% 7 Missing and 2 partials ⚠️
.../apis/transmodel/model/framework/InfoLinkType.java 0.00% 2 Missing ⚠️
...ansmodel/model/siri/sx/PtSituationElementType.java 0.00% 1 Missing ⚠️
...org/opentripplanner/framework/i18n/I18NString.java 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6232      +/-   ##
=============================================
+ Coverage      69.91%   69.94%   +0.02%     
- Complexity     17736    17747      +11     
=============================================
  Files           2006     2006              
  Lines          75526    75536      +10     
  Branches        7730     7733       +3     
=============================================
+ Hits           52804    52832      +28     
+ Misses         20036    20011      -25     
- Partials        2686     2693       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t2gran
Copy link
Member Author

t2gran commented Nov 6, 2024

We need to clearify if the GTFS URL and the SIRI URIs are required or not. The URIs are required in Siri, but there is a integration test witch fails - is the test data invalid?

@t2gran
Copy link
Member Author

t2gran commented Nov 6, 2024

This SIRI SX message can be used to test this - it should not be imported - the infolink uri is missing:

<Siri xmlns="http://www.siri.org.uk/siri" xmlns:ns2="http://www.ifopt.org.uk/acsb" xmlns:ns3="http://www.ifopt.org.uk/ifopt" xmlns:ns4="http://datex2.eu/schema/2_0RC1/2_0" version="2.0">
    <ServiceDelivery>
        <ResponseTimestamp>2024-10-18T16:22:44.302995029+02:00</ResponseTimestamp>
        <ProducerRef>ENT</ProducerRef>
        <MoreData>false</MoreData>
        <SituationExchangeDelivery version="2.0">
            <ResponseTimestamp>2024-10-18T16:22:44.302998817+02:00</ResponseTimestamp>
            <Situations>
                <PtSituationElement>
                    <CreationTime>2024-10-18T07:37:28.952132+02:00</CreationTime>
                    <ParticipantRef>ATB</ParticipantRef>
                    <SituationNumber>TST:SituationNumber:123</SituationNumber>
                    <Source>
                        <SourceType>directReport</SourceType>
                    </Source>
                    <Progress>open</Progress>
                    <ValidityPeriod>
                        <StartTime>2024-10-21T07:37:00+02:00</StartTime>
                        <EndTime>2024-11-08T15:00:00+01:00</EndTime>
                    </ValidityPeriod>
                    <UndefinedReason/>
                    <Severity>normal</Severity>
                    <Priority>5</Priority>
                    <ScopeType>line</ScopeType>
                    <ReportType>general</ReportType>
                    <Keywords/>
                    <Summary>test</Summary>
                    <InfoLinks>
                        <InfoLink>
                            <Label>tomt label</Label>
                        </InfoLink>
                    </InfoLinks>
                    <Affects>
                        <Networks>
                            <AffectedNetwork>
                                <AffectedLine>
                                    <LineRef>NSB:Line:R11</LineRef>
                                    <PublishedLineName>11</PublishedLineName>
                                </AffectedLine>
                            </AffectedNetwork>
                        </Networks>
                    </Affects>
                    <Extensions/>
                </PtSituationElement>
            </Situations>
        </SituationExchangeDelivery>
    </ServiceDelivery>
</Siri>

@t2gran t2gran marked this pull request as ready for review November 6, 2024 14:27
Comment on lines +138 to +140
I18NString.hasNoValue(alert.headerText()) &&
I18NString.hasNoValue(alert.descriptionText()) &&
I18NString.hasNoValue(alert.detailText())
Copy link
Member

Choose a reason for hiding this comment

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

Should this check not be part of TransitAlertBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should and I first put it in the builder - but the builder should not have the validation logic, the main class should. The problem is that if we validate when we construct the new entity, I would have to change the SiriAlertsUpdateHandler more than I like for a simple bug fix.

@t2gran t2gran merged commit 2ead4da into opentripplanner:dev-2.x Nov 7, 2024
5 checks passed
@t2gran t2gran deleted the otp2_npe_situation_url branch November 7, 2024 15:58
t2gran pushed a commit that referenced this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants