-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add pagintion to backfills index and service show pages #422
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ build/ | |
*.iml | ||
*.swp | ||
service/web/**/lib/ | ||
**/.kotlin | ||
|
||
**/site/ | ||
docs/0.x/* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,11 @@ | ||
GROUP=app.cash.backfila | ||
|
||
org.gradle.jvmargs=-Xmx6g -XX:MaxMetaspaceSize=5g -Dfile.encoding=UTF-8 | ||
|
||
org.gradle.caching=true | ||
org.gradle.configuration-cache=true | ||
org.gradle.vfs.watch=true | ||
org.gradle.configureondemand=true | ||
org.gradle.parallel=true | ||
|
||
misk.test.logging=false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package app.cash.backfila.ui.components | ||
|
||
import kotlinx.html.TagConsumer | ||
import kotlinx.html.a | ||
import kotlinx.html.div | ||
import kotlinx.html.nav | ||
import misk.tailwind.icons.Heroicons | ||
import misk.tailwind.icons.heroicon | ||
|
||
fun TagConsumer<*>.Pagination( | ||
nextOffset: String?, | ||
offset: String?, | ||
lastOffset: String?, | ||
basePath: String, | ||
) { | ||
nav("mt-12 flex items-center justify-between border-t border-gray-200 px-4 sm:px-0") { | ||
lastOffset?.let { | ||
div("-mt-px flex w-0 flex-1") { | ||
a(classes = "inline-flex items-center border-t-2 border-transparent pt-4 pr-1 text-sm font-medium text-gray-500 hover:border-gray-300 hover:text-gray-700") { | ||
href = basePath.appendOffsets(lastOffset) | ||
heroicon(Heroicons.MINI_ARROW_LONG_LEFT) | ||
+"""Previous""" | ||
} | ||
} | ||
} | ||
|
||
if (nextOffset != null) { | ||
div("-mt-px flex w-0 flex-1 justify-end") { | ||
a(classes = "inline-flex items-center border-t-2 border-transparent pt-4 pl-1 text-sm font-medium text-gray-500 hover:border-gray-300 hover:text-gray-700") { | ||
href = basePath.appendOffsets(nextOffset, offset ?: "") | ||
+"""Next""" | ||
heroicon(Heroicons.MINI_ARROW_LONG_RIGHT) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
fun String.appendOffsets(nextOffset: String? = null, lastOffset: String? = null): String { | ||
val pathBuilder = StringBuilder(this) | ||
if (nextOffset?.isNotBlank() == true) { | ||
pathBuilder.append("?offset=").append(nextOffset) | ||
} | ||
|
||
if (lastOffset != null) { | ||
if (pathBuilder.contains("?")) { | ||
pathBuilder.append("&") | ||
} else { | ||
pathBuilder.append("?") | ||
} | ||
pathBuilder.append("lastOffset=").append(lastOffset) | ||
} | ||
return pathBuilder.toString() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import app.cash.backfila.service.persistence.BackfillRunQuery | |
import app.cash.backfila.service.persistence.ServiceQuery | ||
import app.cash.backfila.ui.components.DashboardPageLayout | ||
import app.cash.backfila.ui.components.PageTitle | ||
import app.cash.backfila.ui.components.Pagination | ||
import javax.inject.Inject | ||
import javax.inject.Singleton | ||
import kotlinx.html.ButtonType | ||
|
@@ -19,8 +20,13 @@ import kotlinx.html.ul | |
import misk.hibernate.Query | ||
import misk.hibernate.Transacter | ||
import misk.hibernate.newQuery | ||
import misk.hibernate.pagination.Offset | ||
import misk.hibernate.pagination.Page | ||
import misk.hibernate.pagination.idDescPaginator | ||
import misk.hibernate.pagination.newPager | ||
import misk.security.authz.Authenticated | ||
import misk.web.Get | ||
import misk.web.QueryParam | ||
import misk.web.Response | ||
import misk.web.ResponseBody | ||
import misk.web.ResponseContentType | ||
|
@@ -36,14 +42,18 @@ class BackfillIndexAction @Inject constructor( | |
@Get(PATH) | ||
@ResponseContentType(MediaTypes.TEXT_HTML) | ||
@Authenticated(capabilities = ["users"]) | ||
fun get(): Response<ResponseBody> { | ||
val backfills = transacter.transaction { session -> | ||
fun get( | ||
@QueryParam offset: String? = null, | ||
@QueryParam lastOffset: String? = null, | ||
): Response<ResponseBody> { | ||
val (backfills, nextOffset) = transacter.transaction { session -> | ||
queryFactory.newQuery<BackfillRunQuery>() | ||
.orderByUpdatedAtDesc() | ||
.apply { | ||
maxRows = 10 | ||
} | ||
.list(session) | ||
.newPager( | ||
idDescPaginator(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't you want to order by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out the old impl just used the id so I'll stick with that pattern since it matches status quo and is easier. |
||
initialOffset = offset?.let { Offset(it) }, | ||
pageSize = 12, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - put the page size in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going to punt on this to match the existing style across other callsites, but generally agree with this approach of moving to companion object |
||
) | ||
.nextPage(session) ?: Page.empty() | ||
} | ||
|
||
return Response( | ||
|
@@ -62,7 +72,7 @@ class BackfillIndexAction @Inject constructor( | |
} | ||
|
||
// List of Services | ||
div("py-10") { | ||
div("py-5") { | ||
ul("grid grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3") { | ||
role = "list" | ||
|
||
|
@@ -98,6 +108,8 @@ class BackfillIndexAction @Inject constructor( | |
} | ||
} | ||
} | ||
|
||
Pagination(nextOffset?.offset, offset, lastOffset, PATH) | ||
} | ||
}, | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
package app.cash.backfila | ||
|
||
import app.cash.backfila.api.ConfigureServiceAction | ||
import app.cash.backfila.api.ConfigureServiceAction.Companion.RESERVED_VARIANT | ||
import app.cash.backfila.client.Connectors | ||
import app.cash.backfila.dashboard.CreateBackfillAction | ||
import app.cash.backfila.protos.service.ConfigureServiceRequest | ||
import app.cash.backfila.protos.service.CreateBackfillRequest | ||
import app.cash.backfila.service.persistence.BackfilaDb | ||
import app.cash.backfila.service.persistence.BackfillRunQuery | ||
import jakarta.inject.Inject | ||
import misk.hibernate.Query | ||
import misk.hibernate.Transacter | ||
import misk.hibernate.newQuery | ||
import misk.hibernate.pagination.idDescPaginator | ||
import misk.hibernate.pagination.newPager | ||
import misk.scope.ActionScope | ||
import misk.testing.MiskTest | ||
import misk.testing.MiskTestModule | ||
import org.junit.jupiter.api.Test | ||
|
||
@MiskTest(startService = true) | ||
class PaginationTest { | ||
@MiskTestModule | ||
private val module = BackfilaTestingModule() | ||
|
||
@Inject @BackfilaDb | ||
private lateinit var transacter: Transacter | ||
|
||
@Inject private lateinit var queryFactory: Query.Factory | ||
|
||
@Inject private lateinit var configureServiceAction: ConfigureServiceAction | ||
|
||
@Inject private lateinit var createBackfillAction: CreateBackfillAction | ||
|
||
@Inject private lateinit var scope: ActionScope | ||
|
||
@Test | ||
fun happyPath() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we inject the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I'll go ahead and delete this test class, I was just using it to play with the Paginator and have a debugger. |
||
// Seed rows | ||
seedData() | ||
|
||
// Test pagination | ||
val allRows = transacter.transaction { session -> | ||
queryFactory.newQuery<BackfillRunQuery>() | ||
.list(session) | ||
} | ||
val rows = transacter.transaction { session -> | ||
val pager = queryFactory.newQuery<BackfillRunQuery>() | ||
.orderByUpdatedAtDesc() | ||
.newPager(idDescPaginator(), pageSize = 10) | ||
pager.nextPage(session) | ||
?.contents | ||
} | ||
|
||
val a = "a" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unneeded? |
||
} | ||
|
||
fun seedData() { | ||
scope.fakeCaller(service = "deep-fryer") { | ||
configureServiceAction.configureService( | ||
ConfigureServiceRequest.Builder() | ||
.backfills( | ||
listOf( | ||
ConfigureServiceRequest.BackfillData( | ||
"ChickenSandwich", "Description", listOf(), null, | ||
null, false, null, | ||
), | ||
), | ||
) | ||
.connector_type(Connectors.ENVOY) | ||
.build(), | ||
) | ||
} | ||
|
||
for (i in 0..100) { | ||
scope.fakeCaller(user = "molly") { | ||
val response = createBackfillAction.create( | ||
"deep-fryer", | ||
RESERVED_VARIANT, | ||
CreateBackfillRequest.Builder() | ||
.backfill_name("ChickenSandwich") | ||
.build(), | ||
) | ||
} | ||
} | ||
} | ||
} |
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.
nit -
readOnly()
. Could also usereplicaRead
since the replication lag should be acceptable for querying live statusThere 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.
@mpawliszyn does Backfila have read replicas in staging/production?
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.
I think for now the simplicity is worth more than the performance.