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

feat: RoutePatterns table, part 1: Adding to db #2900

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions lib/skate/detours/db/detour.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ defmodule Skate.Detours.Db.Detour do
import Ecto.Changeset

alias Skate.Settings.Db.User
alias Skate.Detours.Db.RoutePattern

typed_schema "detours" do
field :state, :map
belongs_to :author, User
belongs_to :route_pattern, RoutePattern

timestamps()
end
Expand Down
54 changes: 54 additions & 0 deletions lib/skate/detours/db/route_pattern.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Should this live here under detours?
# I don't like how the db folders are scattered throughout /lib subdirectories.
# I'd rather they all live in one subfolder.
# Example: there's db files in /notifications and /skate/detours and /skate/settings, etc
defmodule Skate.Detours.Db.RoutePattern do
@moduledoc """
Ecto Model for `route_patterns` Database table
"""

use Skate.Schema
import Ecto.Changeset

alias Skate.Detours.Db.Detour
alias Skate.Detours.DirectionName

@required_fields [
:hash,
:gtfs_route_pattern_id,
:gtfs_route_pattern_name,
:gtfs_route_pattern_headsign,
# :gtfs_route_pattern_shape,
:gtfs_route_pattern_direction_name,
:gtfs_route_id,
:gtfs_route_name
]

typed_schema "route_patterns" do
field(:hash, :integer)
field(:gtfs_route_pattern_id, :string)
field(:gtfs_route_pattern_name, :string)
field(:gtfs_route_pattern_headsign, :string)
field(:gtfs_route_pattern_direction_name, DirectionName)

# The given route pattern will always have the given route
# Does it need to be stored here as well?
field(:gtfs_route_id, :string)
field(:gtfs_route_name, :string)

field(:gtfs_route_pattern_time_description, :string)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know this existed! I think we should set writable: :insert on these fields so we don't accidentally update them

Suggested change
field(:gtfs_route_pattern_id, :string)
field(:gtfs_route_pattern_name, :string)
field(:gtfs_route_pattern_headsign, :string)
field(:gtfs_route_pattern_direction_name, DirectionName)
# The given route pattern will always have the given route
# Does it need to be stored here as well?
field(:gtfs_route_id, :string)
field(:gtfs_route_name, :string)
field(:gtfs_route_pattern_time_description, :string)
field(:gtfs_route_pattern_id, :string, writable: :insert)
field(:gtfs_route_pattern_name, :string, writable: :insert)
field(:gtfs_route_pattern_headsign, :string, writable: :insert)
field(:gtfs_route_pattern_direction_name, DirectionName, writable: :insert)
# The given route pattern will always have the given route
# Does it need to be stored here as well?
field(:gtfs_route_id, :string, writable: :insert)
field(:gtfs_route_name, :string, writable: :insert)
field(:gtfs_route_pattern_time_description, :string)


# To be added: gtfs_route_pattern_shape (separate table)

has_many(:detours, Detour)

timestamps()
end

def changeset(route_pattern, attrs \\ %{}) do
route_pattern
|> cast(attrs, @required_fields, [:gtfs_route_pattern_time_description])
|> validate_required(@required_fields)
|> unique_constraint([:hash], name: :route_patterns_unique_index)
end
end
25 changes: 22 additions & 3 deletions lib/skate/detours/detours.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ defmodule Skate.Detours.Detours do
import Ecto.Query, warn: false
alias Skate.Repo
alias Skate.Detours.Db.Detour
alias Skate.Detours.Db.RoutePattern
alias Skate.Detours.SnapshotSerde
alias Skate.Detours.Detour.Detailed, as: DetailedDetour
alias Skate.Detours.Detour.WithState, as: DetourWithState
Expand Down Expand Up @@ -198,18 +199,18 @@ defmodule Skate.Detours.Detours do
"""
def upsert_from_snapshot(author_id, %{} = snapshot) do
previous_record =
case Skate.Detours.SnapshotSerde.id_from_snapshot(snapshot) do
case SnapshotSerde.id_from_snapshot(snapshot) do
nil -> nil
id -> Skate.Repo.get(Detour, id)
end

detour_db_result =
author_id
|> Skate.Detours.SnapshotSerde.deserialize(snapshot)
|> SnapshotSerde.deserialize(snapshot)
|> Skate.Repo.insert(
returning: true,
conflict_target: [:id],
on_conflict: {:replace, [:state, :updated_at]}
on_conflict: {:replace, [:state, :updated_at, :route_pattern_id]}
)

case detour_db_result do
Expand All @@ -223,6 +224,24 @@ defmodule Skate.Detours.Detours do
detour_db_result
end

@doc """
Retrieve or insert-and-return a route_pattern given a deserialized route_pattern.
"""
def get_or_create_route_pattern(route_pattern) do
route_pattern_hash = Map.get(route_pattern, :hash)

case Skate.Repo.get_by(RoutePattern, hash: route_pattern_hash) do
nil ->
Skate.Repo.insert!(
route_pattern,
returning: true
)

existing_route_pattern ->
existing_route_pattern
end
end

@doc """
Retrieves a `Skate.Detours.Db.Detour` from the database by it's ID and then resolves the
detour's category via `categorize_detour/2`
Expand Down
29 changes: 29 additions & 0 deletions lib/skate/detours/direction_name.ex
Copy link
Member

Choose a reason for hiding this comment

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

based on our directions.txt extension I'm hesitant to "prematurely" restrict this, but I'd be open to this if we can confirm that bus always has only Inbound and Outbound.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhhh rats. I didn't realize direction could be so varied in gtfs. I do believe we as an agency embrace only the Inbound / Outbound, so I could confirm that with TD.

