tweak: make use of status field in detour list queries #2961
Conversation
| %{ | ||
| active: Map.get(detours, :active, []), | ||
| draft: | ||
| detours | ||
| |> Map.get(:draft, []) | ||
| |> Enum.filter(fn detour -> detour.author_id == user_id end), | ||
| past: Map.get(detours, :past, []) | ||
| active: detours_for_user(user_id, :active), | ||
| draft: detours_for_user(user_id, :draft), | ||
| past: detours_for_user(user_id, :past) | ||
| } | ||
| end |
There was a problem hiding this comment.
suggestion: since this is only used in the detours controller, how do you feel about pushing this implementation into the detours controller so we're not having to concern ourselves with keeping this interface available at this level?
| @@ -162,6 +162,7 @@ defmodule Skate.Detours.Db.Detour do | |||
| :author_id, | |||
| :activated_at, | |||
| :updated_at, | |||
| :status, | |||
There was a problem hiding this comment.
suggestion: can we remove :state_value with the addition of this?
There was a problem hiding this comment.
I'd like to see if we can do this but otherwise approved!
There was a problem hiding this comment.
I believe so. From what I can tell we weren't making use of that field specifically that I can see
There was a problem hiding this comment.
It was used to make the categorize_detour call cheaper https://github.com/mbta/skate/blob/bf-query-with-status-field/lib/skate/detours/detours.ex#L123-L125
There was a problem hiding this comment.
oh right, I left that comment after I changed that function to make use of the status field
There was a problem hiding this comment.
so I think the state_value removal may be a bit more complicated than it appears at first glance. we use that function to get the implicit status value in both directions, to and from the db, so during the “activate” action, the db record would have a status of :draft, but the snapshot being passed via upsert_snapshot has the :active %{"value" => %{"Detour Drawing" => %{"Active" => _}}} value. So my inclination is to make sure it’s not being used when pulling detours from the database as it’s no longer necessary and update the categorize_detour function to handle the status -> new_status transition that may be occurring
| defp apply_user_and_status_filter(query, user_id, :draft) do | ||
| where(query, [detour: d], d.status == :draft and d.author_id == ^user_id) | ||
| end | ||
|
|
||
| defp apply_user_and_status_filter(query, _user_id, status) do | ||
| where(query, [detour: d], d.status == ^status) |
There was a problem hiding this comment.
thought(non-blocking): what do you think of putting these in Skate.Detours.Db.Detour.Queries? I'm not that well versed in Elixir, so I'll defer to you here, but my thinking is that query building functions would belong in the Queries module.
| @@ -162,6 +162,7 @@ defmodule Skate.Detours.Db.Detour do | |||
| :author_id, | |||
| :activated_at, | |||
| :updated_at, | |||
| :status, | |||
There was a problem hiding this comment.
I'd like to see if we can do this but otherwise approved!
Asana ticket: https://app.asana.com/0/1205385723132845/1209213931559671