Feat: Edit Timer! [MVP] 📝

[nelsonic]

Following on from #190 where you were able to run the MVP on localhost 🎉 (and update the docs! 🙌)
You should feel confident enough in you knowledge of the project and Phoenix/LiveView skills
to create a new feature that has been sorely lacking from the MVP. ⏳

This feature is described in more detail in the issue: dwyl/app#282
But the gist from the MVP's perspective is the following:

Todo

• When a person enters the edit mode for an item:

Should:

• display a list of the past timers for that item:

• Editing the timer.start or timer.stop in the UI and clicking/tapping the Update button will:
  • Validate that both the timer.start and timer.stop are valid Date + Time
    i.e: sanitise and attempt to parse the data
  • Confirm that the timer.stop is not before the timer.start i.e. invalid!
  • Save the data

    Note: this is where having a complete history of changes is important to avoid too much "Time Travelling" ...
    Hence: SPIKE: PaperTrail ? technology-stack#106
    But for MVP we don't care what happens we just want to test the simplest implementation of the feature.

• Timers should not be allowed to overlap.
  i.e: the second timer cannot have timer.start set to a date+time earlier than the timer.stop of the first timer.

We can add fancy UI/UX for timer input later for now, just text is fine for MVP/Alpha!

Note: @SimonLab was looking at editing/improving the Timer Functions in #136
We should not need to update those yet (though we very much want to refactor them soon cause my code is 🤮 ...)

@LuchoTurtle

• Please confirm you understand the requirement. 💬 (comment on this issue!)
• Create a new branch on your localhost with the tests, update to BUILDIT.md and code 🆕
• Once you have anything worth committing , git push and 💻
• Create a PR e.g: "PR: Edit Timers Feat: Edit Timer! [MVP] 📝  #195` 🆕
• Keep it assigned to yourself and in-progress until you feel it's in a reviewable state. 🧑‍💻

[nelsonic]

@LuchoTurtle busy reviewing your PR #196
I'm unable to edit the timer.start on my localhost in the UI. 😕 (it's probably me ...)
I've recorded a video but it's too big to upload to GitHub ...
Currently downloading DaVinci Resolve to my MBP to trim and speed up the video. ⏳
But if you have time to sit with me this evening or tomorrow morning, LMK. 👌

[LuchoTurtle]

@nelsonic show me tomorrow morning. It should be fixeable. I've been so into Flutter and 99% close to finishing it. It was super worth it though, the other two issues you've assigned to me for the examples should be much easier after I'm done with learn-flutter.

Sorry for taking this long but I had to do a complete overhaul and added much more info and retained as much as possible..

I digress. No need to send a video. Demo me tomorrow on standup or before and I'll take a look at it 👉😎👉

[nelsonic]

Cool. Will show you IRL or on Standup and we can walk through it. 👌

[nelsonic]

As discussed on standup this morning ... 💬
Editing the current (active) timer was not one of the original requirements. ⬆️
So 100% agree that you couldn't guess that it was a desirable feature. 👌
But I'm fairly certain that our BDFL @iteles will want that functionality 💭
and other people will too. [hypothesis...] 🧑‍🤝‍🧑

So we shouldn't require a timer.end to be defined in order to update the running timer:

LMK if this is clear. 🙏

[LuchoTurtle]

@nelsonic didn't get to have the timer editing with stop at nil working because I noticed that I should be harsher on the verifications. I added a validation that checks if the timer that the user is verifying overlaps with any of the others. It errors out if it does.
I personally think this is important for calculating the accumulating seconds, which could be thrown off if the editing was free-for-allish.

[nelsonic]

Great spot! That should absolutely have been a requirement/acceptance criteria in the OP above! 💭 (adding it now!)

[nelsonic]

Concerned that we're including quite a few hundred lines of Library/Utility Code for Date/Time Parsing in the MVP:

• https://github.com/dwyl/mvp/blame/356d69a2cf5a9e5459aef10089af6876628851fe/BUILDIT.md#L2774-L3080

