Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional failure status codes for WOPI operations #8713

Open
juliusknorr opened this issue Apr 5, 2024 · 4 comments
Open

Additional failure status codes for WOPI operations #8713

juliusknorr opened this issue Apr 5, 2024 · 4 comments
Labels
enhancement New feature or request unconfirmed

Comments

@juliusknorr
Copy link
Member

juliusknorr commented Apr 5, 2024

Is your feature request related to a problem?

When a wopi operation like saving the file fails, there is currently no possibility to give an error message or more detailed information to the user. The integrator can just return a limited list of HTTP status codes, but there are some cases where it would be befinicial if we could report more details to the user so they know why saving has failed.

This came up in nextcloud/richdocuments#3281

Some examples could be:

  • The user quota on the storage exceeded
  • Storage is temporarily unavailable (e.g. because the file is on a remote Nextcloud instance that currently is in maintenance mode)
  • The file lacks write permissions (while it was there when the document was opened, Nextcloud has different mechanisms where the permission may be changed in the meantime by an admin or another user)
  • The file is temporarily locked by another process
  • There may be others

Describe the solution you'd like

For some cases it would be good to have a defined status code where Collabora could then suggest the user to download a copy of the file to avoid loosing changes (if quarantine is not enabled or to avoid admins needing to go grab the file from there)

https://github.com/nextcloud/richdocuments/blob/4cb127a868e8b6e8edc3a39c1a5b78eb94c68916/lib/Controller/WopiController.php#L482

online/wsd/Storage.hpp

Lines 318 to 321 in 52a039e

enum class COOLStatusCode
{
DOC_CHANGED = 1010 // Document changed externally in storage
};

We could extend this list with common known errors.

In addition it would also be great to still have something like a COOLStatusMessage field in the response that integrators can send and is then displayed to the user in the error message when saving fails.

Describe alternatives you've considered

None

Additional context

None

@Ashod
Copy link
Contributor

Ashod commented Apr 10, 2024

Some of these are already supported by the WOPI protocol and supported (with proper message to the user):

  • The user quota on the storage exceeded
    • Storage returns HTTP 413 (Too Large), user sees "Save failed because the document is too large or exceeds the remaining storage space. The document will now be read-only but you may still download it now to preserve a copy locally." (or the equivalent translation).
  • Storage is temporarily unavailable (e.g. because the file is on a remote Nextcloud instance that currently is in maintenance mode)
    • Storage returns 500 error (or the equivalent), user sees "Document cannot be saved, please check your permissions." (or the equivalent translation. This can be improved.)
  • The file lacks write permissions (while it was there when the document was opened, Nextcloud has different mechanisms where the permission may be changed in the meantime by an admin or another user)
    • Storage returns 401, 403, or 404 (UNAUTHORIZED), user sees "Document cannot be saved due to expired or invalid access token." (or the equivalent translation).
  • The file is temporarily locked by another process
    • Identical to "Storage is temporarily unavailable" above.

We can extend these error-codes and have matching user-visible messages added. Similarly for other cases. We just need to choose sensible HTTP codes for each case and agree on them.

@juliusknorr
Copy link
Member Author

Thanks @Ashod that sounds very reasonable. Then we could use those existing cases and in addition add a COOLStatusMessage to the json response of the wopi operations for integrator defined errors messages that should be shown in addition.

On a separate note: I've just tried returning a 413 and it seems that coolwsd is handling that properly but the frontend code base just switches over to readonly without showing any error.

@juliusknorr
Copy link
Member Author

@pedropintosilva In addition, i think in case of a save failure it would be nice if we could improve the error message to directly offer to download a local copy to to avoid loosing any changes.

@Ashod
Copy link
Contributor

Ashod commented Apr 12, 2024

returning a 413 and it seems that coolwsd is handling that properly but the frontend code base just switches over to readonly without showing any error

@pedropintosilva this sounds like a regression. Properly we need to only restore the message that notifies the user of the issue (the error message already says the document will be readonly. We just need to display it (for some reason this._map.fire('warn', {msg: storageError}); is not working properly, I guess).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request unconfirmed
Projects
Status: No status
Development

No branches or pull requests

2 participants