Feature/get profile by username #186
Conversation
There was a problem hiding this comment.
Nice work @MarinJuricev! Even with tests 🙌
1 remark, and two nits
Thanks for contributing, and happy hacktoberfest 👻
| import io.ktor.http.HttpStatusCode | ||
| import io.ktor.http.contentType | ||
|
|
||
| class ProfileRouteSpec : |
There was a problem hiding this comment.
We're missing a test case for when the path parameter is missing, I think that would've resulted MissingRequestParameterException case.
There was a problem hiding this comment.
I assume that the endpoint that we'd want to hit/test is api/profiles/ and assert that our new validation is triggered
However, I'm having difficulties actually hitting the above endpoint with the current Resource setup, I get 404 not found.
I've read through the docs, and tried with the ? approach, but with no luck.
@Resource("/{$USERNAME?}")
data class Username(val parent: ProfileResource = ProfileResource(), val username: String)Do you have any suggestions?
There was a problem hiding this comment.
Strange, I made a small change and it's working fine for me.
There was a problem hiding this comment.
I'm probably missing something obvious, I've tried various things to trigger the api/profiles logic.
The username implementation is marked as optional and a placeholder test that fails on my side is added here
I've tried also to write it without the @Resource implementation so that I just hit /api/profiles, but with no luck 🤷
There was a problem hiding this comment.
For me it's working with the small change I suggested. Could you check if this works for you, if not can you add the failing test case so I can try and reproduce it locally?
Sorry for the delay in reviewing, I was sick this week 🤒
Thanks again for your interest, and contribution!
| import io.ktor.http.HttpStatusCode | ||
| import io.ktor.http.contentType | ||
|
|
||
| class ProfileRouteSpec : |
There was a problem hiding this comment.
Strange, I made a small change and it's working fine for me.
| @@ -0,0 +1,42 @@ | |||
| package io.github.nomisrev.routes | |||
There was a problem hiding this comment.
I had to add this for detekt, I'd prefer not to change the filename. Perhaps we should disable this rule in some other issue.
| package io.github.nomisrev.routes | |
| @file:Suppress("MatchingDeclarationName") | |
| package io.github.nomisrev.routes |
There was a problem hiding this comment.
Agreed, having to force every routes implementation to add the @file:Suppress for detekt isn't deal, something for a future PR 😄
Addressed in 76bea8b
| get<ProfileResource.Username> { | ||
| either { | ||
| val username = parameter(USERNAME) { it }.bind() | ||
| repo.selectProfile(username).bind() | ||
| } | ||
| .respond(HttpStatusCode.OK) | ||
| } |
There was a problem hiding this comment.
Type safe routing from Ktor gives us acces to an ProfileResource.Username instance, and thus safe acces to username.
Spotless also required a small formatting.
| get<ProfileResource.Username> { | |
| either { | |
| val username = parameter(USERNAME) { it }.bind() | |
| repo.selectProfile(username).bind() | |
| } | |
| .respond(HttpStatusCode.OK) | |
| } | |
| get<ProfileResource.Username> { route -> | |
| either { repo.selectProfile(route.username).bind() }.respond(HttpStatusCode.OK) | |
| } |
There was a problem hiding this comment.
Wasn't aware of that TIL, addressed in: 8c1113f
…ers is misleading, we only extract one with that API
…ject inside the type safe API
MarinJuricev
force-pushed
the
feature/GET-profile-by-username
branch
from
8b1f498
to
64dbe4d
Compare
There was a problem hiding this comment.
Perfect @MarinJuricev! Thank you so much for the contribution 👏 👏 👏
|
Awesome, thanks for your time @nomisRev had a blast doing this mini contribution. I'll try to snag another issue in the near future 😄 |
This PR attempts to solve the following issue 154
Missing piece:
Get profile with following( at the time of this PR the FollowPersistence isn't implemented ) and that's preventing us from setting that flow inProfileRouteSpecAny feedback is welcome as BE/Ktor is not my strong suite.