• mvp/lib/app/datetime_parser.ex

  Lines 1 to 196 in 356d69a

  	# Full credit of this module goes to https://dev.to/onpointvn/build-your-own-date-time-parser-in-elixir-50be
  	# Do check the gist -> https://gist.github.com/bluzky/62a20cdb57b17f47c67261c10aa3da8b
  	defmodule App.DateTimeParser do
  	@mapping %{
  	"H" => "(?<hour>\\d{2})",
  	"I" => "(?<hour12>\\d{2})",
  	"M" => "(?<minute>\\d{2})",
  	"S" => "(?<second>\\d{2})",
  	"d" => "(?<day>\\d{2})",
  	"m" => "(?<month>\\d{2})",
  	"y" => "(?<year2>\\d{2})",
  	"Y" => "(?<year>-?\\d{4})",
  	"z" => "(?<tz>[+-]?\\d{4})",
  	"Z" => "(?<tz_name>[a-zA-Z_\/]+)",
  	"p" => "(?<p>PM|AM)",
  	"P" => "(?<P>pm|am)",
  	"%" => "%"
  	}
  	@doc """
  	Parse string to datetime struct
  	**Example**
  	parse("2021-20-10", "%Y-%M-%d")
  	Support format
  	| format | description| value example |
  	| -- | -- | -- |
  	| H | 24 hour | 00 - 23 |
  	| I | 12 hour | 00 - 12 |
  	| M | minute| 00 - 59 |
  	| S | second | 00 - 59 |
  	| d | day | 01 - 31 |
  	| m | month | 01 -12 |
  	| y | 2 digits year | 00 - 99 |
  	| Y | 4 digits year | |
  	| z | timezone offset | +0100, -0330 |
  	| Z | timezone name | UTC+7, Asia/Ho_Chi_Minh |
  	| p | PM or AM | |
  	| P | pm or am | |
  	"""
  	def parse!(dt_string, format \\ "%Y-%m-%dT%H:%M:%SZ") do
  	case parse(dt_string, format) do
  	{:ok, dt} ->
  	dt
  	{:error, message} ->
  	raise "Parse string #{dt_string} with error: #{message}"
  	end
  	end
  	@doc """
  	Parses the string according to the format. Pipes through regex compilation, casts each part of the string to a named regex capture and tries to convert to datetime.
  	"""
  	def parse(dt_string, format \\ "%Y-%m-%dT%H:%M:%SZ") do
  	format
  	|> build_regex
  	|> Regex.named_captures(dt_string)
  	|> cast_data
  	|> to_datetime
  	end
  	@doc """
  	Builds the regex expression to later be captured (have named key-value captures).any()
  	This uses the @mapping structure to name specific parts of the entered string to convert to datetime.
  	"""
  	def build_regex(format) do
  	keys = Map.keys(@mapping) |> Enum.join("")
  	Regex.compile!("([^%]*)%([#{keys}])([^%]*)")
  	|> Regex.scan(format)
  	|> Enum.map(fn [_, s1, key, s2] ->
  	[s1, Map.get(@mapping, key), s2]
  	end)
  	|> to_string()
  	|> Regex.compile!()
  	end
  	@default_value %{
  	day: 1,
  	month: 1,
  	year: 0,
  	hour: 0,
  	minute: 0,
  	second: 0,
  	utc_offset: 0,
  	tz_name: "UTC",
  	shift: "AM"
  	}
  	def cast_data(nil), do: {:error, "invalid datetime"}
  	@doc """
  	Casts each capture of the regex to appropriated format (compatible with DateTime struct)
  	"""
  	def cast_data(captures) do
  	captures
  	|> Enum.reduce_while([], fn {part, value}, acc ->
  	{:ok, data} = cast(part, value)
  	{:cont, [data | acc]}
  	end)
  	|> Enum.into(@default_value)
  	end
  	@value_rages %{
  	"hour" => [0, 23],
  	"hour12" => [0, 12],
  	"minute" => [0, 59],
  	"second" => [0, 59],
  	"day" => [0, 31],
  	"month" => [1, 12],
  	"year2" => [0, 99]
  	}
  	defp cast("P", value) do
  	cast("p", String.upcase(value))
  	end
  	defp cast("p", value) do
  	{:ok, {:shift, value}}
  	end
  	defp cast("tz", value) do
  	{hour, minute} = String.split_at(value, 3)
  	with {:ok, {_, hour}} <- cast("offset_h", hour),
  	{:ok, {_, minute}} <- cast("offset_m", minute) do
  	sign = div(hour, abs(hour))
  	{:ok, {:utc_offset, sign * (abs(hour) * 3600 + minute * 60)}}
  	end
  	end
  	defp cast("tz_name", value) do
  	{:ok, {:tz_name, value}}
  	end
  	defp cast(part, value) do
  	value = String.to_integer(value)
  	valid =
  	case Map.get(@value_rages, part) do
  	[min, max] ->
  	value >= min and value <= max
  	_ ->
  	true
  	end
  	if valid do
  	{:ok, {String.to_atom(part), value}}
  	end
  	end
  	defp to_datetime({:error, _} = error), do: error
  	defp to_datetime(%{year2: value} = data) do
  	current_year = DateTime.utc_now() |> Map.get(:year)
  	year = div(current_year, 100) * 100 + value
  	data
  	|> Map.put(:year, year)
  	|> Map.delete(:year2)
  	|> to_datetime()
  	end
  	defp to_datetime(%{hour12: hour} = data) do
  	# 12AM is not valid
  	if hour == 12 and data.shift == "AM" do
  	{:error, "12AM is invalid value"}
  	else
  	hour =
  	cond do
  	hour == 12 and data.shift == "PM" -> hour
  	data.shift == "AM" -> hour
  	data.shift == "PM" -> hour + 12
  	end
  	data
  	|> Map.put(:hour, hour)
  	|> Map.delete(:hour12)
  	|> to_datetime()
  	end
  	end
  	defp to_datetime(data) do
  	with {:ok, date} <- Date.new(data.year, data.month, data.day),
  	{:ok, time} <- Time.new(data.hour, data.minute, data.second),
  	{:ok, datetime} <- DateTime.new(date, time) do
  	datetime = DateTime.add(datetime, -data.utc_offset, :second)
  	if data.tz_name != "UTC" do
  	{:error, "Only UTC timezone is available"}
  	else
  	{:ok, datetime}
  	end
  	end
  	end
  	end

