-
Notifications
You must be signed in to change notification settings - Fork 534
DD-2075: Hidden fields of a dataset creation form remain visible AND setting a field to 'hidden' is not working #11992 #12017
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
base: develop
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -244,7 +244,7 @@ | |||||||||||
| <div class="panel-body"> | ||||||||||||
| <ui:repeat value="#{metadataBlockVal.value}" var="dsf" varStatus="curField"> | ||||||||||||
| <div class="form-group" role="group" | ||||||||||||
| jsf:rendered="#{((editMode == 'METADATA' or dsf.datasetFieldType.shouldDisplayOnCreate() or !dsf.isEmpty() or dsf.required or dsf.hasRequiredChildren)) or (!datasetPage and dsf.include)}"> | ||||||||||||
| jsf:rendered="#{((editMode == 'METADATA' or dsf.datasetFieldType.shouldDisplayOnCreate() or !dsf.isEmpty() or dsf.required or dsf.hasRequiredChildren) and dsf.include) or (!datasetPage and dsf.include)}"> | ||||||||||||
|
Member
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. I haven't tested but this seems odd - if dsf.isEmpty() is false (there is a value), we wouldn't display it if dsf.include is false? Should it be "or dsf.include" ?
Contributor
Author
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. The fix was done here by comparing the statement with it before modification in 6.7 and set back 'and 6.5 6.7 fix:
Member
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. OK. I think I've convinced myself that, because of dataverse/src/main/java/edu/harvard/iq/dataverse/DatasetPage.java Lines 1907 to 1911 in a843c84
and dsf.include won't break displaying existing values. (It does make me wonder whether or !dsf.isEmpty() is needed though (if you're not in metadata mode, could you have a pre-existing value that has to be displayed? Probably not worth worrying about.)
|
||||||||||||
| <!-- Primitive fields --> | ||||||||||||
| <p:fragment id="editPrimitiveValueFragment" rendered="#{dsf.datasetFieldType.primitive}"> | ||||||||||||
| <p:autoUpdate/> | ||||||||||||
|
|
||||||||||||
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 makes sense for the given use case - trying to hide a field. TLDR: looks like a good fix.
I think I've convinced myself that we can't have the opposite case - a field that is hidden by metadatablock definition and needs to be turned on. If that was a possibility, we'd need something like a refactor to make include a nullable Boolean (like localDsiplayOnCreate). Further, it makes me wonder if we also would have a problem if a field were defined as required and someone wanted to override that. However, in that case it looks like we don't let people change required to false in the UI - the field shows as 'required by Dataverse' instead of having radio buttons. So - in both cases, it looks like there isn't currently a way to create the cases that would break the logic.