dwyl / mvp

📲 simplest version of the @dwyl app
https://mvp.fly.dev
GNU General Public License v2.0
87 stars 2 forks source link

[PR] Adjusting timer shown to user according to timezone. #354

Closed LuchoTurtle closed 1 year ago

LuchoTurtle commented 1 year ago

closes #353

This took much longer than expected mainly because of the strategies that I tried to employ. These are detailed in the comments below.

This PR essentially adjusts the timezone for the user on the client side and maintains UTC-based datetimes on the server to maintain consistency across all timer objects. The client sends the timezone during the mounting phase and is set as a socket assign within the LiveView.

LuchoTurtle commented 1 year ago

To tackle this, I thought it would make sense to preserve the UTC in the server and only show the user the date time according to the browser’s timezone. This entails that whenever a user updates a timer, the UTC equivalent is saved in the database. So between the client and the server (inbound and outbound), there must be a conversion from UTC to the local timezone.

This local timezone depends on the browser and thus will eventually be sent to the liveview from the client.

First strategy

Initially, to do this, I checked a few resources:

I realised I could use a mix of on_mount on the LiveView and create a hook that would be called with on_mount.

Inside this hook, we can use attach_hook to try and assign this local timezone to the socket assigns list.

However, after trying several times to get this to work and pushing events from app.js, I realised that this operation of assigning the local_timezone to the socket assigns from the client was not made exactly on mount. There would be a split second where the component would be re-mounted with the socket assigns properly set.

Although this behaviour was sufficient, I felt it was incorrect. And apparently I wasn’t alone (as shown in https://thepugautomatic.com/2020/05/persistent-session-data-via-localstorage-in-phoenix-liveview/).

Second strategy

So after spending unnecessarily amount of time for this to work, I luckily stumbled upon get_connected_params/1. Wish I had found this earlier, because it was exactly what I needed.

Having scraped the previous method, I went with going for this on the on_mount function within app_live.ex by doing…

hours_offset_fromUTC: get_connect_params(socket)["hours_offset_fromUTC"] || 0

…in the socket assigns. It worked fine.

Why second strategy works and what went wrong

I then tried to pass this new socket assigns hours_offset_fromUTC to the list_timers_changesets/1 inside the timer.ex so I could return the list of Timer changesets with the the updated date times (incrementing/decrementing the number of hours according to the timezone).

However, I wasn’t able change the data property inside the changeset:

Since these weren’t working, I simply had to put some logic inside app_live.html.heex to show the user the correct date time and then change back the timezone when updating the timers.

LuchoTurtle commented 1 year ago

This PR is still in draft because, although everything works when I manually test whilst using the app, I can't seem to reproduce the behaviour while unit testing.

I'm trying to have LiveViewTest assign hours_offset_fromUTC in the socket assigns during the unit test but it's getting ignored...

nelsonic commented 1 year ago

@LuchoTurtle thanks for the the detailed comments on this PR and on the issue #353 đź‘Ś If you need to make changes to the DB please LMK by linking to a comment on Signal so that I can reply fast. đź‘Ť

LuchoTurtle commented 1 year ago

It seems my conundrum has occurred to someone else... https://elixirforum.com/t/how-do-i-test-my-liveview/39344. No responses though :/

codecov[bot] commented 1 year ago

Codecov Report

Merging #354 (0636039) into main (50a68f6) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #354   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          475       480    +5     
=========================================
+ Hits           475       480    +5     
Impacted Files Coverage Δ
lib/app/timer.ex 100.00% <100.00%> (ø)
lib/app_web/live/app_live.ex 100.00% <100.00%> (ø)

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

LuchoTurtle commented 1 year ago

This should be reviewable now.

LuchoTurtle commented 1 year ago

Hey @nelsonic, do I need to resolve the conflicts for you to review this and get this merged?

nelsonic commented 1 year ago

@LuchoTurtle thanks for the ping. I'm just trying to figure out something that I have a bunch of tabs/windows open for right now and will review this next. đź‘Ś

nelsonic commented 1 year ago

If you have a couple of mins please fix the merge-conflicts and re-assign. 🙏

LuchoTurtle commented 1 year ago

@nelsonic should be mergeable now đź‘Ś

nelsonic commented 1 year ago

https://everytimezone.com

every timezone image image
nelsonic commented 1 year ago

Sorry didn't mean to close the PR. 🤦‍♂️ Just wanted to include those screenshots as a comment.

nelsonic commented 1 year ago

@LuchoTurtle for future reference (not just on @dwyl projects ...), the PR is not reviewable if the CI checks aren't passing.

image

In this case it's the Review App that is failing: https://github.com/dwyl/mvp/actions/runs/5023363276/jobs/9015775607#step:5:1493

image

Looks like we need a tech-debt issue for this: https://github.com/dwyl/mvp/issues/373

LuchoTurtle commented 1 year ago

I never submit for review without checking the action status. When I pushed for review, 100% it was passing, or else I'd get an email saying it wasn't. I don't know if it stopped working because of other commits being made to the main branch that re-ran the actions here and made them fail but they were passing when I made the commit, especially after mix format commit