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

fixup: Trip planner itinerary tagging #1897

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

thecristen
Copy link
Collaborator

Summary of changes

Asana Ticket: Trip planner itinerary tagging: implement tie-breaking & new "most direct" tag (tie-breaking with fallback)

Sorry, there's not much here! Most of the actual implementation of the new tag, as well as implementation of tagging in general, is in thecristen/open_trip_planner_client@v0.5.0...v0.6.1 in the OpenTripPlannerClient.ItineraryTag and OpenTripPlannerClient.ItineraryTag.MostDirect modules.

bump open_trip_planner_client version, which adds support for MostDirect tag since v0.6.

this also adjusts the returned data slightly, so when parsing we can use the agency name as opposed to parsing the agency GTFS ID.
@thecristen thecristen requested a review from a team as a code owner February 21, 2024 19:19
@thecristen thecristen requested review from kotva006 and removed request for a team February 21, 2024 19:19
@thecristen thecristen added the dev-blue Deploy to dev-blue label Feb 21, 2024
@thecristen thecristen marked this pull request as draft February 21, 2024 19:35
@thecristen
Copy link
Collaborator Author

This shouldn't be put live until mbta/otp-deploy#35 is merged. However, I'll open this PR to review with a deployment to Dotcom dev-blue once I finish deploying OTP with the needed changes to OTP dev-blue.

@thecristen thecristen marked this pull request as ready for review February 21, 2024 20:03
@@ -109,12 +111,13 @@ defmodule DotCom.Mixfile do
{:logster, "1.1.1"},
{:mail, "0.3.1"},
{:mock, "0.3.8", [only: :test]},
{:mox, "1.1.0", [only: :test]},
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem to be added as dependencies but not used in this pull request

Copy link
Collaborator Author

@thecristen thecristen Feb 22, 2024

Choose a reason for hiding this comment

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

The commit message that added these was fixup merge commit....these were supposed to be in the feat/trip-planner-improvements branch (and are being used there already).

@kotva006
Copy link
Contributor

Is the goal of the open_trip_planner_client to replace the implementation of the open_trip_planner.ex?
I am confused because they both seem to still exist and be doing similar things

@thecristen
Copy link
Collaborator Author

@kotva006 Yeah, just the plan/3 function specifically. See the description of a previous PR to this branch for more: #1883

@kotva006
Copy link
Contributor

@kotva006 Yeah, just the plan/3 function specifically. See the description of a previous PR to this branch for more: #1883

I am not tracking. If the open_trip_planner_client library replaces the plan/3 function, why does the function still exist?

@thecristen
Copy link
Collaborator Author

@kotva006 Did you see the source? https://github.com/mbta/dotcom/blob/cbj/fix-tags/lib/trip_plan/api/open_trip_planner.ex#L18

Our plan/3 function calls the function returned by Application.get_env(:dotcom, :trip_planner, OpenTripPlannerClient). In prod/dev that just refers to the OpenTripPlannerClient.plan/3 function, but in MIX_ENV=test it's configured to point to a different function that's used for testing.

@thecristen thecristen removed the dev-blue Deploy to dev-blue label Feb 27, 2024
@thecristen thecristen merged commit 479a829 into feat/trip-planner-improvements Feb 28, 2024
14 of 18 checks passed
@thecristen thecristen deleted the cbj/fix-tags branch February 28, 2024 14:55
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.

2 participants