search

teslamate-org / teslamate

A self-hosted data logger for your Tesla 🚘
https://docs.teslamate.org
MIT License
6.04k stars 752 forks source link

feat: update to Phoenix HTML 4.1, bump dependencies #4277

Closed JakobLichterfeld closed 3 weeks ago

netlify[bot] commented 1 month ago

Deploy Preview for teslamate ready!

Name Link
Latest commit 91f23a32b84e91cc851ab6b22a2f827dd79e7fe2
Latest deploy log https://app.netlify.com/sites/teslamate/deploys/671de659c9e73d00080dc5d0
Deploy Preview https://deploy-preview-4277--teslamate.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

JakobLichterfeld commented 1 month ago

Thanks @sdwalker for your changes!

brianmay commented 1 month ago

Tests failing?

JakobLichterfeld commented 1 month ago

Tests failing?

Yeah, I assume this is the cause:

warning: defining a Gettext backend by calling

    use Gettext, otp_app: ...

is deprecated. To define a backend, call:

    use Gettext.Backend, otp_app: :my_app

Then, instead of importing your backend, call this in your module:

    use Gettext, backend: MyApp.Gettext

  lib/teslamate_web/gettext.ex:23: TeslaMateWeb.Gettext (module)
sdwalker commented 1 month ago

The mix files need cleaned to only have the phoenix changes and not include the rest of the unrelated package updates

The gettext warnings are simple fixes when updating gettext

The dependencies that aren't keeping up with package releases (websockex, timex) are another source of warnings Bulma can be updated to 1.0.2 if you ignore the slew of deprecation warnings but their devs are keeping up

JakobLichterfeld commented 1 month ago

Yeah, I assume this is the cause:

warning: defining a Gettext backend by calling

    use Gettext, otp_app: ...

is deprecated. To define a backend, call:

    use Gettext.Backend, otp_app: :my_app

Then, instead of importing your backend, call this in your module:

    use Gettext, backend: MyApp.Gettext

  lib/teslamate_web/gettext.ex:23: TeslaMateWeb.Gettext (module)

I fixed it with the help of https://www.yellowduck.be/posts/fixing-the-gettext-warning-in-phoenix

brianmay commented 1 month ago

It looks like this function call https://github.com/teslamate-org/teslamate/blob/30a947a411a3ed7d54e7b1398632f31ce602bf8b/test/teslamate/vault_test.exs#L11 is returning a tuple, and we do not expect a tuple.

This puzzles me though, I don't see any changes here.

Oh, I think this change upgraded Clock, so we got this breaking change: https://github.com/danielberkompas/cloak/commit/267077ec8f1f260d3ad298e62b199a44b1a570b4.

Which is typical fashion wasn't documented as a breaking change :-(

https://github.com/danielberkompas/cloak/blob/master/CHANGELOG.md

brianmay commented 1 month ago

If I am reading the stack trace correctly, this is returning an error tuple: https://github.com/teslamate-org/teslamate/blob/30a947a411a3ed7d54e7b1398632f31ce602bf8b/lib/teslamate/locations.ex#L58

Which seems to indicate we are getting here somehow: https://github.com/teslamate-org/teslamate/blob/2fc749c8997feeacf8eb0a1aa3ed26253ce34f7f/test/support/mocks/geocoder.ex#L150

sdwalker commented 1 month ago

Yeah, I assume this is the cause:

warning: defining a Gettext backend by calling

    use Gettext, otp_app: ...

is deprecated. To define a backend, call:

    use Gettext.Backend, otp_app: :my_app

Then, instead of importing your backend, call this in your module:

    use Gettext, backend: MyApp.Gettext

  lib/teslamate_web/gettext.ex:23: TeslaMateWeb.Gettext (module)

I fixed it with the help of https://www.yellowduck.be/posts/fixing-the-gettext-warning-in-phoenix

They match the local changes I've been using

JakobLichterfeld commented 1 month ago

Oh, I think this change upgraded Clock, so we got this breaking change: danielberkompas/cloak@267077e.

Which is typical fashion wasn't documented as a breaking change :-(

https://github.com/danielberkompas/cloak/blob/master/CHANGELOG.md

Wow, nice catch. Ty! I downgraded Credo and it looks much better now.

JakobLichterfeld commented 1 month ago

2 Test failures which needs to be fixed for the upcoming release (this is the blocker):

  1) test language shows error (TeslaMateWeb.SettingsLiveTest)
