2. Create OAuth callback action #10
Conversation
tests/Feature/HttpRequestMatcher.php
Outdated
| @@ -0,0 +1,125 @@ | |||
| <?php | |||
There was a problem hiding this comment.
This is copied from our library, since test files are not exported. It might be a good idea to expose some test helpers with the library
There was a problem hiding this comment.
Perhaps we don't need this test to be so 'deep', and we can only check that we get a POST to the expected address without necessarily checking everything else - we already check that the library fires off the appropriate requests in its own repo, and we're more interested in whether the session is properly set up rather than whether the access token fetch has all the expected params.
There was a problem hiding this comment.
That's actually a very good point 👍
MuhammadFarag
changed the title
routes/web.php
Outdated
|
|
||
| Route::get('/auth/callback', function (Request $request) { | ||
| OAuth::callback($request->cookie(), $request->query()); | ||
| return redirect('/'); |
There was a problem hiding this comment.
I think we'll probably want to include the shop query param in this redirection, otherwise we'll end up failing in the 'main' action which needs to check if the shop is there.
tests/Feature/HttpRequestMatcher.php
Outdated
| @@ -0,0 +1,125 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Perhaps we don't need this test to be so 'deep', and we can only check that we get a POST to the expected address without necessarily checking everything else - we already check that the library fires off the appropriate requests in its own repo, and we're more interested in whether the session is properly set up rather than whether the access token fetch has all the expected params.
MuhammadFarag
force-pushed
the
update-sesssion-storage
branch
from
283a91d
to
eda87f2
Compare
| $response->assertRedirect("?shop=$this->domain"); | ||
| } | ||
|
|
||
| private function mockClient(): ClientInterface|MockObject |
There was a problem hiding this comment.
I like that we do this generically so we don't accidentally run actual requests. We may want to run it for every test so we're guaranteed not to fire actual requests.
tests/Feature/CallbackTest.php
Outdated
|
|
||
| $client->expects($this->exactly(1)) | ||
| ->method('sendRequest') | ||
| ->with($matcher) | ||
| ->with($this->anything()) |
There was a problem hiding this comment.
Would it be overkill to make this a callback and just check that we're POSTing to the access token address? I'm leaning towards 'yes'.
There was a problem hiding this comment.
Done :) I am not a fan of callbacks, just because of the cryptic message we get on error. But, I am always a fan of more testing :D
tests/Feature/CallbackTest.php
Outdated
| $expectedSession = $this->buildExpectedSession($this->session->getId(), false); | ||
| $actualSession = Context::$SESSION_STORAGE->loadSession($this->session->getId()); | ||
| $this->assertEquals($expectedSession, $actualSession); |
There was a problem hiding this comment.
I think this is worth keeping around - we want to make sure the session is there in this scenario, and that it's setting the access token.
There was a problem hiding this comment.
I think this is internal to the library. The assumption that the library is working correctly, and the dbSessionStorage tests are working correctly.
No description provided.