Added missing tests for user controller github connect#930
Conversation
|
|
||
| assert json["data"]["id"] |> String.to_integer == user.id | ||
| test "requires authentication", %{conn: conn} do | ||
| path = user_path(conn, :github_oauth, %{"code" => "foo", "state" => "bar"}) |
There was a problem hiding this comment.
Unless I'm misreading this entirely, I think what's confusing me about this test is that it implies that valid_code and valid_state somehow work whereas foo and bar would not, when in reality it simply tests that the user is authenticated with our system regardless of the params that make it to GitHub.
Can you clarify @begedin?
There was a problem hiding this comment.
It should work regardless, I'll update.
There was a problem hiding this comment.
@joshsmith To clarify, the controller action was implemented with the code and state in the signature. I've moved attrs into a module variable, so it doesn't imply that specific values are important.
However, the action will only work with attributes containing code and state keys.
Not sure if I should loosen that requirement, or write a test for it. Either way, this PR is about closing #790. If we want to loosen the requirement, then that requires a rewrite of GitHub.User as well, so I would do it separately. Adding a test could go here, but this would be the first time we're writing a test for such a case.
There was a problem hiding this comment.
This addresses totally what I was taking issue with, thanks!
| def request(method, url, headers, body, options) do | ||
| case {method, url} |> for_access_token?() do | ||
| true -> SuccessAPI.request(method, url, headers, body, options) | ||
| false -> |
There was a problem hiding this comment.
I wonder if there's a way to metaprogram this and use an @impl or something?
begedin
force-pushed
the
790-connect-action-error-handling
branch
from
3cf9fa6 to
2aabd9e
Compare
joshsmith
force-pushed
the
790-connect-action-error-handling
branch
from
a90b6fc to
ff4a4db
Compare
2 participants
Closes #790
Makes use of the same
FailureAPImock module used in #805 and #925, so depending on merge order, there may be an easily resolvable conflict.I did not add a test for the changeset case because it doesn't seem to be possible for it to happen, since the only validations are those for presence of github keys, and these cannot be nil.