Review: Requested Changes

- Munge Fix sql query output
- Text:: don't do week in seconds.
- Fix: hide exact task delivery on signup
- Add: note about over-querying db.
- Switch "Campaign filled" button to :secondary color.
- Fix typo.
- Fix: use replace with patch for rider signup links.

lib/bike_brigade/delivery.ex

@@ -199,7 +199,7 @@ defmodule BikeBrigade.Delivery do
           total_tasks: count(t.id),
           filled_tasks:
             sum(fragment("CASE WHEN ? IS NULL THEN 0 ELSE 1 END", t.assigned_rider_id)),
-          rider_ids: fragment("array_agg(?::text)", t.assigned_rider_id)
+          rider_ids: fragment("array_agg(?)", t.assigned_rider_id)
         }
 
     Repo.all(query)
@@ -208,7 +208,7 @@ defmodule BikeBrigade.Delivery do
       fn x ->
         {x.campaign_id,
          %{
-           rider_ids: x.rider_ids,
+           rider_ids_counts: Enum.frequencies(x.rider_ids),
            total_tasks: x.total_tasks,
            filled_tasks: x.filled_tasks
          }}

lib/bike_brigade_web/core_components.ex

@@ -37,7 +37,7 @@ defmodule BikeBrigadeWeb.CoreComponents do
     values: [:none, :small, :normal, :medium, :full]
 
   attr :class, :string, default: nil
-  attr :rest, :global, include: ~w(href patch navigate disabled)
+  attr :rest, :global, include: ~w(href patch navigate disabled replace)
   slot :inner_block, required: true
 
   @button_base_classes [

lib/bike_brigade_web/live/campaign_signup_live/index.ex

@@ -24,7 +24,6 @@ defmodule BikeBrigadeWeb.CampaignSignupLive.Index do
      |> assign(:page, :campaigns)
      |> assign(:page_title, "Campaign Signup List")
      |> assign(:current_week, current_week)
-     # REVIEW: rename this to `campaign_meta` ?
      |> assign(:campaign_task_counts, Delivery.get_total_tasks_and_open_tasks(current_week))
      |> assign(:campaigns, campaigns)}
   end
@@ -84,6 +83,9 @@ defmodule BikeBrigadeWeb.CampaignSignupLive.Index do
     |> Enum.reverse()
   end
 
+  # TODO HACK: right now everytime something about a task, or campaign rider
+  # changes (add, edit, delete), we refetch all tasks and campaign riders.
+  # This may eventually become a problem.
   defp refetch_and_assign_data(socket) do
     week = socket.assigns.current_week
 
@@ -104,8 +106,7 @@ defmodule BikeBrigadeWeb.CampaignSignupLive.Index do
 
   defp get_signup_text(campaign_id, rider_id, campaign_task_counts) do
     count_tasks_for_current_rider =
-      campaign_task_counts[campaign_id].rider_ids
-      |> Enum.count(fn i -> i == Integer.to_string(rider_id) end)
+      campaign_task_counts[campaign_id].rider_ids_counts[rider_id] || 0
 
     cond do
       count_tasks_for_current_rider > 0 ->

lib/bike_brigade_web/live/campaign_signup_live/index.html.heex

@@ -65,8 +65,7 @@
                     <% campaign_tasks_fully_assigned?(c.id, @campaign_task_counts) -> %>
                       <.button
                         size={:xsmall}
-                        color={:primary}
-
+                        color={:secondary}
                         navigate={~p"/campaigns/signup/#{c}/"}>
                         Campaign Filled
                       </.button>

lib/bike_brigade_web/live/campaign_signup_live/show.ex

@@ -3,6 +3,7 @@ defmodule BikeBrigadeWeb.CampaignSignupLive.Show do
 
   import BikeBrigadeWeb.CampaignHelpers
   alias BikeBrigade.Delivery
+  alias BikeBrigade.Locations
 
   @impl true
   def mount(_params, _session, socket) do
@@ -122,7 +123,7 @@ defmodule BikeBrigadeWeb.CampaignSignupLive.Show do
   @impl Phoenix.LiveView
   def handle_info({event, entity}, socket) when event in @broadcasted_infos do
     campaign = socket.assigns.campaign
-    # if a task or a campaign rider changes (ie, if any of hte broadcasted_infos)
+    # if a task or a campaign rider changes (ie, if any of the broadcasted_infos)
     # launches from elsewhere, check if the entity's respective campaign id matches
     # the id of the campaign in the current view; if so, refetch the data.
     if entity.campaign_id == campaign.id do

