dataverse icon indicating copy to clipboard operation
dataverse copied to clipboard

Support deletion of saved searches

Open luddaniel opened this issue 1 year ago • 2 comments

What this PR does / why we need it:

  • Allow to delete saved search by calling DELETE /api/admin/savedsearches/{id}
  • Allow to delete saved search and unlink links related to the deleted saved search by adding ?unlink=true

Which issue(s) this PR closes:

Closes #9317 Closes #10628

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

  • https://guides.dataverse.org/en/latest/api/native-api.html#saved-search must be updated
  • Explain that deletion with unlink=true is not perfect and may be followed /makelinks/all depending on the need if other saved searches could recreate some deleted links or by reindexing some Dataverse or Dataset.

Suggestions on how to test this:

This is how I tested it :

Create Data

create 1 collection + 2 datasets under + publish all with subject "Agricultural Sciences"

Create saved search

create another collection that will be contain linked Data)

savedSearch.json contains :

{
    "query": "*",
    "creatorId": 1,
    "definitionPointId": 5,
    "filterQueries": [
        "subject_ss:Agricultural Sciences"
    ]
}

curl http://localhost:8080/api/admin/savedsearches -X POST -H 'Content-type:application/json' --upload-file savedSearch.json | jq . curl -X PUT http://localhost:8080/api/admin/savedsearches/makelinks/all?debug=true | jq .

Now collection2 contains 3 linked Dvobject.

Remove without unlink

curl http://localhost:8080/api/admin/savedsearches/list | jq . -> 1 item id = 1 curl -X DELETE http://localhost:8080/api/admin/savedsearches/1 | jq . -> OK curl http://localhost:8080/api/admin/savedsearches/list | jq . -> no item

Links are still here

Re-Create saved search

curl -s http://localhost:8080/api/admin/savedsearches -X POST -H 'Content-type:application/json' --upload-file savedSearch.json | jq . curl -X PUT http://localhost:8080/api/admin/savedsearches/makelinks/all?debug=true | jq .

Remove with unlink

curl http://localhost:8080/api/admin/savedsearches/list | jq . -> 1 item id = 2 curl -X DELETE http://localhost:8080/api/admin/savedsearches/2?unlink=true | jq . -> OK curl http://localhost:8080/api/admin/savedsearches/list | jq . -> no item

Links are deleted and not visible in collection2

Docs

Preview updated docs at https://dataverse-guide--10198.org.readthedocs.build/en/10198/api/native-api.html#saved-search

luddaniel avatar Dec 20 '23 17:12 luddaniel

Coverage Status

coverage: 20.62% (-0.008%) from 20.628% when pulling 008507bfb50b339b3a5535729ec3552562675132 on Recherche-Data-Gouv:9317-delete-saved-search into 9bfa6ac805a88dc8ceff808b2e56e437b4cda822 on IQSS:develop.

coveralls avatar Dec 20 '23 17:12 coveralls

This is a great start! @luddaniel how do you feel about adding API tests, an update to the API Guide, and a release note snippet?

Hello @pdurbin, everything has been added to the PR :

  • API Guide update
  • Release note snippet
  • Integration tests (which revealed that API endpoints were not restricted to Superusers only :rofl:), tests were kept simple for now.
  • Superusers only fix

luddaniel avatar Mar 06 '24 17:03 luddaniel

Just a quick update on this PR : @luddaniel is currently in vacations, he'll be able to come back to this PR after August 19th.

DS-INRAE avatar Jul 29 '24 14:07 DS-INRAE

@luddaniel Please note my questions above and resolve the merge conflicts with our develop branch. Thanks!

sekmiller avatar Aug 22 '24 13:08 sekmiller

Hmm, Jenkins hasn't run on this PR since June 14. I just kicked off a run manually: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/view/change-requests/job/PR-10198/

I did run mvn test -Dtest=SavedSearchIT locally, and it passed.

pdurbin avatar Aug 26 '24 20:08 pdurbin

I just kicked off a run manually: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/view/change-requests/job/PR-10198/

The test show as failing but the API tests passed: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10198/14/testReport/

@donsizemore fixed the unrelated jacoco failure in https://github.com/gdcc/dataverse-ansible/commit/9602eccfa9ef2432f171e6ba9363f69cdf7c6bd7

pdurbin avatar Aug 27 '24 14:08 pdurbin

Nice feature. Seems to work fine. I tested it with this:

