-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HCX-50 Add Jasper report connector #71
Conversation
e47b2c4
to
f40a152
Compare
pom.xml
Outdated
<version>${ktor.version}</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.koin</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused dep
import io.ktor.client.engine.apache.Apache | ||
import io.ktor.client.features.auth.Auth | ||
import io.ktor.client.features.auth.providers.* | ||
import io.ktor.client.statement.HttpResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused?
import io.ktor.client.features.defaultRequest | ||
import io.ktor.http.HttpMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems unused
package org.camunda.latera.bss.HttpClient | ||
|
||
import io.ktor.client.request.* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why empty line?
return HttpClient(MockEngine) { | ||
engine { | ||
addHandler { request -> | ||
println(request.url.fullPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
} | ||
"/reportExecutions" -> { | ||
val responseHeaders = headersOf("Content-Type" to listOf(ContentType.Application.Json.toString())) | ||
respond("{\"currentPage\":1,\"exports\":[{\"id\":\"html\",\"status\":\"queued\"}],\"reportURI\":\"/supermart/details/CustomerDetailReport\",\"requestId\":\"f3a9805a-4089-4b53-b9e9-b54752f91586\",\"status\":\"execution\"}", headers = responseHeaders) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we have any more pretty/convenient way to describe the response?
val requestId: String, | ||
val status: String) | ||
|
||
class JasperReport(val jasperUrl: String, val jasperUser: String, val jasperPassword: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure you should precede every param with jasper
, it's kinda obvious
|
||
class JasperReport(val jasperUrl: String, val jasperUser: String, val jasperPassword: String) { | ||
fun executeReport(params: executeReportParams) : executeReportResponse { | ||
val jasperUrl = "${this.jasperUrl}/reportExecutions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same name confuses a bit
test/kotlin/JasperTest.kt
Outdated
assertThat(result.currentPage).isEqualTo(1) | ||
assertThat(result.reportURI).isEqualTo("/supermart/details/CustomerDetailReport") | ||
assertThat(result.requestId).isEqualTo("f3a9805a-4089-4b53-b9e9-b54752f91586") | ||
assertThat(result.status).isEqualTo("execution") | ||
assertThat(result.exports[0].id).isEqualTo("html") | ||
assertThat(result.exports[0].status).isEqualTo("queued") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we compare maps?
data class executeReportParams(val reportUnitUri: String, | ||
val parameters: Map<String, Any>, | ||
val outputFormat: String = "pdf", | ||
val freshData: Boolean = false, | ||
val saveDataSnapshot: Boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's capitalize data class names
also there is enough place to land my plane :)
,.
___( |___
`--- \---'
| \
\ \
__________________\ \___________________
\ \ \
------------------ \-------------------
(\) \ \ (\)
\ ' \
:`(.)-'
` ; `,
|
||
class JasperReport(val url: String, val user: String, val password: String) { | ||
fun executeReport(params: executeReportParams) : executeReportResponse { | ||
val executeReportUrl = "${this.url}/reportExecutions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have trailing slash in this.url
, so let's use any URL builder to avoid duplicated slash.
No description provided.