edisonywh / backoffice

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

New resource url params #48

Open NduatiK opened 3 years ago

NduatiK commented 3 years ago

Do you think it would be useful to allow links to the new resource page to accept parameters that could allow loading of values into the changeset?


For example, lets say I was dealing with university courses and wanted to add a new class/unit to an existing course. The ideal experience would be to go to the course I want to change and click on an Add class. This would load the url /class/new?course_id=1 and open the create form with the course already provided.

edisonywh commented 3 years ago

I really like this idea, on paper this would allow for pre-filling data in general, which could be really helpful.

But I just want to play devil's advocate here and point this out - this could potential abuse. For example, something like association in URL, nefarious admins could change course_id=100 to a course that they're not supposed to create a class for. This might not be a great example since it touches authorization which is a different topic, but my point is, we could accidentally pre-fill unintended column and thus opening up attack vector. I think this idea has merits, but we just need to be mindful about the actual implementation.

Another thing is, it's really tricky with association - the association experience isn't that great yet. I'm talking more in terms of how the UI/UX should look. I'm veering a bit off topic since this is more about association, but I think the context might be helpful as you're talking about associations specifically here.

For example, a belongs_to and has_one would probably mean different view - it doesn't make sense to "Add a class" when it's a belongs_to, and belongs_to should behave more like a search/dropdown select for existing records.

Then let's say has_many for example, there's has_many :through, which can be complicated.

Right now the idea is, there's a small data-table view that displays the has_many association in the page, below the Edit form. That table should have a Add Class button that would bring you to /class/new like you mentioned, though the problem is, right now there's no way for Backoffice to figure out what the correct route should be.

The reason Backoffice don't know what routes are available has to do with the way Backoffice work - it sits right within your router, so there's no centralised place that we can do Backoffice.list_all_resources/0 that can give us all pages. We also would need a lookup map that can tell us, given an atom of a resource that looks like YourApp.Class, we need to figure out the corresponding YourApp.ClassLive.Index in order to get the correct route.

I am very much open to help with the association part, I'm at a bit of a loss so I would appreciate any help there.

NduatiK commented 3 years ago

Whoa! The potential for abuse is something I had not even considered.


though the problem is, right now there's no way for Backoffice to figure out what the correct route should be.

Do you mean something like /admin/classes/new? Is this something we can just add to the form field as an option that can be passed to has_many? This would work, but would not be as elegant as automatically rerouting to the correct page.

Is it possible to provide the course_id inside the socket when we reroute from the course page to the create a new class page? That would remove the attack vector but allow us to pass values forward. I am currently learning LiveView so I'm not sure if this makes sense.


By the way, is there a way to specify which fields to display on an association?

edisonywh commented 3 years ago

But that's the issue, let's say Course has_many :classes, and you're editing Course and you want to add a Class to the course.

You would expect to see a Add Class button when you're editing the Course, and when you click on it, you expect to go /admin/classes/new, but the thing is, Backoffice doesn't know what the correct route is.

Routes are defined in your router.ex, outside of Backoffice's control, so we don't know what the correct route is.

In your Index page where you use Backoffice.Resource.Index, you would specify the current resource (Course), and in your field you can have has_many :classes, but that tells us the relation on the Ecto level (we have the resource name)- Backoffice doesn't even know if Class has implemented as a Backoffice.Resource.Single, so we still can't figure out what the route is.

I have a hackish idea that implements a stub protocol for everything that use Backoffice.Resource.*, and then we can query for something like: "Given a resource named Class, fetch me the corresponding Index/Single page", because if we have the resource module itself we can infer the route by convention. I hope that makes sense?

This is a glaring mistake of my initial idea of letting users own their routes (explicitness), but if this is a big hindrance to association support I am willing to change it.


Can you elaborate on what you mean by fields to display on an association? Is this the has_many table view that's below the edit form? I don't think there's a way to change what fields to show as of now, no

NduatiK commented 3 years ago

If we wanted to retain the flexibility, we would only need to require users to provide the corresponding page.

Something like:

form do 
  ...
  has_many(:classes, MyApp.Class, new_page: MyApp.Backoffice.ClassLive.Single)
end

This would create a Add Class button on the association table if new_page was provided.

When the button is clicked, we could internally get the redirect path using:

     path =  MyAppWeb.Router.Helpers.live_path(
          MyAppWeb.Endpoint,
          MyApp.Backoffice.ClassLive.Single,
          ... # Can't remember how to get the `:new` route path
        )

This would have the additional benefit of giving control over whether or not creating an associated schema is allowed. If a new_page option is not provided, we don't allow the creation of an association.


This is a glaring mistake of my initial idea of letting users own their routes (explicitness), but if this is a big hindrance to association support I am willing to change it.

I'm not sure...I like the flexibility Backoffice provides in naming routes and choosing which schemas and fields to make visible. I believe this is a feature not a bug, I always feel in control of the library.

The solution above might not be the correct way of going about things, but if I understand correctly, one of the core goals of Backoffice is expliciteness. I am not sure if use Backoffice.Resource.* allows for the same flexibility.

edisonywh commented 3 years ago

Apologies for the late reply!

That's actually a really elegant solution that I absolutely did not think of, that's awesome!

I think we don't have to call it new_page, perhaps just page is enough - it would also be used for edit button etc

I always feel in control of the library.

That's a really big point that I wanted when I first started out the project, I'm really glad to hear that you feel the same way :)

I am not sure if use Backoffice.Resource.* allows for the same flexibility.

Can you elaborate on what you mean here? Right now there's use Backoffice.Resource.Index and use Backoffice.Resource.Single, is that what you're referring to?

By the way, I gave a talk on Backoffice during Code BEAM V recently (part of the reason why I was too busy to reply), and I worked up a demo here - https://github.com/edisonywh/backoffice-demo that could probably help out in developing/showcasing some ideas. The video should be available in a couple of months but if you can't wait just DM/email me somewhere (Elixir Slack/Twitter) and I'll get you hooked up!

NduatiK commented 3 years ago

I am not sure if use Backoffice.Resource.* allows for the same flexibility.

Can you elaborate on what you mean here? Right now there's use Backoffice.Resource.Index and use Backoffice.Resource.Single, is that what you're referring to?

I am referring to this:

I have a hackish idea that implements a stub protocol for everything that use Backoffice.Resource.*, and then we can query for something like: "Given a resource named Class, fetch me the corresponding Index/Single page", because if we have the resource module itself we can infer the route by convention. I hope that makes sense?

Your idea, if I understand it correctly would have specified that for a resource that is associated with a resource named Class, clicking Add on the parent resource would open the path for Backoffice.Resource.Class.Index.

I was suggesting that instead of enforcing a naming convention, we could let users supply the name of the module to be used for creating associations.

edisonywh commented 3 years ago

Yeah I definitely agree with you there, I much prefer what you suggested - allow users to pass in target resource itself:

form do 
  ...
  has_many(:classes, MyApp.Class, page: MyApp.Backoffice.ClassLive.Single)
end