The default bug view has changed. See this FAQ.

update to YSI v3 API

RESOLVED FIXED in Thunderbird 16.0

Status

Thunderbird
FileLink
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Bienvenu, Assigned: Maxim Baz)

Tracking

unspecified
Thunderbird 16.0
x86_64
Windows 7

Thunderbird Tracking Flags

(thunderbird14 fixed, thunderbird15 fixed)

Details

Attachments

(5 attachments, 9 obsolete attachments)

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
(Reporter)

Description

5 years ago
Created attachment 631492 [details] [diff] [review]
proposed fix

YSI has a new api for folder mgmt
(Reporter)

Comment 1

5 years ago
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!
(Assignee)

Comment 2

5 years ago
Created attachment 632554 [details] [diff] [review]
proposed fix corrected
(Assignee)

Comment 3

5 years ago
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
(Reporter)

Comment 4

5 years ago
this patch doesn't quite apply against the trunk - the chunk starting with @@ -905,48 +1101,98 @@ nsYouSendItFileUploader.prototype = {

fails.
(Assignee)

Comment 5

5 years ago
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.
Attachment #632554 - Attachment is obsolete: true
(Reporter)

Comment 6

5 years ago
(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!
(Reporter)

Comment 7

5 years ago
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-
(Assignee)

Comment 8

5 years ago
Created attachment 633039 [details] [diff] [review]
proposed fix corrected 3

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)
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
smaller uploads fail as well, in the same way.
(Reporter)

Comment 11

5 years ago
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-
(Assignee)

Comment 12

5 years ago
Created attachment 633408 [details] [diff] [review]
proposed fix corrected 4

I updated jar.mn files, now everything should be OK.
Thank you, David, for helping with this issue.
Attachment #633039 - Attachment is obsolete: true
(Reporter)

Comment 13

5 years ago
Created attachment 633526 [details] [diff] [review]
de-bitrotted patch

Maxim's patch had bit-rotted slightly. Building with this patch now.
Attachment #633526 - Flags: review?(dbienvenu)
(Reporter)

Comment 14

5 years ago
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)
(Reporter)

Comment 15

5 years ago
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
(Assignee)

Comment 16

5 years ago
Created attachment 633656 [details] [diff] [review]
proposed fix corrected 5

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
(Reporter)

Comment 17

5 years ago
Created attachment 633712 [details] [diff] [review]
de-bitrotted patch

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
(Reporter)

Comment 18

5 years ago
Created attachment 633725 [details] [diff] [review]
show how to tell the front end about the file size limit exceeded

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.
(Reporter)

Updated

5 years ago
Attachment #633526 - Attachment is obsolete: true
Attachment #633526 - Flags: ui-review?(bwinton)
(Reporter)

Comment 19

5 years ago
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+
(Assignee)

Comment 20

5 years ago
Created attachment 633741 [details] [diff] [review]
patch for old TB

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...
(Reporter)

Comment 21

5 years ago
Comment on attachment 633741 [details] [diff] [review]
patch for old TB

getting on my review radar
Attachment #633741 - Flags: review?(dbienvenu)
(Reporter)

Comment 22

5 years ago
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+
(Reporter)

Comment 23

5 years ago
Created attachment 634439 [details] [diff] [review]
patch with blank lines fixed

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+
(Reporter)

Comment 24

5 years ago
Comment on attachment 633712 [details] [diff] [review]
de-bitrotted patch

Blake says he likes it.
Attachment #633712 - Flags: ui-review?(bwinton) → ui-review+
(Reporter)

Comment 25

5 years ago
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
Last Resolved: 5 years ago
status-thunderbird14: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
(Reporter)

Comment 26

5 years ago
aurora - http://hg.mozilla.org/releases/comm-aurora/rev/b6fc291dba94
status-thunderbird15: --- → fixed
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
(Reporter)

Comment 28

5 years ago
landed missing check.png on trunk - http://hg.mozilla.org/comm-central/rev/4468b7bdbd88
Thanks, now the file is coming.
(Assignee)

Comment 30

5 years ago
Created attachment 634598 [details] [diff] [review]
same patch for new TB with fixed one string line

I attached the same patch for new TB, which already passed review, with change in single text line.
(Reporter)

Comment 31

5 years ago
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.
Attachment #631492 - Attachment is obsolete: true
Attachment #634598 - Attachment is obsolete: true
Attachment #634598 - Attachment is obsolete: false
Attachment #633725 - Attachment is obsolete: true
Created attachment 639670 [details] [diff] [review]
Patch to revert to old API

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
Attachment #633741 - Attachment is obsolete: false
Created attachment 639704 [details] [diff] [review]
Backout patch for comm-beta

This patch applies cleanly on top of comm-beta, and will revert us back to the old YSI API.

Updated

4 years ago
Assignee: nobody → mbaz
You need to log in before you can comment on or make changes to this bug.