1531595 - Clean up FileLink, part 2

Status: RESOLVED FIXED

(Thunderbird :: FileLink, enhancement)

Description

[Geoff Lankow (:darktrojan)]

In this part (hopefully):

• Use promises instead of request listeners
• Work out what to do if we lack space in the preferences tab
• Use provider icons in the "select a provider" prompt
• Anything else we think of at the time

Comment 1

[Geoff Lankow (:darktrojan)]

• Fix dozens of variables named "provider" that actually refer to an account.

Comment 2

[Geoff Lankow (:darktrojan)]

Philipp, what's your opinion on converting the Box and Hightail providers to WebExtensions and shipping them in the same way as WeTransfer's? Or converting them to WebExtensions and shipping them on ATN? Either way existing users would have to reauthorise their accounts, but I can see a number of upsides.

Comment 3

[Magnus Melin [:mkmelin]]

I think definitely move the to WX.
Indeed moving them to ATN could make sense too.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Moving to ATN it would be good to have a maintainer, maybe even someone at Box/Hightail. I agree migrating to WX though. Maybe we can have some custom migration code that would allow people to keep their tokens? I'm also considering we might need a way for add-ons to write to the logins API specifically for their add-on, without being able to read all the remaining secrets.

Comment 5

[Geoff Lankow (:darktrojan)]

Maybe we can have some custom migration code that would allow people to keep their tokens?

I don't think that's going to be possible, as I had to create a new app on the website to make things work, so the stored tokens are useless.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1531595-cloudfile-part2-1.diff

Here's all the changes I've done so far except the removal of Box and Hightail.

I have a 80%-complete WebExtension for Box, and I think we should just stop supporting Hightail.

The "find more providers…" link I want to connect to a new ATN category, but for now it links to the FileLink tag on ATN.

I'm starting to think there'll be a part 3…

Comment 7

[Magnus Melin [:mkmelin]]

Discussed with the council about Box and Hightail, and there seemed to be agreement we can stop shipping both. But for Box we'll dig into how the setup was done first.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9054118 [details] [diff] [review]
1531595-cloudfile-part2-1.diff

Review of attachment 9054118 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1518,5 @@
> +    attachment.cloudFileAccountKey = cloudFileAccount.accountKey;
> +    if (attachmentItem) {
> +      // Update relevant bits on the attachment list item.
> +      if (!attachmentItem.originalUrl)
> +        attachmentItem.originalUrl = originalUrl;

Can we add some brackets for the one-line ifs and spacing here? This area seems a bit crammed.

@@ +1590,5 @@
> +    if (displayError) {
> +      let url = cloudFileAccount.providerUrlForError &&
> +                cloudFileAccount.providerUrlForError(statusCode);
> +      let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_OK;
> +      if (url)

Hmm I guess the missing brackets are prevailing style in mail. I'm not a fan, but I take that part back if necessary.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1531595-cloudfile-part2-remove-providers-1.diff

Removes the Box and Hightail providers.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9058472 [details] [diff] [review]
1531595-cloudfile-part2-remove-providers-1.diff

Review of attachment 9058472 [details] [diff] [review]:
-----------------------------------------------------------------

r+ pending approval to remove, there are a few things on council side we need to clarify. I'll see if we can get these into an extension, if you happen to already have done that work it would be great! (Yes, probably mostly just taking previous patch versions :)

Comment 11

[Magnus Melin [:mkmelin]]

Last I heard the box is he has the extension code for. Hightail was... less fun so we might just drop it. (They terminated the contract many years ago anyway.)

Comment 12

[Geoff Lankow (:darktrojan)]

Correct, I have an almost-complete extension for Box. I haven't attempted to do Hightail and wasn't planning to.

Comment 13

[Geoff Lankow (:darktrojan)]

What is happening here? Do we land these changes?

Comment 14

[Magnus Melin [:mkmelin]]

This patch can land.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ff752764d9f7
Further improvements to FileLink; r=Fallen
https://hg.mozilla.org/comm-central/rev/4054e820674c
Remove Box and Hightail FileLink providers; r=Fallen