edisonywh / backoffice

Admin tool built with the PETAL stack
MIT License
217 stars 15 forks source link

Default ordering in :index #42

Closed NduatiK closed 3 years ago

NduatiK commented 3 years ago

Hi,

I am using MSSQL for my project and whenever I use the index action, it throws the following error

ORDER BY is mandatory when OFFSET is set in query:

from m0 in MyApp.MemberUser,
  limit: ^...,
  offset: ^...,
  select: m0,
  preload: [[]]

This is caused by MSSQL's requirement that an ordering column be provided. I have solved this by adding |> order_by(:id) to the entries function in your Ecto Resolver.

  defp entries(query, repo, page_number, _, page_size) do
    offset = page_size * (page_number - 1)

    query
    |> offset(^offset)
    |> limit(^page_size)
    |> all(repo)
  end

becomes

  defp entries(query, repo, page_number, _, page_size) do
    offset = page_size * (page_number - 1)

    query
    |> offset(^offset)
    |> limit(^page_size)
    |> order_by(:id)
    |> all(repo)
  end

Would it possible to make this the default behaviour to support MSSQL or to provide a way of defining the order in the use Backoffice.Resource.Index options

NduatiK commented 3 years ago

Oh, and thank you for the library. I've been enjoying it so far

edisonywh commented 3 years ago

👋 Hey! Thanks for the bug report, that is really strange indeed. I actually copied the snippet over from Scrivener.Ecto so I'm surprised that's not working. Perhaps it's also worth reporting to them.

I'm not sure about defaulting an order inside the paginations itself, because:

1) we don't know which column to order_by 2) let's say we order by the primary key, there's issues with like binary_uuid type (order_by that would be weird), and also composite primary key etc. 3) if we do it at this level, user wouldn't know which order it's sorted by (not explicit since it doesn't show up on the query params, though I can see this might be a good thing for a default sort).

I like the second option you pointed out though, to allow user to define a default sorting order. So something like:

use Backoffice.Resource.Index, resolver_opts: [default_order_by: :key_name]

This way there's no magic to users, since they need to explicitly opt it.

And then in search/3 or load/3 we can pipe that into the order_by function.

One concern is that, with that we allow user to pass in preloads/order_by, but maybe a better option is to allow them to supply a query, like so:

use Backoffice.Resource.Index,
  resource: Resource,
  resolver_opts: [query: fn q -> q |> preload([q], [:abc]) |> order_by([q], [desc: :abc]) end]

I haven't given it a lot of thought but what do you think?

By the way, as a workaround, you can pass in ?order_by=[desc]key in your query params (maybe in your Layout module where you define the links), and that should order it as well (https://github.com/edisonywh/backoffice/blob/main/lib/backoffice/filter.ex#L11)

Thanks for giving it a try, I'm actually pleasantly surprised that someone else is using Backoffice. Just a heads up though, it's still super early stage so there's bound to be quite a bit of breaking changes (which is why I didn't put it on Hex), but I really appreciate having more eyes testing it out - let me know what you think, and what problems you faced, your use cases etc, that would be really helpful to make Backoffice better!

NduatiK commented 3 years ago

Ooh. binary_uuid columns, I had not considered those. The issue might be mssql specific and perhaps more specifically related to my database - I will look into Scrivener.Ecto's approach when I get a chance.

The default_order_by option was what I had in mind, but the query option also looks interesting. I am working on an approval process flow and I would like to only show records marked as pending_approval so that would be perfect.

The only question is whether that should be left to custom resolvers. The developer experience is easier with query transformation but I'm not sure what you had in mind for the API.

I agree that magic should be avoided if possible.


I got here because of SlickInbox!!! No problem if the API changes, I am having so much fun with things already. The drop downs on Ecto.Enum are genius and I love the fact that I can define custom actions so easily.

NduatiK commented 3 years ago

Seems like postgres does not require order by but not using it will result in "unpredictable" results.

I looked into scrivener_ecto and like you said, the code does not seem to include any ordering. However, the queries listed on the scrivener_ecto github page all have an order_by clause in the query piped into the paginate function. Seems like its assumed that an order will be provided by the user.


I've tried the url query arguments you suggested but that does not seem to apply on the load function in Backoffice.Resolvers.Ecto which accepts the resource alone:

resources = unquote(resolver).load(unquote(resource), unquote(resolver_opts), %{})

That doesn't seem to work on the initial mount.

edisonywh commented 3 years ago

I am working on an approval process flow and I would like to only show records marked as pending_approval so that would be perfect.

That actually makes sense, it's possible that you do not want your users to be able to see all but rather only a subset at any given time, so this means the filtering/query would need to go in the resolvers' load/3 or search/3 and not have it show up in query params.

The only question is whether that should be left to custom resolvers.

Can you elaborate on what you mean by that?

The developer experience is easier with query transformation but I'm not sure what you had in mind for the API.

Yeah, I think the API can be more well-defined, for example what if I always want to scope the results to my current user? (Authorisation is a separate topic but scoping is a resolver concern). Maybe some sort of lifecycle hooks (before_index, after_index (runs before Repo.all)) or something like that?

That doesn't seem to work on the initial mount.

Ah yes good catch, for some reason I thought load/3 is only used under the hood by search/4. Maybe we should update the initial mount to use search/4?

NduatiK commented 3 years ago

Can you elaborate on what you mean by that?

I was not sure if you had wanted to keep extending the default resolver. However, I've just realised that the "default" resolver is actually an Ecto specific resolver. You can ignore this statement

Oooh...authorization! Definitely a good idea.


I will create a pull request for this once I wrap my mind around the library a bit more. This is my first exposure to liveview so some of the terms are new.

edisonywh commented 3 years ago

I was not sure if you had wanted to keep extending the default resolver. However, I've just realised that the "default" resolver is actually an Ecto specific resolver. You can ignore this statement

Yes! So we can actually go wild with anything we put in resolver_opts, since that's resolvers-dependent.

I will create a pull request for this once I wrap my mind around the library a bit more. This is my first exposure to liveview so some of the terms are new.

Awesome! Feel free to reach out if you need any help whatsoever :)