• mvp/test/app/datetime_parser_test.exs

  Lines 1 to 81 in 356d69a

  	defmodule App.DateTimeParserTest do
  	use App.DataCase
  	alias App.DateTimeParser
  	test "valid parse of valid datetime" do
  	parsed_time =
  	DateTimeParser.parse!("2022-10-27 14:47:56", "%Y-%m-%d %H:%M:%S")
  	{:ok, expected_datetime, 0} = DateTime.from_iso8601("2022-10-27T14:47:56Z")
  	assert parsed_time == expected_datetime
  	end
  	test "valid parse of valid date with %Y-%m-%d format" do
  	parsed_time = DateTimeParser.parse!("2022-10-27", "%Y-%m-%d")
  	{:ok, expected_datetime, 0} = DateTime.from_iso8601("2022-10-27T00:00:00Z")
  	assert parsed_time == expected_datetime
  	end
  	test "non-compatible regex when parsing" do
  	assert_raise Regex.CompileError, fn ->
  	DateTimeParser.parse!("2022-10-27 14:47:56", "%Y-%Y-%Y")
  	end
  	end
  	test "invalid datetime format" do
  	assert_raise RuntimeError, fn ->
  	DateTimeParser.parse!("2022-102-2752 1423:4127:56", "%Y-%m-%d %H:%M:%S")
  	end
  	end
  	test "valid timezone offset (with tz)" do
  	parsed_date =
  	DateTimeParser.parse!("2022-10-27T00:00:00Z+0230", "%Y-%m-%dT%H:%M:%SZ%z")
  	{:ok, expected_datetime, 9000} =
  	DateTime.from_iso8601("2022-10-27T00:00:00+02:30")
  	assert parsed_date == expected_datetime
  	end
  	test "valid timezone offset (with UTC)" do
  	parsed_date =
  	DateTimeParser.parse!(
  	"2022-10-27T00:00:00ZUTC+0230",
  	"%Y-%m-%dT%H:%M:%SZ%Z"
  	)
  	assert parsed_date == ~U[2022-10-27 00:00:00Z]
  	end
  	test "invalid timezone name" do
  	assert_raise RuntimeError, fn ->
  	DateTimeParser.parse!(
  	"2022-10-27T00:00:00ZEtc+0230",
  	"%Y-%m-%dT%H:%M:%SZ%Z"
  	)
  	end
  	end
  	test "valid datetime with PM/AM" do
  	date_under = DateTimeParser.parse!("2022-10-27 06:02pm", "%Y-%m-%d %H:%M%P")
  	date_sup = DateTimeParser.parse!("2022-10-27 06:02PM", "%Y-%m-%d %H:%M%p")
  	assert date_under == date_sup
  	end
  	test "valid datetime with PM/AM with two digits" do
  	parsed_datetime =
  	DateTimeParser.parse!("2022-10-27 06:02pm", "%Y-%m-%d %I:%M%P")
  	assert parsed_datetime == ~U[2022-10-27 18:02:00Z]
  	end
  	test "valid datetime with two-digit year" do
  	parsed_date = DateTimeParser.parse!("10-10-27", "%y-%m-%d")
  	assert parsed_date == ~U[2010-10-27 00:00:00Z]
  	end
  	end

If we need to write our own date/time parsing library this should be captured somewhere as a comment in this issue.
Not against writing our own parser, just need to know which requirements we had/have that aren't met by an existing library. Because all code has to be maintained which means we pay "rent" for each line we write. 💭

[LuchoTurtle]

The reason I decided to not use Timex is that I didn't think it was worth it to import a whole library just for the sole purpose of parsing a date on a specified format. I tried to keep things lightweight on this one.

However, you raise a good point about having to "pay rent" for each line we write. I thought it'd be a good idea to have a small parser util instead of importing a fully-fledged library just to parse strings into DateTimes. But since this is a point of concern, I'll switch to Timex 👍 (shame those tests will go to waste though haha 😂 )

[nelsonic]

@LuchoTurtle to be clear, I'm not at all against writing Library Code. We do it all the time, e.g: https://github.com/dwyl/useful
In fact we have a Date function on our backlog: dwyl/useful#13

The only reason I'm flagging the addition of DateTimeParser in the MVP is that this kind of date-parsing code is not business logic. If we need to write a custom date parser then we should 100% capture all the requirements and write our own Library, e.g: https://hex.pm/packages/dateutils

But if we writing a parser just because we didn't do enough research into the available packages then "That's a Nicht Nicht" 😜

Again, perfectly happy for us to have our own DateUtils if we genuinely need it.
Even if it only has one function to start with, that's fine.

[nelsonic]

Importing a library for a single function in the MVP is fine provided it meets our exact needs. ✅
The Elixir/Erlang compiler will tree-shake the rest so it's efficient. 👌