Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PR] Edit timer #196

Merged
merged 55 commits into from
Nov 22, 2022
Merged

[PR] Edit timer #196

merged 55 commits into from
Nov 22, 2022

Conversation

LuchoTurtle
Copy link
Member

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 LuchoTurtle added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person elixir Pull requests that update Elixir code labels Oct 28, 2022
@LuchoTurtle LuchoTurtle self-assigned this Oct 28, 2022
@LuchoTurtle
Copy link
Member Author

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

@LuchoTurtle LuchoTurtle marked this pull request as draft October 28, 2022 10:39
@nelsonic
Copy link
Member

@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. 👌

Adding parser and error handling. Broadcasting doesn't work properly.
@LuchoTurtle LuchoTurtle temporarily deployed to dwylauth October 31, 2022 10:53 Inactive
@LuchoTurtle
Copy link
Member Author

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
Copy link
Member

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" ... 😉

The missing two lines are dependent on the feedback to the form and broadcast of information.
@LuchoTurtle
Copy link
Member Author

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 🤔

@LuchoTurtle LuchoTurtle added the in-progress An issue or pull request that is being worked on by the assigned person label Nov 16, 2022
@nelsonic
Copy link
Member

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
Copy link
Member

Thanks for taking a look at this. 👌

@nelsonic nelsonic added technical A technical issue that requires understanding of the code, infrastructure or dependencies priority-1 Highest priority issue. This is costing us money every minute that passes. user-feedback Feedback from people using the App labels Nov 17, 2022
@LuchoTurtle LuchoTurtle temporarily deployed to dwylauth November 19, 2022 21:17 Inactive
@LuchoTurtle LuchoTurtle temporarily deployed to dwylauth November 19, 2022 21:42 Inactive
@LuchoTurtle LuchoTurtle temporarily deployed to dwylauth November 21, 2022 10:10 Inactive
@LuchoTurtle
Copy link
Member Author

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

@LuchoTurtle LuchoTurtle requested a review from nelsonic November 21, 2022 13:28
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Nov 21, 2022
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Nov 21, 2022
@nelsonic nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Nov 22, 2022
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these update @LuchoTurtle 👌
Edit Timer MVP Looking good. Might require some UI/UX iteration. 💭
But let's get some feedback from @iteles & @Stephanymtr ASAP. :shipit:

@nelsonic nelsonic merged commit 91d76ec into main Nov 22, 2022
@nelsonic nelsonic deleted the edit-timer branch November 22, 2022 00:57
@nelsonic
Copy link
Member

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

image

Thanks again @LuchoTurtle 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or enhancement of existing functionality in-review Issue or pull request that is currently being reviewed by the assigned person priority-1 Highest priority issue. This is costing us money every minute that passes. technical A technical issue that requires understanding of the code, infrastructure or dependencies user-feedback Feedback from people using the App
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Feat: Edit Timer! [MVP] 📝
2 participants