Fix mixed column type error when combining paginated pages (DP-1547)

[jimmyday12]

Summary

• Bug: When paginating a large export, `readr::type_convert()` runs independently on each page. If a field (e.g. Post Code) is numeric on some pages but blank on others, per-page type inference produces `` on numeric pages and `` on blank pages — `dplyr::bind_rows()` then throws `Can't combine `..1$Post Code` and `..18$Post Code` `.
• Fix: In `.combine_paginated_pages()`, coerce all page columns to character before `bind_rows()`, then re-run `readr::type_convert()` on the full combined dataset. Types are now inferred from all values at once rather than page-by-page, and blank values correctly become `NA` rather than causing a type conflict.
• Tests: Two new regression tests — one reproduces the exact `Can't combine` error with the old code path, one verifies the fix with correct output types. The `guess_col_type = FALSE` path is also covered.

Relates to

DP-1547 (NSWIS — mixed column types when combining paginated pages)

Test plan

• `devtools::test()` passes (15/15 in `test-export_handler.R`)
• Install package and run `sb_get_event()` against a form with blank fields across page boundaries
• Verify blank field values appear as `NA` in the returned tibble rather than erroring

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Fixed paginated exports failing when pages had mismatched column types by applying safer, per-page binding and consistent type alignment.
  • When type detection is enabled, time columns are preserved during conversion and restored afterward; when disabled, only identifier metadata columns are coerced to numeric while other columns keep their expected formats.

• Tests

  • Added regression coverage for mixed-type “Post Code” inputs and for behavior with type detection both enabled and disabled.

[coderabbitai]

📝 Walkthrough

Walkthrough

R/export_handler.R expands .combine_paginated_pages() to coerce all page columns to character before bind_rows(), then optionally re-infers types via readr::type_convert() while preserving start_time/end_time. When guess_col_type is disabled, metadata ID columns are coerced to numeric. New tests reproduce the numeric-vs-blank type conflict and assert correct behavior for both guess_col_type settings.

Changes

Paginated page combining type-coercion fix

Layer / File(s)

Summary

Type-coercion fix and regression tests R/export_handler.R, tests/testthat/test-export_handler.R

.combine_paginated_pages() now coerces all columns to character before bind_rows(). When guess_col_type is enabled, start_time/end_time are temporarily removed, remaining columns are re-typed via readr::type_convert(), and time columns are re-attached. When disabled, ID columns (user_id, entered_by_user_id, event_id) are coerced to numeric. Tests add a make_mock_page() helper and cover both guess_col_type=TRUE (Post Code → double, blanks → NA) and guess_col_type=FALSE (Post Code stays character, user_id stays double).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Teamworksapp/smartabaseR#24: Both PRs modify R/export_handler.R's .combine_paginated_pages() logic to correctly combine paginated page tibbles (including fixing type-mismatch behavior during bind_rows and preserving metadata/time column types).

Suggested reviewers

• rparastw

Poem

🐇 When pages arrive with types all in a mess,
I coerce them to char — no more distress!
Then type_convert restores what was meant to be,
With start_time and end_time kept safe, just for me.
Blank cells turn to NA, doubles emerge true —
No more bind_rows errors, hurrah, we're brand new! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly describes the main change: fixing a mixed column type error that occurs when combining paginated pages, and includes the ticket reference (DP-1547).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/mixed-col-types-pagination

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/testthat/test-export_handler.R (1)

102-118: ⚡ Quick win

Add an assertion for metadata ID types in the guess_col_type = FALSE test.

This test currently validates only Post Code. Please also assert user_id remains numeric to guard the documented metadata-column contract in paginated mode.

Suggested assertion

   expect_s3_class(result, "data.frame")
   expect_equal(nrow(result), 4L)
   # Without re-typing, column stays character (coerced from double then not re-converted)
   expect_type(result$`Post Code`, "character")
+  expect_type(result$user_id, "double")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/testthat/test-export_handler.R` around lines 102 - 118, The test for
`.combine_paginated_pages() with guess_col_type = FALSE` currently only
validates that the `Post Code` column remains character type, but it should also
assert that metadata ID columns like `user_id` maintain their numeric type to
verify the documented metadata-column contract in paginated mode. Add an
additional expect_type assertion after the existing `Post Code` check to verify
that result$user_id remains numeric, ensuring metadata columns are not affected
by the guess_col_type setting.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@R/export_handler.R`:
- Around line 200-213: All columns in the combined data frame are coerced to
character type in the bind_rows operation (lines 200-205), but when
guess_col_type is FALSE, there is no restoration step to convert metadata ID
columns back to numeric type, violating the documented output contract. After
the conditional block that handles type conversion when
isTRUE(arg$option$guess_col_type), add logic to ensure metadata ID columns are
converted back to numeric type regardless of the guess_col_type setting. This
can be done by identifying the ID columns (such as those ending in "_id" or
explicitly defined metadata ID column names) and using dplyr::mutate with
as.numeric to restore their proper types after the combined data frame is
finalized.

---

Nitpick comments:
In `@tests/testthat/test-export_handler.R`:
- Around line 102-118: The test for `.combine_paginated_pages() with
guess_col_type = FALSE` currently only validates that the `Post Code` column
remains character type, but it should also assert that metadata ID columns like
`user_id` maintain their numeric type to verify the documented metadata-column
contract in paginated mode. Add an additional expect_type assertion after the
existing `Post Code` check to verify that result$user_id remains numeric,
ensuring metadata columns are not affected by the guess_col_type setting.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cb75c108-a1e9-404f-ba2f-2cf755e2d5cf

📥 Commits

Reviewing files that changed from the base of the PR and between fdef2ff and b3e2986.

📒 Files selected for processing (2)

• R/export_handler.R
• tests/testthat/test-export_handler.R