Skip to content
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

[WIP] Feature: Add (Nested) Validation to Create / Update / Delete Args #23

Closed

Conversation

johannesschobel
Copy link
Contributor

Dear @doug-martin ,

this PR addresses the issue discussed in #19 .
in particular, the commits presented below add respective @ValidateNested() and @Type() decorators as discussed.

Please note, that this PR contains a breaking change, as i changed one @Field from input to id to better fit the overall API. This breaking change is, however, encapsulated in one single commit!

Further - I was not able to properly run all tests via npm run test. There are 2 tests related to create one and create many in the create.resolver.spec file that won't pass. If you can tell me, how to properly fix the tests (or adapt my implementation) i would be happy to re-work specific parts.

All the best and thank you very much for your time and effort!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9620cf1 on johannesschobel:feat/validation into 045022e on doug-martin:master.

@johannesschobel
Copy link
Contributor Author

hmmmmmmmmmmmmmmmm... wait.. strange.. die travis CI build says it passes.. however, if you look into the output message, there are 2 tests failing!?
thats the same 2 tests that also fail on my machine

See here: https://travis-ci.org/doug-martin/nestjs-query/jobs/653485852#L394

import { TodoItemInputDTO } from '../dto/todo-item-input.dto';

@ArgsType()
export class CreateOneTodoItemArgs extends CreateOneArgsType(TodoItemInputDTO) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required for the example?

@Field(() => ID, { description: 'The id of the record to delete.' })
input!: string | number;
id!: string | number;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should update the docs to reflect this change.

@doug-martin
Copy link
Owner

Hmm not sure why travis is showing as passing, Ill have to take a look at that. I think you can change the deepEqual to a objectContaining from ts-mockito and it might fix it. I think the difference is that it is actually transforming the input into the correct type now.

@doug-martin doug-martin reopened this Feb 23, 2020
@doug-martin
Copy link
Owner

I think the failing test issue should be resolved now. If you can rebase and push back up we can test it out.

@doug-martin
Copy link
Owner

@johannesschobel Im going to issue a new PR with your changes merged and fixed tests.

One thing Im going to revert is the renaming input to id. Ill do this in another PR but Im going to follow the recommendations outlined in https://blog.apollographql.com/designing-graphql-mutations-e09de826ed97

@doug-martin
Copy link
Owner

doug-martin commented Feb 24, 2020

Sorry I'd didnt explain why...I need the changes for a project I'm working on also.

@johannesschobel johannesschobel deleted the feat/validation branch February 24, 2020 06:58
@johannesschobel johannesschobel restored the feat/validation branch February 24, 2020 06:58
@johannesschobel
Copy link
Contributor Author

sure, no problem.. glad i could help (although you did most of it, haha)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants