search

germsvel / phoenix_test

MIT License
144 stars 20 forks source link

Fix: support links with only query parameters #80

Open jounimakela opened 3 months ago

jounimakela commented 3 months ago

This commit resolves an issue where links containing only query parameters without a specified path would incorrectly direct to the root path instead of the current path, unlike standard browser behavior.

Example of a link without a path:

<a href="?lang=en">English</a>

Previously, click_link/2 would direct to root path. In this commit the link correctly retains the current page while applying the query parameters.

germsvel commented 3 months ago

@jounimakela thanks for opening this issue.

I think the problem isn't with phoenix_test itself. We're passing the path to Phoenix.ConntTest.get, which seems to treat that as the root path. Would you mind opening an issue in that repo?

I think if we want to fix that here, we should fix it for all links and not just for when we do a click on static pages. That means we should probably clean up the path that is passed to PhoenixTest.visit/2before sending it to Phoenix.ConnTest.get/2.

You can see that here: https://github.com/germsvel/phoenix_test/blob/e1a240893b70448acc0146ec1830b0c1fbb789bc/lib/phoenix_test.ex#L214-L215

I'm open to the idea of us handling it here (in visit/2) and opening an issue in Phoenix.ConnTest (just to let them know. not sure if they'd fix it). Would you be up for that?

germsvel commented 3 months ago

I think this is also related to #72 (though probably need different fixes). Just noting that here.

germsvel commented 2 months ago

@jounimakela any thoughts on my comment from before? (https://github.com/germsvel/phoenix_test/pull/80#issuecomment-2149702937)

jounimakela commented 2 months ago

Apologies for my delayed response.

I'm open to the idea of us handling it here (in visit/2) and opening an issue in Phoenix.ConnTest (just to let them know. not sure if they'd fix it). Would you be up for that?

Sounds good! I'll make a proposal for a fix and open an issue.

germsvel commented 2 months ago

Thank you! 🎉 And no apologies needed. This is open source, so we all work on it when we can. 😄

Just wanted to check if this was still something you wanted to fix, and was doing my due diligence here.

jounimakela commented 1 month ago

Fixing the path is now in PhoenixTest.visit/2. I am not too happy about having the helper function in the top-level module.

I'll also squash commits once we are happy with the changes.

Thoughts?

germsvel commented 1 month ago

Thanks for making the change!

I am not too happy about having the helper function in the top-level module.

Yeah, I agree. Something about it doesn't feel great. I think it's because if feels like a band-aid (which it is) on something that Phoenix.ConnTest should handle.

Were you able to open the issue on the phoenix repo? I'd love for this to be fixed there, and we can include the band-aid here until it's fixed. But I don't want to include the band-aid in PhoenixTest forever.

jounimakela commented 1 month ago

Were you able to open the issue on the phoenix repo? I'd love for this to be fixed there, and we can include the band-aid here until it's fixed. But I don't want to include the band-aid in PhoenixTest forever.

Open now! https://github.com/phoenixframework/phoenix/issues/5877

jounimakela commented 1 month ago

@germsvel looks like Phoenix.ConnTest.get isn't going to change.