smartmetals / phoenix_datatables

Library to implement server-side API for the jQuery DataTables library with Elixir and Phoenix Framework
MIT License
14 stars 7 forks source link

Support for "NULLS LAST" ordering #20

Closed devshane closed 5 years ago

devshane commented 5 years ago

Hello and thanks for the lib.

I need to order data where nulls are ordered last. I'm doing this now with a fragment in the order_by in def_order_relation/1:

if dir == :desc do
  order_by(queryable, unquote(bindings), [fragment("? DESC NULLS LAST", field(t, ^column))])
else
  order_by(queryable, unquote(bindings), [{^dir, field(t, ^column)}])
end

I'd like to create a PR that supports this generically, but I'm not sure I'm considering all the use cases and I can't quite figure out the best way to do so. I considered adding an argument use PhoenixDatatables.Repo, nulls_last: true and passing it down the chain:

This doesn't feel like the cleanest solution. Is this something you're interested in helping me with?

Thanks.

jeremyjh commented 5 years ago

Probably it shouldn't be an option on the Repo __using__ because user may have many different datatables in their application using that same repository. Probably it should be an option you pass to fetch_datatable; currently there is a columns option that you can use to control - on the server-side - which fields are sortable/filterable for a given invocation. Maybe it should be a new option nulls_last: true enables the option for whatever the sort field is on a given invocation. Unfortunately we don't pass the entire options down to Query.sort presently, we are passing it options[:column] but I'd willing to break compatibility here because we'll need this flexibility in the future. Technically sort is a public API but it is not likely many people use it directly.

devshane commented 5 years ago

Ok, thanks. I started a PR but just realized NULLS LAST is only valid for PostgreSQL. Is this acceptable?

jeremyjh commented 5 years ago

Sure, thats fine since its an option, we should just note that in the documentation for this option.

jeremyjh commented 5 years ago

Resolved in #21