lib/bike_brigade_web/live/campaign_signup_live/show.html.heex

@@ -17,7 +17,7 @@
       <.get_delivery_size task={task}/>
     </:col>
     <:col :let={task} label="Recipient"><%= task.dropoff_name %></:col>
-    <:col :let={task} label="Dropoff Location"><%= task.dropoff_location.address %></:col>
+    <:col :let={task} label="Dropoff Neighbourhood"><%= Locations.neighborhood(task.dropoff_location) %></:col>
     <:col :let={task} label="Notes"><%= task.delivery_status_notes %>
       <.truncated_riders_notes note={task.rider_notes || "--"}/>
     </:col>
@@ -45,9 +45,8 @@
           color={:primary}
           size={:xsmall}
           class="w-28"
-          patch={~p"/campaigns/signup/#{@campaign}/task/#{task.id}"}
-
-          >
+          replace
+          patch={~p"/campaigns/signup/#{@campaign}/task/#{task.id}"}>
           Signup
         </.button>
 

lib/bike_brigade_web/live/campaign_signup_live/signup_rider_form_component.ex

@@ -29,7 +29,7 @@ defmodule BikeBrigadeWeb.CampaignSignupLive.SignupRiderFormComponent do
     case Delivery.create_campaign_rider(attrs) do
       {:ok, _cr} ->
         {:ok, _task} = Delivery.update_task(task, %{assigned_rider_id: rider_id})
-        {:noreply, socket |> push_redirect(to: ~p"/campaigns/signup/#{campaign}")}
+        {:noreply, socket |> push_patch(to: ~p"/campaigns/signup/#{campaign}", replace: true)}
 
       {:error, %Ecto.Changeset{} = changeset} ->
         {:noreply, assign(socket, :changeset, changeset)}

lib/bike_brigade_web/live/campaign_signup_live/signup_rider_form_component.html.heex

@@ -18,8 +18,8 @@
       <:radio value={false} label="No" />
     </.radio_group>
     <:actions>
-      <.button type="submit" phx-disable-with="Adding...">
-        Add
+      <.button type="submit" phx-disable-with="Signing up...">
+        Signup
       </.button>
     </:actions>
   </.simple_form>

test/bike_brigade_web/live/campaign_signup_live_test.exs

@@ -4,7 +4,6 @@ defmodule BikeBrigadeWeb.CampaignSignupLiveTest do
 
   import Phoenix.LiveViewTest
 
-  @week_in_sec 604_800
 
   describe "Index - General" do
     setup ctx do
@@ -30,9 +29,9 @@ defmodule BikeBrigadeWeb.CampaignSignupLiveTest do
       campaign =
         fixture(:campaign, %{
           program_id: ctx.program.id,
-          delivery_start: LocalizedDateTime.now() |> DateTime.add(@week_in_sec),
+          delivery_start: LocalizedDateTime.now() |> DateTime.add(7, :day),
           delivery_end:
-            LocalizedDateTime.now() |> DateTime.add(@week_in_sec) |> DateTime.add(60, :second)
+            LocalizedDateTime.now() |> DateTime.add(7, :day) |> DateTime.add(60, :second)
         })
 
       {:ok, live, _html} = live(ctx.conn, ~p"/campaigns/signup")
@@ -47,9 +46,9 @@ defmodule BikeBrigadeWeb.CampaignSignupLiveTest do
       campaign =
         fixture(:campaign, %{
           program_id: ctx.program.id,
-          delivery_start: LocalizedDateTime.now() |> DateTime.add(-@week_in_sec),
+          delivery_start: LocalizedDateTime.now() |> DateTime.add(-7, :day),
           delivery_end:
-            LocalizedDateTime.now() |> DateTime.add(-@week_in_sec) |> DateTime.add(60, :second)
+            LocalizedDateTime.now() |> DateTime.add(-7, :day) |> DateTime.add(60, :second)
         })
 
       {:ok, live, _html} = live(ctx.conn, ~p"/campaigns/signup")
@@ -143,7 +142,7 @@ defmodule BikeBrigadeWeb.CampaignSignupLiveTest do
     test "we see pertinent task information", ctx do
       {:ok, _live, html} = live(ctx.conn, ~p"/campaigns/signup/#{ctx.campaign.id}/")
       assert html =~ ctx.task.dropoff_name
-      assert html =~ ctx.task.dropoff_location.address
+      assert html =~ BikeBrigade.Locations.neighborhood(ctx.task.dropoff_location)
     end
 
     test "Invalid route for campaign shows flash and redirects", ctx do