-
Notifications
You must be signed in to change notification settings - Fork 12
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
Version 4.37 #131
Version 4.37 #131
Conversation
nickdickinson
commented
Sep 23, 2024
•
edited by jamiewhths
Loading
edited by jamiewhths
- Legacy roles are no longer supported from 4.37 onwards
- Potential breaking change: the maxDepth parameter in column styles is now set to two by default; parent and reference columns no longer expand indefinitely
- New functions for new role and permissions system (Support role management for new role scheme #114)
- Added ability to update sub-forms (formSchema() should accept parentFormId argument #119)
- Addressed cyclic structures in reference columns and parent forms and added maxDepth parameter to column styles. (Recursion error in getRecords() when evaluating header names for certain form schema #132)
…. Added temp.R tests with some of these cases.
…ssions, renaming adminPermissions to managementPermissions, various fixes, changing handling of resources in the canonization of AI API objects, fixes and additions to database tests and snapshots
Merge remote-tracking branch 'origin/master' into version-4.37 # Conflicts: # NEWS.md # tests/testthat/_snaps/databases.md
V4.37 under construction
… and adding maxDepth to styles; Some additional tests
R/forms.R
Outdated
#' @export | ||
createFormSchemaFromData <- function(x, databaseId, label, folderId = databaseId, keyColumns = character(), requiredColumns = keyColumns, logicalAsSingleSelect = TRUE, logicalText = c("True","False"), codes = rep(NA_character_, ncol(x)), upload = FALSE) { | ||
createFormSchemaFromData <- function(x, databaseId, label, folderId, keyColumns = character(), requiredColumns = keyColumns, logicalAsSingleSelect = TRUE, logicalText = c("True","False"), codes = rep(NA_character_, ncol(x)), upload = FALSE, parentId = folderId, parentFormId = NULL, parentIdColumn = NULL) { |
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.
rename parentIdColumn
to parentRecordIdColumn
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 noted an inconsistency. In importRecords we use parentIdColumn but I think we should rename this to be parentRecordIdColumn to match and to be consistent also with submitPending() and addRecord(). Since it would be a breaking change, I'll just deprecate the old parameter.
R/forms.R
Outdated
stopifnot("Some key columns do not exist in the data.frame provided" = keyColumns %in% providedCols) | ||
stopifnot("Some required columns do not exist in the data.frame provided" = keyColumns %in% providedCols) | ||
|
||
fmSchema <- formSchema(databaseId = databaseId, label = label, folderId = folderId) | ||
stopifnot("The parentIdColumn does not exist in the data.frame provided" = parentIdColumn %in% providedCols) |
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.
check this logic
R/forms.R
Outdated
if(!missing(folderId)) { | ||
if (!missing(parentId) && !is.null(parentId) && folderId != parentId) { | ||
warning("folderId does not match parentId. folderId is deprecated in createFormSchemaFromData(). Ignoring folderId and using parentId.") | ||
folderId = parentId |
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.
This is odd, we should only be doing this when parentId is NULL and folderId is provided
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.
This needs to be the same logic as formSchema
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.
This is definitely a bug as folderId is now never provided when uploading schema
R/records.R
Outdated
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.
Lets add a test to cover long reference branches and cyclical references, to ensure both are handled safely by these methods
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.
Also, make sure the expression built by these column styles use form/field ids in all cases even when field codes have been defined, as these are guaranteed to be generate correct expressions.
man/allColumnStyle.Rd
Outdated
@@ -4,10 +4,12 @@ | |||
\alias{allColumnStyle} | |||
\title{A form table style including all columns with configurable label names} | |||
\usage{ | |||
allColumnStyle(columnNames = c("code", "label")) | |||
allColumnStyle(columnNames = c("code", "label"), maxDepth = 3) |
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.
maxDepth should default to 2 in all cases. This will allow users to pull in fields from directly reference forms which probably covers the vast majority of cases.
tests/testthat/testthat-problems.rds
Outdated
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.
might need to add to .gitignore?
…n importRecords() and createFormSchemaFromData(); Other fixes to createFormSchemaFromData() logic and checks; added regex sanitation check for ids in formFieldSchema(); deprecation warning in roleFilter(); update to News; delete testhat-problems.rds
… parentIdColumn in importRecords(); Tests for the number of columns of deeply nested subforms and references and cycles with reference fields; documentation update