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.

Sign up for GitHub

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

Jump to bottom

Support phx-trigger-action #135

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Support phx-trigger-action #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
01b9a05
Support phx-trigger-action
ftes Oct 5, 2024
ea23778
Separate trigger_form_action and redirect (token flow)
ftes Oct 23, 2024
de3c1e8
Remove .envrc
ftes Oct 23, 2024
95053a5
Revert "Separate trigger_form_action and redirect (token flow)"
ftes Oct 26, 2024
9060e9b
Add tests
ftes Oct 26, 2024
b61d1f3
Remove double raise and add test
ftes Oct 28, 2024
d62c264
More descriptive input names for trigger action form
ftes Oct 28, 2024
7f8f72c
add redirect tests
ftes Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/phoenix_test/element/form.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ defmodule PhoenixTest.Element.Form do
|> build()
end

def find(html, selector) do
html
|> Query.find(selector)
|> case do
{:found, element} -> {:found, build(element)}
{:found_many, elements} -> {:found_many, Enum.map(elements, &build/1)}
:not_found -> :not_found
end
end

def find_by_descendant!(html, descendant) do
html
|> Query.find_ancestor!("form", descendant_selector(descendant))
Expand Down
17 changes: 16 additions & 1 deletion lib/phoenix_test/live.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,22 @@ defmodule PhoenixTest.Live do
end

defp maybe_redirect(html, session) when is_binary(html) do
maybe_put_patch_path(session)
case Form.find(html, "form[phx-trigger-action]") do
Copy link
Owner

Choose a reason for hiding this comment

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

I find it a little strange that we'd be looking for a form inside maybe_redirect.

I imagine it'd be more duplication to do this in the places where we're submitting forms, but I wonder if we could find a better design for this?

I'd image there are places where we're dealing with a form (probably through a phx-change) where we already have the form data structure. Maybe we could check for a Form.phx_trigger_action? there? (similar to how we do it with other attributes like that)

Copy link
Contributor Author

@ftes ftes Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I think the phx-trigger-action is a bit like a client side redirect.

Two things to consider:

  • phx-trigger-action can be set on any re-render, not just when clicking a submit button for a form
  • phx-trigger-action can be set on any form, not necessarily the one the user is interacting with

I agree that the common use case is:

  1. User submits form
  2. handle_event("save" assigns trigger_submit: true
  3. Browser performs HTTP POST

But I think we should limit the implementation to that common use case.

I think its best to mimmick LiveView JS to support all kinds of uses cases:
Whenever something changed, check the HTML for forms with phx-trigger-action.
If present, then submit that form.

That's what I tried to to in this PR.

We could separate maybe_redirect and maybe_trigger_action.
Do you think that would help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit e2dd5b4

I separated them into two separate post processing steps.
I tried to keep both functions completely separate by introducing a token flow.

Feels over-engineered. Maybe just renaming maybe_redirect to maybe_trigger_form_action_or_redirect would be good enough?

Copy link
Owner

Choose a reason for hiding this comment

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

Feels over-engineered. Maybe just renaming maybe_redirect to maybe_trigger_form_action_or_redirect would be good enough?

Yeah, I agree. I appreciate you taking the time to try a different approach to see if we landed at something better. But it does seem to introduce more confusion. I think your original maybe_redirect implementation was best -- especially since you explained how trigger_action works. I thought it was more limited.

I think its best to mimmick LiveView JS to support all kinds of uses cases:

I think that's a good north star for sure. As we evaluate what to include and not include, I think there are two levels that I like to keep in mind:

  1. PhoenixTest not lie to users -- so, tests shouldn't pass if production is going to break.
  2. It's less bad for tests to break when production would pass (e.g. maybe we don't have a complete implementation). Ideally, we can match production, but I think this should always be secondary to (1).

All that being said, any chance to revert the last commit, and I think we might be good to go? Do you think it'd be helpful to write test cases for the scenarios you mentioned? (when a different event happens and the trigger-action still happens?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted and added tests.

Also stumbled over #143.

:not_found ->
maybe_put_patch_path(session)

{:found, form} ->
active_form = session.active_form
active_form? = form.selector == active_form.selector
form_data = form.form_data ++ if(active_form?, do: active_form.form_data, else: [])

