debug client: support stop ids in from / to inputs #6133

richardkoszegi · 2024-10-09T13:55:29Z

Summary

Added support for location ids in from/to fields in debug client.

Implementation Details:

Added locationConverter: converts strings to Location objects and back based on LocationStringParser class.
LocationInputField: a value state introduced for the input field. The new useEffect hook updates this state when location parameter changes. On the onBlur event, this component fires the new onLocationChange event if a valid location can be parsed from the input.
SearchBar: updates tripQueryVariables based on the changed location.

codecov · 2024-10-09T14:02:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.71%. Comparing base (e595147) to head (bc1bca0).
Report is 79 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6133      +/-   ##
=============================================
- Coverage      69.71%   69.71%   -0.01%     
+ Complexity     17698    17693       -5     
=============================================
  Files           2008     2008              
  Lines          75830    75827       -3     
  Branches        7764     7762       -2     
=============================================
- Hits           52867    52862       -5     
- Misses         20248    20255       +7     
+ Partials        2715     2710       -5

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

🚨 Try these New Features:

JS Bundle Analysis - Avoid shipping oversized bundles

leonardehrenfried · 2024-10-15T10:06:30Z

We talked about it and @t2gran has requested that you look up the coordinate of the stop and place the flag icon on the map. Would that be too much to ask?

I personally like this change but @testower is the React expert and needs to review it.

richardkoszegi · 2024-10-16T14:53:35Z

We talked about it and @t2gran has requested that you look up the coordinate of the stop and place the flag icon on the map. Would that be too much to ask?

I personally like this change but @testower is the React expert and needs to review it.

I hope it won't be a big change, so it can be implemented in this pr. 🙂

I think I just need to write a stopId -> coordinates resolver using the quay query and call it in the NavigationMarkers component to set the flags.

testower · 2024-10-16T19:14:32Z

client/src/components/SearchBar/SearchBar.tsx

