User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

1. Add an attachment

2. Upload it to a Filelink provider ("Convert to ..." from the context menu)
   => A link to the uploaded file is inserted into the message along with the file's name.

3. Rename the attachment in the attachment list

Actual results:

The file name stays the same on the cloud storage and hence the downloaded file has the old name. Also the name automatically inserted into the message stays the same.

But in the MIME part with the X-Mozilla-Cloud-Part header the name property is set to the new name. I have no idea if this header is used by any reipient software. If it does, recipients might be confused if their download has a different file name than the one stated in the message.

Expected results:

It should not be possible to rename a Filelink attachment.

or

Renaming should actually work for a Filelink attachment:

1. Add a onFileRenamed event to the cloudFile API
2. Trigger this event if a Filelink attachment is renamed
3. Update the file name in the message. This might not be possible if the user changed the message after the template with fiele name and url was inserted.

This might be related to bug 1669829.

Geoff, this is the third one of the fileLink bug series by this reporter, linked up in see also.

The "onFileRenamed" idea might be a bad one as there are situations in which a cloudFile Addon is not able to rename the file, eg if the message is a stored draft and Thunderbird was closed between creating the draft and renaming the attachment.

Sorry, I don't know how to test the patch from phabricator and how to comment on it directly. So I'm commenting here after just reading the patch:

If I understand correctly the patch only changes the name of the file in the HTML inserted into the message. This will most likely confuse recipients: The message says "I've uploaded new-name.doc..." but the downloaded file still has the name "old-name.doc". This will even raise security concerns.
So to me this solution only makes sense if the actual download is also renamed. But this is impossible in at least two situations:

1. The message is a stored draft and TB has been restarted between upload and renaming the attachment. In this case there is no way to tell the FileLink Addon which file to rename as the UploadId is reset by the Thunderbird restart.
2. There are cloud storage APIs that don't have a rename/move call.

So I strongly suggest to deactivate the "rename" entry from an attachment's context menu if it is a Filelink attachment.

Commenting here is fine.

So the bug should not only make the names match in the email/body, but also on the server?

Then this must be channeled through the cloudFile add-on and it must handle the rename request. Which could fail due to a file with that name existing already.

But: Is this really an issue? How is your add-on handling the situation when an uploaded file has a name which exists on the server already? Don't you pick a different name on the server then? But that also creates a mismatch between the used name in the email and the actual name on the server. Isn't that the same situation?

How about this:

Let onFileUpload/onFileRename return an error, which indicates the file-exists-already-scenario, and make TB prompt for a new filename.

Furthemore, it is the job of the cloud file add-on to "know" if it can really do renames or if it has to re-upload under the new given filename (and delete the old one). So there must be a way to trigger a new upload from the onRenameEvent, or the manifest must state wether rename is supported or not and TB needs to issue a onFileDelete request and a new onFileUploaded request in these cases.

Would that solve all your remarks, besides the draft-one, which is a bug of its own (it must be fixed by keeping drafts information stored somewhere).

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

So the bug should not only make the names match in the email/body, but also on the server?

IMHO it should.

Then this must be channeled through the cloudFile add-on and it must handle the rename request. Which could fail due to a file with that name existing already.

I guess that renaming attachments is a rarely used feature. And even more rarely for attachments that have already been FileLinked.
Just to make this possible cloudFileRename would have to be added to the cloudFile API which is elegantly lean so far.
IMHO the hassle of implementing it in every FileLink provider (if it's possible at all) is disproportionate to the limited use of the function.

But: Is this really an issue? How is your add-on handling the situation when an uploaded file has a name which exists on the server already? Don't you pick a different name on the server then? But that also creates a mismatch between the used name in the email and the actual name on the server. Isn't that the same situation?

It is the same situation. In the case of conflicting names my Addons don't pick a new name but move the existing file to a sub-folder before uploading the new one. So both files can have the same name on the server. I chose this implementation specifically to keep the names on the server and in the message in sync.

(In reply to Johannes Endres from comment #8)

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

So the bug should not only make the names match in the email/body, but also on the server?

IMHO it should.

... or it should disable "Rename" for Filelinke'd attachments. I'd prefer that for the reasons given above.

We could disable rename for all cloud provider add-ons, who do not support rename = who have not added a listener for the proposed onFileRename event.

If the onFileRename event does not return success, the rename request is aborted with an Error displayed to the user (for example if the add-on cannot handle the filename-exists-already-scenario)

If I did not miss anything (besides drafts, which I consider a bug of its own), this should solve all the mentioned issues?

I think your subfolder trick to get around the filename issue is a great idea!

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

We could disable rename for all cloud provider add-ons, who do not support rename = who have not added a listener for the proposed onFileRename event.

If the onFileRename event does not return success, the rename request is aborted with an Error displayed to the user (for example if the add-on cannot handle the filename-exists-already-scenario)

If I did not miss anything (besides drafts, which I consider a bug of its own), this should solve all the mentioned issues?

Sounds reasonable to me.

Could the server URL change during rename? Should the onFileRename event also allow to return a different/updated url?

It definitely could, think of WebDAV where the url contains the plain file name.

And I think it's a great idea to make the new url optional. In ownCloud/Nextcloud the url doesn't change and the additional API fetch would slow the process down unnecessarily.

A few things I would like to get your opinion on:

• pop an alert if the provider does not support renaming, when renaming is tried.

  • just deactivating the rename-menu-entry is not telling the user why it is not possible

• add a special "FileExists" abortReason for upload and rename, which will pop a system message notifying the user

  • this is a common case error, add-on devs should not need to add a locale and the popup code for that

• we could even allow to return arbitrary error message (which the add-on needs to localize), which will be shown to the user if rename/upload was aborted

Furthermore:

In case a provider does not support rename/move at all, onFileRename could be implemented as delete+re-upload. What information do you need in the onFileRename event to be able to do it? Do you need access to the fileInfo object of the original upload or are you able to get the needed data just from the uploadId?

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

A few things I would like to get your opinion on:

• pop an alert if the provider does not support renaming, when renaming is tried.
  • just deactivating the rename-menu-entry is not telling the user why it is not possible

"Renaming a/an $1 attachment is not possible", cloud_file.name? Looks reasonable to me.

• add a special "FileExists" abortReason for upload and rename, which will pop a system message notifying the user
  • this is a common case error, add-on devs should not need to add a locale and the popup code for that

I'm against defining abortReasons, especially this one. Many different things might go wrong depending on the server's API. To split those possible reasons between the ones we thought about and additional ones from the Addon doesn't make sense to me. Again I'd prefer a lean cloudFile API.

• we could even allow to return arbitrary error message (which the add-on needs to localize), which will be shown to the user if rename/upload was aborted

• I'm not sure that aborted should be true on error. Currently an error message is only shown if the onFileUpload handler throws something, not if it returns aborted=true. I'd prefer to reserve aborted for onFileUploadAbort. Geoff might have an opinion on this.

• IMHO showing an error message provided by the Addon would be great. The current generic message confuses users. I see two ways to go:

  1. Define an optional boolean error property in the return object (default false if not present) and an optional string property errorMsg. This string might only contain the reason to add to the current "Upload failed" message from Thunderbird. Or use the current message as the default if the Addon doesn't return an errorMsg and only show the errorMsg if present.
  2. Just show the message of the Error object (in addition to the "Upload failed" message) if the Addon throws one.

As a lazy programmer I'd prefer option 2 as I would not have to return the error up the call stack.

Furthermore:
'
In case a provider does not support rename/move at all, onFileRename could be implemented as delete+re-upload. What information do you need in the onFileRename event to be able to do it? Do you need access to the fileInfo object of the original upload or are you able to get the needed data just from the uploadId?

Oh please don't! One of my Addon's users is a film producer who regularly uploads large files, which sometimes takes hours. That renaming triggers an upload makes even less sense than in the case of bug 1669829.

May I suggest thinking about the API more from a CRUD perspective and HTTP verbs. E.g. rename would be HTTP MOVE (in webdav) and could have a number of outcomes depending on the outcome. If rename is not implemented, it's HTTP 501 and so on...

@Magnus: Can you give some more details what you have in mind? What is still missing after this patch landed?

@Johannes: I updated the patch. It should do the following:

• adjust the added templates after renaming
• adding a onFileRename event with the following schema

{
        "name": "onFileRename",
        "type": "function",
        "description": "Fired when an already uploaded file should be renamed.",
        "parameters": [
          {
            "name": "account",
            "$ref": "CloudFileAccount",
            "description": "The account used for the file upload."
          },
          {
            "type": "integer",
            "name": "fileId",
            "minimum": 1,
            "description": "An identifier for the file which should be renamed."
          },
          {
            "type": "string",
            "name": "newName",
            "description": "The new name of the file."
          },
          {
            "name": "tab",
            "$ref": "tabs.Tab",
            "description": "The tab where the rename was initiated. Currently only available for the message composer."
          }
        ],
        "returns": {
          "type": "object",
          "properties": {
            "error": {
              "choices": [
                {
                  "type": "boolean"
                },
                {
                  "type": "string"
                }
              ],
              "description": "Report an error to the user. Set it to true for showing a generic error message, or set it to a specific error message.",
              "optional": true
            },
            "url": {
              "type": "string",
              "description": "The URL where the renamed file can be accessed.",
              "optional": true
            }
          }
        }
      },

• Adding the same error return value to onFileUpload

If you cannot build this on your own, I have a try run, where you can download the build. For Windows 64bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/NShQafBkSOqBctwka0sPvw/runs/0/artifacts/public/build/target.zip

Feedback is appreciated.

Open questions:

• Should we do a local rename only (with a mismatch between name in email and name on server), if the provider has not registered a onFileRename event - or should we fail like the patch is currently doing it

• Should the custom error message be appended to the generic message like the patch is doing it, or should only the custom message be printed (should we then extend the title to include filename and provider name, so it is clear who is responsible for the error?)

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

If you cannot build this on your own, I have a try run, where you can download the build. For Windows 64bit:

Is there a Linux version, too?

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

Open questions:

• Should we do a local rename only (with a mismatch between name in email and name on server), if the provider has not registered a onFileRename event - or should we fail like the patch is currently doing it

In my opinion we should fail (with a message to the user) to keep everything in sync.

• Should the custom error message be appended to the generic message like the patch is doing it, or should only the custom message be printed (should we then extend the title to include filename and provider name, so it is clear who is responsible for the error?)

I'm slightly in favor of replacing the entire message (with the current one as the default). The Addon can be more specific like "Upload worked, but sharing failed". This might be confusing if it also says "Upload failed".

Is there / should there be a way to point to further information on solving the problem? For example: With *cloud sharing fpr some users fails because of a server (mis)configuration. It might be useful to tell the user where to find the steps to solution.

Linux 32bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/baqcEL8gTN-Qfbqp1k-yEQ/runs/0/artifacts/public/build/target.tar.bz2

Linux 64bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/M5oyn2Q1QGO9rpAUI_aAAw/runs/0/artifacts/public/build/target.tar.bz2

Depends on D132280

Depends on D132299

I split this up in two patches. Patch #1 does some housekeeping and disables rename for cloud files and is intended to be uplifted to ESR. Patch #2 adds the onFileRename listener and allows to properly rename cloud files. The second patch will not be uplifted.

Requesting checking of D132299.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/7408db3e8e11
Disable rename for cloud attachment, deprecate ArrayBuffer data_format and throw Exceptions instead of integers. r=mkmelin

The actual patch is still pending review.

Requesting checkin for D132300.

Unable to rebase.

Requesting checkin for D132300.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1943c713d51f
Add onFileRename listener and re-enable rename for cloud attachments. r=mkmelin

Comment on attachment 9252740 [details]
Bug 1669897 - Disable rename for cloud attachment, deprecate ArrayBuffer data_format and throw Exceptions instead of integers. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Renaming does not properly work in TB91 and this bug disables it completely (real fix cannot be uplifted), this patch also adds a deprecation note, to inform developers about an upcoming change in the API. This patch is also back porting internal changes, to make further uplifts possible.

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

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

Comment on attachment 9252740 [details]
Bug 1669897 - Disable rename for cloud attachment, deprecate ArrayBuffer data_format and throw Exceptions instead of integers. r=mkmelin

[Triage Comment]
Approved for esr91

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/175318cfc6c4
Fix failure of onFileRename if no object or url is returned. r=mkmelin.

John, is just the one patch being uplifted, or all three?

Just that one patch.

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