session.conn
|> PhoenixTest.Static.build()
|> PhoenixTest.Static.submit_form(form.selector, form_data)
Copy link
Owner

Choose a reason for hiding this comment

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

I have a question here. Do you know what happens in a normal LiveView if a form has something like a phx-change that redirects you somewhere and it has a phx-trigger-action? Which redirect "wins out"?

In this case, we're giving "priority" to the phx-trigger-action, which I imagine is the right call since most times I'd expect phx-change not to be doing the redirecting. But I'm curious if we know what happens in those scenarios (and if we should even concern ourselves with them, or if it's just an edge case that we shouldn't cover).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you maybe have misread the code. Or I'm wrong. Either way, I'm adding a test to clarify.

My understanding:
This implementation already prioritizes redirects or patches.
phx-trigger-action is only followed if no patch or redirect happened.
I'm pretty sure this is also what LiveView does.

These two clauses match first:

defp maybe_redirect({:error, {:redirect, %{to: path}}}, session) do
defp maybe_redirect({:error, {:live_redirect, %{to: path}}} = result, session) do

So, given

<form phx-trigger-action={@trigger_submit} phx-change="redirect">
handle_event("redirect", _, socket) do
  socket
  |> push_navigate(to: "/elsewhere")
  |> assign(:trigger_submit: true)
end

a change to the form will cause a redirect to /elsewhere. And ignore the phx-trigger-action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double checked the actual behaviour:

  • redirect: wins over phx-trigger-action (✅ this PR is ok)
  • navigate: wins over phx-trigger-action (✅ this PR is ok)
  • patch: is executed before phx-trigger-action, both are applied (✅ this PR is ok)

So I think we're fine.

You're right in that phx-trigger-action is given priority over maybe_put_patch_path.
But if there's a phx-trigger-action, there's no point in updating the session.current_path, because we're about to POST to another path anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once again, running the same test against an actual browser as well as the Live driver would give peace of mind here :).

Comment on lines +351 to +353
Copy link
Owner

Choose a reason for hiding this comment

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

I would imagine our implementation here is good. But I haven't done stuff with phx-trigger-action, so want to ask. Do you know if we should be using LiveViewTest's follow_trigger_action function to get the method and data to send?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the follow_trigger_action, we should be fine.
It does the same things we're doing (ensure phx-trigger-action attr present, then use method and action to HTTP submit the form).

Should we use follow_trigger_action anyway?

Maybe.

