All: Resolves #1266: Add support for onedrive for Business #3433
Conversation
…ntly not working. Very bad coded at the moment
tessus
added
enhancement
|
I have tested the code now on Android and Linux with an OneDrive for Business and Personal account and it is working. I haven't added any automated tests as there are currently only rough tests for syncing and therefore the current tests are sufficient as far as I can see. I don't understand the purpose of the "OneDrive Dev" sync target which is only visible when you build joplin from source. No other sync target has an extra Dev target. I had to add an extra line to make it also work with "OneDrive Dev" which I almost forgot. So @laurent22 I think the code is finish but I am happy to improve the code when you see anything. |
| return `${r.parentReference.path}/${r.name}`; | ||
| } | ||
|
|
||
| authCodeUrl(redirectUri) { | ||
| const query = { | ||
| client_id: this.clientId_, | ||
| scope: 'files.readwrite offline_access', | ||
| scope: 'files.readwrite offline_access sites.readwrite.all', |
There was a problem hiding this comment.
How will that work with existing apps that have already authorised, but with a different scope?
There was a problem hiding this comment.
This is no problem because all requests to the OneDrive API also work without the "sites.readwrite.all" permission for OneDrive Personal accounts. The "sites.readwrite.all" permission is only important for Business accounts. To make sure I also have tested the PR with an OneDrive Personal Account and without the "sites.readwrite.all" permission without problems. This might only be problematic if a user has logged in with an OneDrive Business account in a previous version. In this case the user might think that he only has to upgrade Joplin in order to use his account but this is no true as he also has to log out and log in again before he can use his OneDrive Business account.
|
Thanks for your review @laurent22. I have now solved all your comments. In particular the potential error is now shown in the sidebar. Let me know if other changes are required. |
|
Looks good, thanks for the update @jonath92! |
The code works already but I still need to make some minor improvements (error handling, testing). I also need to check it on mobile (although I am pretty sure it will work). So it is still work in progress .