@@ -29,6 +29,26 @@ export function SearchBar({ onRoute, tripQueryVariables, setTripQueryVariables,
  const [showServerInfo, setShowServerInfo] = useState(false);
  const target = useRef(null);

+  const setFromLocation = useCallback(


This is perfectly fine pattern, but we have done it slightly differently - passing tripQueryVariables and setTripQueryVariables into the input components, and calling them there. I suggest keeping to this for consistency.

I moved these functions to the LocationInputField component.

client/src/util/locationConverter.ts

richardkoszegi · 2024-10-18T10:26:52Z

I implemented the stopId -> coordinate resolver with the quay query, and it works fine with stops, but not with stop places / stop-areas or any other nodes that have id (eg. vehicle rental stations).

I think it's not the frontend's job to decide, which query has to be called to resolve the coordinates, so it would be nice, if the graphql api has a generic place query, that resolves the coordinates for any id. What do you think about it?

t2gran · 2024-10-22T16:53:35Z

client/src/components/MapView/useCoordinateResolver.ts

+        longitude,
+        latitude,


My knowledge of JS is limited - but it looks like the arguments order is swapped according to the interface definition above. As a practice to avoid errors I like to always do lat, long never long, lat (doc, params, args, execution order and so on).

These are named object properties, order actually doesn't matter. But I agree in principle to keep them consistently ordered anyway.

I changed the order of the params to follow the convention. (Although it wasn't a bug, because I used here the JavaScript's property shorthand syntax).

testower · 2024-10-22T18:09:42Z

There are still unresolved issues in this PR imho. Maybe need to discuss in developer meeting? Although I'm unable to join until week after next week.

# Conflicts: # client/src/hooks/useTripQuery.ts

leonardehrenfried · 2024-11-18T12:53:54Z

client/src/util/locationConverter.ts

+
+const LAT_LON_PATTERN = '(' + DOUBLE_PATTERN + ')(\\s*,\\s*|\\s+)(' + DOUBLE_PATTERN + ')';
+
+const ID_SEPARATOR = ':';


Sorry it took me a while to review this, but in the Transmodel API the IDs are not required to have a colon in it (although in practise they very often do).

Do we need the check for the valid feed id? We could simply say that if it's not a coordinate, then it's quay id, couldn't we?

I removed the ID checking part, so any input, that not a valid coordinate pair is processed as an ID input.

On the other hand, I needed to implement error handling around the coordinate and trip plan query, because if the id was invalid, the queries threw an error. The simplest fix was a try-catch around them and an error console log (until a more precise error handling is implemented).

… errors

t2gran · 2024-11-18T16:51:49Z

I implemented the stopId -> coordinate resolver with the quay query, and it works fine with stops, but not with stop places / stop-areas or any other nodes that have id (eg. vehicle rental stations).

Does the Debug UI lookup stop by name and uses the coordinate for the plan/trip query? If so, that is not what we want - instead the ID of the stop should be used as input to the trip-query. There are differences in the logic on the server when coordinates are provided and stop/station/multi-modal-stops/group-of-stop-plases ids are provided. It is important to be able to test that.

leonardehrenfried · 2024-11-18T17:04:11Z

I implemented the stopId -> coordinate resolver with the quay query, and it works fine with stops, but not with stop places / stop-areas or any other nodes that have id (eg. vehicle rental stations).

Does the Debug UI lookup stop by name and uses the coordinate for the plan/trip query? If so, that is not what we want - instead the ID of the stop should be used as input to the trip-query. There are differences in the logic on the server when coordinates are provided and stop/station/multi-modal-stops/group-of-stop-plases ids are provided. It is important to be able to test that.

No, you have to paste the ID of the stop (not station) into the search bar. The lookup is necessary because we want to show a pin on the map, not because we want to know the name.

leonardehrenfried

I just tried this and can't actually paste anything into the search bars. DOes it work for you, @richardkoszegi ?

richardkoszegi · 2024-11-19T12:07:31Z

I just tried this and can't actually paste anything into the search bars. DOes it work for you, @richardkoszegi ?

Yes, it works for me. (I checked it in Chrome, Firefox, Opera, and Edge). Are the inputs not editable, or the query not runs automatically?

leonardehrenfried

I must have been stupid. I cannot reproduce, so will approve.

testower

There are still comments that have not been addressed - EDIT: which may be because I didn't submit the review until now 🤦 sorry about that

client/src/components/SearchBar/LocationInputField.tsx

client/src/util/locationConverter.ts

testower · 2024-10-22T18:08:30Z

client/src/components/MapView/useCoordinateResolver.ts

+        longitude,
+        latitude,


These are named object properties, order actually doesn't matter. But I agree in principle to keep them consistently ordered anyway.

client/src/util/locationConverter.ts

testower · 2024-11-22T07:59:59Z

I tested this locally - seems to work as advertised!

richardkoszegi requested a review from a team as a code owner October 9, 2024 13:55

flaktack added the Skip Changelog label Oct 9, 2024

habrahamsson-skanetrafiken requested review from testower and leonardehrenfried October 15, 2024 09:07

t2gran added this to the 2.7 (next release) milestone Oct 16, 2024

testower requested changes Oct 16, 2024

View reviewed changes

richardkoszegi force-pushed the debug-ui-place-id-planning branch from 0233a71 to fdcd183 Compare October 17, 2024 13:04

debug client: support stop ids in from / to inputs

aa48a32

richardkoszegi force-pushed the debug-ui-place-id-planning branch from fdcd183 to a96aba7 Compare October 17, 2024 15:14

t2gran reviewed Oct 22, 2024

View reviewed changes

testower added the OTP Debug UI The OTP bundled client label Nov 6, 2024

debug client: resolve coordinates for stop ids

17228b2

richardkoszegi force-pushed the debug-ui-place-id-planning branch from a96aba7 to 17228b2 Compare November 12, 2024 10:46

richardkoszegi added 2 commits November 18, 2024 11:18

Merge branch 'dev-2.x' into debug-ui-place-id-planning

ac98067

# Conflicts: # client/src/hooks/useTripQuery.ts

Remove unneeded eslint-disable-line

ab96fec

leonardehrenfried reviewed Nov 18, 2024

View reviewed changes

debug client: turn off location validation in inputs and handle query…

a2ff9e1

… errors

leonardehrenfried requested changes Nov 18, 2024

View reviewed changes

leonardehrenfried previously approved these changes Nov 19, 2024

View reviewed changes

leonardehrenfried requested a review from testower November 19, 2024 12:57

testower requested changes Nov 19, 2024

View reviewed changes

Remove unnecessary comment

7f267f7

richardkoszegi dismissed leonardehrenfried’s stale review via 7f267f7 November 21, 2024 09:35

debug client: simplify location parser coordinate format

bc1bca0

leonardehrenfried requested a review from testower November 21, 2024 10:17

testower approved these changes Nov 22, 2024

View reviewed changes

leonardehrenfried approved these changes Nov 22, 2024

View reviewed changes

leonardehrenfried merged commit 9aed1e3 into opentripplanner:dev-2.x Nov 22, 2024
6 checks passed

flaktack deleted the debug-ui-place-id-planning branch November 22, 2024 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debug client: support stop ids in from / to inputs #6133

debug client: support stop ids in from / to inputs #6133

richardkoszegi commented Oct 9, 2024

codecov bot commented Oct 9, 2024 •

edited

Loading

leonardehrenfried commented Oct 15, 2024

richardkoszegi commented Oct 16, 2024

testower Oct 16, 2024

richardkoszegi Oct 18, 2024

richardkoszegi commented Oct 18, 2024

t2gran Oct 22, 2024 •

edited

Loading

testower Oct 22, 2024

richardkoszegi Nov 19, 2024

testower commented Oct 22, 2024

leonardehrenfried Nov 18, 2024

richardkoszegi Nov 19, 2024

t2gran commented Nov 18, 2024

leonardehrenfried commented Nov 18, 2024

leonardehrenfried left a comment

richardkoszegi commented Nov 19, 2024

leonardehrenfried left a comment

testower left a comment •

edited

Loading

testower Oct 22, 2024

testower commented Nov 22, 2024


		const LAT_LON_PATTERN = '(' + DOUBLE_PATTERN + ')(\\s,\\s\|\\s+)(' + DOUBLE_PATTERN + ')';

		const ID_SEPARATOR = ':';

debug client: support stop ids in from / to inputs #6133

debug client: support stop ids in from / to inputs #6133

Conversation

richardkoszegi commented Oct 9, 2024

Summary

Implementation Details:

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

leonardehrenfried commented Oct 15, 2024

richardkoszegi commented Oct 16, 2024

testower Oct 16, 2024

Choose a reason for hiding this comment

richardkoszegi Oct 18, 2024

Choose a reason for hiding this comment

richardkoszegi commented Oct 18, 2024

t2gran Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

testower Oct 22, 2024

Choose a reason for hiding this comment

richardkoszegi Nov 19, 2024

Choose a reason for hiding this comment

testower commented Oct 22, 2024

leonardehrenfried Nov 18, 2024

Choose a reason for hiding this comment

richardkoszegi Nov 19, 2024

Choose a reason for hiding this comment

t2gran commented Nov 18, 2024

leonardehrenfried commented Nov 18, 2024

leonardehrenfried left a comment

Choose a reason for hiding this comment

richardkoszegi commented Nov 19, 2024

leonardehrenfried left a comment

Choose a reason for hiding this comment

testower left a comment • edited Loading

Choose a reason for hiding this comment

testower Oct 22, 2024

Choose a reason for hiding this comment

testower commented Nov 22, 2024

codecov bot commented Oct 9, 2024 •

edited

Loading

t2gran Oct 22, 2024 •

edited

Loading

testower left a comment •

edited

Loading