curl -H "X-Dataverse-key:$API_TOKEN" -X DELETE "http://localhost:8080/api/admin/savedsearches/3?unlink=true"

Merging! Thanks, @luddaniel! ❤️

pdurbin avatar Aug 27 '24 14:08 pdurbin

I'd be remiss if I didn't say that if I had access to push to https://github.com/Recherche-Data-Gouv I would have made the following small tweaks (and, if I had the energy, would have convert the old style API docs to the new style with environment variables).

This is all minor stuff though and not worth holding up progress. Perhaps it could go in a future pull request.

pdurbin@beamish dataverse % git diff --word-diff doc 
diff --git a/doc/sphinx-guides/source/api/native-api.rst b/doc/sphinx-guides/source/api/native-api.rst
index b16ea55bd2..9725602df4 100644
--- a/doc/sphinx-guides/source/api/native-api.rst
+++ b/doc/sphinx-guides/source/api/native-api.rst
@@ -5853,7 +5853,7 @@ Saved Search
~~~~~~~~~~~~

The Saved Search, Linked Dataverses, and Linked Datasets features are only accessible to superusers except for linking a dataset. The following API endpoints were added to help people with access to the "admin" API make use of these features in their current form. Keep in mind that they are partially experimental.
The update of all saved search is run by a timer once a week (See :ref:`saved-search-timer`) so if you just created a saved search, you can run manually {+the+} ``makelinks`` endpoint that will find new dataverses and datasets that match the saved search and then link the search results to the dataverse in which the saved search is defined.

List all saved searches. ::

@@ -5865,7 +5865,7 @@ List a saved search by database id. ::

Delete a saved search by database id.

The ``unlink=true`` query parameter unlinks all links (linked dataset or Dataverse collection) associated with the deleted saved search. Use of this parameter should be well considered as you cannot know if the links were created manually or by the saved search. After deleting a saved search with ``unlink=true``, we recommend running ``/makelinks/all`` just in case there was a dataset that was linked by another saved search. (Saved searches can link the same dataset.) Reindexing might be necessary as [-well.::-]{+well. ::+}

  DELETE http://$SERVER/api/admin/savedsearches/$id?unlink=true

pdurbin@beamish dataverse % git diff src            
diff --git a/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java b/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
index 33a11a2df2..e6519c9ff3 100644
--- a/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
+++ b/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
@@ -181,7 +181,7 @@ public class SavedSearches extends AbstractApiBean {
         try {
             wasDeleted = savedSearchSvc.delete(doomedId, unlink);
         } catch (Exception e) {
-            return error(INTERNAL_SERVER_ERROR, "Problem while trying to unlink links of saved search id " + doomedId);
+            return error(INTERNAL_SERVER_ERROR, "Problem while trying to unlink links of saved search id " + doomedId + ". Exception: " + e.getLocalizedMessage());
         }
 
         if (wasDeleted) {
diff --git a/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java b/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java
index 90357596c2..cdcd92d930 100644
--- a/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java
+++ b/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java
@@ -22,7 +22,7 @@ public class SavedSearchIT {
 
     @BeforeAll
     public static void setUpClass() {
-
+        RestAssured.baseURI = UtilIT.getRestAssuredBaseUri();
     }
 
     @AfterAll
@@ -52,6 +52,7 @@ public class SavedSearchIT {
         createDatasetResponse2.prettyPrint();
         Integer datasetId2 = UtilIT.getDatasetIdFromResponse(createDatasetResponse2);
 
+        // TODO: put all these methods in UtilIT
         // missing body
         Response resp = RestAssured.given()
                 .contentType("application/json")
pdurbin@beamish dataverse % 

pdurbin avatar Aug 27 '24 14:08 pdurbin

Thanks for the suggestions Phil! We'll address them in a future PR indeed. Just in case for future PRs, I've also invited you to Recherche-Data-Gouv :)

DS-INRAE avatar Sep 02 '24 09:09 DS-INRAE

@DS-INRA I joined! Thanks!

pdurbin avatar Sep 03 '24 21:09 pdurbin

@pdurbin I added your suggestions in a new issue so we don't forget them :) :

  • #10893

DS-INRAE avatar Sep 30 '24 12:09 DS-INRAE

@DS-INRAE thanks! No big deal. It's minor stuff. And next time I'll push little changes like this myself before merging, now that you've given me access. 😄 😈

pdurbin avatar Sep 30 '24 20:09 pdurbin