-
Notifications
You must be signed in to change notification settings - Fork 8
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
122 - Extend dataset and file models to contain owner information #127
122 - Extend dataset and file models to contain owner information #127
Conversation
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.
Some early feedback, understanding that you have pending changes to implement. Thanks.
return transformHtmlToMarkdown(metadataFieldValuePayload); | ||
} else if (Array.isArray(metadataFieldValuePayload)) { | ||
if (isArrayOfSubfieldValue(metadataFieldValuePayload)) { | ||
return metadataFieldValuePayload.map((v) => transformPayloadToDatasetMetadataSubfieldValue(v)); |
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.
Minor detail - Can we rename "v" to something more understandable?
const transformPayloadToDatasetMetadataFields = ( | ||
metadataFieldsPayload: MetadataFieldPayload[], | ||
): DatasetMetadataFields => { | ||
return metadataFieldsPayload.reduce((acc: DatasetMetadataFields, field: MetadataFieldPayload) => { |
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.
Minor detail - Can we rename "acc" to something more understandable? Like datasetMetadataFieldsResult
or simply 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 don't like result
because it means finished, but the variable in this case is the accumulator so it's not really the result until the reduce it's finished, I rename it to datasetMetadataFieldsMap
, hope that's ok
@@ -0,0 +1,12 @@ | |||
export interface DvObjectOwnerNode { |
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 think dv-object should be considered as a sub-domain in the package. I would place this file and any other dvObject related file in the core subdomain, as the dvObject is a concept shared across the different subdomains (collections, files and datasets).
…vascript into feat/122-extend-dataset-and-file-models-to-contain-owner-information
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.
LGTM
What this PR does / why we need it:
This PR extends the payload of the dataset and file model to add a new owner property with the owner information of the object.
Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
I also added some types for the file and dataset payload to avoid using any.
Suggestions on how to test this:
Is there a release notes update needed for this change?:
Dataset and file models extended to add object owner information
Additional documentation: