-
Notifications
You must be signed in to change notification settings - Fork 2.1k
companion: return nextPagePath for drive/dropbox #1652
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
companion: return nextPagePath for drive/dropbox #1652
Conversation
|
Hi, thanks for the PR! We’ll try to review as soon as possible. |
ifedapoolarewaju
left a comment
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.
Thank you for this PR! I really appreciate the work, and the cleanup done for GoogleDrive. I left some comments here and there. Please let me know what you think.
|
|
||
| exports.getItemRequestPath = (item) => { | ||
| // If it's from a Team Drive, add the Team Drive ID as a query param. | ||
| // If it's from/is a Team Drive, add the Team Drive ID as a query param. |
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.
is this a typo?
| // If it's from a Team Drive, add the Team Drive ID as a query param. | ||
| // If it's from/is a Team Drive, add the Team Drive ID as a query param. | ||
| // The server needs the Team Drive ID to list files in a Team Drive folder. | ||
| if (exports.isTeamDrive(item)) { |
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.
does this mean that we can delete the if block that is right under this one?
|
|
||
| let where = { | ||
| fields: DRIVE_FILES_FIELDS, | ||
| pageToken: query.nextPageToken, |
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.
maybe we should make the nextPage query name the same for all providers? so each provider would use it as it requires internally. But between companion and uppy client it will be the same query name. For instagram we are already using max_id but we can change it and still support max_id for backward compatibility.
|
@ifedapoolarewaju The team drive thing wasn't a typo but it was a bit redundant - I did some more testing and cleaned that up, the Also updated all providers to use |
|
Thank you for the work! 💪 |
|
@ifedapoolarewaju No problem - has any progress been made on that task? If not I could get to it this week |
|
@stephentuso no work has begun on this yet |
|
@stephentuso FYI, I realized that the current frontend implementation should already work with the API changes you made in this PR, so there should be no need to add/change anything on the frontend |
…gination companion: return nextPagePath for drive/dropbox
See #1611 - this is only the backend changes, the frontend will need an update for the issue to be completely resolved
Was the idea for all the
adapter.jsfiles to have the same interface? Drive/dropbox required different parameters, I updated the instagramgetNextPagePathto match.Team drive support appeared to be broken, so I made a few changes to get it working. Also, the current team drive API being used is deprecated and I think its usage can be simplified some, I didn't want too make to many changes re that in this PR though