DD-2075: Hidden fields of a dataset creation form remain visible AND setting a field to 'hidden' is not working #11992

[aliassheikh]

DD-2075: Hidden fields of a dataset creation form remain visible AND setting a field to 'hidden' is not working #11992

What this PR does / why we need it:

We are in the process of upgrading to version 6.7.1.
While testing, I noticed that if I set a field (in this case from the citation block) to hidden, this does not apply.
These are the steps I followed:

• Create a new dataverse
• Set the fields 'Series' and 'related material' to hidden by unchecking the checkboxes.
• Click done.
• Click 'save changes'.
• Add a dataset. I expected these fields not to be visible when creating a new one, but I still see them.

Which issue(s) this PR closes:

• Closes Setting a field to 'hidden' is not working #11992

Special notes for your reviewer:

Suggestions on how to test this:
See the instructions above, but add creating a dataset in your test collection that has a value in one of your test fields first. Then, as above, try to mark a field as hidden in the JSF UI, save, verify that the collection edit display shows it as hidden and that the field is not displayed when editing metadata in a new dataset, and that the existing value still displays in the existing dataset.
Should also check that the relevant API works - perhaps that's easiest by editing in the SPA. In addition to setting whether a field is hidden, regression testing should probably also assure that setting displayOnCreate still works.

FYI: I requested a release note but approved already, so also check that one was created.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

[janvanmansum]

@qqmyers : would you have a look at this?

[coveralls]

coverage: 24.184%. remained the same
when pulling fe42efb on DANS-KNAW:DD-2075-IQSS-11992
into 0ee4268 on IQSS:develop.

[qqmyers]

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" ?

[aliassheikh]

The fix was done here by comparing the statement with it before modification in 6.7 and set back 'and dsf.include' in the statement. To avoid any regression, nothing else is changed in this fix.

6.5

jsf:rendered="#{(
                (editMode == 'METADATA' or dsf.datasetFieldType.displayOnCreate        or !dsf.isEmpty() or dsf.required) and dsf.include)
                or (!datasetPage and dsf.include)}">

6.7

jsf:rendered="#{(
                (editMode == 'METADATA' or dsf.datasetFieldType.shouldDisplayOnCreate() or !dsf.isEmpty() or dsf.required or dsf.hasRequiredChildren))
                or (!datasetPage and dsf.include)}">

fix:

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)}">

[qqmyers]

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

	for ( DatasetField dsf : mapDatasetFields.values()) {
	if (dsf != null){
	dsf.setInclude(true);
	}
	}

, include will always be true if the value is non-empty (in the dataset page) so adding 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.)

[qqmyers]

@aliassheikh - sorry for the delay. I finally got time to look at this. As you can see from the other comments, I've convinced myself that the changes here won't break any other use cases. The one thing you should add would be a release note (see the docs/releasenotes folder - just add a file with this PR number that says a bug making it impossible to hide metadata fields in subcollections (possibly introduced in v6.6) has been fixed. I'll go ahead and approve now so it can get into the QA queue.

[qqmyers]

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

	for ( DatasetField dsf : mapDatasetFields.values()) {
	if (dsf != null){
	dsf.setInclude(true);
	}
	}

, include will always be true if the value is non-empty (in the dataset page) so adding 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.)

[qqmyers]

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.

[sekmiller]

@aliassheikh, I'm not able to build this branch locally. Would you be able to update it to the current version of develop?
thanks!