Closed Bug 744037 Opened 8 years ago Closed 8 years ago

Add UbuntuOne support for Filelink

Categories

(Thunderbird :: FileLink, defect)

x86
All
defect
Not set

Tracking

(thunderbird14 fixed, thunderbird15 fixed)

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

People

(Reporter: mconley, Assigned: james)

References

Details

Attachments

(4 files, 5 obsolete files)

We're aiming to support UbuntuOne with the Filelink feature.
Attached patch initial patch from Ubuntu (obsolete) — Splinter Review
Attachment #616588 - Flags: review?(dbienvenu)
Attached patch fix a few issues (obsolete) — Splinter Review
This fixes a bit rot issue with the previous patch, and makes uploading files that we don't know the mime type of work (it was failing because the code wasn't catching the exception, so I've fixed that). I've also fixed the offline error to use throw.

With those changes, I was able to sign up for a UbuntuOne account and send a filelink, so the basics seem to work.
Attachment #616588 - Attachment is obsolete: true
Attachment #616588 - Flags: review?(dbienvenu)
Attachment #617120 - Flags: review?(dbienvenu)
Attached patch The latest version of my work (obsolete) — Splinter Review
Here is my latest work on the Ubuntu One backend, squashed down to a single changeset and rebased on tip.  I'm just rebuilding to test the backend, so if I discover any issues I'll post an updated patch.

I hope I've incorporated the offlineErr changes from David's update correctly.

This version also includes mozmill tests, giving a similar level of coverage as the other backends.

One thing missing from the patch at the moment is the referral codes in the links.  Once we've sorted out what they should be, I'll post an update.
I noticed that the try/catch hadn't been added to _uploaderCallback() as part of the offline error handling, so I've added that.  This seems to have all of David's changes, so this is the patch to review.

As with the last patch, it still needs the referral codes in some of the links, but that shouldn't prevent the code itself from being reviewed.
Attachment #617120 - Attachment is obsolete: true
Attachment #617224 - Attachment is obsolete: true
Attachment #617120 - Flags: review?(dbienvenu)
Attachment #617270 - Flags: review?(dbienvenu)
Assignee: nobody → james
Status: NEW → ASSIGNED
Comment on attachment 617270 [details] [diff] [review]
Fix offline handling in _uploaderCallback

overall, this is looking pretty good. I'll work on reviewing it over the next couple days.
Comment on attachment 617270 [details] [diff] [review]
Fix offline handling in _uploaderCallback

sorry for the delay in getting to this:

these don't seem to be used:

+  _authToken: "",
+  _lastErrorStatus : 0,
+  _lastErrorText : "",

I tried uploading a 43MB file - it never seemed to finish, even after waiting several minutes. A 300K message finished in a few seconds. A 9MB attachment failed after about 3 minutes. This might be something with my network, but it worked with other providers in about 20 seconds.

The code looks good, in general. I tried changing my password on the UbuntuOne site, removed my auth token from the password manager, and my next attempt to upload caused a password prompt in TB, so that's good. Changing my password maybe should invalidate my auth token, but that's something to worry about on the server side...

I'll run through the mozmill tests in a minute.

You'll want a ui review from Blake. The one thing I noticed is that during account setup, once I enter an e-mail address, the "get new account link" mostly disappears, but not quite, so it looks a bit off.

I notice you have new strings in management.dtd and settings.dtd. We're way past string freeze for TB 14, so we'll need to think about what to do there.
Attachment #617270 - Flags: ui-review?(bwinton)
Comment on attachment 617270 [details] [diff] [review]
Fix offline handling in _uploaderCallback

self doesn't seem to be used here either:

+    aOptions = this._overrideDefault(kDefaultReturnHeader, aOptions);
+    let self = this;

or

+    let subjectString = Cc["@mozilla.org/supports-string;1"]
+                        .createInstance(Ci.nsISupportsString);
+    subjectString.data = aString;

but other than that and the previous comment, this looks OK. The failure to upload large files worries me, so we should figure that out.
Attachment #617270 - Flags: review?(dbienvenu) → review+
Comment on attachment 617270 [details] [diff] [review]
Fix offline handling in _uploaderCallback

For the first screen, I would prefer things to line up with themselves and with the other elements on the screen, similarly to the way they do in http://is.gd/AjY10y  (That also adds a "Learn More…" link which you can redirect to an Ubuntu One page.)

Other than that, I'm pretty happy with it, so ui-r=me with those fixed.

