Review timer functions

[SimonLab]

I've noticed a few things that I want to update or have a detailed looked at to make sure I didn't miss something:

    {:ok, _timer} =
      Timer.start(%{
        item_id: item.id,
        person_id: person_id,
        start: NaiveDateTime.utc_now()
      })

• Remove the person_id value as the schema doesn't have it:

  schema "timers" do
    field :item_id, :id
    field :start, :naive_datetime
    field :stop, :naive_datetime

    timestamps()
  end

• Make the stop function consistent with the start, ie pass the stop value as parameter:

{:ok, _timer} = Timer.stop(%{id: timer_id})

• Combine start/stop function to update function

• Review how toggle stop the timer. This mean that when a done item returns to active status we also stop the timer again?

 def handle_event("toggle", data, socket) do
    # Toggle the status of the item between 3 (:active) and 4 (:done)
    status = if Map.has_key?(data, "value"), do: 4, else: 3

    # need to restrict getting items to the people who own or have rights to access them!
    item = Item.get_item!(Map.get(data, "id"))
    Item.update_item(item, %{status: status})
    Timer.stop_timer_for_item_id(item.id)

    AppWeb.Endpoint.broadcast(@topic, "update", :toggle)
    {:noreply, socket}
  end

[nelsonic]

@SimonLab these are all valid questions.
agree with removing person_id from the Timer.start 👍
Timer.stop should allow the passing of the stop value but should default to now.

[SimonLab]

I've started to break to many things.
I need to review first this section to understand fully how the item/timers structure is created:

mvp/lib/app/item.ex

