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

Fix put route params #701

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

tmbb
Copy link

@tmbb tmbb commented Dec 14, 2024

Currently, when you have a route with parameters (e.g. live_resource "users/:user_id/posts", PostLive), you will get a bunch of errors in Backpex which come from the fact that Backpex.Router.get_path/5 function struggles with route parameters that are not controlled by Backpex. This funcion (through some helper functions) will try to rebuild the full URL from the route path given (which contains the .../:user_id/... part) and the given parameters by merging the parameters into the route. Currently, the get_path/5 function doesn't use all the available parameters from the socket assigns. This can be a problem if you want a view of posts belonging to a given user, for example.

With these changes, the get_path function will merge the params variable with the values taken from the socket.assigns. However, I'm not sure this is really stable because I'm probably using some private APIs that are not meant to be used. I'm not aware of any public API to access LiveView.Socket assigns at the stage of the socket lifecycle in which get_path/5 is invoked.

I thought of adding some tests here, but that would require building a whole test suite from the ground up because Backpex doesn't have any integration tests already set up. I might do it when I have the time for that.

So this is definitely not ready to be merged into the main repo, but I think it's a good basis for finding a good way of handling route parameters set "outside" of Backpex.

@Flo0807
Copy link
Collaborator

Flo0807 commented Dec 15, 2024

Hey! Thanks for the contribution 🚀 I have also experienced some problems when working with route params, so this will help us a lot. Unfortunately, it might take some time for someone to look into this due to limited time, just want to let you know.

@Flo0807
Copy link
Collaborator

Flo0807 commented Feb 14, 2025

Hey @tmbb,

I had some time to look into this issue. I wondered why we need to access some private APIs here as we assign the list of params in the Backpex mount/3 function. This list contains all the route parameters currently used in the URL. So the params passed to get_path should contain the user_id key in your case.

I have found that sometimes the list of params passed to get_path is empty, and the reason seems to be the default Backpex implementation for return_to/4. We use this function to assign a route that we can navigate to in certain actions, such as saving an item. We passed an empty params map here instead of reading it from the assigns.

You can see the changes I made in this merge request: #856

So the problem seems to be fixed now and will be included in the upcoming Backpex 0.11 release. I'd like to hear from you if this also fixes the problem you described in this merge request.

@tmbb
Copy link
Author

tmbb commented Feb 14, 2025

Hey! It turns out Backpex is not a good fit for my current project so I've stopped looking into it. Although I like the approach of only having to define ecto schemas and have an automatic CRUD interface, I always end up wanting to customize the interface and doing things not supported by the framework. I've decided to keep improving my own CRUD generators: https://github.com/tmbb/mandarin

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 this pull request may close these issues.

2 participants