Closed Bug 763008 Opened 8 years ago Closed 7 years ago

update to YSI v3 API

Categories

(Thunderbird :: FileLink, defect)

x86_64
Windows 7
defect
Not set

Tracking

(thunderbird14 fixed, thunderbird15 fixed)

RESOLVED FIXED
Thunderbird 16.0
Tracking Status
thunderbird14 --- fixed
thunderbird15 --- fixed

People

(Reporter: Bienvenu, Assigned: mbaz)

Details

Attachments

(5 files, 9 obsolete files)

75.33 KB, patch
Bienvenu
: review+
Bienvenu
: ui-review+
Details | Diff | Splinter Review
47.00 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
35.64 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
75.32 KB, patch
Details | Diff | Splinter Review
46.99 KB, patch
Details | Diff | Splinter Review
Attached patch proposed fix (obsolete) — Splinter Review
YSI has a new api for folder mgmt
I've run with this patch - basic link creation seems to work fine, and the options ui looks good too. What's broken is our mozmill tests. This may be because our ysi mozmill fake server needs to be updated to deal with the new api. mconley can probably help with that if you have questions.

to see the mozmill failures, you can do the following:

cd <objdir>
make SOLO_FILE=cloudfile/test-cloudfile-backend-yousendit.js mozmill-one

we're getting the following error:

{"key":null,"id":null,"type":"BAS","policy":null,"version":"v3","password":null,
"role":null,"email":"john@example.com","firstname":"John","lastname":"User","cre
ated":null,"account":{"passwordProtect":"Pay-per-use","returnReceipt":"Pay-per-u
se","availableStorage":"2147483648","billingPlan":null,"controlExpirationDate":n
ull,"dropboxUrl":null,"knowledgeBase":"Yes","maxDownloadBWpermonth":"1073741824"
,"maxFileDownloads":"100","maxFileSize":"104857600","premiumDelivery":null,"veri
fyRecipientIdentity":"Included"},"storage":{"currentUsage":1045,"storageQuota":2
147483648},"status":null,"errorStatus":null}
2012-06-11 12:14:03     YouSendIt       INFO    user info response parsed = [obj
ect Object]
2012-06-11 12:14:03     YouSendIt       INFO    available storage = 2147482603 m
ax file size = 52428800
2012-06-11 12:14:03     YouSendIt       INFO    checkFolderID
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "JSON.parse: unexpected character" {file: "fi
le:///c:/builds/tbirdhq/objdir-tb/mozilla/dist/bin/components/nsYouSendIt.js" li
ne: 543}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "
0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native fram
e :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************
Test Failure: waitFor: Timeout exceeded for 'function () requestObserver.success

perhaps because the test code doesn't know about the new folderId command. You may want to look at mail\test\mozmill\shared-modules\test-cloudfile-yousendit-helpers.js

Also, side note, if you look at the diff, you'll see a few warnings about files not ending with a new line. You should fix those in your next patch, thx!
Attached patch proposed fix corrected (obsolete) — Splinter Review
Hello David,

Thanks for your comments. I fixed tests and new line endings, and attached new diff. Please take a look.

Thanks!

(In reply to Maxim Baz from comment #2)
> Created attachment 632554 [details] [diff] [review]
> proposed fix corrected
this patch doesn't quite apply against the trunk - the chunk starting with @@ -905,48 +1101,98 @@ nsYouSendItFileUploader.prototype = {

fails.
Attached patch proposed fix corrected 2 (obsolete) — Splinter Review
Sorry, that was my mistake. 
I attached new patch, I checked - it's applying without errors.
Attachment #632554 - Attachment is obsolete: true
(In reply to Maxim Baz from comment #5)
> Created attachment 632739 [details] [diff] [review]
> proposed fix corrected 2
> 
> Sorry, that was my mistake. 
> I attached new patch, I checked - it's applying without errors.

thx, yes, that applied, and the tests pass now, thx!
Comment on attachment 632739 [details] [diff] [review]
proposed fix corrected 2

I tried attaching a file > 50MB, and I got the following error on the console:

Request] response = {"name":"Apps","id":"15625805","type":"private","size":"0","
revision":"1339342730950","folders":{"folder":[{"name":"Mozilla Thunderbird","id
":"15626766","type":"private","size":"20016","revision":"1339633107853","folders
":{"folder":[]},"parentid":"15625805","ownedByStorage":null,"currentUsage":null,
"storageQuota":null,"createdOn":"2012-06-10T08:38:51-07:00","files":{"file":[]},
"fileCount":null,"folderCount":null,"ownedByUser":null,"readable":"true","update
dOn":"2012-06-13T17:18:27-07:00","writeable":"true","status":null,"errorStatus":
null}]},"parentid":"0","ownedByStorage":null,"currentUsage":null,"storageQuota":
null,"createdOn":"2012-06-10T08:38:50-07:00","files":null,"fileCount":"0","folde
rCount":"1","ownedByUser":null,"readable":"true","updatedOn":"2012-06-10T08:38:5
0-07:00","writeable":"true","status":null,"errorStatus":null}
2012-06-13 17:21:33     YouSendIt       INFO    Looking for a child folder
2012-06-13 17:21:33     YouSendIt       INFO    saveFolderId
2012-06-13 17:21:33     YouSendIt       INFO    in cancel upload
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._uploader is null" {file: "file:///C:/b
uilds/tbirdhq/objdir-tb/mozilla/dist/bin/components/nsYouSendIt.js" line: 288}]'
 when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021
 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unkn
own filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

so I couldn't see the upgrade UI...
Attachment #632739 - Flags: review-
Attached patch proposed fix corrected 3 (obsolete) — Splinter Review
It's strange, anyway sorry for that!
I checked that part of code more carefully...and attached new fix, please review it.
Attachment #632739 - Attachment is obsolete: true
Attachment #633039 - Flags: review?(dbienvenu)
Maxim, I tried attaching a 63 MB file with the new patch - now I don't see any error on the console, but it doesn't seem to be uploading, and the user doesn't see any error either.
smaller uploads fail as well, in the same way.
Comment on attachment 633039 [details] [diff] [review]
proposed fix corrected 3

minus based on upload not working for me...
Attachment #633039 - Flags: review?(dbienvenu) → review-
Attached patch proposed fix corrected 4 (obsolete) — Splinter Review
I updated jar.mn files, now everything should be OK.
Thank you, David, for helping with this issue.
Attachment #633039 - Attachment is obsolete: true
Attached patch de-bitrotted patch (obsolete) — Splinter Review
Maxim's patch had bit-rotted slightly. Building with this patch now.
Attachment #633526 - Flags: review?(dbienvenu)
Comment on attachment 633526 [details] [diff] [review]
de-bitrotted patch

cool, this patch works for me...requesting ui review.
Attachment #633526 - Flags: ui-review?(bwinton)
Comment on attachment 633526 [details] [diff] [review]
de-bitrotted patch

you can use let instead of var here:
+  if ("wrappedJSObject" in window.arguments[0]) {
+    var storage = parseInt(window.arguments[0].wrappedJSObject.storage);
+    storage = (storage / 1024 / 1024 / 1024).toFixed(2);
+    var currentStorage = document.getElementById('currentStorage');
+    currentStorage.textContent = currentStorage.textContent.replace('#XXX', storage);


You don't need the braces here (mozilla style is mostly not to include unneeded braces)
+    if (aFile.fileSize > 2147483648) {
+      return this._fileExceedsLimit(aCallback, '2GB', 0);
+    }
+    if (aFile.fileSize > this._maxFileSize) {
+      return this._fileExceedsLimit(aCallback, 'Limit', 0);
+    }
+    if (aFile.fileSize > this._availableStorage) {
+      return this._fileExceedsLimit(aCallback, 'Quota', 
+                                    aFile.fileSize + this._fileSpaceUsed);
+    }

let here as well:

+    var args = {storage: aStorageSize};
+    args.wrappedJSObject = args;

is the + 0 really needed here?

+          this._fileSpaceUsed = parseInt(storage.currentUsage) + 0;
+          this._availableStorage = parseInt(storage.storageQuota) - this._fileSpaceUsed;

this doesn't look right - either the indentation is wrong, or the braces are wrong:
+      if (req.status >= 200 && req.status < 400) {
+        this.log.info("request status = " + req + " response = " +
+                      req.responseText);
+		
+      if(aFoundCallback && docResponse.folders)
+        aFoundCallback(docResponse.folders);
+      }

same thing here:
+      if (req.status >= 200 && req.status < 400) {
+        this.log.info("request status = " + req + " response = " +
+                      req.responseText);
+		
+      if(aSuccessCallback)
+        aSuccessCallback(docResponse.id)
+      }
+

you can use the ? operator here instead of if else:

+        if (this.file.leafName.length > 120) 
+          this.callback(this.requestObserver, Ci.nsIMsgCloudFileProvider.uploadExceedsFileNameLimit);
+        else
+          this.callback(this.requestObserver, Ci.nsIMsgCloudFileProvider.uploadErr);

space after for here:
+    for(let i in this._foldersId) {
+      this._server.registerPathHandler(kFolderPath + this._foldersId[i],
+                                       null);
+    }

all tests pass, and this is looking fairly good. But minusing for the above comments.
Attachment #633526 - Flags: review?(dbienvenu) → review-
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink
Attached patch proposed fix corrected 5 (obsolete) — Splinter Review
Thanks, David. 
I fixed all problems you told me and some similar problems as well.
Can you review new patch?
Thanks!
Attachment #633408 - Attachment is obsolete: true
de-bitrotted patch - Maxim, you should update your mercurial repo to pick up the latest changes, so your patches will apply cleanly going forward.
Attachment #633656 - Attachment is obsolete: true
Maxim, this just shows you how to tell the compose window front end about the file size limit exceeded error and have it put up its own message. It's not a useful patch, but I said I'd put it in the bug, so I did. Per my e-mail, this is what you'd do:

   The way it works is that the YSI file link module would return either uploadWouldExceedQuota or uploadExceedsFileLimit errors. And, it would put the appropriate urls for those errors in the method providerUrlForError. If you do that, Thunderbird will put up a prompt asking the user if they want to upgrade, and if the user says yes, we'll open the default browser with the url you provide.
Attachment #633526 - Attachment is obsolete: true
Attachment #633526 - Flags: ui-review?(bwinton)
Comment on attachment 633712 [details] [diff] [review]
de-bitrotted patch

this looks ok to me; sending over to Blake for UI review.

One thought is that we might want to use this prettier UI in a more general way, if possible. This would entail invoking it from MsgComposeCommands.js instead of nsYouSendIt.js. But I'll let you and Blake talk about that...you're certainly free to do your own UI.
Attachment #633712 - Flags: ui-review?(bwinton)
Attachment #633712 - Flags: review+
Attached patch patch for old TBSplinter Review
I attached patch for old TB. 
Removed all unnecessary files.

By the way, I don't know what "de-bitrotted" means, but I always pull changes and update repo before creating patch. And when I created previous 2 patches, there were no updates in repo...
Comment on attachment 633741 [details] [diff] [review]
patch for old TB

getting on my review radar
Attachment #633741 - Flags: review?(dbienvenu)
Comment on attachment 633741 [details] [diff] [review]
patch for old TB

this seems to work fine. There are a couple blanks lines with spaces on them. I'll upload a version with that fixed in a minute.
Attachment #633741 - Flags: review?(dbienvenu) → review+
this is ready to land, though I'd like to land the trunk patch first just to get a little more testing, if possible.
Attachment #633741 - Attachment is obsolete: true
Attachment #634439 - Flags: review+
Comment on attachment 633712 [details] [diff] [review]
de-bitrotted patch

Blake says he likes it.
Attachment #633712 - Flags: ui-review?(bwinton) → ui-review+
landed on trunk - http://hg.mozilla.org/comm-central/rev/ee9bf9095b6e
landed on beta - http://hg.mozilla.org/releases/comm-beta/rev/40dbdb482fac

I'll land on Aurora next.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Got an error on Linux:
RuntimeError: File "mail/cloudfile/YouSendIt/check.png" not found in /home/zoe/hg/comm-central/mail/themes/gnomestripe, /home/zoe/hg/comm-central/objdir-thunderbird/mail/themes/gnomestripe
landed missing check.png on trunk - http://hg.mozilla.org/comm-central/rev/4468b7bdbd88
Thanks, now the file is coming.
I attached the same patch for new TB, which already passed review, with change in single text line.
http://hg.mozilla.org/comm-central/rev/c22b706b16e2 landed for string change. Since this has only been checked in for an hour, I didn't change the string id, which normally we do when the meaning of a string changes.
Is there an error in "errorCloudFileNameLimit.message=Uploading %2$S to %1$S contains has more than 120 characters in its name." ? 

"%2$S contains more than 120 characters in its name." seems to have more sense, but maybe I just don't understand the original sentence.
Attached patch Patch to revert to old API (obsolete) — Splinter Review
In the event that the v3 API bugs (bug 771073, bug 771131, bug 771132) cannot be resolved, we'll need to back this out.  This patch will reverse these changes.
Comment on attachment 639670 [details] [diff] [review]
Patch to revert to old API

Hrm, so this doesn't apply on comm-beta - give me a sec to re-tool this.
Attachment #639670 - Attachment is obsolete: true
This patch applies cleanly on top of comm-beta, and will revert us back to the old YSI API.
Assignee: nobody → mbaz
You need to log in before you can comment on or make changes to this bug.