Skip to content
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

Implement getItemURL for Dropbox #1130

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ragnis
Copy link
Member

@Ragnis Ragnis commented Jun 2, 2018

@raucao
Copy link
Member

raucao commented Nov 17, 2018

@Ragnis Is this actually still work in progress? Looks to me like this is basically done.

@raucao
Copy link
Member

raucao commented Feb 14, 2019

@Ragnis Would be great if you could update us on the status here. See my question from November.

Ragnis and others added 3 commits February 14, 2019 13:10
The test assumes that if a file is successfully fetched from a /public/
directory then the download URL can also be used as a public URL. That
is however not true since DropBox requires all public URLs to be
explicitly created.
@Ragnis Ragnis force-pushed the feature/1052-reimplement-getitemurl branch from 8ec6a83 to 26be9bf Compare February 14, 2019 11:30
@ghost ghost assigned Ragnis Feb 14, 2019
@ghost ghost added the in progress label Feb 14, 2019
@Ragnis
Copy link
Member Author

Ragnis commented Feb 14, 2019

I guess it is done. I re-based the code to latest master and also fixed the tests. I also removed a failing test which was based on false assumptions.

@Ragnis Ragnis changed the title [WIP] Implement getItemURL for Dropbox Implement getItemURL for Dropbox Feb 14, 2019
Copy link
Member

@raucao raucao left a comment

Choose a reason for hiding this comment

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

I just played around with this branch in Sharesome and found a logical bug with the implementation:

The way the function works on a normal BaseClient is that the path argument is relative to the client scope. So when used in a module, the publicClient would have a base of mymodulename/public/. Hence the makePath() call here: https://github.com/remotestorage/remotestorage.js/blob/master/src/baseclient.js#L299

However, this implementation doesn't add the base path, and so _itemRefs aren't matched correctly when calling getItemURL() with a relative path (in the shares module that would just be a filename e.g.). This results in undefined being returned instead of the stored public URL from _itemRefs.


Another thing, but I'm not 100% sure how to solve it, is that storeFile() actually returns a URL on a normal BaseClient, but we only overwrite put in the Dropbox class, and returning a simple URL from that broke it for me when I tried to add it.

@@ -978,7 +1009,8 @@ Dropbox.prototype = {
},

/**
* Requests the link for an already-shared file or folder.
* Requests the link for a shared file or folder. Resolves to undefined if
* the requested file or folder has not bee shared.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* the requested file or folder has not bee shared.
* the requested file or folder has not been shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants