1627497 - cloudFile: allow update of service_url and name (to support *cloud and other providers that work for more than one account)

Status: RESOLVED FIXED

(Thunderbird :: FileLink, enhancement)

Description

[Johannes Endres]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36 Edge/18.18363

Steps to reproduce:

I made a Filelink provider that works for more than one service: *cloud, https://addons.thunderbird.net/de/thunderbird/addon/filelink-nextcloud-owncloud/.

Actual results:

Thunderbird inserts the name of my extension with the download link instead of the name of the actual service the file was uploaded to. This irritates users: https://gitlab.com/joendres/filelink-nextcloud/-/issues/76

Also the name not linked to the actual service.

Expected results:

My extension should be able to set the name property of the CloudFileAccount in updateAccount, so that Thunderbird inserts the correct name with the download url.
Alternatively the return object of onFileUpload could have a name property.

Similarly it would be great to have an (updatable) property service_url in CloudFileAccount or in the return object of onFileUpload.

Comment 1

[John Bieling (:TbSync)]

What is the benefit of changing the service_url? The service_url is already per account and not per provider so each account has its correct service_url?

I am still catching up on FileLink issues, I may simply have not fully understood the internals yet.

Edit: That statement was indeed wrong.

Comment 2

[Magnus Melin [:mkmelin]]

I guess we should just not link the extension at all in that text... it's slightly odd to do so.

Comment 3

[John Bieling (:TbSync)]

I am slowly getting behind this. Johannes is setting his ATN page as his service_url in the manifest, that is why it is added to the template. The service_url of the manifest is not used anywhere else, its sole purpose is to add a link in the template.

His management page is asking for an independent server url per account which is just used by him, not by the API.

So what this bug is really about is to better support multi provider FileLink add-ons. Every provider that can be self hosted will not be able to properly set a service_url, as the add-on needs to hard code that in the manifest. And as Johannes correctly pointed out, this needs to be a setting per account or per file.

The things which might need customization are:

• provider image
• provider name
• provider link

Ways to get this done:

1. making the entire template customizable
2. extending the onFileUpload event to accept a name, an image and a link in its return value and fall_back to the current behavior, if none provided.
3. extending the CloudFileAccount to accept an image and a link and allowing the update function to change name, icon and link

I am in favor for option number #2.
#1 is shifting the burden of localization to the add-on
#3 would require the add-on to actually change the cloudFile account name, which feels wrong. That is a user picked value and the provider should not change that.

@Johannes: Sorry for needing so long to understand what this is about, and sorry for not having seen this bug earlier.

Comment 4

[Johannes Endres]

(In reply to John Bieling (:TbSync) from comment #3)

Ways to get this done:

1. making the entire template customizable
2. extending the onFileUpload event to accept a name, an image and a link in its return value and fall_back to the current behavior, if none provided.
3. extending the CloudFileAccount to accept an image and a link and allowing the update function to change name, icon and link

I am in favor for option number #2.
#1 is shifting the burden of localization to the add-on
#3 would require the add-on to actually change the cloudFile account name, which feels wrong. That is a user picked value and the provider should not change that.

There are two sub-options to #1:
a. Make it customizable by the Add-On, meaning the Add-On returns the entire HTML snippet to TB. I strongly agree that this is a bad idea.
b. Make it customizable by a separate API or UI. This is what bug 1643729 is about.

We should look at customization of the name on the one hand and of image/link on the other hand separately, because the name can already be changed by the user.

I'm in favor of option #3 for image&link:

• I think there should be a way for the Add-On not to set an image or link at all. In option #2 this would have to be done by some special return value, because "no value" already has a different meaning.
• I can't think of a situation where the Add-On would want to return different images or links for different files uploaded to the same service. In other words: image and link seem to be properties of the account, not the uploaded file.

For the provider name, the question at hand is not how to change it, but if the Add-On should be allowed to change it at all.
I'm in favor of this option, because I still think it's not obvious for users how to change the name. So for most of them the name is not "user picked" but "the name of the Add-On for all my accounts". The option to change that via the API (with some caveat in the docs) seems an improvement to me.

Comment 5

[John Bieling (:TbSync)]

Using the return type is the simplest implementation, which gives the developer the most flexibility. Changing the account type probably means we also have to change the account setup UI to allow the user himself to set icon and the link, to be consistent.

When using the return type of the onFileUpload event to provide the data for the template,

• you can compare the provider name and the account name to see if the user picked a custom name. If yes, use that in the template, if not, override it with your autodetected provider name.
• you can but do not need to use a different icon per file, you have the account information and can use the appropriate icon.
• we can implement that returning false forces to not use an icon at all

Comment 6

[John Bieling (:TbSync)]

Attachment: Bug 1627497 - Extend onFileUpload event to optionally return values for the CloudFile template. r=darktrojan

Edit: Removed test commit messages from learning how to fold changesets.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d3aee7f4cae8
Extend onFileUpload event to optionally return values for the CloudFile template. r=darktrojan

Comment 8

[John Bieling (:TbSync)]

Comment on attachment 9250155 [details]
Bug 1627497 - Extend onFileUpload event to optionally return values for the CloudFile template. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Missing template feature. Needed primarily to fix merge issues for the later cloud file patches.

Testing completed (on c-c, etc.):
Baked on 96 for 4 weeks.

Risk to taking this patch (and alternatives if risky):
Low

Comment 9

[Rob Lemley [:rjl]]

Comment on attachment 9250155 [details]
Bug 1627497 - Extend onFileUpload event to optionally return values for the CloudFile template. r=darktrojan

[Triage Comment]
Prerequisite for other approved FileLink uplifts in 91.4.1. (bug 1667652, bug 1715405, bug 1669897, bug 1743203)

Comment 10

[Rob Lemley [:rjl]]

Thunderbird 91.4.1:
https://hg.mozilla.org/releases/comm-esr91/rev/cb582d392903