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

Transition to mainline .inputs_for code #466

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PragTob
Copy link
Collaborator

@PragTob PragTob commented Nov 10, 2024

https://hexdocs.pm/phoenix_live_view/1.0.0-rc.7/Phoenix.Component.html#inputs_for/1

  • makes a lot of code unnecessary
  • same time, the tests don't seem to be working any more, which isn't great and I have no idea how to really resolve

Gif showcasing the feature working

(sorry about the editor bleeding in, see it as a feature)

Peek 2024-11-10 13-11

Notes:

  • Right now the error on a label is only shown when you first put in text and then delete it again. This still stops the whole brainstorming from being saved (error is shown at the top, not visible int he gif but can also be hard to spot). So, if you add a couple of empty labels that'll stop all your other editions from being saved. We should probably make the error appear immediately.

https://hexdocs.pm/phoenix_live_view/1.0.0-rc.7/Phoenix.Component.html#inputs_for/1

* makes a lot of code unnecessary
* same time, the tests don't seem to be working any more,
which isn't great and I have no idea how to really resolve
@PragTob
Copy link
Collaborator Author

PragTob commented Nov 10, 2024

The tests will fail like this:

root@a24346dbfa92:/app# mix test
Compiling 1 file (.ex)
............................................................

  1) test removes idea label (MindwendelWeb.Admin.BrainstormingLive.EditTest)
     test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:97
     ** (ArgumentError) no push or navigation command found within JS commands: [["dispatch",{"event":"change"}]]
     code: |> render_click()
     stacktrace:
       (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/test/live_view_test.ex:1106: Phoenix.LiveViewTest.call/2
       test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:108: (test)

....

  2) test does not remove idea label when idea is attached to this label (MindwendelWeb.Admin.BrainstormingLive.EditTest)
     test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:119
     ** (ArgumentError) no push or navigation command found within JS commands: [["dispatch",{"event":"change"}]]
     code: |> render_click()
     stacktrace:
       (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/test/live_view_test.ex:1106: Phoenix.LiveViewTest.call/2
       test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:138: (test)

.

  3) test adds and immediately saves new idea label (MindwendelWeb.Admin.BrainstormingLive.EditTest)
     test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:60
     ** (ArgumentError) no push or navigation command found within JS commands: [["dispatch",{"event":"change"}]]
     code: |> render_click()
     stacktrace:
       (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/test/live_view_test.ex:1106: Phoenix.LiveViewTest.call/2
       test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:69: (test)

...................................******..............***............................................................
Finished in 2.0 seconds (0.00s async, 2.0s sync)
186 tests, 3 failures, 9 skipped

Randomized with seed 958874

I haven't found a satisfactory way to work around this and will work in the forums. Changing it to emit a custom event feels wrong when the docs say to do this. The right move would probably be to emit "save" (as the outer form does on phx-change) but since it's in the official docs I'd like to know if there's something else/better to do.

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 10, 2024

Right using the save event also breaks it and sends along the value of the event (which I guess I should have known):

  1) test removes idea label (MindwendelWeb.Admin.BrainstormingLive.EditTest)
     test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:97
     ** (EXIT from #PID<0.539.0>) an exception was raised:
     
          ** (FunctionClauseError) no function clause matching in MindwendelWeb.Admin.BrainstormingLive.Edit.handle_event/3
     
          The following arguments were given to MindwendelWeb.Admin.BrainstormingLive.Edit.handle_event/3:
          
              # 1
              "save"
          
              # 2
              %{"value" => "0"}
          

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 10, 2024

Created a thread on the elixir forum asking for help: https://elixirforum.com/t/tests-with-render-click-phx-click-js-dispatch-change-fail-best-way-to-fix-testing-inputs-for/67387

The one where we delete a label which we shouldn't have creates
some problems. Will take a look later at displaying the proper
errors and giving it the proper constaint.
@PragTob
Copy link
Collaborator Author

PragTob commented Nov 11, 2024

Basically works but one of the test cases now needs some work and maybe even some feature work as that's the one about removing labels of associated ideas not working.

  1) test does not remove idea label when idea is attached to this label (MindwendelWeb.Admin.BrainstormingLive.EditTest)
     test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:121
     ** (EXIT from #PID<0.2494.0>) an exception was raised:
         ** (Ecto.ConstraintError) constraint error when attempting to delete struct:
     
         * "idea_idea_labels_idea_label_id_fkey" (foreign_key_constraint)
     
     If you would like to stop this constraint violation from raising an
     exception and instead add it as an error to your changeset, please
     call `foreign_key_constraint/3` on your changeset with the constraint
     `:name` as an option.
     
     The changeset has not defined any constraint.
     
             (ecto 3.12.4) lib/ecto/repo/schema.ex:878: anonymous fn/4 in Ecto.Repo.Schema.constraints_to_errors/3
             (elixir 1.15.8) lib/enum.ex:1693: Enum."-map/2-lists^map/1-1-"/2
             (ecto 3.12.4) lib/ecto/repo/schema.ex:862: Ecto.Repo.Schema.constraints_to_errors/3
             (ecto 3.12.4) lib/ecto/repo/schema.ex:839: Ecto.Repo.Schema.apply/4
             (ecto 3.12.4) lib/ecto/repo/schema.ex:609: anonymous fn/13 in Ecto.Repo.Schema.do_delete/4
             (ecto 3.12.4) lib/ecto/association.ex:948: Ecto.Association.Has.on_repo_change/5
             (ecto 3.12.4) lib/ecto/association.ex:934: Ecto.Association.Has.on_repo_change/5
             (ecto 3.12.4) lib/ecto/association.ex:648: anonymous fn/8 in Ecto.Association.on_repo_change/7
             (elixir 1.15.8) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
             (ecto 3.12.4) lib/ecto/association.ex:644: Ecto.Association.on_repo_change/7
             (elixir 1.15.8) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
             (ecto 3.12.4) lib/ecto/association.ex:589: Ecto.Association.on_repo_change/4
             (ecto 3.12.4) lib/ecto/repo/schema.ex:1018: Ecto.Repo.Schema.process_children/5
             (ecto 3.12.4) lib/ecto/repo/schema.ex:1096: anonymous fn/3 in Ecto.Repo.Schema.wrap_in_transaction/6
             (ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1400: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
             (db_connection 2.7.0) lib/db_connection.ex:1756: DBConnection.run_transaction/4
             (mindwendel 0.2.8) lib/mindwendel/brainstormings.ex:154: Mindwendel.Brainstormings.update_brainstorming/2
             (mindwendel 0.2.8) lib/mindwendel_web/live/admin/brainstorming_live/edit.ex:54: MindwendelWeb.Admin.BrainstormingLive.Edit.handle_event/3
             (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/channel.ex:508: anonymous fn/3 in Phoenix.LiveView.Channel.view_handle_event/3
             (telemetry 1.3.0) /app/deps/telemetry/src/telemetry.erl:324: :telemetry.span/3

.....................***......******.......................15:34:38.057 [error] Postgrex.Protocol (#PID<0.383.0>) disconnected: ** (DBConnection.ConnectionError) owner #PID<0.3177.0> exited

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 11, 2024

I knew I hated nested inputs for.

I'm not quite sure but currently it explodes saying it has no constraints defined - but we do have constraints! Or well, I fixed that.

I'm not 100% sure what ecto does, but it feels like it creates a new changeset which loses the constraints to delete them which robably has some reason. I'm deep debugging into ecto but I need a break now.

[(ecto 3.12.4) lib/ecto/association.ex:643: Ecto.Association.on_repo_change/7]
changesets #=> [
  #Ecto.Changeset<action: :replace, changes: %{}, errors: [],
   data: #Mindwendel.Brainstormings.IdeaLabel<>, valid?: true, ...>,
  #Ecto.Changeset<
    action: :update,
    changes: %{position_order: 0},
    errors: [],
    data: #Mindwendel.Brainstormings.IdeaLabel<>,
    valid?: true,
    ...
  >,
  #Ecto.Changeset<
    action: :update,
    changes: %{position_order: 1},
    errors: [],
    data: #Mindwendel.Brainstormings.IdeaLabel<>,
    valid?: true,
    ...
  >,
  #Ecto.Changeset<
    action: :update,
    changes: %{position_order: 2},
    errors: [],
    data: #Mindwendel.Brainstormings.IdeaLabel<>,
    valid?: true,
    ...
  >,
  #Ecto.Changeset<
    action: :update,
    changes: %{position_order: 3},
    errors: [],
    data: #Mindwendel.Brainstormings.IdeaLabel<>,
    valid?: true,
    ...
  >
]

[(ecto 3.12.4) lib/ecto/association.ex:647: Ecto.Association.on_repo_change/7]
action #=> :replace

replacing starts....
[(ecto 3.12.4) lib/ecto/association.ex:937: Ecto.Association.Has.on_repo_change/5]
changeset #=> #Ecto.Changeset<action: :delete, changes: %{}, errors: [],
 data: #Mindwendel.Brainstormings.IdeaLabel<>, valid?: true, ...>

[(ecto 3.12.4) lib/ecto/association.ex:938: Ecto.Association.Has.on_repo_change/5]
changeset.constraints #=> []

[(ecto 3.12.4) lib/ecto/association.ex:939: Ecto.Association.Has.on_repo_change/5]
changeset.validations #=> []

[(ecto 3.12.4) lib/ecto/association.ex:954: Ecto.Association.Has.on_repo_change/5]
changeset #=> #Ecto.Changeset<action: :delete, changes: %{}, errors: [],
 data: #Mindwendel.Brainstormings.IdeaLabel<>, valid?: true, ...>

[(ecto 3.12.4) lib/ecto/repo/schema.ex:575: Ecto.Repo.Schema.do_delete/4]
changeset #=> #Ecto.Changeset<action: :delete, changes: %{}, errors: [],
 data: #Mindwendel.Brainstormings.IdeaLabel<>, valid?: true, ...>

we heerreeee
[(ecto 3.12.4) lib/ecto/repo/schema.ex:864: Ecto.Repo.Schema.constraints_to_errors/3]
changeset #=> #Ecto.Changeset<action: :delete, changes: %{}, errors: [],
 data: #Mindwendel.Brainstormings.IdeaLabel<>, valid?: true, ...>



  1) test does not remove idea label when idea is attached to this label (MindwendelWeb.Admin.BrainstormingLive.EditTest)
     test/mindwendel_web/live/admin/brainstorming_live/edit_test.exs:121
     ** (EXIT from #PID<0.485.0>) an exception was raised:
         ** (Ecto.ConstraintError) constraint error when attempting to delete struct:
     
         * "idea_idea_labels_idea_label_id_fkey" (foreign_key_constraint)
     
     If you would like to stop this constraint violation from raising an
     exception and instead add it as an error to your changeset, please
     call `foreign_key_constraint/3` on your changeset with the constraint
     `:name` as an option.
     
     The changeset has not defined any constraint.
     
             (ecto 3.12.4) lib/ecto/repo/schema.ex:882: anonymous fn/4 in Ecto.Repo.Schema.constraints_to_errors/3
             (elixir 1.15.8) lib/enum.ex:1693: Enum."-map/2-lists^map/1-1-"/2
             (ecto 3.12.4) lib/ecto/repo/schema.ex:866: Ecto.Repo.Schema.constraints_to_errors/3
             (ecto 3.12.4) lib/ecto/repo/schema.ex:841: Ecto.Repo.Schema.apply/4
             (ecto 3.12.4) lib/ecto/repo/schema.ex:611: anonymous fn/13 in Ecto.Repo.Schema.do_delete/4
             (ecto 3.12.4) lib/ecto/association.ex:960: Ecto.Association.Has.on_repo_change/5
             (ecto 3.12.4) lib/ecto/association.ex:945: Ecto.Association.Has.on_repo_change/5
             (ecto 3.12.4) lib/ecto/association.ex:653: anonymous fn/8 in Ecto.Association.on_repo_change/7
             (elixir 1.15.8) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
             (ecto 3.12.4) lib/ecto/association.ex:645: Ecto.Association.on_repo_change/7
             (elixir 1.15.8) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
             (ecto 3.12.4) lib/ecto/association.ex:589: Ecto.Association.on_repo_change/4
             (ecto 3.12.4) lib/ecto/repo/schema.ex:1025: Ecto.Repo.Schema.process_children/5
             (ecto 3.12.4) lib/ecto/repo/schema.ex:1103: anonymous fn/3 in Ecto.Repo.Schema.wrap_in_transaction/6
             (ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1400: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
             (db_connection 2.7.0) lib/db_connection.ex:1756: DBConnection.run_transaction/4
             (mindwendel 0.2.8) lib/mindwendel/brainstormings.ex:154: Mindwendel.Brainstormings.update_brainstorming/2
             (mindwendel 0.2.8) lib/mindwendel_web/live/admin/brainstorming_live/edit.ex:54: MindwendelWeb.Admin.BrainstormingLive.Edit.handle_event/3
             (phoenix_live_view 1.0.0-rc.7) lib/phoenix_live_view/channel.ex:508: anonymous fn/3 in Phoenix.LiveView.Channel.view_handle_event/3
             (telemetry 1.3.0) /app/deps/telemetry/src/telemetry.erl:324: :telemetry.span/3


Finished in 0.3 seconds (0.3s async, 0.00s sync)
8 tests, 1 failure, 7 excluded

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 17, 2024

Alright, I debugged it through and there doesn't seem to be a way around this (currently).

Why is this failing?

To the best of my understanding as the record is supposed to be deleted (via drop_params) the cast_assoc is basically creating a new changeset by which we lose the changesets' defined constraints leading to the blow up.

In the following code our changeset is referred to as on_cast as we get it from the with option (or default, for us with):

https://github.com/elixir-ecto/ecto/blob/892dc85ed4bdae4b83218693ef6dc3726da80252/lib/ecto/changeset.ex#L1272-L1283

    on_cast = Keyword.get_lazy(opts, :with, fn -> on_cast_default(type, related) end)
    sort = opts_key_from_params(:sort_param, opts, params)
    drop = opts_key_from_params(:drop_param, opts, params)

    changeset =
      if is_map_key(params, param_key) or is_list(sort) or is_list(drop) do
        value = Map.get(params, param_key)
        original = Map.get(data, key)
        current = Relation.load!(data, original)
        value = cast_params(relation, value, sort, drop)

        case Relation.cast(relation, data, value, current, on_cast) do

However, Relation.cast then uses it to create a function based on do_cast which uses on_cast (which includes the constraints, as this is the changeset we hand), which is used further down the line:

https://github.com/elixir-ecto/ecto/blob/892dc85ed4bdae4b83218693ef6dc3726da80252/lib/ecto/changeset/relation.ex#L108

  def cast(%{related: mod} = relation, owner, params, current, on_cast) do
    pks = mod.__schema__(:primary_key)
    fun = &do_cast(relation, owner, &1, &2, &3, &4, on_cast)

But in the cast of removing the element (via drop_params) do_cast calls on_replace - but this one drops the context of on_cast wholly by ignoring it and hence losing the constraints:

https://github.com/elixir-ecto/ecto/blob/892dc85ed4bdae4b83218693ef6dc3726da80252/lib/ecto/changeset/relation.ex#L136-L138

  defp do_cast(relation, _owner, nil = _params, current, _allowed_actions, _idx, _on_cast) do
    on_replace(relation, current)
  end

This means we end up with a changeset that has no foreign keys defined and hence the Repo.update fails loudly.

Assessment

I still think inputs_for is a great option for what we want to do/implement here. If it wasn't for that one shortcoming. I'll report it to ecto off the clock and see what the team thinks.

There are woarkounds, but I don't think they're worth it.

So, I think that for now sticking to the existing solution is better (we could tidy it up). I'd wait what I hear from the ecto team before we move forward - generally the team is awesome and responds to issues quickly.

Workarounds

  1. We could capture the error, but that doesn't have the context of the label - we could then query for every label which one has ideas associated and manually add the error - overly involved and liable to race conditions
  2. we could also manually check if ideas have labels and error out the update, but again, this one is vulnerable to race conditions

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 17, 2024

email on the mailing list: https://groups.google.com/g/elixir-ecto/c/r9BZq13U39E

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 18, 2024

Update: it's acknowledged as something ecto can improve and should be an easy fix.

@PragTob
Copy link
Collaborator Author

PragTob commented Nov 24, 2024

Back link go #362 for visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant