Skip to content
This repository has been archived by the owner on May 17, 2024. It is now read-only.

[Major] Require samples for all operations #32

Merged
merged 2 commits into from
Feb 19, 2018
Merged

Conversation

ibolmo
Copy link
Contributor

@ibolmo ibolmo commented Feb 16, 2018

This will require a bump in major. We will now require samples from all (trigger, creates, and search) operations.

We'll need to coordinate the release to NPM, and other repos.

@ibolmo ibolmo self-assigned this Feb 16, 2018
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

Everything looks good, just need to revert the manual version change. @eliangcs on monday you can merge this and major. We can also do it on (our) tuesday if we need to check anything else.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "zapier-platform-schema",
"version": "4.3.2",
"version": "5.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

don't change this manually, we set it during the deploy process.

Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

👍 Good to merge once we revoke the version bump in package.json.

@ibolmo
Copy link
Contributor Author

ibolmo commented Feb 19, 2018

@xavdid went ahead and dropped that commit (-f pushed to this branch).

Thanks!

@ibolmo ibolmo merged commit b91cb64 into master Feb 19, 2018
@ibolmo ibolmo deleted the require-samples branch February 19, 2018 20:57
@codebycaleb
Copy link
Contributor

A user from the zapier-platform slack brought up an issue with these changes. To be brief, the user pointed out that with the changes to BasicPollingOperationSchema, any ResourceSchema's list.operation (which is a BasicPollingOperationSchema) will also require the sample field. The specific example they provided was with regards to defining a dynamic drop down field. The sample provided wouldn't necessarily be helpful to be used.

@ptrcko
Copy link

ptrcko commented Feb 23, 2018

Since upgrading to 5.0.0, zapier push fails, but zapier validate does not
https://cbo.d.pr/fy060g

I reverted back and was able to push live.

I think it may have to do with the sample field as I have not included them on all methods. As above, it

@xavdid
Copy link
Contributor

xavdid commented Feb 23, 2018

@b4Beta Can you run zapier validate --debug and see if the output is different?

@ptrcko
Copy link

ptrcko commented Feb 23, 2018

It was not different apart from the more verbose output

Preparing to build and upload your app.

  Copying project to temp directory     done!
  Installing project dependencies     done!
  Applying entry point file     done!
  Building app definition.json     done!
  Validating project ..

Error! We hit some validation errors, try running `zapier validate` to see them!
(Use --debug flag and run this command again to get more details.)

C:\Users\Zapier\Documents\path\to\file>zapier validate --debug

Validating project locally.
No structural errors found during validation routine.
This project is structurally sound!

Checking app style.
>> POST https://zapier.com/api/platform/cli/style-check
>> ....{{App specific stuff}}...
<< 200
<< {"errors": {}, "warnings": {}}

No style errors found during validation routine.
Your app looks great!

@xavdid
Copy link
Contributor

xavdid commented Feb 23, 2018

weird. can you build verbose? It seems like the error is happening and mistakenly being called a validation error

@ptrcko
Copy link

ptrcko commented Feb 23, 2018

  Applying entry point file     done!
  Building app definition.json     done!
  Validating project
Error: We hit some validation errors, try running `zapier validate` to see them!
    at C:\Program Files (x86)\Nodist\bin\node_modules\zapier-platform-cli\lib\utils\build.js:314:13

Error!

The line in that file is:

   if (!_.isEmpty(errors)) {
      throw new Error('We hit some validation errors, try running `zapier validate` to see them!');
    }

I added console.log(error) before it and got a number of issues that aren't making it to the terminal.

I'm not sure they are correct:

{ property: 'instance.resources.customer.get.operation',
  message: 'requires property "sample"',
  schema: '/BasicOperationSchema',
  instance: [Object],
  name: 'required',
  argument: 'sample',
  stack: 'instance.resources.customer.get.operation requires property "sample"',
  codeLinks: [Object],
  docLinks: [Object] },

