[PR] Edit `timer`

[LuchoTurtle]

Should close #195

Still a work in progress. The user when editing an item will see a list of timers that he can edit.

[LuchoTurtle]

Even though this feature should be and is super simple to implement, I'm getting the go around implementing proper form validation and getting updated structs from the LiveView to the Phoenix server so I can update timers with their IDs.

This shouldn't be hard but it's weird because the way form is implemented when editing an item should also work on this occasion but I'm getting 0 events in the Phoenix server 😞 .

I'll keep trying

[nelsonic]

@LuchoTurtle thanks for opening this PR to track/share your progress! 🎉 Don’t worry about something not working first time. That’s normal. Just keep sharing your progress/frustrations and if you hit a brick wall, give as much detail as possible so one of us can help you figure it out. 👌

[LuchoTurtle]

Slow progress so far, from the user story perspective. Since I was practically obliged to use Bluzkly's datetime parser gist (even more so if we are in the future wanting to accommodate other formats), since I was stuck with sending error feedback from the server to the form, I tried to focus on testing what I had done.

Since I have to test this parser, I had to battle a bit with VSCode to try and get debugging working but I never did (got this The database for App.Repo couldn't be dropped: ERROR 55006 (object_in_use) database "app_test" is being accessed by other users error), so I've just been Logger.debuging my way through. It's taking a lot more than expected, it's hard to get code coverage on edge cases...

[nelsonic]

Please don't worry about things taking longer than you expected. ⏳ Just stay focussed on learning, capturing your learning 📝 and the feature will be a byproduct of that learning. 🪄 Very few companies/orgs think this way; we do! 👌 remember Logger.debug --> dbg/1

If you want to remote pair on anything please LMK (I'm always available!) 👨‍💻 But equally I understand the merits of "figuring it out for yourself" ... 😉

[LuchoTurtle]

Just finished all possible test coverage. The two lines that are missing relate to the final stretch: feedback to user and a peculiar situation that happens when I broadcast the new items list to the rest of the users. The timer form appears to duplicate after broadcasting 🤔

[codecov[bot]]

Codecov Report

Merging #196 (38b26ae) into main (f5bd6f1) will decrease coverage by 0.34%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              main     #196      +/-   ##
===========================================
- Coverage   100.00%   99.65%   -0.35%     
===========================================
  Files           13       13              
  Lines          221      292      +71     
===========================================
+ Hits           221      291      +70     
- Misses           0        1       +1     

Impacted Files	Coverage Δ
lib/app/item.ex	100.00% <ø> (ø)
lib/app/timer.ex	100.00% <100.00%> (ø)
lib/app_web/live/app_live.ex	98.90% <100.00%> (-1.10%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[LuchoTurtle]

Added all tests so coverage should all be sorted. Form validation upon submission should now work properly, with feedback to the user.

Experimented with a few options about schemaless changesets, nested form changesets but those weren't working properly. So I went with the route of adding a list of editing_timers (which is an array of Timer changesets) that are assigned to the socket dynamically whenever a user edits an item. This is better than the initial version that fetched all nested timers on mount, which could cause slow mount times in the future with a large amount of timers - the current approach is much more ad-hoc.

An index input field was also added. This is only for optimization purposes. If we know the index of the timer we want to edit within the changeset timer array, we can update the array with O(1) by just replacing the element through index access.

[LuchoTurtle]

Added some docs and changed the BUILD file with updated information 😄 .

[nelsonic]

Ran:

git checkout edit-timer
mix deps.get
mix c

Got:

Compiling 29 files (.ex)
warning: variable "e" is unused (if the variable is not meant to be used, prefix it with an underscore)
  lib/app_web/live/app_live.ex:214: AppWeb.AppLive.handle_event/3

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def handle_event/3" was previously defined (lib/app_web/live/app_live.ex:33)
  lib/app_web/live/app_live.ex:164

warning: module attribute @nodoc was set but never used
  lib/app_web/live/app_live.ex:132

Generated app app

08:44:18.521 [info] extension "citext" already exists, skipping
warning: function create_profile_with_no_name/1 is unused
  test/app_web/controllers/profile_controller_test.exs:75

.warning: variable "person" is unused (if the variable is not meant to be used, prefix it with an underscore)
  test/app_web/controllers/tag_controller_test.exs:97: AppWeb.TagControllerTest.create_person/1

warning: variable "now" is unused (if the variable is not meant to be used, prefix it with an underscore)
  test/app_web/live/app_live_test.exs:140: AppWeb.AppLiveTest."test update an item's timer"/1

I can address these as part of my review or you can tidy up the warnings and re-assign to me. 💭

[LuchoTurtle]

I got it, thanks for the feedback 👍

[LuchoTurtle]

Should be fixed, sorry about that.

[nelsonic]

@LuchoTurtle nice one. Thanks for updating. I will continue reviewing after standup. :ok_hand:

[LuchoTurtle]

Just pushed an overlap validation on the timer being updated with the others of the same item. I will get on with having the stop field not required and being possible to edit 👌

[LuchoTurtle]

@nelsonic I've added the feature and the referring tests you asked for. However, I won't ask you to review it just yet, I want to make it prettier (as you mentioned at the standup) and make it possible to delete timers. 👌

[nelsonic]

@LuchoTurtle tidying the UI/UX can be done in a subsequent iteration. ✨ And deleting timers isn't actually something we want at this point. 🔪 It could certainly be a feature in the future. But right now none of our "Alpha" testers have requested it. 💭

[LuchoTurtle]

@nelsonic fair enough. Though I think deleting should be a part of the whole "editing timers" panorama. Sometimes it's easier to delete a timer when editing one, making it is much faster to manage overlapping scenarios.

It's your call though 🎉

[nelsonic]

@LuchoTurtle please open an issue to describe the use-case/need for deleting timers. 🆕 We've not had the feature/requirement described anywhere (yet) so I'm curious what the real-world scenario would be. 💭 Going to take another look at the PR now. 👌

[LuchoTurtle]

@nelsonic I don't know why you're reviewing the code right now. Before the discussion about deleting timers, I was going to refactor the code before submitting it for review (hence why I still had the "in-progress" label :P), since I know a chunk of the business logic was in the app_live.ex file. I thank you for the feedback nonetheless. 🎉

[nelsonic]

Ah, apologies. I didn't expect anyone to be online at 22:00 on a Saturday evening. 💭 Assigning back to you. 👤 Will get some sleep instead. 👌

[LuchoTurtle]

@nelsonic just updated the refactored code, with the addition that now the user knows whether it start or stop is invalid. Error handling should be much more readable and the business logic should be well-placed. Tests and build.md file are both also updated :)

[LuchoTurtle]

Just committed the changes requested 😄

[nelsonic]

When attempting to edit a timer, getting the following error in console:

[error] GenServer #PID<0.728.0> terminating
** (FunctionClauseError) no function clause matching in NaiveDateTime.compare/2
    (elixir 1.14.1) lib/calendar/naive_datetime.ex:1025: NaiveDateTime.compare(~N[2022-11-16 15:40:36], nil)
    (app 1.0.0) lib/app/timer.ex:205: anonymous fn/4 in App.Timer.update_timer_inside_changeset_list/3
    (elixir 1.14.1) lib/enum.ex:2468: Enum."-reduce/3-lists^foldl/2-0-"/3
    (app 1.0.0) lib/app/timer.ex:199: App.Timer.update_timer_inside_changeset_list/3
    (app 1.0.0) lib/app_web/live/app_live.ex:143: AppWeb.AppLive.handle_event/3
    (phoenix_live_view 0.18.2) lib/phoenix_live_view/channel.ex:382: anonymous fn/3 in Phoenix.LiveView.Channel.view_handle_event/3
    (telemetry 1.1.0) /mvp/deps/telemetry/src/telemetry.erl:320: :telemetry.span/3
    (phoenix_live_view 0.18.2) lib/phoenix_live_view/channel.ex:216: Phoenix.LiveView.Channel.handle_info/2
    (stdlib 4.1.1) gen_server.erl:1123: :gen_server.try_dispatch/4
    (stdlib 4.1.1) gen_server.erl:1200: :gen_server.handle_msg/6
    (stdlib 4.1.1) proc_lib.erl:250: :proc_lib.wake_up/3

Can show you on Zoom if you have time. 🧑‍💻

[LuchoTurtle]

Sure thing, I'm joining the standup link

[LuchoTurtle]

Apparently, this bug happens when I'm editing a timer and try to edit an old timer. Additionally, I ought to check updating the cumulative timer count after updating.

[nelsonic]

Editing times is mostly working for me but I occasionally get these errors:

[error] GenServer #PID<0.722.0> terminating
** (FunctionClauseError) no function clause matching in NaiveDateTime.compare/2
    (elixir 1.14.1) lib/calendar/naive_datetime.ex:1025: NaiveDateTime.compare(~N[2022-11-16 17:21:07], nil)
    (app 1.0.0) lib/app/timer.ex:205: anonymous fn/4 in App.Timer.update_timer_inside_changeset_list/3
    (elixir 1.14.1) lib/enum.ex:2468: Enum."-reduce/3-lists^foldl/2-0-"/3
    (app 1.0.0) lib/app/timer.ex:199: App.Timer.update_timer_inside_changeset_list/3
    (app 1.0.0) lib/app_web/live/app_live.ex:143: AppWeb.AppLive.handle_event/3
    (phoenix_live_view 0.18.2) lib/phoenix_live_view/channel.ex:382: anonymous fn/3 in Phoenix.LiveView.Channel.view_handle_event/3
    (telemetry 1.1.0) /mvp/deps/telemetry/src/telemetry.erl:320: :telemetry.span/3
    (phoenix_live_view 0.18.2) lib/phoenix_live_view/channel.ex:216: Phoenix.LiveView.Channel.handle_info/2
    (stdlib 4.1.1) gen_server.erl:1123: :gen_server.try_dispatch/4
    (stdlib 4.1.1) gen_server.erl:1200: :gen_server.handle_msg/6
    (stdlib 4.1.1) proc_lib.erl:240: :proc_lib.init_p_do_apply/3

The UI does not display an error and everything appears to continue working. So from the person using the App's perspective it appears to work. 👌 Just want to make sure this error condition doesn't get committed to main and then we end up with tech-debt to pay. 💭

[nelsonic]

Thanks for taking a look at this. 👌

[LuchoTurtle]

I pushed the tests to 100% coverage @nelsonic . It seems this commit broke some auth tests :/

[nelsonic]

Working in prod: https://mvp.fly.dev 🚀

Thanks again @LuchoTurtle 🙌