Fix 7874-Dataset-template-authors-ordering

[alejandratenorio]

What this PR does / why we need it: When saving a template, the order of authors is lost. This PR fixes that error.

Which issue(s) this PR closes: #7874

Closes #7874

Special notes for your reviewer:

Suggestions on how to test this:

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:

[qqmyers]

This looks like a fix, but - wouldn't the other checking in tidyUpFields also make sense before saving the template? If so, perhaps there's also an opportunity to refactor - make tidyUpFields() take dsv.getDatasetFields / template.getDatasetFields as the parameter (perhaps moving that method to a util class).

Also - seeing that the current tidyUpFields is called from other Commands - should the tidying here also be done in the template commands (on principle/to help if there's a future template API where order also needs to be defined)?

[alejandratenorio]

Hi @qqmyers,

Yes, my first idea was to run the tidyUpFields() method, however, tidyUpFields also removes all empty fields and this creates another error when editing a template,

If you save an author without affiliation or identifier, when you try to edit it, these fields are no longer displayed. What do you think?

[qqmyers]

Hmm, but that doesn't happen in dataset metadata. Probably some other method getting called to compensate in datasets. While it would be nice to have templates and datasets work the same, I don't know that its worthwhile to ask you to go investigating when what you've put in the PR is a fix for the issue. Unless somebody else has info or has a reason that not adding the empty field removal and trailing space removal that happens in tidyUpFields() is going to a big deal, I'd say it's OK as is - perhaps just adding a note in the code about this issue/difference with datasets.

[scolapasta]

So the method that makes sure tou don't have missing fields when editing a dataset is DatasetVersionUI (it also looks like it might handle the orderingsinc it has a sort:
private List initDatasetFields(boolean createBlanks) {

it is private because it's called by:
public DatasetVersionUI initDatasetVersionUI(DatasetVersion datasetVersion, boolean createBlanks) {

which does more than the template would need.

But to avoid technical debt, we'd probably want to rely on trying to use these (possibly needing to be centralized) methods and tidyUpFields.

[djbrooke]

Hey @alejandratenorio! Thanks for the PR, I hope you are well. @scolapasta and @qqmyers have provided some feedback. Please take a look when you have a chance.

I'll assign this to myself and keep this in the review column for now so that we don't lose track of this.

[alejandratenorio]

Hi @djbrooke ,
Sorry, this week I was working in other tasks, I will check their comments and let you know my comments as soon as possible.

[djbrooke]

Thanks @alejandratenorio, please let us know if you have any questions!

[alejandratenorio]

Hi all,

I was checking the initDatasetVersionUI and initDatasetFields methods, it would be complicated to use them because they are linked to the datasetVersion, we would have to rebuild a lot of methods. Do you think we should try it?

[djbrooke]

Hi @alejandratenorio - I'll move this over to the review column so we can provide some guidance.

[qqmyers]

I'm not sure how to connect things to the initDatasetFields method, but here's a ~concrete proposal:

• Change AbstractDatasetCommand.tidyUpFields(DatasetVersion) to be a new DatasetFieldUtil.tidyUpFields(List<DatasetField>, boolean removeBlanks) method.
• Only run the remove blank fields loop (

  dataverse/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AbstractDatasetCommand.java

  Lines 124 to 129 in 80a42de

  	Iterator<DatasetField> dsfIt = dsv.getDatasetFields().iterator();
  	while (dsfIt.hasNext()) {
  	if (dsfIt.next().removeBlankDatasetFieldValues()) {
  	dsfIt.remove();
  	}
  	}

  ) if removeBlanks==true
• Change the three commands that call tidyUpFields to use the new method tidyUpFields(dsv) -> tidyUpFields(dsv.getDatasetFields(), true);
• Change your code to call tidyUpFields(template.getDatasetFields(), false);

The net affect of that is just having your code also run the trimTrailingSpaces on the template fields (probably good), and a small amount of code consolidation. Whether this is worth doing, or if there's a better option, I'll leave to @scolapasta.

(Separate issue: when I was looking at DatasetVersionUI, it seemed to me that

dataverse/src/main/java/edu/harvard/iq/dataverse/DatasetVersionUI.java

Lines 374 to 395 in 80a42de

	//sort via display order on dataset field
	Collections.sort(retList, new Comparator<DatasetField>() {
	public int compare(DatasetField d1, DatasetField d2) {
	int a = d1.getDatasetFieldType().getDisplayOrder();
	int b = d2.getDatasetFieldType().getDisplayOrder();
	return Integer.valueOf(a).compareTo(Integer.valueOf(b));
	}
	});
	return sortDatasetFields(retList);
	}
	private List<DatasetField> sortDatasetFields (List<DatasetField> dsfList) {
	Collections.sort(dsfList, new Comparator<DatasetField>() {
	public int compare(DatasetField d1, DatasetField d2) {
	int a = d1.getDatasetFieldType().getDisplayOrder();
	int b = d2.getDatasetFieldType().getDisplayOrder();
	return Integer.valueOf(a).compareTo(Integer.valueOf(b));
	}
	});
	return dsfList;
	}

defines and runs the same sort twice?)

[scolapasta]

I think a similar approach as above could be done for initDatasetFields, ie. it could be removed it a DatasetFieldUtil taking in a list of Datasetfields (and some other parameters: a list of metadatablocks and an object (either the version or the template)).

That said, I'm also fine with an incremental improvement as the one @qqmyers suggested since it's a step on this direction. If you're willing to try more and also refactor initDatasetFields (so you wouldn't need the extra boolean for blanks, that's fine too.

@qqmyers I agree about the extra sort, so maybe that's a small bonus cleanup that can be done when this is worked on.

[alejandratenorio]

Hi guys,

Thanks for clarifying the way, we are fine with an incremental improvement, two stages. This way we'll be working with tidyUpFields, then we do the PR and fix the problem in our installation. Later, we continue with initDatasetFields.

[alejandratenorio]

Hi @qqmyers,

Happy new year! Returning to this issue, tidyUpFields method is protected, CuratePublishedDatasetVersionCommand, UpdateDatasetVersionCommand and CreateDatasetVersionCommand can use it because this classes extends from AbstractDatasetCommand. So, I have a problem with TemplatePage class, this class can´t see it.
Do you think it would be worthwhile to do it public & static? Would there be any problem?

[qqmyers]

@alejandratenorio - Happy New Year to you too! I was suggesting that rather than leave the method in AbstractDatasetCommand, create a new DatasetFieldUtil class (e.g. in edu.harvard.iq.dataverse.util) with just this one new public static method. I think that's a little clearer than leaving it in the abstract command class and having non-commands call it. (It could go in some other util class if you see an appropriate one.)

[alejandratenorio]

Hi @qqmyers
I added the DatasetFieldUtil class and have done the commit, I would like to know your opinion.

[qqmyers]

Looks good overall to me. I had one suggested change unless there's a reason it was done as it is.

[kcondon]

@alejandratenorio Hi, our db schema has changed slightly due to a column being dropped in a recent PR. Please refresh this branch from develop so that I can deploy and test this. I have read this pr closely so I can begin testing right away once it is done. Thanks!

[alejandratenorio]

Hi @qqmyers, @kcondon

It's done!