As this is a resource it follows the format of:

  key: 'customer',
  noun: 'Customer',

  get: {
    display: {
      .....
    },
    operation: {
      inputFields: [
      .....
      ],
      perform: getCustomerById
    }
  },

  list: {
    display: {
      ....
    },
    operation: {
      perform: listCustomers
    }
  },

  search: {
    display: {
      .....
    },
    operation: {
      inputFields: [
      ....
      ],
      perform: searchCustomers,
      performGet : (z, bundle) => {return bundle.inputData;}
    }
  },

  create: {
    display: {
      ....
    },
    operation: {
      inputFields: [
        .....
      ],
      perform: createCustomer,
      performGet : getCustomerById
    }
  },

  sample: {
   ......
  },

  outputFields: [
  .....
  ]

Should sample be added to each get, list, search and create - that would be a lot of code change

@xavdid
Copy link
Contributor

xavdid commented Feb 23, 2018

Very strange. I'm going to file a separate issue and tag you and we can go from there.

@ptrcko
Copy link

ptrcko commented Feb 23, 2018

I updated my comment with more details.

@xavdid
Copy link
Contributor

xavdid commented Feb 23, 2018

Ah! This is less mysterious than I thought. Definitely a bug though.

The short answer is yes, each of those needs a sample. The good news is that you can put a single sample at the top of the file and use it in each of the [get|list|search|create] methods.

@ptrcko
Copy link

ptrcko commented Feb 23, 2018

Mmmm...That is quite a bit of work and not the documented way
https://github.com/zapier/zapier-platform-example-app-resource/blob/master/resources/recipe.js

Shows module.exports with one sample as the second last property of the object - that is how I have implemented it.

@xavdid
Copy link
Contributor

xavdid commented Feb 23, 2018

Ah, sorry about that. It's a recent change and we're still in the process of updating documentation. If you've already written the sample, it should be an easy fix!

Instead of

module.exports = {
  key: 'recipe',
  noun: 'Recipe',

  get: {
    display: {
      label: 'Get Recipe',
      description: 'Gets a recipe.',
    },
    operation: {
      inputFields: [
        { key: 'id', required: true },
      ],
      perform: () => { },
    },
  },

  sample: {
    id: 1,
    createdAt: 1472069465,
    name: 'Best Spagetti Ever',
    authorId: 1,
    directions: '1. Boil Noodles\n2.Serve with sauce',
    style: 'italian',
  },

  outputFields: [
    { key: 'id', label: 'ID' },
    { key: 'createdAt', label: 'Created At' },
    { key: 'name', label: 'Name' },
    { key: 'directions', label: 'Directions' },
    { key: 'authorId', label: 'Author ID' },
    { key: 'style', label: 'Style' },
  ]
};

change it to

const sample = {
  id: 1,
  createdAt: 1472069465,
  name: 'Best Spagetti Ever',
  authorId: 1,
  directions: '1. Boil Noodles\n2.Serve with sauce',
  style: 'italian',
}

module.exports = {
  key: 'recipe',
  noun: 'Recipe',

  get: {
    display: {
      label: 'Get Recipe',
      description: 'Gets a recipe.',
    },
    operation: {
      inputFields: [
        { key: 'id', required: true },
      ],
      perform: () => { },
      sample: sample // ADD THIS LINE
    },
  },

  sample: sample, // now uses the variable

  outputFields: [
    { key: 'id', label: 'ID' },
    { key: 'createdAt', label: 'Created At' },
    { key: 'name', label: 'Name' },
    { key: 'directions', label: 'Directions' },
    { key: 'authorId', label: 'Author ID' },
    { key: 'style', label: 'Style' },
  ]
};

If you don't want to do this, you're also free to continue using v4 for now. This is totally a pain point though, so we'll see what we can do about that.

@ptrcko
Copy link

ptrcko commented Mar 1, 2018

Just to let you know that the scaffold for resource has not been updated.

@xavdid
Copy link
Contributor

xavdid commented Mar 1, 2018

@b4Beta It's on my list to fix this week!

In the meantime, do you mind popping over to zapier/zapier-platform-cli#250 and posting a code example to help me reproduce the issue where build reports errors, but validate doesn't? I used both the resource scaffold and made a new app with the resource template and both failed validate correctly. Your full resource would be great, if possible.

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

Successfully merging this pull request may close these issues.

5 participants