-
Notifications
You must be signed in to change notification settings - Fork 534
Review Datasets #11753
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?
Review Datasets #11753
Conversation
This comment has been minimized.
This comment has been minimized.
dc893bc to
7c629a2
Compare
This comment has been minimized.
This comment has been minimized.
|
Next Tuesday I'll add to this PR . properties files for the two metadata blocks |
|
I put the two properties files for the metadata blocks into this branch's src/main/java/propertyFiles folder |
This comment has been minimized.
This comment has been minimized.
ef2f080 to
db4c453
Compare
This comment has been minimized.
This comment has been minimized.
db4c453 to
44e2c17
Compare
This comment has been minimized.
This comment has been minimized.
44e2c17 to
e41d0e3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
poikilotherm
left a comment
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.
Here's a first review. Tried to be thorough, might not have captured everything.
| .add("availableLicenses", availableLicenses); | ||
| } | ||
|
|
||
| public String getDisplayName(Locale locale) { |
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.
Is it really necessary to duplicate the code for displayName and description?
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.
| .add("availableLicenses", availableLicenses); | ||
| } | ||
|
|
||
| public String getDisplayName(Locale locale) { |
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.
All the log messages should use lambas to only execute the string concatenations when actually being logged.
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.
Where were you when @michbarsinai opened the following issue? 😅
Should we re-open it? 🤔
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.
@poikilotherm as an IntelliJ user, you want me to click on one of these?
| logger.fine("Locale is English, returning default display name: " + displayName); | ||
| return displayName; | ||
| } | ||
| String propertiesFile = "datasetTypes_" + locale.toLanguageTag() + ".properties"; |
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 burns unnecessary CPU cycles every time it's looked up but is only used for a debugging log message.
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 happy to change this. What do you suggest?
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AbstractCreateDatasetCommand.java
Outdated
Show resolved
Hide resolved
| @@ -5853,7 +5867,7 @@ public Response deleteDatasetType(@Context ContainerRequestContext crc, @PathPar | |||
| try { | |||
| idToDelete = Long.parseLong(doomed); | |||
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 method should also allow using a name or an ID, as the other methods. This is especially important now that we return a list of names for allowed types for a collection.
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.
Sure, I can add delete by name. When I do, I'll also resolve #11753 (comment)
|
|
||
| List<String> invalidDatasetTypes = new ArrayList<>(); | ||
|
|
||
| String[] allowedDatasetTypeNames = stringValue.split(","); |
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.
Technically, all of this class does things typically located in a service layer. But fine... At least, we should not use a simple comma separated list - a JSON list is much cleaner, as we are using a JSON API and other places like settings etc also support using of JSON Arrays. At some point we really should refactor the API, service and command layer to deal with all of this in a less chaotic way.
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.
As I said on the call yesterday (thanks!), I'm a bit weirded out by the idea of passing JSON in a query parameter, but sure, let's keep talking about it.
| } | ||
| } | ||
|
|
||
| if (!invalidDatasetTypes.isEmpty()) { |
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.
It is debatable if the validation of the names should be done in this class or in a separate service layer. As we currently don't have a service layer for DatasetTypes, maybe this one will do. (This is a design flaw of our REST API, which mixes being entry and service layer a lot. As stated above, this whole class mimics a service layer...)
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.
Maybe we're due for another rewrite! 😅
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseAttributeCommand.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
c075909 to
8091c45
Compare
Also, add note about how "Data Type" (kindOfData) metadata field is used in resourceType for DataCite exports. See also datacite/datacite-suggestions#214
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.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Julian Gautier <jggautier@ucla.edu>
Co-authored-by: Julian Gautier <jggautier@ucla.edu>
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.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
📦 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:
Makes creation of review datasets possible.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
<resourceType resourceTypeGeneral="Other">Review</resourceType>Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Yes, the facets under "Dataset Type" are changing from human readable and translatable ("Dataset" from getDisplayName) to machine-readable ("dataset" from getName). This is to fix #11758 and IQSS/dataverse-frontend#809
Before
After
Note that I'm aware that even after this fix, when you click any of the machine readable names, such as "software", the friendly name will appear above the search results like this:
I left a note in the xhtml about this. I don't think this is worth fixing on the JSF side. In the SPA, the values will consistently be the machine readable versions, such as "software" (lower case).
Is there a release notes update needed for this change?:
Included