744037 - Add UbuntuOne support for Filelink

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

We're aiming to support UbuntuOne with the Filelink feature.

Comment 1

[David :Bienvenu]

Attachment: initial patch from Ubuntu

Comment 2

[David :Bienvenu]

Attachment: fix a few issues

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.

Comment 3

[James Henstridge]

Attachment: The latest version of my work

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.

Comment 4

[James Henstridge]

Attachment: Fix offline handling in _uploaderCallback

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.

Comment 5

[David :Bienvenu]

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 6

[David :Bienvenu]

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.

Comment 7

[David :Bienvenu]

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.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 10

[James Henstridge]

Attachment: Updated patch based on review comments

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.

Comment 11

[James Henstridge]

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 12

[David :Bienvenu]

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.

Comment 13

[David :Bienvenu]

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?

Comment 14

[Mark Banner (:standard8)]

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

Comment 15

[David :Bienvenu]

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!

Comment 16

[James Henstridge]

Attachment: Add referral codes to one.ubuntu.com links

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 17

[David :Bienvenu]

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.

Comment 18

[David :Bienvenu]

referral codes landed on trunk - http://hg.mozilla.org/comm-central/rev/0bc9fd424556

Comment 19

[James Henstridge]

Attachment: cloudfiles-ubuntuone-aurora.patch

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.

Comment 20

[David :Bienvenu]

Attachment: de-bitrotted patch for beta

Comment 21

[David :Bienvenu]

pushed to beta - http://hg.mozilla.org/releases/comm-beta/rev/7e5fd53cc408

Comment 22

[David :Bienvenu]

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  […]

Comment 23

[James Henstridge]

Attachment: Fix the typo in _acquireToken() that Florian pointed out

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

Comment 24

[Mark Banner (:standard8)]

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.

Comment 25

[Mark Banner (:standard8)]

Also in aurora and beta:

https://hg.mozilla.org/releases/comm-aurora/rev/25662480aa1e
https://hg.mozilla.org/releases/comm-beta/rev/c7243ee102fc