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.

Sign up for GitHub

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

Add Harvesting Client name to the Metadata Source facet #10464

Merged
merged 4 commits into from
Apr 10, 2024
Merged

Add Harvesting Client name to the Metadata Source facet #10464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31784a9
Add Harvesting Client name to the Metadata Source facet
jp-tosca Apr 4, 2024
59d5578
Add release note
jp-tosca Apr 5, 2024
c435bc9
Integration Test added
jp-tosca Apr 5, 2024
8b52741
Update 10464-add-name-harvesting-client-facet.md
landreev Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/release-notes/10464-add-name-harvesting-client-facet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
The Metadata Source facet has been updated to show the name of the harvesting client rather than grouping all such datasets under 'harvested'

TODO: for the v6.13 release note: Please add a full re-index using http://localhost:8080/api/admin/index to the upgrade instructions.
3 changes: 2 additions & 1 deletion src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,8 @@ public SolrInputDocuments toSolrDocs(IndexableDataset indexableDataset, Set<Long

if (dataset.isHarvested()) {
solrInputDocument.addField(SearchFields.IS_HARVESTED, true);
solrInputDocument.addField(SearchFields.METADATA_SOURCE, HARVESTED);
solrInputDocument.addField(SearchFields.METADATA_SOURCE,
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Probably need a release note about this feature that also notes that (async/background) reindexing is needed to populate the facet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @qqmyers 👋🏼

Thanks! I will add the release note in a few just looking at some other PR right now but I will add it ASAP. Regarding the tests, I am not sure I will also look at this.

Best,
Juan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note added and test condition added to the harvesting test to search by the new collection name. 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting what I said during standup in writing:
Rather than using the HarvestingClient's nickname for this facet, we should probably use the name of the local collection into which the client is harvesting.
The clear advantages of doing that:

  1. This mirrors what we are doing for the local datasets: solrInputDocument.addField(SearchFields.METADATA_SOURCE, rootDataverse.getName());
  2. The name of the collection is likely to be more descriptive/better-looking to a human user
  3. While both the client nickname and the name of the local collection are chosen by the local admin, it is far easier to change the latter. The former is not editable at all. With the current implementation, if a prod. instance admin realizes that they named the harvesting client oai_3 and that's what's going to show up in the facet, the only way for them to address it would be to delete the client (and all the content associated with it), and re-create it with a better-looking nickname, then re-harvest.
  4. This will make it unnecessary to add an extra field with a descriptive label to the client class (as was mentioned during standup).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would happen if someone has multiple clients harvested into the same collection? 🤔 should we consider this scenario! Also @DS-INRA made some comments a few minutes ago on the issue confirming that the name would be what they need which is as I understand is the other PR associated with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to harvest into the same collection, yes. But then one could make an argument that if the local admin wants their users to see these datasets from different OAI archives (or sets/clients) show as the same collection to their users, they may actually prefer to have them under the same facet too...
All that said, I would agree that it makes sense to implement it the way the original requestor wants it to work. But let's make sure everybody is on the same page. Let me ask some followup questions in the issue.

dataset.getHarvestedFrom() != null ? dataset.getHarvestedFrom().getName() : HARVESTED);
} else {
solrInputDocument.addField(SearchFields.IS_HARVESTED, false);
solrInputDocument.addField(SearchFields.METADATA_SOURCE, rdvName); //rootDataverseName);
Expand Down
8 changes: 6 additions & 2 deletions src/test/java/edu/harvard/iq/dataverse/api/HarvestingClientsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
import static jakarta.ws.rs.core.Response.Status.ACCEPTED;
import static jakarta.ws.rs.core.Response.Status.OK;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* This class tests Harvesting Client functionality.
Expand Down Expand Up @@ -272,6 +270,12 @@ private void harvestingClientRun(boolean allowHarvestingMissingCVV) throws Inte
} while (i<maxWait);

System.out.println("Waited " + i + " seconds for the harvest to complete.");

Response searchHarvestedDatasets = UtilIT.search("metadataSource:" + nickName, normalUserAPIKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep an eye on it - will not surprise me if it needs an extra second here, occasionally, for the indexing of all the harvested datasets to complete.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jp-tosca @stevenwinship
Hah! This appears to be exactly what's happening now, since I merged this PR yesterday.
The new, weird failures - data.total_count doesn't match. Expected: <8> Actual: <5>

Copy link
Contributor

Choose a reason for hiding this comment

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

JP, this was a bad call on my part - I should've asked you to run Jenkins 3 times in a row on the PR before merging it.

searchHarvestedDatasets.prettyPrint();
searchHarvestedDatasets.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.total_count", equalTo(expectedNumberOfSetsHarvested));

// Fail if it hasn't completed in maxWait seconds
assertTrue(i < maxWait);
Expand Down
Loading