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.

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