Lines 97 to 247 in 7e94e47

	# 🐲 H E R E B E D R A G O N S! 🐉
	# ⏳ Working with Time is all Dragons! 🙄
	# 👩‍💻 Feedback/Pairing/Refactoring Welcome! 🙏
	@doc """
	`items_with_timers/1` Returns a List of items with the latest associated timers.
	## Examples
	iex> items_with_timers()
	[
	%{text: "hello", person_id: 1, status: 2, start: 2022-07-14 09:35:18},
	%{text: "world", person_id: 2, status: 7, start: 2022-07-15 04:20:42}
	]
	"""
	#
	def items_with_timers(person_id \\ 0) do
	sql = """
	SELECT i.id, i.text, i.status, i.person_id, t.start, t.stop, t.id as timer_id FROM items i
	FULL JOIN timers as t ON t.item_id = i.id
	WHERE i.person_id = $1 AND i.status IS NOT NULL
	ORDER BY timer_id ASC;
	"""
	Ecto.Adapters.SQL.query!(Repo, sql, [person_id])
	|> map_columns_to_values()
	|> accumulate_item_timers()
	end
	@doc """
	`map_columns_to_values/1` takes an Ecto SQL query result
	which has the List of columns and rows separate
	and returns a List of Maps where the keys are the column names and values the data.
	Sadly, Ecto returns rows without column keys so we have to map them manually:
	ref: https://groups.google.com/g/elixir-ecto/c/0cubhSd3QS0/m/DLdQsFrcBAAJ
	"""
	def map_columns_to_values(res) do
	Enum.map(res.rows, fn row ->
	Enum.zip(res.columns, row) |> Map.new() |> AtomicMap.convert()
	end)
	end
	@doc """
	`map_timer_diff/1` transforms a list of items_with_timers
	into a flat map where the key is the timer_id and the value is the difference
	between timer.stop and timer.start
	If there is no active timer return {0, 0}.
	If there is no timer.stop return Now - timer.start
	## Examples
	iex> list = [
	%{ stop: nil, id: 3, start: nil, timer_id: nil },
	%{ stop: ~N[2022-07-17 11:18:24], id: 1, start: ~N[2022-07-17 11:18:18], timer_id: 1 },
	%{ stop: ~N[2022-07-17 11:18:31], id: 1, start: ~N[2022-07-17 11:18:26], timer_id: 2 },
	%{ stop: ~N[2022-07-17 11:18:24], id: 2, start: ~N[2022-07-17 11:18:00], timer_id: 3 },
	%{ stop: nil, id: 2, start: seven_seconds_ago, timer_id: 4 }
	]
	iex> map_timer_diff(list)
	%{0 => 0, 1 => 6, 2 => 5, 3 => 24, 4 => 7}
	"""
	def map_timer_diff(list) do
	Map.new(list, fn item ->
	if is_nil(item.timer_id) do
	# item without any active timer
	{0, 0}
	else
	{item.timer_id, timer_diff(item)}
	end
	end)
	end
	@doc """
	`timer_diff/1` calculates the difference between timer.stop and timer.start
	If there is no active timer OR timer has not ended return 0.
	The reasoning is: an *active* timer (no end) does not have to
	be subtracted from the timer.start in the UI ...
	Again, DRAGONS!
	"""
	def timer_diff(timer) do
	# ignore timers that have not ended (current timer is factored in the UI!)
	if is_nil(timer.stop) do
	0
	else
	NaiveDateTime.diff(timer.stop, timer.start)
	end
	end
	@doc """
	`accumulate_item_timers/1` aggregates the elapsed time
	for all the timers associated with an item
	and then subtract that time from the start value of the *current* active timer.
	This is done to create the appearance that a single timer is being started/stopped
	when in fact there are multiple timers in the backend.
	For MVP we *could* have just had a single timer ...
	and given the "ugliness" of this code, I wish I had done that!!
	But the "USP" of our product [IMO] is that
	we can track the completion of a task across multiple work sessions.
	And having multiple timers is the *only* way to achieve that.
	If you can think of a better way of achieving the same result,
	please share: https://github.com/dwyl/app-mvp-phoenix/issues/103
	This function *relies* on the list of items being orderd by timer_id ASC
	because it "pops" the last timer and ignores it to avoid double-counting.
	"""
	def accumulate_item_timers(items_with_timers) do
	# e.g: %{0 => 0, 1 => 6, 2 => 5, 3 => 24, 4 => 7}
	timer_id_diff_map = map_timer_diff(items_with_timers)
	# e.g: %{1 => [2, 1], 2 => [4, 3], 3 => []}
	item_id_timer_id_map =
	Map.new(items_with_timers, fn i ->
	{i.id,
	Enum.map(items_with_timers, fn it ->
	if i.id == it.id, do: it.timer_id, else: nil
	end)
	# stackoverflow.com/questions/46339815/remove-nil-from-list
	|> Enum.reject(&is_nil/1)}
	end)
	# this one is "wasteful" but I can't think of how to simplify it ...
	item_id_timer_diff_map =
	Map.new(items_with_timers, fn item ->
	timer_id_list = Map.get(item_id_timer_id_map, item.id, [0])
	# Remove last item from list before summing to avoid double-counting
	{_, timer_id_list} = List.pop_at(timer_id_list, -1)
	{item.id,
	Enum.reduce(timer_id_list, 0, fn timer_id, acc ->
	Map.get(timer_id_diff_map, timer_id) + acc
	end)}
	end)
	# creates a nested map: %{ item.id: %{id: 1, text: "my item", etc.}}
	Map.new(items_with_timers, fn item ->
	time_elapsed = Map.get(item_id_timer_diff_map, item.id)
	start =
	if is_nil(item.start),
	do: nil,
	else: NaiveDateTime.add(item.start, -time_elapsed)
	{item.id, %{item | start: start}}
	end)
	# Return the list of items without duplicates and only the last/active timer:
	|> Map.values()
	# Sort list by item.id descending (ordered by timer_id ASC above) so newest item first:
	|> Enum.sort_by(fn i -> i.id end, :desc)
	end
	end

I think the functions linked to the timers are correct and I misunderstood the structure

[nelsonic]

@SimonLab yeah, the disclaimer was my attempt to warn you. 🤪
If you can wrap your head around it, I’m very happy for you to attempt to simplify it. But it needs to be time-boxed. 👌🏻

[nelsonic]

@SimonLab if you have time to review this please go for it. 🤞