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

Error rendering filters when two filter fields share the same prefix #242

Closed
jonathonadler opened this issue Jun 21, 2021 · 4 comments · Fixed by #243
Closed

Error rendering filters when two filter fields share the same prefix #242

jonathonadler opened this issue Jun 21, 2021 · 4 comments · Fixed by #243

Comments

@jonathonadler
Copy link

jonathonadler commented Jun 21, 2021

I have a template with the following two search filters (some fields omitted):

      <div class="field">
        <label>Email</label>
        <%= filter_select(:user, :email, @conn.params) %>
        <%= filter_string_input(:user, :email, @conn.params) %>
      </div>

      <div class="field">
        <label>Email confirmed at</label>
        <%= filter_date_input(:user, :email_confirmed_at, @conn.params) %>
      </div>

My filter config has:

  defp filter_config(:users) do
    defconfig do
      number(:id)
      text(:email)
      text(:roles)
      text(:first_name)
      text(:last_name)
      date(:email_confirmed_at)
      date(:inserted_at)
      date(:updated_at)
    end
  end

Whenever I search by "email_confirmed_at", I get an error:

Protocol.UndefinedError at GET /admin/users
protocol Phoenix.HTML.Safe not implemented for %{"end" => "2021-06-03", "start" => "2021-06-08"} of type Map. This protocol is implemented for the following type(s): Decimal, Phoenix.LiveComponent.CID, Phoenix.LiveView.Component, Phoenix.LiveView.Comprehension, Phoenix.LiveView.Rendered, Time, Integer, Tuple, List, DateTime, NaiveDateTime, Float, Atom, Phoenix.HTML.Form, BitString, Date

filters

    ["user[email_contains]", "user[first_name_contains]", "user[last_name_contains]", "user[id_equals]"]

user

    %{"email_confirmed_at_between" => %{"end" => "2021-06-03", "start" => "2021-06-08"}}

The error goes away if I comment out the following line in my template:

 <%#  <%= filter_string_input(:user, :email, @conn.params) %> -->

The cause appears to be related to the find_param implementation in filter_view.ex:

  defp find_param(params, pattern, type \\ :string) do
    pattern = to_string(pattern)

    result =
      Enum.find(params || %{}, fn {key, _val} ->
        String.starts_with?(key, pattern)
      end)

    cond do
      result == nil && type == :string -> {"#{pattern}_contains", nil}
      result == nil && type == :number -> {"#{pattern}_equals", nil}
      result == nil && type == :date -> {"#{pattern}_before", nil}
      result != nil -> result
    end
  end

It seems like the wrong field ("email") is found first within the map. I don't think the String.starts_with? logic is the correct approach here.

Parameters: %{"filters" => ["user[email_contains]", "user[first_name_contains]", "user[last_name_contains]", "user[id_equals]"], "user" => %{"email_confirmed_at_between" => %{"end" => "2021-06-03", "start" => "2021-06-08"}}}
@cpjolicoeur
Copy link
Member

@jonathonadler Interesting find. I'll try to create a failing test case for regression purposes.

Do you have any interest it trying to make a fix for this issue and submitting a Pull Request?

If not, I'll try to get on this sometime this week to update & fix the issue.

cpjolicoeur added a commit that referenced this issue Jun 22, 2021
cpjolicoeur added a commit that referenced this issue Jun 22, 2021
cpjolicoeur added a commit that referenced this issue Jun 22, 2021
cpjolicoeur added a commit that referenced this issue Jun 22, 2021
@cpjolicoeur
Copy link
Member

@jonathonadler I merged PR #243 with a fix for this. Could you try using the master branch of Torch locally and confirm with me that this fixes your issue?

If so, I'll go ahead and cut a new official release of Torch with this fix included.

@jonathonadler
Copy link
Author

Thanks @cpjolicoeur! I really appreciate your efforts here. I'm really glad you took up the PR because I'm still learning Elixir and may have struggled to implement so quickly. I tested locally against master branch and it now works perfectly.

@cpjolicoeur
Copy link
Member

@jonathonadler Version 3.6.2 has been released to hex.

Thanks for the bug report and help with this.

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 a pull request may close this issue.

2 participants