elixirdrops / kerosene

Pagination for Ecto and Pheonix.
https://github.com/elixirdrops/kerosene
MIT License
231 stars 39 forks source link

Path params added as query param in links #22

Open scarfacedeb opened 7 years ago

scarfacedeb commented 7 years ago

When current path contains path params, kerosene produces urls with duplicated param keys in a query.

e.g. I have a path with brand_id param: /:brand_id/products/.
When I pass the whole params map to Repo.paginate, it generates incorrect page links, such as: /asus/products?brand_id=asus&page=2.

One way to resolve it is to drop those path keys from params when passing it to Repo.paginate:

Repo.paginate(query, Map.drop(params, ["brand_id"]))

But it makes the code more brittle and difficult to update later, because we'll need to keep track of such params.

I looked into source code and I understand why it's happening.
The question is - Is there a better way to avoid such issues?

Maybe we could introduce some option to drop such keys automatically.

use Kerosene, exclude_params: ~w(locale brand_id)

I'm not particularly fond of this solution, because it's global option that applies to all Repo.paginate calls. But it could work fine for global path params, such as locale.

What do you think?

allyraza commented 7 years ago

Hm weird we could use phoenix's route helpers to generate the links, but I would like to avoid a dependency on phoenix

we could do something like exclude_params, let me think about it

scarfacedeb commented 7 years ago

@allyraza Scrivener uses routes when Phoenix is present: https://github.com/mgwidmann/scrivener_html/blob/master/lib/scrivener/html.ex#L126

But it looks like meta magic to me.

Other option would be to pass path directly, like this:

path: &product_path(Endpoint, :index, brand.slug, &1)
syamilmj commented 7 years ago

My current workaround is to pass the params explicitly, e.g.:

Repo.paginate(query, Map.take(params, ["page"]))