In future the implementations might drift apart (theoretically, not saying it's likely).
Using follow_trigger_action under the hood would prevent that.

No, rather be consistent with how we submit static forms from LiveViews

We have a precedent in PhoenixTest: static form submits from a LiveView (without phx-trigger-action).

phoenix_test/lib/phoenix_test/live.ex

Lines 299 to 302 in d62c264

Form.has_action?(form) ->
session.conn
|> PhoenixTest.Static.build()
|> PhoenixTest.Static.submit_form(selector, form_data)

For that we could use LiveViewTest.submit_form. Which uses the same helper function as LiveViewTest.follow_trigger_action.

For the sake of simplicity (my interpretation) we don't do that and rather just call the Static driver directly.


{:found_many, _} ->
raise ArgumentError, "Found multiple forms with phx-trigger-action."
end
end

defp maybe_put_patch_path(session) do
Expand Down
47 changes: 47 additions & 0 deletions test/phoenix_test/live_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,53 @@ defmodule PhoenixTest.LiveTest do
|> assert_has("#form-data", text: "hidden_race: hobbit")
end

test "phx-trigger-action causes POST to static view", %{conn: conn} do
conn
|> visit("/live/index")
|> fill_in("Trigger action", with: "engage")
|> submit()
|> assert_has("#form-data", text: "trigger_action_hidden_input: trigger_action_hidden_value")
|> assert_has("#form-data", text: "trigger_action_input: engage")
end

test "phx-trigger-action from outside the form", %{conn: conn} do
conn
|> visit("/live/index")
|> click_button("Trigger from elsewhere")
|> assert_has("#form-data", text: "trigger_action_hidden_input: trigger_action_hidden_value")
|> refute_has("#form-data", text: "trigger_action_input:")
end

test "phx-trigger-action performed after patch", %{conn: conn} do
conn
|> visit("/live/index")
|> fill_in("Patch and trigger action", with: "let's go")
|> assert_path("/page/create_record")
end

test "phx-trigger-action ignored if view redirects", %{conn: conn} do
conn
|> visit("/live/index")
|> click_button("Redirect and trigger action")
|> assert_path("/live/page_2")
end

test "phx-trigger-action ignored if view navigates", %{conn: conn} do
conn
|> visit("/live/index")
|> click_button("Navigate and trigger action")
|> assert_path("/live/page_2")
end

test "raises an error if multiple forms have phx-trigger-action", %{conn: conn} do
assert_raise ArgumentError, ~r/Found multiple forms/, fn ->
conn
|> visit("/live/index")
|> click_button("Trigger multiple")
|> assert_has("#form-data", text: "hidden: trigger_action_hidden_value")
end
end

test "raises an error if field doesn't have a `name` attribute", %{conn: conn} do
assert_raise ArgumentError, ~r/Field is missing a `name` attribute/, fn ->
conn
Expand Down
73 changes: 73 additions & 0 deletions test/support/index_live.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,47 @@ defmodule PhoenixTest.IndexLive do

<div id="hook" phx-hook="SomeHook"></div>
<div id="hook-with-redirect" phx-hook="SomeOtherHook"></div>

<form
id="trigger-form"
phx-submit="trigger-form"
phx-trigger-action={@trigger_submit}
action="/page/create_record"
method="post"
>
<input type="hidden" name="trigger_action_hidden_input" value="trigger_action_hidden_value" />
<label>Trigger action <input type="text" name="trigger_action_input" /></label>
</form>

<button phx-click="trigger-form">Trigger from elsewhere</button>
Copy link
Owner

Choose a reason for hiding this comment

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

👍


<form
id="trigger-multiple-form-1"
phx-submit="trigger-form"
phx-trigger-action={@trigger_multiple_submit}
/>

<form
id="trigger-multiple-form-2"
phx-submit="trigger-form"
phx-trigger-action={@trigger_multiple_submit}
/>

<button phx-click="trigger-multiple-forms">Trigger multiple</button>

<form
id="redirect-and-trigger-form"
phx-change="patch-and-trigger-form"
phx-trigger-action={@redirect_and_trigger_submit}
action="/page/create_record"
method="post"
>
<label>
Patch and trigger action <input type="text" name="patch_and_trigger_action" />
</label>
<button phx-click="redirect-and-trigger-form">Redirect and trigger action</button>
<button phx-click="navigate-and-trigger-form">Navigate and trigger action</button>
</form>
"""
end

Expand All @@ -463,6 +504,9 @@ defmodule PhoenixTest.IndexLive do
|> assign(:show_form_errors, false)
|> assign(:cities, [])
|> assign(:hidden_input_race, "human")
|> assign(:trigger_submit, false)
|> assign(:trigger_multiple_submit, false)
|> assign(:redirect_and_trigger_submit, false)
|> allow_upload(:avatar, accept: ~w(.jpg .jpeg))
|> allow_upload(:main_avatar, accept: ~w(.jpg .jpeg))
|> allow_upload(:backup_avatar, accept: ~w(.jpg .jpeg))
Expand Down Expand Up @@ -499,6 +543,35 @@ defmodule PhoenixTest.IndexLive do
}
end

def handle_event("trigger-form", _form_data, socket) do
{:noreply, assign(socket, :trigger_submit, true)}
end

def handle_event("trigger-multiple-forms", _form_data, socket) do
{:noreply, assign(socket, :trigger_multiple_submit, true)}
end

def handle_event("patch-and-trigger-form", _form_data, socket) do
{:noreply,
socket
|> assign(:redirect_and_trigger_submit, true)
|> push_patch(to: "/live/index_no_layout")}
end

def handle_event("redirect-and-trigger-form", _form_data, socket) do
{:noreply,
socket
|> redirect(to: "/live/page_2")
|> assign(:redirect_and_trigger_submit, true)}
end

def handle_event("navigate-and-trigger-form", _form_data, socket) do
{:noreply,
socket
|> push_navigate(to: "/live/page_2")
|> assign(:redirect_and_trigger_submit, true)}
end

def handle_event("set-hidden-race", form_data, socket) do
race =
case form_data["name"] do
Expand Down
Loading