-
Notifications
You must be signed in to change notification settings - Fork 494
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
Dataverse Featured Items support #11124
base: develop
Are you sure you want to change the base?
Conversation
…averseFeaturedItemCommand
… the featured item image from the client
This comment has been minimized.
This comment has been minimized.
1 similar comment
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.
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.
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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big PR that adds some very useful functionality. I'm leaving some initial feedback and assigning @GPortas to take a look.
@@ -0,0 +1,112 @@ | |||
package edu.harvard.iq.dataverse.api; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up that some tests are failing at https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-11124/36/testReport/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have access to Jenkins. Can you tell me exactly which tests are failing? I can also request temporal access to Jenkins.
|
||
The ``file`` parameter must be specified for each image we want to attach to featured items. Note that images can be shared between featured items, so ``fileName`` can have the same value in different featured items. | ||
|
||
The ``id`` parameter must be ``0`` for new items or set to the item's identifier for updates. The ``fileName`` parameter should be empty to exclude an image or match the name of a file sent in a ``file`` parameter to set a new image. ``keepFile`` must always be set to ``false``, unless it's an update to a featured item where we want to preserve the existing image, if one exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! This is a bit confusing! The examples as well, with all the -F
arguments. Did you consider other ways of accepting input, such as having all input in a JSON file? Are you happy with the final result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to simplify the input, but couldn't make it simpler due to how the API handles Multipart Form Data input type. For example, the inability to send optional parameters while maintaining the relationship between properties of the same object in the form forced me to structure it this way (e.g. using 0 as id, or keepFile=false when not needed). Currently, js-dataverse successfully calls the endpoint, and it works correctly in the SPA (check the related PRs: IQSS/dataverse-client-javascript#235 and IQSS/dataverse-frontend#575). I will be happy to simplify it further in the future if we find a better way to send the data.
@Entity | ||
@Table(indexes = @Index(columnList = "displayOrder")) | ||
public class DataverseFeaturedItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a SQL upgrade script because we're creating a new table, is that right?
.addTags("h1", "h2", "h3", "kbd", "hr", "s", "del", "map", "area") | ||
.addAttributes("img", "usemap") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused what's going on here. Are we allowing more HTML tags and attributes? Does this affect other parts of the app, besides features items? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We only allow more HTML tags for featured items, where for example we need to prevent HTML sanitization from removing important tags like className. Previous use cases work as before. The many changes you see are simply due to the refactoring I did to selectively support the new logic for featured items.
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
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. |
What this PR does / why we need it:
CRUD endpoints for Collection Featured Items have been implemented. In particular, the following endpoints have been implemented:
Which issue(s) this PR closes:
Suggestions on how to test this:
Follow the docs added to native-api to make curl calls and test the different operations on featured items.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
Yes, attached.
Preview docs at https://dataverse-guide--11124.org.readthedocs.build/en/11124/api/native-api.html#list-collection-featured-items