-
Notifications
You must be signed in to change notification settings - Fork 8
specify datasetType when creating a dataset #360
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
Conversation
Note that it depends on this PR: IQSS/dataverse-client-javascript#354 However, there's a new PR to try instead: IQSS/dataverse-client-javascript#360
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.
Looks awesome!
Could you also add or edit test cases in integration test test/integration/datasets/DatasetsRepository.test.ts
and functional test test/functional/datasets/CreateDataset.test.ts
? it may be harder to test if we don't have datasetType on DatasetDTO and dataset model.
Also, the useCase.md
may need to be updated with new parameter.
newDataset: DatasetDTO, | ||
collectionId = ROOT_COLLECTION_ID | ||
collectionId = ROOT_COLLECTION_ID, | ||
datasetType?: string |
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.
Could you add the param to the description above @param {string}...
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, added in 156963d.
testMetadataBlocks, | ||
ROOT_COLLECTION_ID | ||
ROOT_COLLECTION_ID, | ||
undefined |
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.
You could add an additional test to make sure datasetType would be called here.
For example,
you could create a function createDatasetwithDatasetType
in DataseHelper
and do something similar with this
expect(datasetsRepositoryStub.createDatasetwithDatasetType).toHaveBeenCalledWith( testDataset, testMetadataBlocks, ROOT_COLLECTION_ID, datasetType )
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 idea. Added in 48cc5dc. Thanks for the help with this!
Closing. As discussed with @ChengShi-1 we're thinking a better order for PRs would be:
|
What this PR does / why we need it:
At dataset creation time, we need to allow
datasetType
to be passed so that types other than the default ("dataset") can be persisted to the database.Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
We also need to support listing dataset types ( https://guides.dataverse.org/en/6.7.1/api/native-api.html#list-dataset-types ) but perhaps that should be a separate issue and pull request?
Suggestions on how to test this:
datasetType
field. In the JSON, it looks like this:Is there a release notes update needed for this change?:
Yes, we should add something like this:
You can now specify a datasetType when creating a dataset.
Additional documentation:
None.