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

add type json-schemas for all data models #124

Merged
merged 16 commits into from
Aug 30, 2017
Merged

Conversation

michael-smith-nz
Copy link
Member

@michael-smith-nz michael-smith-nz commented Aug 1, 2017

In this PR:

  • added docs to readme explaining json-schema and json-schema-faker
  • profile, taskPlan and taskRecipe json schema
  • profile story now uses json-faker for mock data (see gif below)
  • feathers hook for validating taskPlan and taskRecipe
  • tests for feathers validation
  • minor bug fixes that were blocking me

Not in this PR

  • schemas for models that are not being used by feathers (e.g taskWork)
  • dogstack auth schemas

mock-profile

closes #118

@michael-smith-nz michael-smith-nz changed the title chore(docs): add json schema info add type json-schemas for all data models Aug 1, 2017
@@ -105,11 +105,6 @@ export default compose(
toggleEdit: ({ setEditing }) => () => setEditing(not)
}),
connectForm({
form: 'profile',
initialValues: {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ahdinosaur How can I fill the form with data from props? When I remove the initialValues the form is empty. I'm needing this to fill the Profile story with fake data.

Copy link
Member

Choose a reason for hiding this comment

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

right, ideally we pass it in from here. i think we did something about this in #120, one sec.

Copy link
Member

Choose a reason for hiding this comment

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

right, we pass in initivalValues and enableReinitialize. can either cherry-pick from there, merge that pull request as is and rebase on top, or add those lines here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ahdinosaur Thanks! Added those lines (see below commit)

@michael-smith-nz michael-smith-nz self-assigned this Aug 3, 2017

const Ajv = require('ajv')
const { validateSchema } = require('feathers-hooks-common')
const schema = require('../schemas/taskPlan')
Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @ahdinosaur Would love some help here as I've been working on this problem for a while with no luck.
I'm getting this error when running this test:

   [SyntaxError: Error resolving $ref pointer "http://localhost:3000/tasks/schemas/taskRecipe.json#/id". 
    Token "id" does not exist.]

Even when the id property is in the schema

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

it seems the library expects to find referenced schemas at a particular remote URL, according to the JSON schema standard. we want to somehow locally pass in all the schemas up front and tell it not to look remotely.

"type": "object",
"properties": {
"id": {
"$ref": "#/id"
Copy link
Member

Choose a reason for hiding this comment

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

or maybe this should be #/definitions/id?

@@ -226,6 +226,12 @@ cp cobuy-config/*.js cobuy/config
heroku run npm run db migrate:latest --app=cobuy
```

### JSON schema
Copy link
Member

Choose a reason for hiding this comment

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

👍 yay docs 📔

Copy link
Member

@ahdinosaur ahdinosaur left a comment

Choose a reason for hiding this comment

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

🥇

"required": [
"id",
"agentId",
"name"
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure if name is required. or at least, at the moment i think we create "empty" profiles when we create a group agent to be updated later.

avatar: 'http://dinosaur.is/images/mikey-small.jpg'
}
}
const profile = jsf(schema)
Copy link
Member

Choose a reason for hiding this comment

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

fk ya

"description": "Id referencing profile"
},
"agentId": {
"type": "integer",
Copy link
Member

Choose a reason for hiding this comment

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

can this be

{
  "$ref": "/agents/Agent#/definitions/id"
}

oh wait nevermind, we don't have a schema for Agent. all good!

"description": "Person or group or related agents (e.g. admins) being assigned"
},
"taskRecipeId": {
"$ref": "/tasks/taskRecipe#/definitions/id",
Copy link
Member

Choose a reason for hiding this comment

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

sweet. 🍈

{
"id": "/tasks/taskPlan",
"title": "Task Plan",
"description": "An assignment of the task to an agent",
Copy link
Member

Choose a reason for hiding this comment

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

yes! thanks you.

"description": "Id referencing task plan"
},
"parentTaskPlanID": {
"type": "integer",
Copy link
Member

Choose a reason for hiding this comment

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

can this be a $ref like taskRecipeId?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good catch

const Ajv = require('ajv')
const taskPlanSchema = require('../schemas/taskPlan')
const taskRecipeSchema = require('../schemas/taskRecipe')
const ajv = new Ajv({$data: true})
Copy link
Member

Choose a reason for hiding this comment

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

i wonder if this should be a singleton somewhere in a module. like we export the ajv object with all the schemas added in app/schema.js. then we'd require that here.

all: [
// encodeParams
]
create: [validateSchema(taskPlanSchema, ajv), encodeParams],
Copy link
Member

Choose a reason for hiding this comment

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

very legit.

minor nit: maybe format for each hook to be on its own line.

@@ -46,11 +51,11 @@ function createChildTaskPlans (hook) {
parentTaskPlanId: hook.result.id,
assigneeId: hook.data.assigneeId,
taskRecipeId: childTaskRecipe.id,
params: hook.data.params
params: hook.result.params
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

@@ -0,0 +1,34 @@
import test from 'ava'
Copy link
Member

Choose a reason for hiding this comment

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

woo hoo tests 😊

@michael-smith-nz
Copy link
Member Author

@ahdinosaur Changes and rebase made ready for merge

@ahdinosaur ahdinosaur merged commit 114dc88 into master Aug 30, 2017
@ahdinosaur ahdinosaur removed the review label Aug 30, 2017
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.

add type json-schemas for all data models
2 participants