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

Fix field name to match Dredd #37

Merged
merged 2 commits into from
Aug 25, 2017
Merged

Fix field name to match Dredd #37

merged 2 commits into from
Aug 25, 2017

Conversation

IngmarStein
Copy link
Contributor

Fixes a bug introduced in 0.3.0

Ingmar Stein added 2 commits August 21, 2017 14:51
Fixes a bug introduced in 0.3.0
@ddelnano
Copy link
Collaborator

@IngmarStein can you provide more context on what the issue is? In the dredd docs it says that the key should be schema.

@IngmarStein
Copy link
Contributor Author

Sorry, I could have been clearer but it seems you already found that dredd's documentation was out of sync and that the code actually uses bodySchema instead of schema.

@IngmarStein
Copy link
Contributor Author

To add some detail: goodman 0.3.0 introduced all kinds of validation errors in our tests (mainly optional fields not being optional) and they were fixed by the change in this PR.

@ddelnano
Copy link
Collaborator

Yea basically I was trying to understand how bad this change was for users since the integration tests still passed. I get this is the right change but I'd like to add additional testing that would would have failed the PR that broke things. Do you have any of the failure output that can help determine what the appropriate tests would be?

@IngmarStein
Copy link
Contributor Author

IngmarStein commented Aug 22, 2017

I got many errors similar to At '/0/accounts' Missing required property: accounts.
This example above relates to a data structure defined as follows:

## Access (object,fixed-type)

+ id:                    2                (number) - Unique id
+ name                                    (string) - Name of the access
+ enabled:               true             (boolean) - Flag if the access is enabled
+ is_pin_saved:          false            (boolean) - Flag indicating whether the PIN is saved with the access
+ accounts                                (array[Account],optional,fixed-type) - The accounts that belong to the access
+ provider_id:          `DE-BIN-10010010` (string) - Financial institution reference

I assume that the optional flag was lost in transmission due to the field name mismatch.

The expected response is simply defined as

+ Response 200 (application/json; charset=utf-8)

    + Attributes (array[Access], fixed-type)

@IngmarStein
Copy link
Contributor Author

Maybe you can merge this PR and add a regression test in a separate one?

@ddelnano ddelnano merged commit 87fd9cd into snikch:master Aug 25, 2017
@ddelnano
Copy link
Collaborator

Yea I thought I would have time sooner rather than later but I'll have to add a test next chance I get.

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.

2 participants