From 96583b7902f80ad2dc66c1868c5b4184b777c958 Mon Sep 17 00:00:00 2001 From: Brian Fauble Date: Thu, 14 Dec 2023 10:27:18 -0700 Subject: [PATCH] simplify params parsing and update affected controllers/states --- .../controllers/prediction_controller.ex | 6 ++--- apps/api_web/lib/api_web/params.ex | 22 ++++++++--------- apps/api_web/test/api_web/params_test.exs | 17 ++++++------- apps/state/lib/state/trip.ex | 11 ++------- apps/state/lib/state/vehicle.ex | 14 +++-------- apps/state/test/state/trip_test.exs | 24 ++++++++++++------- 6 files changed, 41 insertions(+), 53 deletions(-) diff --git a/apps/api_web/lib/api_web/controllers/prediction_controller.ex b/apps/api_web/lib/api_web/controllers/prediction_controller.ex index 20fe94b3..b2f055be 100644 --- a/apps/api_web/lib/api_web/controllers/prediction_controller.ex +++ b/apps/api_web/lib/api_web/controllers/prediction_controller.ex @@ -238,10 +238,8 @@ defmodule ApiWeb.PredictionController do defp add_revenue_matchers(matchers, :error), do: add_revenue_matchers(matchers, {:ok, :REVENUE}) - defp add_revenue_matchers(matchers, {:ok, :ALL}), do: matchers - - defp add_revenue_matchers(matchers, {:ok, revenue_matcher}) do - for matcher <- matchers do + defp add_revenue_matchers(matchers, {:ok, revenue_matchers}) do + for revenue_matcher <- List.wrap(revenue_matchers), matcher <- matchers do Map.put(matcher, :revenue, revenue_matcher) end end diff --git a/apps/api_web/lib/api_web/params.ex b/apps/api_web/lib/api_web/params.ex index 06c28810..6aa5f786 100644 --- a/apps/api_web/lib/api_web/params.ex +++ b/apps/api_web/lib/api_web/params.ex @@ -211,22 +211,20 @@ defmodule ApiWeb.Params do @doc """ Parse revenue filter to valid params """ - def revenue(value) when is_binary(value) do - value + def revenue(values) when is_binary(values) do + values |> split_on_comma() - |> MapSet.new() - |> MapSet.intersection(MapSet.new(["REVENUE", "NON_REVENUE"])) - |> MapSet.to_list() - |> Enum.sort() + |> Enum.reduce_while({:ok, []}, fn + "REVENUE", {:ok, acc} -> {:cont, {:ok, [:REVENUE | acc]}} + "NON_REVENUE", {:ok, acc} -> {:cont, {:ok, [:NON_REVENUE | acc]}} + _, _ -> {:halt, :error} + end) |> revenue() end - for value <- ~w(NON_REVENUE REVENUE)a do - def revenue([unquote(Atom.to_string(value))]), do: {:ok, unquote(value)} - end - - def revenue(["NON_REVENUE", "REVENUE"]), do: {:ok, :ALL} - def revenue(_value), do: :error + def revenue({:ok, []}), do: :error + def revenue(nil), do: :error + def revenue(val), do: val @doc """ Parses and integer value from params. diff --git a/apps/api_web/test/api_web/params_test.exs b/apps/api_web/test/api_web/params_test.exs index 7f532f1f..862c1c27 100644 --- a/apps/api_web/test/api_web/params_test.exs +++ b/apps/api_web/test/api_web/params_test.exs @@ -156,23 +156,24 @@ defmodule ApiWeb.ParamsTest do assert :error = Params.revenue("revenue") assert :error = Params.revenue("non_revenue") assert :error = Params.revenue("revenue,non_revenue") - assert {:ok, :REVENUE} = Params.revenue("REVENUE,non_revenue") - assert {:ok, :NON_REVENUE} = Params.revenue("revenue,NON_REVENUE") + assert :error = Params.revenue("REVENUE,non_revenue") + assert :error = Params.revenue("revenue,NON_REVENUE") end test "it parses a list of values" do - assert {:ok, :ALL} = Params.revenue("REVENUE,NON_REVENUE") - assert {:ok, :ALL} = Params.revenue("NON_REVENUE,REVENUE") - assert {:ok, :ALL} = Params.revenue("REVENUE,other,NON_REVENUE") - assert {:ok, :ALL} = Params.revenue("NON_REVENUE,,REVENUE") + assert {:ok, [:NON_REVENUE, :REVENUE]} = Params.revenue("REVENUE,NON_REVENUE") + assert {:ok, [:REVENUE, :NON_REVENUE]} = Params.revenue("NON_REVENUE,REVENUE") + assert :error = Params.revenue("REVENUE,other,NON_REVENUE") + assert {:ok, [:REVENUE, :NON_REVENUE]} = Params.revenue("NON_REVENUE,,REVENUE") assert :error = Params.revenue("NOT,VALID,PARAMS") end test "it parses single values" do - assert {:ok, :REVENUE} = Params.revenue("REVENUE") - assert {:ok, :NON_REVENUE} = Params.revenue("NON_REVENUE") + assert {:ok, [:REVENUE]} = Params.revenue("REVENUE") + assert {:ok, [:NON_REVENUE]} = Params.revenue("NON_REVENUE") assert :error = Params.revenue("INVALID") assert :error = Params.revenue("ALL") + assert :error = Params.revenue("") end end end diff --git a/apps/state/lib/state/trip.ex b/apps/state/lib/state/trip.ex index 4f6f6609..1985767a 100644 --- a/apps/state/lib/state/trip.ex +++ b/apps/state/lib/state/trip.ex @@ -18,7 +18,7 @@ defmodule State.Trip do optional(:direction_id) => direction_id, optional(:date) => Date.t(), optional(:route_patterns) => [String.t()], - optional(:revenue_status) => :all | :non_revenue | :revenue + optional(:revenue) => [:NON_REVENUE | :REVENUE] } @fetch_trips {:fetch, "trips.txt"} @@ -171,7 +171,7 @@ defmodule State.Trip do |> build_filters(:direction_id, filters[:direction_id]) |> build_filters(:route_pattern_id, filters[:route_patterns]) |> build_filters(:id, filters[:ids]) - |> build_filters(:revenue, filters[:revenue]) + |> build_filters(:revenue, Map.get(filters, :revenue, :REVENUE)) idx = get_index(filters) trips = State.Trip.Added.select(matchers, idx) ++ State.Trip.select(matchers, idx) @@ -182,13 +182,6 @@ defmodule State.Trip do end end - defp build_filters(matchers, :revenue, :ALL), do: matchers - - defp build_filters(matchers, :revenue, nil), do: build_filters(matchers, :revenue, :REVENUE) - - defp build_filters(matchers, :revenue, value) when value in [:NON_REVENUE, :REVENUE], - do: build_filters(matchers, :revenue, [value]) - defp build_filters(matchers, _key, nil), do: matchers defp build_filters(matchers, key, values) do diff --git a/apps/state/lib/state/vehicle.ex b/apps/state/lib/state/vehicle.ex index d0af37a6..1004dfc2 100644 --- a/apps/state/lib/state/vehicle.ex +++ b/apps/state/lib/state/vehicle.ex @@ -20,7 +20,7 @@ defmodule State.Vehicle do optional(:routes) => [Model.Route.id(), ...], optional(:direction_id) => Model.Direction.id() | nil, optional(:route_types) => [Model.Route.route_type(), ...], - optional(:revenue) => :ALL | :NON_REVENUE | :REVENUE + optional(:revenue) => [:NON_REVENUE | :REVENUE] } @impl State.Server @@ -101,18 +101,10 @@ defmodule State.Vehicle do end end - defp build_filters(matchers, :revenue, revenue_param, _filters) do - revenue = revenue_service_value(revenue_param) - - for matcher <- matchers do - Map.put(matcher, :revenue, revenue) - end + defp build_filters(matchers, key, values, _filters) do + for matcher <- matchers, value <- List.wrap(values), do: Map.put(matcher, key, value) end - defp revenue_service_value(:ALL), do: :_ - defp revenue_service_value(:NON_REVENUE), do: :NON_REVENUE - defp revenue_service_value(_), do: :REVENUE - @spec do_post_search_filter([Vehicle.t()], filter_opts) :: [Vehicle.t()] defp do_post_search_filter(vehicles, %{labels: labels}) when is_list(labels) do labels = MapSet.new(labels) diff --git a/apps/state/test/state/trip_test.exs b/apps/state/test/state/trip_test.exs index 9dbf2cfa..abd93212 100644 --- a/apps/state/test/state/trip_test.exs +++ b/apps/state/test/state/trip_test.exs @@ -507,50 +507,56 @@ defmodule State.TripTest do "trip8" ] - assert mapped_and_sorted_filter_by(%{routes: ["rev-route-9"], revenue: :REVENUE}) == + assert mapped_and_sorted_filter_by(%{routes: ["rev-route-9"], revenue: [:REVENUE]}) == [ "trip1", "trip2", "trip6" ] - assert mapped_and_sorted_filter_by(%{routes: ["rev-route-10"], revenue: :REVENUE}) == + assert mapped_and_sorted_filter_by(%{routes: ["rev-route-10"], revenue: [:REVENUE]}) == [ "trip3", "trip4", "trip8" ] - assert mapped_and_sorted_filter_by(%{route_pattern_id: 1, revenue: :REVENUE}) == + assert mapped_and_sorted_filter_by(%{route_pattern_id: 1, revenue: [:REVENUE]}) == ["trip1", "trip2", "trip3", "trip4", "trip6", "trip8"] - assert mapped_and_sorted_filter_by(%{routes: ["rev-route-9"], revenue: :ALL}) == [ + assert mapped_and_sorted_filter_by(%{ + routes: ["rev-route-9"], + revenue: [:NON_REVENUE, :REVENUE] + }) == [ "trip1", "trip2", "trip5", "trip6" ] - assert mapped_and_sorted_filter_by(%{routes: ["rev-route-10"], revenue: :ALL}) == [ + assert mapped_and_sorted_filter_by(%{ + routes: ["rev-route-10"], + revenue: [:NON_REVENUE, :REVENUE] + }) == [ "trip3", "trip4", "trip7", "trip8" ] - assert mapped_and_sorted_filter_by(%{route_pattern_id: 1, revenue: :ALL}) == + assert mapped_and_sorted_filter_by(%{route_pattern_id: 1, revenue: [:NON_REVENUE, :REVENUE]}) == ["trip1", "trip2", "trip3", "trip4", "trip5", "trip6", "trip7", "trip8"] - assert mapped_and_sorted_filter_by(%{routes: ["rev-route-9"], revenue: :NON_REVENUE}) == + assert mapped_and_sorted_filter_by(%{routes: ["rev-route-9"], revenue: [:NON_REVENUE]}) == ["trip5"] assert mapped_and_sorted_filter_by(%{ routes: ["rev-route-10"], - revenue: :NON_REVENUE + revenue: [:NON_REVENUE] }) == ["trip7"] - assert mapped_and_sorted_filter_by(%{route_pattern_id: 1, revenue: :NON_REVENUE}) == + assert mapped_and_sorted_filter_by(%{route_pattern_id: 1, revenue: [:NON_REVENUE]}) == ["trip5", "trip7"] end