-
Notifications
You must be signed in to change notification settings - Fork 15
Allow file stashing within a create/action #45
Conversation
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.
This is looking great! I'd only like to see an extra test since we're specifically mentioning the trigger now, ideally test search, hydration, and create as well (proper passing and failing conditions).
@@ -110,7 +110,7 @@ describe('file upload', () => { | |||
.catch(done); | |||
}); | |||
|
|||
it('should fail if not being called from an hydrator event', (done) => { | |||
it('should fail if being called from a trigger event', (done) => { |
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.
Looks good! Please consider adding a test for a search (and maybe passing ones for hydration and create) since you're being specific here.
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.
All set!
src/tools/create-file-stasher.js
Outdated
const isRunningOnHydrator = _.get(input, '_zapier.event.method', '').indexOf('hydrators.') === 0; | ||
if (!isRunningOnHydrator) { | ||
return ZapierPromise.reject(new Error('Cannot stash files outside an hydration function/method.')); | ||
const isRunningOnHydratorOrCreate = () => { |
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.
This looks good! I'd suggest considering keeping isRunningOnHydrator
and adding a isRunningOnCreate
, then you can do the check with if (!isRunningOnHydrator && !isRunningOnCreate)
. I think it might become more clear and easier to add a isRunningOnSearch
later if we find it necessary. Again, no need to take the suggestion!
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.
Great idea - done!
@BrunoBernardino Can you double check things, and see if it looks all-clear for merging? |
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.
This looks really clear and well-tested now! Great job! 👍
Oh, and please don't release a new version after merging this. I'm expecting to ship other things this week and will take care of releasing a new version with these changes included this week or next. |
Needed for situations where an action/create receives the file's contents from the API/endpoint, and needs to stash that for use in future Zap steps.