Add ability to generate query params from table state#14
Conversation
📝 WalkthroughWalkthroughAdds URL query parameter serialization/deserialization for datatables: new TableStateQueryParams utilities, convenience methods on TableState, comprehensive tests, docs and example updates, plus a small test dependency addition in build.sbt. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TableState
participant TableStateQueryParams
participant URL
Client->>TableState: toQueryString()
TableState->>TableStateQueryParams: toQueryString(state)
TableStateQueryParams->>TableStateQueryParams: serialize filters/sort/page/pageSize
TableStateQueryParams->>URL: return query string
URL-->>Client: URL with query
Client->>URL: provide query string
URL->>TableStateQueryParams: fromQueryString(query)
TableStateQueryParams->>TableStateQueryParams: parse -> TableStateParams
TableStateQueryParams->>TableState: applyToState(state, params)
TableState-->>Client: updated TableState
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@build.sbt`:
- Around line 30-32: Move the "io.github.cquiroz" %%% "scala-java-time" %
"2.6.0" dependency out of the test scope and into the main JS classpath so JS
compilation can resolve java.time types used by shared code like
TableStateQueryParams (which imports java.time.LocalDate); update the existing
.jsSettings(...) entry that currently adds the dependency with % "test" to
instead add it to the main JS project scope (remove the test classifier) so the
dependency is available to CrossType.Pure shared sources.
In `@forms4s-core/src/main/scala/forms4s/datatable/TableStateQueryParams.scala`:
- Around line 161-172: Serialization currently emits identical keys for
different FilterState variants (FilterState.TextValue, SelectValue,
MultiSelectValue, BooleanValue) using FilterPrefix+columnId, causing ambiguous
parsing; change the serialization in TableStateQueryParams (the match arms for
FilterState.TextValue, SelectValue, MultiSelectValue, BooleanValue) to encode
the filter kind in the key (e.g. append ".text", ".select", ".multi", ".bool" to
s"$FilterPrefix$columnId"), update the corresponding parsing logic in the same
file to read that suffix and recreate the exact FilterState variant (or
alternatively accept a TableDef-based type hint in the parser if you prefer
inferring types), and adjust any unit tests/docs that assert round-trip behavior
to expect the new keyed format so select/text/boolean filters round-trip
faithfully.
🧹 Nitpick comments (1)
forms4s-core/src/test/scala/forms4s/datatable/TableStateQueryParamsSpec.scala (1)
217-280: Consider a select-filter round‑trip test.Round‑trip coverage is strong; adding a select filter round‑trip would guard against the TextValue/SelectValue ambiguity in parsing.
forms4s-core/src/main/scala/forms4s/datatable/TableStateQueryParams.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@forms4s-examples/src/main/scala/forms4s/example/docs/DatatableExample.scala`:
- Around line 52-59: The example comments are misleading because the shown
output assumes filters/sorting but the `tableState` used is empty; update the
snippet so `tableState` is mutated/constructed with the corresponding sort, page
and filters before calling `toQueryString`/`toQueryParams` (e.g. set sort to
"name:asc", page to 1, add filter f.department=Engineering and
f.salary.min=50000) so the printed example output matches the actual return
values of `tableState.toQueryString` and `tableState.toQueryParams`.
🧹 Nitpick comments (2)
forms4s-core/src/test/scala/forms4s/datatable/TableStateQueryParamsSpec.scala (1)
17-28: Missing test coverage forDateRangeValuefilters.The
TableDefsetup doesn't include a date column, and there are no tests forDateRangeFilterserialization/deserialization. The implementation inTableStateQueryParams.scala(lines 235-240, 265-268) supports date range filters with ISO-8601 format.Consider adding a date column to the test model and corresponding tests:
🧪 Suggested additions for DateRange coverage
// Add to Person case class: case class Person(name: String, age: Int, department: String, active: Boolean, tags: Set[String], hireDate: LocalDate) // Add import: import java.time.LocalDate // Add to tableDef columns: Column[Person, LocalDate]("hireDate", "Hire Date", _.hireDate, _.toString) .withFilter(ColumnFilter.dateRange(d => Some(d))), // Add test cases: "serializes date range filter - from only" in { val state = initialState.update(TableUpdate.SetFilter("hireDate", FilterState.DateRangeValue(Some(LocalDate.of(2020, 1, 1)), None))) assert(state.toQueryString == "f.hireDate.min=2020-01-01") } "date range filter roundtrip" in { val state = initialState.update(TableUpdate.SetFilter("hireDate", FilterState.DateRangeValue(Some(LocalDate.of(2020, 1, 1)), Some(LocalDate.of(2020, 12, 31))))) val qs = state.toQueryString val newState = initialState.loadFromQueryString(qs) assert(newState.filters("hireDate") == state.filters("hireDate")) }forms4s-core/src/main/scala/forms4s/datatable/TableStateQueryParams.scala (1)
149-172: Consider functional style forapplyToState.The method works correctly, but the imperative style with
varcould be refactored to a more functional approach usingfoldLeft. This is optional and purely stylistic.♻️ Optional: Functional refactor
def applyToState[T](state: TableState[T], params: TableStateParams): TableState[T] = { - var updated = state - - params.filters.foreach { case (columnId, parsedValue) => - val filterState = toFilterState(parsedValue, state.definition, columnId) - filterState.foreach { fs => - updated = updated.update(TableUpdate.SetFilter(columnId, fs)) - } - } - - params.sort.foreach { s => - updated = updated.update(TableUpdate.SetSort(s.columnId, s.direction)) - } - - params.pageSize.foreach { size => - updated = updated.update(TableUpdate.SetPageSize(size)) - } - - params.page.foreach { p => - updated = updated.update(TableUpdate.SetPage(p)) - } - - updated + val withFilters = params.filters.foldLeft(state) { case (s, (columnId, parsedValue)) => + toFilterState(parsedValue, state.definition, columnId) + .map(fs => s.update(TableUpdate.SetFilter(columnId, fs))) + .getOrElse(s) + } + val withSort = params.sort.foldLeft(withFilters) { (s, sort) => + s.update(TableUpdate.SetSort(sort.columnId, sort.direction)) + } + val withSize = params.pageSize.foldLeft(withSort) { (s, size) => + s.update(TableUpdate.SetPageSize(size)) + } + params.page.foldLeft(withSize) { (s, p) => + s.update(TableUpdate.SetPage(p)) + } }
Solves #13
Summary by CodeRabbit
New Features
Documentation
Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.