-
Notifications
You must be signed in to change notification settings - Fork 562
Add Guestbook Response Sorting by fields #12568
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
base: develop
Are you sure you want to change the base?
Changes from all commits
f18ca56
a598986
dd63f3a
22b9ab9
9718f06
8efa6be
b7f7d2b
90e4c2f
ca7d64e
7bb0836
315c68e
07c8ff0
9ac10d9
d4cccdc
75c4436
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ## Feature ## | ||
| This feature adds the ability to sort the Guestbook Responses in the API /api/guestbooks/{id}/responses. Responses can be sorted by Event Type(type), File Name(file), User Name(user), and Response Date(date) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; | ||
| import edu.harvard.iq.dataverse.authorization.users.User; | ||
| import edu.harvard.iq.dataverse.externaltools.ExternalTool; | ||
| import edu.harvard.iq.dataverse.search.SortBy; | ||
| import edu.harvard.iq.dataverse.util.StringUtil; | ||
| import jakarta.ejb.EJB; | ||
| import jakarta.ejb.Stateless; | ||
|
|
@@ -19,6 +20,7 @@ | |
| import jakarta.persistence.PersistenceContext; | ||
| import jakarta.persistence.Query; | ||
| import jakarta.persistence.TypedQuery; | ||
| import jakarta.persistence.criteria.*; | ||
| import org.apache.commons.text.StringEscapeUtils; | ||
|
|
||
| import java.io.IOException; | ||
|
|
@@ -112,24 +114,53 @@ public List<Long> findAllIds(Long dataverseId) { | |
| return em.createQuery("select o.id from GuestbookResponse o, Dataset d where o.dataset.id = d.id and d.owner.id = " + dataverseId + " order by o.responseTime desc", Long.class).getResultList(); | ||
| } | ||
|
|
||
| public List<GuestbookResponse> findAllByGuestbookId(Long guestbookId) { | ||
| return findAllByGuestbookId(guestbookId, null, null); | ||
| private Order getOrderBy(CriteriaBuilder cb, Path<Object> pathObj, boolean isDescending) { | ||
| return isDescending ? cb.desc(pathObj) : cb.asc(pathObj); | ||
| } | ||
| public List<GuestbookResponse> findAllByGuestbookId(Long guestbookId, Integer offset, Integer limit) { | ||
| public List<GuestbookResponse> findAllByGuestbookId(Long guestbookId, String sortField, String sortOrder, Integer offset, Integer limit) { | ||
| if (guestbookId != null) { | ||
| TypedQuery<GuestbookResponse> query = em.createQuery("select o from GuestbookResponse as o where o.guestbook.id = " + guestbookId + " order by o.responseTime desc", GuestbookResponse.class); | ||
| if (offset != null) { | ||
| query.setFirstResult(offset); | ||
| CriteriaBuilder cb = em.getCriteriaBuilder(); | ||
| CriteriaQuery<GuestbookResponse> cq = cb.createQuery(GuestbookResponse.class); | ||
| Root<GuestbookResponse> guestbookResponseRoot = cq.from(GuestbookResponse.class); | ||
|
|
||
| boolean isDescending = sortOrder != null && sortOrder.equalsIgnoreCase(SortBy.DESCENDING); | ||
| Order order; | ||
| String orderField = (sortField == null) ? "" : sortField.toLowerCase(); | ||
| switch(orderField) { | ||
| case "date": | ||
| order = getOrderBy(cb, guestbookResponseRoot.get("responseTime"), isDescending); | ||
| break; | ||
| case "type": | ||
| order = getOrderBy(cb, guestbookResponseRoot.get("eventType"), isDescending); | ||
|
stevenwinship marked this conversation as resolved.
|
||
| break; | ||
| case "file": | ||
| Join<GuestbookResponse, DataFile> dataFileJoin = guestbookResponseRoot.join("dataFile", JoinType.INNER); | ||
| order = getOrderBy(cb, dataFileJoin.get("fileMetadatas").get("label"), isDescending); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filemetadatas is a collection and AI flags this as something that won't work. We either need more joins to find the filemetadata that is in the latest version that isn't a draft (unless the user can see drafts?), or possibly punt and use the datafile name (more efficient, possibly confusing for files with name changes). (FWIW: the json returned in the call (not changed in the PR) ~incorrectly uses the label from the latest filemetadata, potentially showing the label in draft). That said, it does look like the test for file ordering worked. AI points out (and I didn't confirm) that the test case is for one datasetversion so there is only one filemetadata. Minimally, I guess we need a test to confirm that having mutliple versions (with the files renamed and/or not in all versions?) works (and gives us the latest non-draft names?). If it does, then AI would be wrong w.r.t. whether this line actually works in postgres.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RE: showing the label from the draft. I'm writing another test that has multiple files, multiple versions, and a renamed file |
||
| break; | ||
| case "user": | ||
| order = getOrderBy(cb, guestbookResponseRoot.get("name"), isDescending); | ||
| break; | ||
| default: | ||
| order = null; | ||
| } | ||
| if (limit != null) { | ||
| query.setMaxResults(limit); | ||
|
|
||
| cq.where(cb.equal(guestbookResponseRoot.get("guestbook").get("id"), guestbookId)); | ||
| if (order != null) { | ||
| cq.orderBy(order, getOrderBy(cb, guestbookResponseRoot.get("id"), isDescending)); | ||
| } | ||
| cq.distinct(true); | ||
|
|
||
| int firstResult = offset == null ? 0 : offset; | ||
| int pageSize = limit == null ? Integer.MAX_VALUE : limit; | ||
|
|
||
| return query.getResultList(); | ||
| return em.createQuery(cq) | ||
| .setFirstResult(firstResult) | ||
| .setMaxResults(pageSize) | ||
| .getResultList(); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /* | ||
| This method is used for streaming downloads of guestbook responses, in | ||
| CSV format, both for individual guestbooks, and for entire dataverses | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ | |
| import io.restassured.path.json.JsonPath; | ||
| import io.restassured.path.xml.XmlPath; | ||
| import io.restassured.response.Response; | ||
| import jakarta.json.Json; | ||
| import jakarta.json.JsonObject; | ||
| import jakarta.json.JsonObjectBuilder; | ||
| import jakarta.ws.rs.core.Response.Status; | ||
|
|
@@ -3953,7 +3952,7 @@ public void testDownloadFileWithGuestbookResponse() throws IOException, JsonPars | |
| Response publishResponse = UtilIT.publishDataverseViaNativeApi(parentDataverseAlias, ownerApiToken); | ||
| assertEquals(200, publishResponse.getStatusCode()); | ||
| // Create a Parent Guestbook | ||
| Guestbook parentGuestbook = UtilIT.createRandomGuestbook(parentDataverseAlias, null, ownerApiToken); | ||
| UtilIT.createRandomGuestbook(parentDataverseAlias, null, ownerApiToken); | ||
|
|
||
| // Create Dataverse | ||
| String dataverseAlias = createDataverseGetAlias(ownerApiToken); | ||
|
|
@@ -4181,28 +4180,63 @@ public void testDownloadFileWithGuestbookResponse() throws IOException, JsonPars | |
| .statusCode(OK.getStatusCode()); | ||
| JsonPath jsonPath = JsonPath.from(guestbookListResponses.body().asString()); | ||
| int totalCount = jsonPath.getList("data.responses").size(); | ||
| int totalCountFromJson = jsonPath.getInt("data.guestbook.responseCount"); | ||
| assertTrue(totalCount > 0); | ||
| assertNotNull(jsonPath.getString("data.responses[0].name")); | ||
|
|
||
| // Test Get All Responses Sorted | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI noted that tests don't cover mutiple versions which may affect file name sorting (see earlier comment), doesn't check the next/prev urls keep the sort order, or check for bad params giving an error (a suggested addition to the code), or test the no sort field case (is that done elsewhere?). (It also notes that there's no check that the sort order is stable without the secondary sort over id (again see above) though I don't know how one could reliably cause failures for that if there is no secondary sort.) |
||
| testSortByField(guestbook.getId(), "file", "asc", 0, Integer.MAX_VALUE, null, ownerApiToken); | ||
| testSortByField(guestbook.getId(), "user", "asc", null, null, null, ownerApiToken); | ||
| testSortByField(guestbook.getId(), "user", "desc",null, null, null, ownerApiToken); | ||
| testSortByField(guestbook.getId(), "date", "asc", null, null, null, ownerApiToken); | ||
| testSortByField(guestbook.getId(), "date", "desc",null, null, null, ownerApiToken); | ||
| // Test Get All Responses Sorted with errors | ||
| testSortByField(guestbook.getId(), "bad", null, null, null, BundleUtil.getStringFromBundle("guestbookResponses.invalidSortField"), ownerApiToken); | ||
| testSortByField(guestbook.getId(), null, "bad", null, null, BundleUtil.getStringFromBundle("guestbookResponses.invalidSortOrder"), ownerApiToken); | ||
| testSortByField(guestbook.getId(), null, null, -1, null, BundleUtil.getStringFromBundle("guestbookResponses.invalidOffset"), ownerApiToken); | ||
| testSortByField(guestbook.getId(), null, null, null, 0, BundleUtil.getStringFromBundle("guestbookResponses.invalidLimit"), ownerApiToken); | ||
|
|
||
| // Test Get Responses with pagination | ||
| int pages = 4; // total should be 17. set to 4 pages | ||
| int limit = (totalCount / pages) + 1; // should be 5 per page. we should see 5, 5, 5, 2 | ||
| int pagedTotalCount = 0; | ||
| int totalCountFromJson = 0; | ||
| for (int i = 0; i < pages; i++) { | ||
| int offset = limit * i; | ||
| guestbookListResponses = UtilIT.getGuestbooksResponses(guestbook.getId(), offset, limit, ownerApiToken); | ||
| guestbookListResponses = UtilIT.getGuestbooksResponses(guestbook.getId(), "date", null, offset, limit, ownerApiToken); | ||
| guestbookListResponses.prettyPrint(); | ||
| jsonPath = JsonPath.from(guestbookListResponses.body().asString()); | ||
| pagedTotalCount += jsonPath.getList("data.responses").size(); | ||
| totalCountFromJson = jsonPath.getInt("data.pagination.totalResponses"); | ||
| // 'No duplicate ids' was manually verified. Just make sure the count is good. If there were duplicates the count would be high | ||
| } | ||
| // verify all counts are good and equal | ||
| assertEquals(totalCount, pagedTotalCount); | ||
| assertEquals(pagedTotalCount, totalCountFromJson); | ||
| } | ||
|
|
||
| private void testSortByField(Long id, String sortField, String order, Integer offset, Integer limit, String errorMsg, String token) { | ||
| Response guestbookListResponses = UtilIT.getGuestbooksResponses(id, sortField, order, offset, limit, token); | ||
| Map<String,String> fieldMap = Map.of("type", "type", "user", "name", "file", "fileName", "date", "date"); | ||
| String fieldName = sortField != null ? fieldMap.get(sortField) : null; | ||
| boolean isDescending = order != null && order.equalsIgnoreCase("desc"); | ||
| guestbookListResponses.prettyPrint(); | ||
| if (errorMsg == null) { | ||
| guestbookListResponses.then().assertThat().statusCode(OK.getStatusCode()); | ||
| JsonPath jsonPath = JsonPath.from(guestbookListResponses.body().asString()); | ||
| int totalCount = jsonPath.getList("data.responses").size(); | ||
| assertTrue(totalCount > 0); | ||
| String lastFieldValue = jsonPath.getString("data.responses[0]." + fieldName).toLowerCase(); // The sort seems to be case-insensitive | ||
| for (int i = 1; i < totalCount; i++) { | ||
| String fieldValue = jsonPath.getString("data.responses[" + i + "]." + fieldName).toLowerCase(); | ||
| assertTrue(isDescending ? fieldValue.compareTo(lastFieldValue) <= 0 : fieldValue.compareTo(lastFieldValue) >= 0); | ||
| lastFieldValue = fieldValue; | ||
| } | ||
| } else { | ||
| guestbookListResponses.then().assertThat() | ||
| .statusCode(BAD_REQUEST.getStatusCode()) | ||
| .body("message", equalTo(errorMsg)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetFileCitationFormatted() { | ||
| Response createUser = UtilIT.createRandomUser(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.