Conversation
… the (default) output of the api. (#9763)
| return embargoCitationDate; | ||
| } | ||
| } | ||
| // @todo: remove this commented-out code once/if the PR passes review - L.A. |
There was a problem hiding this comment.
Yes, good to remove commented out code.
src/main/java/edu/harvard/iq/dataverse/search/SearchIncludeFragment.java
Outdated
Show resolved
Hide resolved
| Newest, | ||
| Oldest, | ||
| Size, | ||
| Type |
There was a problem hiding this comment.
I recognize this from #9693. Does it matter which PR is merged first?
There was a problem hiding this comment.
I would prefer #9693 to be merged first, and then resolve the conflict in this branch (I added a database lookup optimization on top of the code introduced in that PR).
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
…ults to "true". (#9763)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…lier PR, as part of a quick fix for #9803
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
avoid conflict with V6.0.0.1__9599-guestbook-at-request.sql
pdurbin
left a comment
There was a problem hiding this comment.
I didn't run the code but it looks good. Conflicts resolved. Tests added. Docs look good.
I did notice a sql script naming conflict so I bumped the version. And I merged the latest from develop.
I'm only holding off on moving this to QA because Jenkins tests are failing (the "fire off installer" step). I started a thread in Slack about it: https://iqss.slack.com/archives/C010LA04BCG/p1697206082190619
This comment has been minimized.
This comment has been minimized.
|
I just kicked this off to get a second opinion from our container-based API test runner: https://github.com/gdcc/api-test-runner/actions/runs/6509472756 |
pdurbin
left a comment
There was a problem hiding this comment.
Jenkins is still failing but GitHub Actions passed: https://github.com/gdcc/api-test-runner/actions/runs/6509472756
Approved.
|
I ran Jenkins again. It's now saying @landreev should we be worried about this? From https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9883/17/consoleFull |
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
There was a problem hiding this comment.
We're having some testing challenges within Jenkins at the moment...
- https://iqss.slack.com/archives/C010LA04BCG/p1697466092510429
- https://iqss.slack.com/archives/C010LA04BCG/p1697466281385699
... but I'm pretty sure they are unrelated to this PR. (Tests passed in GitHub Actions.) Moving to ready for QA.
What this PR does / why we need it:
(copy-and-pasting from the comments in the linked issue):
I'm addressing the performance issues in the versions api via a combination of the following approaches:
/api/datasets/{id}/versionsincludeFilesis added to both/api/datasets/{id}/versionsand/api/datasets/{id}/versions/{vid}(true by default), providing an option to drop the file information from the outputThe following real life datasets from IQSS prod. service are used in the sample tests below:
/api/datasets/{id}/versions:/api/datasets/{id}/versions/{vid}:Note:
/api/datasets/4554342/versionsand/api/datasets/4554342/versions/draftproduce the same amount of output because the former api was called without authentication, so only one, published version was included.Note: tests above were run on the dedicated IQSS test system (not in prod., which is a beefier and faster system).
Which issue(s) this PR closes:
Closes #9763
Special notes for your reviewer:
Please take an extra look at the framework for caching the adjusted "embargo publication date" that I added, replacing the real time lookup that used to be in
Dataset.getCitationDatet(), and especially the flyway script added in the PR.Suggestions on how to test this:
The description above outlines the way I tested the branch during dev.
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: