-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
pCloud component #1392
base: master
Are you sure you want to change the base?
pCloud component #1392
Conversation
This pull request is being automatically deployed with Vercel (learn more). pipedream-docs – ./docs🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs/2EamTarqhRERXzcUeL1S5CEPqcjE pipedream-docs-redirect-do-not-edit – ./docs🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs-redirect-do-not-edit/4U8GUrkjHJxf1xVkjfZaKsN9FEJJ |
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.
@sergioeliot2039 thank you for this! Per the Guidelines, please make the following changes:
- Rebase off master
- Fix all eslint issues
- Refactor all props that are used more than once into the app file
- Refactor all API access code to use the pcloud-sdk-js where possible, unless there is a good reason not to do so (please explain in detail wherever this is the case)
- Move all
require()
statements to the top of the file, and declare them in apackage.json
file in the component dir - Remove the extraneous
make-an-api-call/make-an-api-call.js
action - Add
options()
methods to thefileId
andfolderId
props to populate options
type: "action", | ||
props: { | ||
pcloud, | ||
domainLocation: { propDefinition: [pcloud, "domainLocation"] }, |
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.
@dylburger can we move this into the app auth settings like we did for Mailgun?
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.
@dylburger this is good idea, to have Pipedream UI prompt for a domain or region, so that it is available in the auths variable. In my radar, besides pCloud, Zoho CRM prompts for region. her are the relevant URLs in the respective API documentation.
pCloud: https://docs.pcloud.com/methods/oauth_2.0/authorize.html
ZohoCrm: https://www.zoho.com/crm/developer/docs/api/v2/multi-dc.html
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.
Yes, currently locationid
gets returned in the access token request, which is the numeric ID corresponding to the location, so we can add this to the $auth
variables. It looks like that might have to be mapped to EU
/ US
here (we can't currently run those kind of custom transformations within the system that parses these variables from the OAuth flow), but you'll have access to the ID and shouldn't have to prompt the user.
I'll add that ASAP Monday.
@dylburger could you please comment on this items rised by @compwright
|
@sergioeliot2039 if the library is maintained by the company, and it's not visibly out-of-date or archived on GitHub (for example), I'll usually investigate it. If it's not well-documented or clearly buggy, I'll move to using the REST API. But since the JS client will normally facilitate a lot of the interaction with the API, it's normally worth our time to consider it. I think the number of weekly downloads with this one also speaks to the popularity of the app, and less the JS client itself. Re: the Make an API call action, I actually wouldn't implement that. Getting it right with Pipedream actions may take time, and since our users are more technical than Integromat, I think they'll be able to more easily use the SDK / API directly for operations that aren't implemented in Pipedream. We have a large backlog of other, higher-priority actions, so I'd prefer to focus on those. |
Also even if it isn’t maintained super often, it should still provide a layer of insulation against breakage should we the API itself change.
|
@compwright pCloud is ready for review. btw, from this app a user requested the "upload file" action |
I don't think it is a bug so much as it is perhaps incomplete/confusing documentation. For context on that variable, see pCloud/pcloud-sdk-js@682ed7c Can you give their SDK another try with this in mind? |
@compwright I gave another try and this is what I found. The pCloud SDK is intended to be used under a browser. I tried a couple of example and it breaks with the error This is invariant(client_id, " const oauthUrl = buildOauthUrl({ window.open(oauthUrl, "oauth", "width=680,height=700"); PLease your input. |
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 coming along, thanks for your work on this @sergioeliot2039!
@sergioeliot2039 re: the pCloud SDK, you are incorrect about it being designed only for use in the browser. The SDK provides two types of clients: I'd still like for us to switch this over to using their SDK, hopefully this gets you going in the right direction. If you have further issues let me know and I'm happy to help! |
@compwright @dylburger |
… review, eslint issues fixups
"When you'd like an event to be emitted, by checking on the file's created or modified timestamps. Note: When you manually upload files on pCloud, the file originals's `created` and `modified` timestamps are preserved. When using pCloud API's `uploadfile` endpoint, these fields can be set by specifying the `mtime` and `ctime` parameters.", | ||
default: "Created", | ||
}, | ||
showdeleted: { |
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.
@sergioeliot2039 this still needs to be done.
}, | ||
hooks: { | ||
async deploy() { | ||
const files = []; |
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.
@sergioeliot2039 this still needs to be done.
type: "action", | ||
props: { | ||
pcloud, | ||
fileId: { |
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.
@sergioeliot2039 please refactor all prop definitions into the main app file.
"overwrite", | ||
], | ||
}, | ||
modifiedTime: { |
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.
Move definition to app file
optional: true, | ||
}, | ||
createdTime: { | ||
type: "integer", |
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.
Reuse previous definition
"If set, the uploaded file will be renamed, if file with the requested name exists in the folder.", | ||
default: true, | ||
}, | ||
mtime: { |
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.
Reuse time field definition
"Must be a UNIX timestamp", | ||
optional: true, | ||
}, | ||
ctime: { |
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.
Reuse time field definition
], | ||
description: "ID of the folder you'd like to watch for created or modified files.", | ||
}, | ||
event: { |
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.
Move definition to app file
].includes(this.emmitOn) | ||
? "New created" | ||
: "Modified"; | ||
const eventDate = [ |
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.
@sergioeliot2039 this still needs to be done
: pcloudEvent.modified; | ||
return { | ||
id: pcloudEvent.fileid, | ||
summary: `${newOrModified} file "${pcloudEvent.name}"`, |
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.
@sergioeliot2039 This still needs to be done.
@sergioeliot2039 do you still have this on your radar to finish up? Do you need any help with it? Or do you want us to find someone else to take it over? |
I have pcloud on hold around September 7th at the request of @dylburger on Slack here. Back then we were discussing, in pCloud components, of including the account region prop available inside the auths variable, this dicussion is also on Slack |
@dylburger @dannyroosevelt being said that pcloud is on hold, i have in my radar this issue of pcloud "FIle UPload" action, this was a request by a user on Issue 934. Perhaps we should create a separate PR for File Upload, and only ship this action, and let the user enter the region prop, instead of have it provided in the auths variable. We'd leave the other actions on hold. |
* [App:pCloud] Importing changes from PR #1392 * [App:pCloud] Description adjustments * [App:pCloud] Syntax adjustments * [App:pCloud] Linking doc in description * [App:pCloud] Creating common action and $summary * [App;pCloud] Improving prop sharing and naming * [App:pCloud] Further refining descriptions, plus fixes * [App:pCloud] Prop sharing improvements * [App:pCloud] Customizing shared prop descriptions per component * [App:pCloud] Creating location validation * Initial PR adjustments * [App:pCloud] Creating file validation * [App:pCloud] Watch folder improvements * [App:pCloud] Description updates * [App:pCloud] Including docs in descriptions * pnpm-lock.yaml pCloud #938 * [App:pCloud] Code review requested adjustments #1 * [App:pCloud] Adjusting custom description props * [App:pCloud] Changing common props to propDefinitions * [App:pCloud] Code review adjustments * [App:pCloud] Fixing await inside try/catch
Sources:
Actions:
Make an API CallDev notes: