Code review comment for lp://staging/~gary-wzl77/account-plugins/onedrive_provider

Revision history for this message
Alberto Mardegan (mardy) wrote :

Hi Gary, after trying out this branch I would like to request some changes. You can see that I've proposed a branch, please merge it into this one.

The most serious problem I've found is that I had a live.com account I had been using for testing, but I never used onedrive before with that account. When I tried to create the account in my phone the authentication went fine, I could get the access code, but then getting the username was failing:

=================
ui-proxy.cpp 184 onDataReady QMap(("code", QVariant(QString, "finished") ) ( "data" , QVariant(QVariantMap, QMap(("UrlResponse", QVariant(QString, "https://login.live.com/oauth20_desktop.srf?lc=1040#access_token=XXX&token_type=bearer&expires_in=3600&scope=onedrive.readwrite onedrive.appfolder onedrive.readonly&user_id=AAAAAAAAAAAAAAAAAAAAAPywB7MPl1COV-NhKCYqWDw") ) ) ) ) ( "delay" , QVariant(int, 0) ) ( "id" , QVariant(int, 1) ) ( "interface" , QVariant(QString, "com.nokia.singlesignonui") ) )
browser-request.cpp 125 ~BrowserRequestPrivate
browser-request.cpp 325 closeView
qml: error: 403
qml: response text: {"error":{"code":"accessDenied","message":"Access Denied"}}
qml: Removing credentials...
qml: ====== PLUGIN FINISHED ======
provider-request.cpp 175 deny
=================

I tried many times, and it always failed. Then I tried to go to the onedrive site from the browser, and after logging in (with the same account) I was asked to accept the terms & conditions, and then I got to the main web page of onedrive where I can see my folders (all empty, since I never used it).
After doing this step, I tried creating the account on the phone again, and this time it worked.

To me, it sounds like a bug in the onedrive server, but I wonder if we should handle it better; maybe you can change the code in the onedrive QML plugin, and if the username cannot be obtained you can just do 'callback("")' (that is, you return an empty username) instead of calling cancel(). What do you think?

Also, I don't think we should include the user ID in the display name, it makes the display name too long (in my phone, it occupies 2 rows!). Maybe we should use the user ID only if the user name is empty. Can you please change this?

review: Needs Fixing

« Back to merge proposal