Error:      test/teslamate_web/live/settings_test.exs:189
     ** (FunctionClauseError) no function clause matching in Floki.find/2

     The following arguments were given to Floki.find/2:

         # 1
         nil

         # 2
         "p.help"

     Attempted function clauses (showing 2 out of 2):

         def find(html, selector) when -is_binary(html)-
         def find(html_tree_as_tuple, selector) when -is_list(html_tree_as_tuple)- or -is_binary(html_tree_as_tuple)- or -tuple_size(html_tree_as_tuple) == 3- or -tuple_size(html_tree_as_tuple) == 2- and (-elem(html_tree_as_tuple, 0) === :pi- or -elem(html_tree_as_tuple, 0) === :comment-) or -tuple_size(html_tree_as_tuple) == 4- and -elem(html_tree_as_tuple, 0) == :doctype-

     code: TestHelper.eventually(fn ->
     stacktrace:
       (floki 0.36.2) lib/floki.ex:277: Floki.find/2
       test/teslamate_web/live/settings_test.exs:231: anonymous fn/2 in TeslaMateWeb.SettingsLiveTest."test language shows error"/1
       (teslamate 1.30.2-dev) test/support/test_helper.ex:7: TestHelper.eventually/3
       test/teslamate_web/live/settings_test.exs:210: (test)

................................

  2) test Edit validates changes when editing of a geo-fence (TeslaMateWeb.GeoFenceLiveTest)
Error:      test/teslamate_web/live/geofence_live_test.exs:112
     Assertion with == failed
     code:  assert error_html ==
              "<span class=\"help is-danger pl-15\" phx-feedback-for=\"geo_fence_#{kind}\">can't be blank</span>"
     left:  "<span class=\"help is-danger pl-15\" phx-feedback-for=\"geo_fence[name]\">can't be blank</span>"
     right: "<span class=\"help is-danger pl-15\" phx-feedback-for=\"geo_fence_name\">can't be blank</span>"
     stacktrace:
       (elixir 1.17.2) lib/enum.ex:1703: Enum."-map/2-lists^map/1-1-"/2
       test/teslamate_web/live/geofence_live_test.exs:150: (test)
JakobLichterfeld commented 1 month ago

first is related to: https://github.com/teslamate-org/teslamate/blob/2897640b4140732ec1a6a9026546e2a7ef6405b3/test/teslamate_web/live/settings_test.exs#L231

second is related to: https://github.com/teslamate-org/teslamate/blob/2897640b4140732ec1a6a9026546e2a7ef6405b3/test/teslamate_web/live/geofence_live_test.exs#L158

brianmay commented 1 month ago

I think there have been some minor changes in how field names are generated:

<div class="field is-expanded">
        <div class="control">
<input class="input" id="geo_fence_name" name="geo_fence[name]" placeholder="Name" type="text" value="">
        </div>

          <p class="help is-danger"><span class="help is-danger pl-15" phx-feedback-for="geo_fence[name]">can't be blank</span></p>

      </div>

What confused me for a while is that the phx-feedback-for points to the name not the id field, from the docs:

The phx-feedback-for annotation specifies the name (or id, for backwards compatibility) of the input it belongs to. Failing to add the phx-feedback-for attribute will result in displaying error messages for form fields that the user has not changed yet (e.g. required fields further down on the page).

https://hexdocs.pm/phoenix_live_view/form-bindings.html

That explains the 2nd error at least.

sdwalker commented 1 month ago

first is related to:

https://github.com/teslamate-org/teslamate/blob/2897640b4140732ec1a6a9026546e2a7ef6405b3/test/teslamate_web/live/settings_test.exs#L231

Breaks upgrading floki 0.35.2 => 0.35.3

sdwalker commented 1 month ago

0.19.0 (2023-05-29) Backwards incompatible changes

Drop support for passing an id to the phx-feedback-for attribute. An input name must be passed instead.
JakobLichterfeld commented 4 weeks ago

Breaks upgrading floki 0.35.2 => 0.35.3

We love it, when there are breaking changes in patch releases... Even if it's not mentioned in the changelog: https://hexdocs.pm/floki/changelog.html#0-35-3-2024-01-25

JakobLichterfeld commented 4 weeks ago

We love it, when there are breaking changes in patch releases... Even if it's not mentioned in the changelog: https://hexdocs.pm/floki/changelog.html#0-35-3-2024-01-25

Downgrading floki is not an option, as phoenix_live_view depend on 0.36+

JakobLichterfeld commented 4 weeks ago

I got rid of the second error, but quite sure this can be handled better. Pushes are welcome

Imo only the test was wrong, as we set phx_feedback_for correct with name instead of id: https://github.com/teslamate-org/teslamate/blob/844b7720c24180b5013ba1a300d6cff4a5df3e11/lib/teslamate_web/views/error_helpers.ex#L16

sdwalker commented 4 weeks ago

Test passes if the div match is removed

https://github.com/teslamate-org/teslamate/blob/c7fd300942fbed4975142c803f73afe35f822507/test/teslamate_web/live/settings_test.exs#L217-L230