Conversation
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.
qqmyers
left a comment
There was a problem hiding this comment.
Looks good overall. I made a few suggestions for some clean up.
| } | ||
|
|
||
| protected Template findTemplateOrDie(Long templateId, Dataverse dataverse) throws WrappedResponse { | ||
| protected Template findAllTemplatesOrDie(Long templateId, Dataverse dataverse) throws WrappedResponse { |
There was a problem hiding this comment.
maybe findTemplateInDataverseOrDie or similar? - it doesn't return all templates, just one.
| */ | ||
|
|
||
| String templateName = json.getString("name"); | ||
| if (!templateName.isEmpty() && !templateName.isBlank()){ |
There was a problem hiding this comment.
isBlank is true if the String isEmpty, so don't need both. (Could it be null?)
|
|
||
| return created("/dataverses/template/" + updated.getId(), jsonTemplate(updated)); | ||
| } catch (JsonParseException ex) { | ||
| logger.log(Level.SEVERE, "Semantic error parsing dataset update Json: " + ex.getMessage(), ex); |
There was a problem hiding this comment.
a nit: a json parsing error is syntactic not semantic - maybe just "Error parsing..."
| Template template = findTemplateOrDie(templateId); | ||
| Dataverse dataverse = template.getDataverse(); | ||
|
|
||
| if (requestBody.getName() != null && !requestBody.getName().isEmpty()) { |
There was a problem hiding this comment.
isBlank() instead of isEmpty()?
|
|
||
| if (requestBody.getName() != null && !requestBody.getName().isEmpty()) { | ||
| String licenseName = requestBody.getName(); | ||
| License license = licenseSvc.getByNameOrUri(licenseName); |
There was a problem hiding this comment.
Probably out of scope - this method is supposed to allow using a name or URI (which is actually meant as an identifier) but using LicenseUpdateRequest limits this to a name. It's the same for the Datasets method so this PR isn't introducing anything new, but supporting URIs would be a useful addition at some point.
| datasetVersionField.setControlledVocabularyValues(new ArrayList<>()); | ||
| datasetVersionField.setDatasetFieldCompoundValues(new ArrayList<>()); | ||
| } else { | ||
| datasetVersionField.setSingleValue(""); |
There was a problem hiding this comment.
I see setSingleValue(null) in the Dataset deleteMetadata call - but this is what's done in the UpdateDatasetFieldsCommand - any reason for the difference? I guess it makes sense to leave it unless you can justify null.
| /* | ||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Licenses/license-default.txt to change this license | ||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Classes/Class.java to edit this template | ||
| */ |
| /* | ||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Licenses/license-default.txt to change this license | ||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Classes/Class.java to edit this template | ||
| */ |
| /* | ||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Licenses/license-default.txt to change this license | ||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Classes/Class.java to edit this template | ||
| */ |
| @@ -0,0 +1,86 @@ | |||
| /* | |||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Licenses/license-default.txt to change this license | |||
| * Click nbfs://nbhost/SystemFileSystem/Templates/Classes/Class.java to edit this template | |||
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: Allows user to update the metadata, instructions, license, terms of use or terms of access of a given template
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this: There are several new api endpoints one of which is used to either update the license or the custom terms of access for a given template (see release notes and updated documentation)
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?: included
Additional documentation: included