Skip to content
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

8452 multiple coll mode #8473

Merged
merged 7 commits into from
Mar 14, 2022
Merged

8452 multiple coll mode #8473

merged 7 commits into from
Mar 14, 2022

Conversation

landreev
Copy link
Contributor

@landreev landreev commented Mar 8, 2022

What this PR does / why we need it:

Makes the field collectionMode multiple.
There's one installation where they made these changes in the custom copy of the block they are using. It seems like a sensible modification. But it is now resulting in failed DDI exports for them on datasets that actually have collectionMode fields populated; since the code in ddiExportUtil was hard-coded to a single primitive value. (more details in the linked issue)

Which issue(s) this PR closes:

Closes #8452

Special notes for your reviewer:

My main question is, was I supposed to leave backward compatibility code in there, for an installation that upgrades to 5.10, but forgets to re-import the updated social_science.tsv?
So, something along the lines of

if (collModeFieldDTO.getMultiple())
{
   writeI18NElementList(xmlw, "collMode", collModeFieldDTO.getMultipleVocab(), DatasetFieldConstant.collectionMode, collModeFieldDTO.getTypeClass(), "socialscience", lang);
} else {
   // fallback to the old assumption, for somebody who hasn't updated their metadata block yet:
   writeI18NElement(xmlw, "collMode", version, DatasetFieldConstant.collectionMode, lang);
}

in its current form it just assumes that it's a multiple field.

Suggestions on how to test this:
Please update the metadata block (use the tsv from my branch - https://github.com/IQSS/dataverse/blob/8452-multiple-collMode/scripts/api/data/metadatablocks/social_science.tsv; the url in the release note isn't going to work until we release...); create a dataset with multiple collMode values; export as DDI, confirm that the multiple values are in the resulting XML.

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:

@coveralls
Copy link

coveralls commented Mar 8, 2022

Coverage Status

Coverage increased (+0.02%) to 18.87% when pulling a96f5b9 on 8452-multiple-collMode into 1487650 on develop.

@landreev landreev requested a review from qqmyers March 8, 2022 21:22
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

It's an interesting backward compatibility question re: whether to check for single values (per the reviewer note) - I think that makes sense - just from the perspective of not making the new install break if you test before updating the block.
(If I understand correctly, the only indication of multiple is in the field type (other than that multiple could have several datasetfieldvalue entries associated with one datasetfield) so there's no problem with existing/legacy entries (since we're going in the single-multiple direction)). So there's no need to have the if single logic for this reason).

I guess I'm OK with either with a slight preference to add the check for the single valued type.

Otherwise - just the one issue where I think the block edit now has it as CVV without any values.

scripts/api/data/metadatablocks/social_science.tsv Outdated Show resolved Hide resolved
writeI18NElement(xmlw, "collMode", version, DatasetFieldConstant.collectionMode, lang);
FieldDTO collModeFieldDTO = dto2FieldDTO(version, DatasetFieldConstant.collectionMode, "socialscience");
if (collModeFieldDTO != null) {
writeI18NElementList(xmlw, "collMode", collModeFieldDTO.getMultipleVocab(), DatasetFieldConstant.collectionMode, collModeFieldDTO.getTypeClass(), "socialscience", lang);
Copy link
Member

Choose a reason for hiding this comment

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

FWIW: This works because getMultipleVocab and getMultiplePrimitive do the same thing (w.r.t. the block above isn't using CVV whereas SciencesPO's block is).

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Just leaving a question.

Comment on lines -976 to +978
"value": "CollectionMode"
"value": [
"CollectionMode"
]
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that the native JSON format is changing? If so we should indicate this under "Backward Incompatibilities" in the release notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the answer is "yes". (I'm not 100% sure, tbh. the above change in the .json file was needed for the tests to pass. There is a small chance that a real json import would still work; and the single primitive value would be accepted... I should've tested that).

OK, I'll look for an example of an advertised backward incompatibility in the past release notes.

Copy link
Member

@pdurbin pdurbin Mar 9, 2022

Choose a reason for hiding this comment

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

Thanks. If it helps, the 5.10 release notes already have a "Backwards Incompatibilities" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an incompatibility, yes.

@landreev landreev added this to the 5.10 milestone Mar 9, 2022
@landreev
Copy link
Contributor Author

@kcondon I have added some language to the main release notes directly, about the schema update that's needed in order to index any such multiple fields. Since we are recommending a from-scratch install of the latest stable Solr version as part of the release, the installations that follow that recommendation will end up with the latest schema in the process. But I did spell it out for any holdouts too.

@kcondon kcondon self-assigned this Mar 14, 2022
@kcondon
Copy link
Contributor

kcondon commented Mar 14, 2022

Note this requires the schema to be updated.

1 similar comment
@kcondon
Copy link
Contributor

kcondon commented Mar 14, 2022

Note this requires the schema to be updated.

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.

DDI/OAI-DDI exports crash for some records in 5.9
5 participants