-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix(createDatasetDto): datasetName field in the CreateDatasetDto(obsoletes) should be required #1574
base: master
Are you sure you want to change the base?
fix(createDatasetDto): datasetName field in the CreateDatasetDto(obsoletes) should be required #1574
Conversation
@@ -20,4 +20,13 @@ export class CreateDatasetDto extends UpdateDatasetDto { | |||
}) | |||
@IsString() | |||
readonly type: 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.
@Junjiequan I think that the UpdateDatasetDto
includes this already and the CreateDatasetDto
is just an extension of the Update one.
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.
Sorry, I have mistyped. It should be update-dataset-obsolete.dto.ts
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 am not sure if these changes are really needed as some of them are already there
throw new HttpException( | ||
"Instrument with the same unique name already exists", | ||
HttpStatus.BAD_REQUEST, | ||
{ |
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 have already done some improvements here in the error handling. Check this PR: https://github.com/SciCatProject/scicat-backend-next/pull/1541/files#diff-36c6f6d5764ae45a2bb3b57d024b0c83c9ad714d79735e13787fb8104be763d6
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 have reverted it, let's use what you have done there, I can leave a comment there.
The rest should be fine
Description
This PR explicitly overrides the datasetName field in CreateDatasetDto to ensure it is required.
Motivation
Previously, datasetName was optional in
CreateRawDatasetObsoleteDto
andCreateDerivedDatasetObsoleteDto
, due to its inheritance fromUpdateDatasetObsoleteDto
.Fixes
Changes:
Tests included
Documentation
official documentation info