Skip to content

Conversation

@labkey-matthewb
Copy link
Contributor

@labkey-matthewb labkey-matthewb commented Oct 28, 2025

Rationale

Simplify TableViewForm with an eye toward making array binding less convoluted

Related Pull Requests

Changes

@labkey-matthewb labkey-matthewb requested a review from XingY October 31, 2025 17:29
@XingY XingY requested a review from Copilot November 2, 2025 21:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request refactors TableViewForm and related classes to modernize the form data binding API. The key changes include:

  • Removing the DynaBean interface from TableViewForm (moved to BeanViewForm only)
  • Replacing deprecated set() and get() methods with setValueToBind() and getAsString()
  • Changing internal storage from Map<String, String> to Map<String, Object> to support arrays and other types
  • Deleting obsolete TableWrapperDynaClass and QueryWrapperDynaClass classes
  • Refactoring string array handling to support Google Sheets-compatible multi-value parsing
  • Updating method signatures to use proper generic types (Class<?> instead of raw Class)

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
api/src/org/labkey/api/data/TableViewForm.java Core refactoring: removed DynaBean interface, changed API from get/set to getAsString/setValueToBind, updated internal storage to support non-string values
api/src/org/labkey/api/data/BeanViewForm.java Restored DynaBean interface support here, delegating to parent's new API methods
api/src/org/labkey/api/data/ConvertHelper.java Updated string array converter to use new Google Sheets-compatible parsing
api/src/org/labkey/api/util/PageFlowUtil.java Added Google Sheets-compatible multi-value parsing/formatting methods, replaced StringUtils with Strings.CS for case-sensitive operations
study/src/.../StudyPropertiesController.java Updated to use new setValueToBind API
study/src/.../StudyController.java Updated to use new getAsString API
study/src/.../CohortController.java Updated to use new setValueToBind API
issues/src/.../IssuesController.java Updated to use new getValuesToBind/setValuesToBind and getAsString APIs
core/src/.../TableViewFormTestCase.java Added comprehensive tests for array and boolean binding, updated existing tests
api/src/.../StringBeanDynaClass.java Fixed raw type warnings, removed commented code
api/src/.../ColumnRenderPropertiesImpl.java Refactored getJavaClass to use static helper method
api/src/.../*DisplayColumn.java Updated multiple display columns to use new API
announcements/src/.../insert.jsp, respond.jsp Updated JSP files to use getAsString
api/src/.../TableWrapperDynaClass.java Deleted obsolete class
api/src/.../QueryWrapperDynaClass.java Deleted obsolete class
Comments suppressed due to low confidence (4)

api/src/org/labkey/api/data/TableViewForm.java:1

  • Missing spaces around the '=' operator. Should be 'int i = p.getIndex();' to follow Java coding conventions.
/*

api/src/org/labkey/api/data/TableViewForm.java:1

  • Missing spaces around operators for consistency. Should be 'return i < s.length - 1 ? s[i + 1] : '\0';'.
/*

api/src/org/labkey/api/data/TableViewForm.java:1

  • Missing space around the '-' operator. Should be 'if (i >= s.length - 1)' for consistency.
/*

api/src/org/labkey/api/data/TableViewForm.java:1

  • Missing spaces around '+' operators. Should be 'p.setIndex(i + 1);' and 'return s[i + 1];' for consistency.
/*

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@labkey-matthewb labkey-matthewb merged commit 476a7ed into develop Nov 3, 2025
6 of 9 checks passed
@labkey-matthewb labkey-matthewb deleted the fb_tvf_string_array branch November 3, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants