-
Notifications
You must be signed in to change notification settings - Fork 187
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
PDE-5636 feat(cli): include extra fields for analytics #939
Conversation
…pier/zapier-platform into PDE-5636_expand_analyticsBody
…pier/zapier-platform into PDE-5636_expand_analyticsBody
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.
I did some quick tests to make sure commands still work. And they do! Approving.
However, while testing, I noticed the debug logs print undefined
request URLs.
This may be not related to this PR. But maybe you can fix it along the way? You can fix it by replacing this line:
debug(`>> ${requestOptions.method} ${requestOptions.url}`); |
with:
debug(`>> ${requestOptions.method} ${requestOptions.url || res.url}`);
// Some commands ( like "zapier convert" ) won't have an app directory when called. | ||
// Instead, having the app ID in the arguments. | ||
// In this case, we fallback to using "integrationid" in arguments ( if it's there ) | ||
// and don't want to "explode" if appID is missing |
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.
Nice catch!
Thanks for the review and help testing @eliangcs! Followed your suggestion and fixed the debug log! |
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.
Thanks for the change! Approved.
This pull request expands the analytics functionality in the Zapier CLI by updating the way arguments are recorded and adding additional information to the analytics payload.
Changes
Updated
analytics.js
:app_id
in the analytics payload, which is fetched from the linked app configuration OR integrationid" in arguments since some commands ( like "zapier convert" ) won't have a linked app directory when called.Updated
ZapierBaseCommand.js
:Object.keys(this.args)
tothis.args
. This allows us to later capture app ID argument value based on the name and populatenumArgs
and parts offlags
. This is not meant to capturethis.args
in analytics as shown in tests belowImpact
The inclusion of app ID will provide better insights into CLI usage patterns. There is another ticket to track work required for adding "arguments" effectively and safely PDE-5641
Tests
Tested the changes locally