-
Notifications
You must be signed in to change notification settings - Fork 495
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
Updates the updateDataverse API endpoint to support cases where an "inherit from parent" configuration is desired #11026
Conversation
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.
@@ -118,11 +118,18 @@ The fully expanded example above (without environment variables) looks like this | |||
|
|||
You should expect an HTTP 200 response and JSON beginning with "status":"OK" followed by a representation of the updated Dataverse collection. | |||
|
|||
Same as in :ref:`create-dataverse-api`, the request JSON supports an optional ``metadataBlocks`` object, with the following supported sub-objects: | |||
Same as in :ref:`create-dataverse-api`, the request JSON supports a ``metadataBlocks`` object, with the following supported sub-objects: |
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.
Can I omit the metadataBlocks object now? (just wondering why you took out the optional here.) And if so, how would that be handled?
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.
Good point, thanks for noticing because it wasn’t properly explained. I’ve mentioned again that it’s optional and added a note explaining what happens if metadataBlocks
is not sent in the JSON.
|
||
private void processInputLevels(CommandContext ctxt) { | ||
if (inputLevels != null) { | ||
if (!inputLevels.isEmpty()) { | ||
dataverse.addInputLevelsMetadataBlocksIfNotPresent(inputLevels); | ||
} | ||
ctxt.fieldTypeInputLevels().deleteFacetsFor(dataverse); |
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.
Why are we deleting facets in process input levels?
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.
The name of the method is a bit tricky. They are not search facets, but input levels. Also, this behavior is the same as before; I have just relocated the code (See current develop: https://github.com/IQSS/dataverse/blob/develop/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AbstractWriteDataverseCommand.java#L74).
} | ||
}); | ||
} else if (resetRelationsOnNullValues) { | ||
ctxt.fieldTypeInputLevels().deleteFacetsFor(dataverse); |
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.
Same question - does this belong in process input levels?
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.
Answered in the comment above!
When it comes to omitting any of these fields in the request JSON: | ||
|
||
- Omitting ``facetIds`` or ``metadataBlockNames`` causes the Dataverse collection to inherit the corresponding configuration from its parent. | ||
- Omitting ``inputLevels`` removes any existing input levels in the Dataverse 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.
Would it be better to say "removes any existing custom input levels" or "restores default input levels"?
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
📦 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates/clarifications.
No issues found during testing, Merging PR |
What this PR does / why we need it:
The updateDataverse API endpoint has been updated to support an "inherit from parent" configuration for metadata blocks, facets, and input levels.
When it comes to omitting any of these fields in the request JSON:
facetIds
ormetadataBlockNames
causes the Dataverse collection to inherit the corresponding configuration from its parent.inputLevels
removes any existing input levels in the Dataverse collection.Previously, not setting these fields meant keeping the existing ones in the Dataverse.
Which issue(s) this PR closes:
Special notes for your reviewer:
I have applied this behavior only to the updateDataverse endpoint, preserving the previous behavior of the UpdateDataverseCommand in other places for backwards compatibility.
Suggestions on how to test this:
curl -H "X-Dataverse-key:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" -X PUT "http://localhost:8080/api/dataverses/dvAlias" --upload-file update-dataverse.json
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.
Additional documentation:
None