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

Make productionPlace multiple, facetable, and enabled for Advanced Search #9253 #9254

Merged
merged 10 commits into from
Feb 9, 2023

Conversation

philippconzett
Copy link
Contributor

@philippconzett philippconzett commented Jan 4, 2023

What this PR does / why we need it:
Allows depositors to define multiple instances of the metadata field productionPlace in the Citation Metadata block, to refine searches based on the content in the field using the search facets, as well as using the field in the Advanced Search option.

Which issue(s) this PR closes:

Closes #9253

Special notes for your reviewer:
No.

Suggestions on how to test this:

  1. Create a test dataset with multiple instances of the productionPlace metadata field.
  2. Filter results using the field in the filter facets.
  3. Conducting a search using the field in the Advanced Search option.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No. There are already other metadata fields that are repeatable, facetable, and enabled for Advanced Search.

Is there a release notes update needed for this change?:
Should briefly mention that the field now is repeatable, facetable, and enabled for Advanced Search.

Additional documentation:
No.

philippconzett and others added 2 commits January 5, 2023 06:38
Please feel free not use these notes and just list the change under Complete List of Changes.
@pdurbin
Copy link
Member

pdurbin commented Jan 6, 2023

@philippconzett and I update the Solr schema.xml in a2f5d12

We did this in docker-aio. I'm not sure why it says "Production Place" instead of "Production Location". (Maybe some cruft on my machine?) That update was made here:

Anyway, Solr was happy to take the data once we made that change (making it allow multiples).

Screen Shot 2023-01-06 at 12 29 43 PM

@pdurbin
Copy link
Member

pdurbin commented Jan 9, 2023

@mreekie
Copy link

mreekie commented Jan 10, 2023

Prio meeting with Stefano.

  • Moved from Community Backlog to ordered backlog

@jggautier
Copy link
Contributor

jggautier commented Jan 26, 2023

The changes in the citation.tsv file look good, and the changes in the schema.xml make sense to me, although I know less about that.

I think the release notes just need to say "Production Location", the field's new name, instead of "Production Place", the old name. @philippconzett I can change that if that's okay, unless you'd like to.

@pdurbin
Copy link
Member

pdurbin commented Jan 26, 2023

Heads up that we should also fix the failing test I mentioned above.

@jggautier
Copy link
Contributor

Oooo, sorry. I saw that but it had been a while since you posted it so I thought it might've been resolved.

Is the failing test saying that the API endpoint for importing a dataset using DDI Codebook xml doesn't like that the Production Location field can have multiple values now? So that code has to be adjusted?

@pdurbin
Copy link
Member

pdurbin commented Jan 26, 2023

An xml file needs to be changed (and probably the surrounding tests?).

Here, I guess? https://github.com/IQSS/dataverse/blob/v5.12.1/doc/sphinx-guides/source/_static/api/ddi_dataset.xml#L37

Via https://github.com/IQSS/dataverse/blob/v5.12.1/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java#L504

Hopefully this PR isn't a backward incompatible change. importDatasetDDIViaNativeApi is being called in that test.

Replace "Production Place" with "Production Location"
@philippconzett
Copy link
Contributor Author

@jggautier Thanks for spotting the naming issue in the release notes. I have replaced "Production Place" with "Production Location".

@pdurbin I earlier mentioned that I wanted the field to be included in Advanced search, but I never got the issue and the PR updated accordingly. If still possible, I could do it later today. If this would mess up your ongoing work with the PR, I could submit a new issue/PR about Advanced Search. Please let me know which option you prefer. Thanks!

@pdurbin
Copy link
Member

pdurbin commented Feb 1, 2023

@philippconzett I'm not assigned as a reviewer. Let's see what @scolapasta and @jggautier (who are) think.

@scolapasta
Copy link
Contributor

@philippconzett I'm happy with you making those changes here. It'll make it more complete (and easier to review and test together).

@pdurbin
Copy link
Member

pdurbin commented Feb 1, 2023

Speaking of tests, please don't forget about the failing tests!

Screen Shot 2023-02-01 at 10 55 48 AM

@philippconzett philippconzett changed the title Make productionPlace multiple and facetable #9253 Make productionPlace multiple, facetable, and enabled for Advanced Search #9253 Feb 2, 2023
@philippconzett
Copy link
Contributor Author

Thanks, @scolapasta. I've edited the files 9253-productionPlace.md and citation.tsv accordingly, but left the file schema.xml untouched. Please let me know I the changes make sense and if anything needs to be improved.

@jggautier
Copy link
Contributor

I've reviewed the release notes and citation.tsv files and they look good to me.

I'm not sure if the schema.xml file needs to be changed anymore. And I'm probably not the best person to resolve the testing failures, so I'm going to remove myself as a reviewer now. Thanks all!

@jggautier jggautier removed their assignment Feb 2, 2023
@sekmiller
Copy link
Contributor

@philippconzett I will update the ImportDDIServiceBean so that it will know to expect multiple Production locations are possible. I'll also add the upgrade instructions that I mentioned above.

@coveralls
Copy link

coveralls commented Feb 7, 2023

Coverage Status

Coverage: 20.009% (-0.0005%) from 20.01% when pulling 0978dbd on philippconzett:patch-2 into ac7aef7 on IQSS:develop.

@philippconzett
Copy link
Contributor Author

Thanks for adding this, @sekmiller. (I don't how enough about schema.xml to add anything substantial to this.)

@sekmiller sekmiller removed their assignment Feb 8, 2023
Copy link
Contributor

@scolapasta scolapasta left a comment

Choose a reason for hiding this comment

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

@sekmiller Looks good - can you make one more change and add a second Prod location to the ddi that is used for testing? That way we actually exercise this change.

@scolapasta scolapasta removed their assignment Feb 8, 2023
@kcondon kcondon self-assigned this Feb 9, 2023
@kcondon kcondon merged commit ceffb6f into IQSS:develop Feb 9, 2023
@scolapasta scolapasta added this to the 5.13 milestone Feb 9, 2023
landreev added a commit that referenced this pull request Apr 19, 2023
…made

multiple in PR #9254; would be great to put together a process for developers
who need to make changes to fields in metadata blocks that would help
them to know of all the places where changes like this need to be made.
(not the first time, when something breaks, in ddi export specifically, after
a field is made multiple). #3648
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Metadata Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Feature Request/Idea: Make Production Location repeatable, facetable, and enabled for Advanced Search
8 participants