Thanks,
Blake.
Attachment #617270 - Flags: ui-review?(bwinton) → ui-review+
I believe this patch suffers from the same problem that our YouSendIt implementation did with forcing auth prompts to spawn from compose windows (see https://bugzilla.mozilla.org/show_bug.cgi?id=756239)

The same fix in that bug should be applied to this patch.
Here is an updated patch based on the review comments:

- I've updated to use the settings form to use addAccountDialog.css so that it matches the design Blake requested.
- I've included the fix for the password dialog parent window from bug 756239.
- I've removed the unused variables in the main code and tests.  These sections of code were adapted from the Dropbox backend, so it could probably make the same changes.

As far as the string freeze goes, only the &ubuntuOneMgmt.viewSettings; string ("View my account settings on one.ubuntu.com") is specific to Ubuntu One.  The settings ones could probably reuse the YouSendIt strings if needed: while we ask for an email address, people would probably know what was required if we asked for a user name.  If anyone has any better suggestions, I'm open to them.
Attachment #617270 - Attachment is obsolete: true
Attachment #625926 - Flags: ui-review?(bwinton)
Attachment #625926 - Flags: review?(dbienvenu)
I forgot to add: we do have some engineers looking into the problem with the larger file uploads through the REST API this backend is using.  They have identified the problem, so it should hopefully be fixed soon.
Comment on attachment 625926 [details] [diff] [review]
Updated patch based on review comments

You don't usually need to request a new review if a review has been granted with just small comments needing to be addressed. I'll try to land this on the trunk in the next day or so, and we'll try to figure out what to do about the strings for the alpha branch.
Attachment #625926 - Flags: ui-review?(bwinton)
Attachment #625926 - Flags: review?(dbienvenu)
pushed to trunk, thx, James! - http://hg.mozilla.org/comm-central/rev/436d34c9eba1

Now we have to figure out what to do about the strings. Standard8, is it crazy to do late l10n on the couple new strings and land this for aurora? Or should we steal some strings and hack up an aurora-only patch?
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
(In reply to David :Bienvenu from comment #13)
> pushed to trunk, thx, James! -
> http://hg.mozilla.org/comm-central/rev/436d34c9eba1
> 
> Now we have to figure out what to do about the strings. Standard8, is it
> crazy to do late l10n on the couple new strings and land this for aurora? Or
> should we steal some strings and hack up an aurora-only patch?

Yes, we can't do l10n changes on aurora. So if we can find other strings then we'll have to go with that.
OK, James, then you should pull a comm-aurora repository (hg.mozilla.org/comm-aurora), apply your patch, minus the new strings, and try embedding the existing yousendit strings, or whatever other strings you can steal, and propose a patch for aurora (alpha). Thx!
This is another patch for trunk: It updates the links to https://one.ubuntu.com/ to include a referral code so that we can track the subscriptions that come from Thunderbird users.

I'll have the Aurora patch ready shortly, which will include this change too.
Comment on attachment 626731 [details] [diff] [review]
Add referral codes to one.ubuntu.com links

this looks reasonable. I can land this on the trunk later today.
Attachment #626731 - Flags: review+
Here is a patch against comm-aurora, containing the changes from the two revisions from trunk.

The Aurora specific changes include:

* For the management panel, use the "View settings for this account" string from msgAccountCentral.dtd in place of "View my account settings on one.ubuntu.com".

* For the account setup panel, use the "Learn more" and "Need an account?" strings from the YouSendIt backend, and the "Email address:" string from accountCreation.dtd

I believe I've maintained the intent of these borrowed strings, so the translations should make sense in this new context.
Attachment #627112 - Flags: review?(dbienvenu)
Attachment #627112 - Attachment is obsolete: true
Attachment #627112 - Flags: review?(dbienvenu)
Attachment #632287 - Flags: review?(dbienvenu)
Attachment #632287 - Flags: review?(dbienvenu)
Attachment #632287 - Flags: review+
Attachment #632287 - Flags: approval-comm-beta+
Depends on: 764312
James, as pointed out by Florian, this looks like a typo:

http://mxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsUbuntuOne.js#502

4:26:17 PM - florian: ".bind(this). this)"
4:26:54 PM - florian: I would expect a comma there; and I don't understand how this can be  […]
Here is a patch to fix the problem Florian discovered.  I guess the typo would have resulted in me passing "undefined" as the error callback to doXHRequest().
Attachment #633940 - Flags: review?(dbienvenu)
Assignee: james → nobody
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink
Assignee: nobody → james
Attachment #633940 - Flags: review?(dbienvenu) → review+
Comment on attachment 633940 [details] [diff] [review]
Fix the typo in _acquireToken() that Florian pointed out

Checked in:

https://hg.mozilla.org/comm-central/rev/13c4be7e7d31

We'll also want this on aurora and beta as that's where the rest of the code has landed.
Attachment #633940 - Flags: approval-comm-beta+
Attachment #633940 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.