Allow setting method when using the test helper

[aduth]

What are you trying to accomplish?

We have components whose logic varies depending based on the request method, which is not easy to test with the current with_request_url helper.

What approach did you choose and why?

This adds a new method option to with_request_url. This follows the pattern previously established with the host option which exists to similarly specify the host associated with the test request.

[BlakeWilliams]

We have components whose logic varies depending based on the request method, which is not easy to test with the current with_request_url helper.

Can you share a real world example/use case? I'd like to understand the use case better before we move forward here.

[aduth]

We have components whose logic varies depending based on the request method, which is not easy to test with the current with_request_url helper.

Can you share a real world example/use case? I'd like to understand the use case better before we move forward here.

Sure thing. We have a component which renders a set of navigation links and highlights the current page. It's implemented by comparing the current request's handler (request[:controller] and request[:action]) against what would be the handler for a given link, using recognize_path.

There's some challenge in forms with both a new and create action, using the same path and GET and POST methods respectively. We want to render :new for an invalid form. To support this, the component needs to consider the request method when calling recognize_path to be able to compare against the current request handler. It's currently difficult to simulate this in tests for the component.

Maybe easier to share the actual code, effect, and our current workaround:

• Fix tab navigation current path detection for non-GET request 18F/identity-idp#8800
• Fix tab navigation current path detection for non-GET request 18F/identity-idp#8800 (comment)

[reeganviljoen]

@Spone @aduth any updates on this pr

[aduth]

@reeganviljoen From my perspective, I'm expecting this to be ready for review as-is. Based on the discussion at #1805 (comment), I don't have a clear sense that there's a suggested rename, and I'm reasonably happy with the current naming absent any objections.

I rebased the branch to resolve merge conflicts that existed due to drift.

Happy to make any revisions if suggestions are made!

[reeganviljoen]

@Spone any insights ?

[Spone]

Looks good to me. Maybe @BlakeWilliams can review this too?

@aduth can you please resolve the conflicts?

[aduth]

@aduth can you please resolve the conflicts?

Done 👍