On the other hand, I had a related dilemma. I just realized that we need to be able to repopulate this field in the state machine: "directionNames" => %{"0" => "Outbound", "1" => "Inbound"} so that we can create this component
Screenshot 2024-11-20 at 1 34 12 PM

I'll need to adjust the db schema accordingly. Maybe store the map and the direction id instead of the direction name

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need to store that info on a Detour as it's only relevant when we're first picking the route pattern right?

That's a good point though that we'll want to somehow gate the "route pattern picker" part based on some criteria, like if the route they're viewing is no longer current. A detour shouldn't be saved to the database in this state though. So we should be able to discard this info and make the state machine refetch this info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah that's right! Let me think about this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok had to doublecheck:

it's only relevant when we're first picking the route pattern right?

Unfortunately, no, because of this line in DiversionPage:
const routeDirection = routePattern && route.directionNames[routePattern.directionId]

which used at every stage of the detour-creation workflow. So that will need to be refactored a bit on the frontend, which will necessitate some more tweaks on the backend

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't spend too much time on it if it's tricky, but I think you could reference Schedule.Gtfs.RoutePattern.id() and Schedule.Gtfs.Route.id() in the schema via https://hexdocs.pm/typed_ecto_schema/TypedEctoSchema.html#module-overriding-the-typespec-for-a-field

Copy link
Member

Choose a reason for hiding this comment

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

And if you need to create new type aliases that'd be fine.
As for where they'd live, I think starting out with them in the Ecto Schema file is fine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gottttttt it, ok. I'm having a little trouble, but expect to figure it out. But in the meantime, follow-up question: does enforcing this schema on input also enforce it on retrieval? (That might impact whether I try to get through the trickiness or just use strings.) Also do we have any examples of using custom types in the field of a typed_schema?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we have an example, but I'm picturing something like this, I've not tested it though and can't confirm it works as is.

field(:gtfs_route_pattern_id, :string) :: Schedule.Gtfs.RoutePattern.id()

Copy link
Collaborator Author

@hannahpurcell hannahpurcell Nov 25, 2024

Choose a reason for hiding this comment

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

Ahhhh I see, thank you. I'll test it now. The typed_schema definitely had some magic going on that prevented putting the RoutePattern within the field call.

Edit: Yes, that works!

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
defmodule Skate.Detours.DirectionName do
@moduledoc false

use Ecto.Type

@type t :: String.t()

@valid_directions ["Inbound", "Outbound"]

@impl true
def type, do: :string

@impl true
def cast(direction), do: allow_valid_direction(direction)

@impl true
def load(direction), do: allow_valid_direction(direction)

@impl true
def dump(direction), do: allow_valid_direction(direction)

defp allow_valid_direction(direction) do
if direction in @valid_directions do
{:ok, direction}
else
:error
end
end
end
43 changes: 42 additions & 1 deletion lib/skate/detours/snapshot_serde.ex
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,17 @@ defmodule Skate.Detours.SnapshotSerde do
Converts a XState JSON Snapshot to Detours Database Changeset
"""
def deserialize(user_id, %{} = snapshot) do
route_pattern =
snapshot
|> deserialize_route_pattern()
|> Skate.Detours.Detours.get_or_create_route_pattern()

Skate.Detours.Db.Detour.changeset(
%Skate.Detours.Db.Detour{
# `id` is `nil` by default, so a `nil` `id` should be fine
id: id_from_snapshot(snapshot),
author_id: user_id
author_id: user_id,
route_pattern_id: Map.get(route_pattern, :id)
},
%{
# Save Snapshot to DB until we've fully transitioned to serializing
Expand All @@ -32,6 +38,41 @@ defmodule Skate.Detours.SnapshotSerde do
def id_from_snapshot(%{"context" => %{"uuid" => id}}), do: id
def id_from_snapshot(%{"context" => %{}}), do: nil

# Converts the RoutePattern from a XState Snapshot to ready for DB insertion
defp deserialize_route_pattern(%{
"context" => %{
"route" =>
%{
"id" => route_id,
"name" => route_name,
"directionNames" => direction_map
} = route,
"routePattern" =>
%{
"id" => route_pattern_id,
"name" => route_pattern_name,
"headsign" => headsign,
"directionId" => direction_id,
"timeDescription" => time_description
} = routePattern
}
}) do
direction_name = direction_map[Integer.to_string(direction_id)]

%Skate.Detours.Db.RoutePattern{
hash: route_pattern_hash(Map.merge(route, routePattern)),
gtfs_route_pattern_id: route_pattern_id,
gtfs_route_pattern_name: route_pattern_name,
gtfs_route_pattern_headsign: headsign,
gtfs_route_pattern_direction_name: direction_name,
gtfs_route_id: route_id,
gtfs_route_name: route_name,
gtfs_route_pattern_time_description: time_description
}
end

defp route_pattern_hash(map), do: :erlang.phash2(map)

@doc """
Builds XState Snapshot from Detours Database object
"""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
defmodule Skate.Repo.Migrations.CreateRoutePatternsTable do
use Ecto.Migration

def change do
execute("CREATE TYPE direction_name AS ENUM ('Inbound', 'Outbound')")

create table(:route_patterns) do
add :hash, :integer
add :gtfs_route_pattern_id, :string
add :gtfs_route_pattern_name, :string
add :gtfs_route_pattern_headsign, :string
add :gtfs_route_pattern_direction_name, :direction_name
add :gtfs_route_id, :string
add :gtfs_route_name, :string
add :gtfs_route_pattern_time_description, :string
timestamps()
end

create(
unique_index(
:route_patterns,
[:hash],
name: :route_patterns_unique_index
)
)
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
defmodule Skate.Repo.Migrations.DetourHasOneRoutePattern do
use Ecto.Migration

def change do
alter table(:detours) do
add :route_pattern_id, references(:route_patterns)
end
end
end
Loading